Re: [PATCH/RFC 1/5] drm/edid: Reorganise the DisplayID parsing code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 21, 2019 at 09:50:01PM +0300, Laurent Pinchart wrote:
> This change started as an attempt to remove the forward declaration of
> validate_displayid(), and ended up reorganising the DisplayID parsing
> code to fix serial intertwined issues.
> 
> The drm_parse_display_id() function, which parses a DisplayID block, is
> made aware of whether the DisplayID comes from an EDID extension block
> or is direct DisplayID data. This is a layering violation, this should
> be handled in the caller. Similarly, the validate_displayid() function
> should not take an offset in the data buffer, it should also receive a
> pointer to the DisplayID data.
> 
> In order to simplify the callers of drm_find_displayid_extension(), the
> function is modified to return a pointer to the DisplayID data, not to
> the EDID extension block. The pointer can then be used directly for
> validation and parsing, without the need to add an offset.
> 
> For consistency reasons the validate_displayid() function is renamed to
> drm_displayid_is_valid() and modified to return a bool, and the
> drm_parse_display_id() renamed to drm_parse_displayid().
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_edid.c | 104 ++++++++++++++++++-------------------
>  1 file changed, 52 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 82a4ceed3fcf..7c6bc5183b60 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1342,7 +1342,6 @@ MODULE_PARM_DESC(edid_fixup,
>  
>  static void drm_get_displayid(struct drm_connector *connector,
>  			      struct edid *edid);
> -static int validate_displayid(u8 *displayid, int length, int idx);
>  
>  static int drm_edid_block_checksum(const u8 *raw_edid)
>  {
> @@ -2927,17 +2926,49 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
>  	return edid_ext;
>  }
>  
> +static bool drm_displayid_is_valid(u8 *displayid, unsigned int length)

const* would be nice.

same in many other places in the series.

> +{
> +	struct displayid_hdr *base;
> +	u8 csum = 0;
> +	int i;
> +
> +	base = (struct displayid_hdr *)displayid;

Can be done when declaring the variable.

> +
> +	DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
> +		      base->rev, base->bytes, base->prod_id, base->ext_count);
> +
> +	if (base->bytes + 5 > length)
> +		return false;
> +
> +	for (i = 0; i <= base->bytes + 5; i++)
> +		csum += displayid[i];
> +
> +	if (csum) {
> +		DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);
> +		return false;
> +	}
> +
> +	return true;
> +}
>  
>  static u8 *drm_find_displayid_extension(const struct edid *edid)
>  {
> -	return drm_find_edid_extension(edid, DISPLAYID_EXT);
> +	u8 *ext = drm_find_edid_extension(edid, DISPLAYID_EXT);
> +
> +	if (!ext)
> +		return NULL;
> +
> +	/*
> +	 * Skip the EDID extension block tag number to return the DisplayID
> +	 * data.
> +	 */
> +	return ext + 1;
>  }
>  
>  static u8 *drm_find_cea_extension(const struct edid *edid)
>  {
> -	int ret;
> -	int idx = 1;
> -	int length = EDID_LENGTH;
> +	int idx;
> +	int length = EDID_LENGTH - 1;
>  	struct displayid_block *block;
>  	u8 *cea;
>  	u8 *displayid;
> @@ -2952,11 +2983,10 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
>  	if (!displayid)
>  		return NULL;
>  
> -	ret = validate_displayid(displayid, length, idx);
> -	if (ret)
> +	if (!drm_displayid_is_valid(displayid, length))
>  		return NULL;
>  
> -	idx += sizeof(struct displayid_hdr);
> +	idx = sizeof(struct displayid_hdr);

It's a bit silly/fragile that everyone has to do this. I'd rather
eliminate this idx initialzation from the callers of
for_each_displayid_db() entirely.

>  	for_each_displayid_db(displayid, block, idx, length) {
>  		if (block->tag == DATA_BLOCK_CTA) {
>  			cea = (u8 *)block;

This here is also borked. We should validate the size of the
CEA ext block somewhere.

> @@ -4711,29 +4741,6 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
>  	return quirks;
>  }
>  
> -static int validate_displayid(u8 *displayid, int length, int idx)
> -{
> -	int i;
> -	u8 csum = 0;
> -	struct displayid_hdr *base;
> -
> -	base = (struct displayid_hdr *)&displayid[idx];
> -
> -	DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
> -		      base->rev, base->bytes, base->prod_id, base->ext_count);
> -
> -	if (base->bytes + 5 > length - idx)
> -		return -EINVAL;
> -	for (i = idx; i <= base->bytes + 5; i++) {
> -		csum += displayid[i];
> -	}
> -	if (csum) {
> -		DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);
> -		return -EINVAL;
> -	}
> -	return 0;
> -}
> -
>  static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *dev,
>  							    struct displayid_detailed_timings_1 *timings)
>  {
> @@ -4809,9 +4816,8 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
>  					struct edid *edid)
>  {
>  	u8 *displayid;
> -	int ret;
> -	int idx = 1;
> -	int length = EDID_LENGTH;
> +	int idx;
> +	int length = EDID_LENGTH - 1;
>  	struct displayid_block *block;
>  	int num_modes = 0;
>  
> @@ -4819,11 +4825,10 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
>  	if (!displayid)
>  		return 0;
>  
> -	ret = validate_displayid(displayid, length, idx);
> -	if (ret)
> +	if (!drm_displayid_is_valid(displayid, length))
>  		return 0;
>  
> -	idx += sizeof(struct displayid_hdr);
> +	idx = sizeof(struct displayid_hdr);
>  	for_each_displayid_db(displayid, block, idx, length) {
>  		switch (block->tag) {
>  		case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
> @@ -5424,23 +5429,17 @@ static int drm_parse_tiled_block(struct drm_connector *connector,
>  	return 0;
>  }
>  
> -static int drm_parse_display_id(struct drm_connector *connector,
> -				u8 *displayid, int length,
> -				bool is_edid_extension)
> +static int drm_parse_displayid(struct drm_connector *connector,
> +			       u8 *displayid, int length)
>  {
> -	/* if this is an EDID extension the first byte will be 0x70 */
> -	int idx = 0;
>  	struct displayid_block *block;
> +	int idx;
>  	int ret;
>  
> -	if (is_edid_extension)
> -		idx = 1;
> +	if (!drm_displayid_is_valid(displayid, length))
> +		return -EINVAL;
>  
> -	ret = validate_displayid(displayid, length, idx);
> -	if (ret)
> -		return ret;
> -
> -	idx += sizeof(struct displayid_hdr);
> +	idx = sizeof(struct displayid_hdr);
>  	for_each_displayid_db(displayid, block, idx, length) {
>  		DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n",
>  			      block->tag, block->rev, block->num_bytes);
> @@ -5468,8 +5467,9 @@ static int drm_parse_display_id(struct drm_connector *connector,
>  static void drm_get_displayid(struct drm_connector *connector,
>  			      struct edid *edid)
>  {
> -	void *displayid = NULL;
> +	void *displayid;
>  	int ret;
> +
>  	connector->has_tile = false;
>  	displayid = drm_find_displayid_extension(edid);
>  	if (!displayid) {
> @@ -5477,16 +5477,16 @@ static void drm_get_displayid(struct drm_connector *connector,
>  		goto out_drop_ref;
>  	}
>  
> -	ret = drm_parse_display_id(connector, displayid, EDID_LENGTH, true);
> +	ret = drm_parse_displayid(connector, displayid, EDID_LENGTH - 1);
>  	if (ret < 0)
>  		goto out_drop_ref;
>  	if (!connector->has_tile)
>  		goto out_drop_ref;
>  	return;
> +
>  out_drop_ref:
>  	if (connector->tile_group) {
>  		drm_mode_put_tile_group(connector->dev, connector->tile_group);
>  		connector->tile_group = NULL;
>  	}
> -	return;
>  }
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux