Re: [PATCH v2] drm/ast: DisplayPort edid supports 256 bytes

[Thomas Zimmermann <tzimmermann@suse.de>]

(cc Villa, Jani)

Hi

Am 17.03.26 um 13:19 schrieb Jammy Huang:
> DisplayPort supports EDID up to 256 bytes (blocks 0 and 1). Update the
> block check to allow these two blocks while returning 0 for any
> additional extension blocks.
>
> Furthermore, remove the manual EDID byte manipulation logic. The DRM
> core (drm_edid) already handles error correction and checksum
> validation.

Does that really work? AFAICT

- The allocations in _drm_do_get_edid() [1] do not guarantee that the 
memory is cleared to zero.
- If the helper detects an error, it will conditionally print an error [2].
- As ast is polling the EDID, it'll fill the kernel log over time with 
these warnings.

I think we'd still need some sort of 'valid error state' that signals an 
EOF that is not an error, but still counts as an invalid header.

Best regards
Thomas


[1] 
https://elixir.bootlin.com/linux/v6.19.8/source/drivers/gpu/drm/drm_edid.c#L2366
[2] 
https://elixir.bootlin.com/linux/v6.19.8/source/drivers/gpu/drm/drm_edid.c#L1919

>
> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> ---
> ASPEED DisplayPort's EDID size can be 256 bytes at most. Thus, EDID
> blocks fetched can be 0 and 1.
> ---
> Changes in v2:
> Becasue drm-edid will handle invalid EDID if happen, we have 2 changes
> below.
> - Return 0 for the number of block more than 1.
> - Remove modification of EDID
> - Link to v1: https://lore.kernel.org/r/20260313-upstream_ast_dp_edid-v1-1-2a75b7c091b2@aspeedtech.com
> ---
>   drivers/gpu/drm/ast/ast_dp.c | 18 ++----------------
>   1 file changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 9d07dad358c..282c694218c 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -88,8 +88,8 @@ static int ast_astdp_read_edid_block(void *data, u8 *buf, unsigned int block, si
>   	int ret = 0;
>   	unsigned int i;
>   
> -	if (block > 0)
> -		return -EIO; /* extension headers not supported */
> +	if (block > 1)
> +		return 0;
>   
>   	/*
>   	 * Protect access to I/O registers from concurrent modesetting
> @@ -154,20 +154,6 @@ static int ast_astdp_read_edid_block(void *data, u8 *buf, unsigned int block, si
>   		ediddata[2] = ast_get_index_reg(ast, AST_IO_VGACRI, 0xda);
>   		ediddata[3] = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdb);
>   
> -		if (i == 31) {
> -			/*
> -			 * For 128-bytes EDID_1.3,
> -			 * 1. Add the value of Bytes-126 to Bytes-127.
> -			 *		The Bytes-127 is Checksum. Sum of all 128bytes should
> -			 *		equal 0	(mod 256).
> -			 * 2. Modify Bytes-126 to be 0.
> -			 *		The Bytes-126 indicates the Number of extensions to
> -			 *		follow. 0 represents noextensions.
> -			 */
> -			ediddata[3] = ediddata[3] + ediddata[2];
> -			ediddata[2] = 0;
> -		}
> -
>   		memcpy(buf, ediddata, min((len - i), 4));
>   		buf += 4;
>   	}
>
> ---
> base-commit: 5ee8dbf54602dc340d6235b1d6aa17c0f283f48c
> change-id: 20260313-upstream_ast_dp_edid-5fe6adf7ad36
>
> Best regards,

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)