Re: [PATCH v2 3/9] drm/ast: astdp: Use struct drm_edid and helpers

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

On 12/08/2024 11:30, Thomas Zimmermann wrote:
> Convert ASTDP support to struct drm_edid and its helpers. Simplifies
> and modernizes the EDID handling.
>
> The driver reads 4 bytes at once, but the overall read length is now
> variable. Therefore update the EDID read loop to never return more than
> the requested bytes.
>
> The device does not seem to support EDID extensions, as the driver
> actively clears any such information from the main EDID header. As
> the new interface allows for reading extension blocks for EDID, make
> sure that the block is always 0 (i.e., the main header). A later
> update might fix that.
>
> v2:
> - fix reading if len is not a multiple of 4

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>

> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
>   drivers/gpu/drm/ast/ast_dp.c | 55 +++++++++++++++++++-----------------
>   1 file changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 217c155f0874..22c4f2a126e9 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -20,11 +20,15 @@ static bool ast_astdp_is_connected(struct ast_device *ast)
>   	return true;
>   }
>   
> -static int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
> +static int ast_astdp_read_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
> +	struct ast_device *ast = data;
> +	size_t rdlen = round_up(len, 4);
>   	int ret = 0;
> -	u8 i;
> +	unsigned int i;
> +
> +	if (block > 0)
> +		return -EIO; /* extension headers not supported */
>   
>   	/*
>   	 * Protect access to I/O registers from concurrent modesetting
> @@ -35,13 +39,23 @@ static int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
>   	/* Start reading EDID data */
>   	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xe5, (u8)~AST_IO_VGACRE5_EDID_READ_DONE, 0x00);
>   
> -	for (i = 0; i < 32; i++) {
> +	for (i = 0; i < rdlen; i += 4) {
> +		unsigned int offset;
>   		unsigned int j;
> +		u8 ediddata[4];
> +		u8 vgacre4;
> +
> +		offset = (i + block * EDID_LENGTH) / 4;
> +		if (offset >= 64) {
> +			ret = -EIO;
> +			goto out;
> +		}
> +		vgacre4 = offset;
>   
>   		/*
>   		 * CRE4[7:0]: Read-Pointer for EDID (Unit: 4bytes); valid range: 0~64
>   		 */
> -		ast_set_index_reg(ast, AST_IO_VGACRI, 0xe4, i);
> +		ast_set_index_reg(ast, AST_IO_VGACRI, 0xe4, vgacre4);
>   
>   		/*
>   		 * CRD7[b0]: valid flag for EDID
> @@ -65,7 +79,7 @@ static int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
>   			vgacrd7 = ast_get_index_reg(ast, AST_IO_VGACRI, 0xd7);
>   			if (vgacrd7 & AST_IO_VGACRD7_EDID_VALID_FLAG) {
>   				vgacrd6 = ast_get_index_reg(ast, AST_IO_VGACRI, 0xd6);
> -				if (vgacrd6 == i)
> +				if (vgacrd6 == offset)
>   					break;
>   			}
>   		}
> @@ -93,7 +107,8 @@ static int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
>   			ediddata[2] = 0;
>   		}
>   
> -		ediddata += 4;
> +		memcpy(buf, ediddata, min((len - i), 4));
> +		buf += 4;
>   	}
>   
>   out:
> @@ -330,29 +345,17 @@ static const struct drm_encoder_helper_funcs ast_astdp_encoder_helper_funcs = {
>   
>   static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
>   {
> -	void *edid;
> -	int succ;
> +	struct drm_device *dev = connector->dev;
> +	struct ast_device *ast = to_ast_device(dev);
> +	const struct drm_edid *drm_edid;
>   	int count;
>   
> -	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
> -	if (!edid)
> -		goto err_drm_connector_update_edid_property;
> -
> -	succ = ast_astdp_read_edid(connector->dev, edid);
> -	if (succ < 0)
> -		goto err_kfree;
> -
> -	drm_connector_update_edid_property(connector, edid);
> -	count = drm_add_edid_modes(connector, edid);
> -	kfree(edid);
> +	drm_edid = drm_edid_read_custom(connector, ast_astdp_read_edid_block, ast);
> +	drm_edid_connector_update(connector, drm_edid);
> +	count = drm_edid_connector_add_modes(connector);
> +	drm_edid_free(drm_edid);
>   
>   	return count;
> -
> -err_kfree:
> -	kfree(edid);
> -err_drm_connector_update_edid_property:
> -	drm_connector_update_edid_property(connector, NULL);
> -	return 0;
>   }
>   
>   static int ast_astdp_connector_helper_detect_ctx(struct drm_connector *connector,