[PATCH 1/3] drm/edid: Allow looking for ext blocks starting from a specified index

[PATCH 1/3] drm/edid: Allow looking for ext blocks starting from a specified index

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apparently EDIDs with multiple DispID ext blocks is a thing, so prepare
for iterating through multiple ext blocks of the same type by
passing the starting ext block index to drm_find_edid_extension(). Well
also have drm_find_edid_extension() update the index to point to the
next ext block on success. Thus we should be able to call
drm_find_edid_extension() in loop.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d8372d63851b..f2531d51dfa2 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3188,7 +3188,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
 /*
  * Search EDID for CEA extension block.
  */
-static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
+static u8 *drm_find_edid_extension(const struct edid *edid,
+				   int ext_id, int *ext_index)
 {
 	u8 *edid_ext = NULL;
 	int i;
@@ -3198,23 +3199,26 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
 		return NULL;
 
 	/* Find CEA extension */
-	for (i = 0; i < edid->extensions; i++) {
+	for (i = *ext_index; i < edid->extensions; i++) {
 		edid_ext = (u8 *)edid + EDID_LENGTH * (i + 1);
 		if (edid_ext[0] == ext_id)
 			break;
 	}
 
-	if (i == edid->extensions)
+	if (i >= edid->extensions)
 		return NULL;
 
+	*ext_index = i + 1;
+
 	return edid_ext;
 }
 
 
 static u8 *drm_find_displayid_extension(const struct edid *edid,
-					int *length, int *idx)
+					int *length, int *idx,
+					int *ext_index)
 {
-	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT);
+	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
 	struct displayid_hdr *base;
 	int ret;
 
@@ -3241,14 +3245,18 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
 	struct displayid_block *block;
 	u8 *cea;
 	u8 *displayid;
+	int ext_index;
 
 	/* Look for a top level CEA extension block */
-	cea = drm_find_edid_extension(edid, CEA_EXT);
+	ext_index = 0;
+	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
 	if (cea)
 		return cea;
 
 	/* CEA blocks can also be found embedded in a DisplayID block */
-	displayid = drm_find_displayid_extension(edid, &length, &idx);
+	ext_index = 0;
+	displayid = drm_find_displayid_extension(edid, &length, &idx,
+						 &ext_index);
 	if (!displayid)
 		return NULL;
 
@@ -5195,8 +5203,10 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
 	int length, idx;
 	struct displayid_block *block;
 	int num_modes = 0;
+	int ext_index = 0;
 
-	displayid = drm_find_displayid_extension(edid, &length, &idx);
+	displayid = drm_find_displayid_extension(edid, &length, &idx,
+						 &ext_index);
 	if (!displayid)
 		return 0;
 
@@ -5870,11 +5880,13 @@ void drm_update_tile_info(struct drm_connector *connector,
 			  const struct edid *edid)
 {
 	const void *displayid = NULL;
+	int ext_index = 0;
 	int length, idx;
 	int ret;
 
 	connector->has_tile = false;
-	displayid = drm_find_displayid_extension(edid, &length, &idx);
+	displayid = drm_find_displayid_extension(edid, &length, &idx,
+						 &ext_index);
 	if (!displayid) {
 		/* drop reference to any tile group we had */
 		goto out_drop_ref;
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/edid: Iterate through all DispID ext blocks

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apparently there are EDIDs in the wild with multiple DispID extension
blocks. Iterate through them all.

In one particular case the tile information is specicied in the
second DispID ext block, and since the current parser only looks
at the first DispID ext block we don't notice that we're dealing
with a tiled display.

While at it change a few functions to return void since we have
no use for the errno.

References: https://gitlab.freedesktop.org/drm/intel/-/issues/27
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 84 +++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index f2531d51dfa2..dcb23563d29d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3248,6 +3248,7 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
 	int ext_index;
 
 	/* Look for a top level CEA extension block */
+	/* FIXME: make callers iterate through multiple CEA ext blocks? */
 	ext_index = 0;
 	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
 	if (cea)
@@ -3255,20 +3256,20 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
 
 	/* CEA blocks can also be found embedded in a DisplayID block */
 	ext_index = 0;
-	displayid = drm_find_displayid_extension(edid, &length, &idx,
-						 &ext_index);
-	if (!displayid)
-		return NULL;
+	for (;;) {
+		displayid = drm_find_displayid_extension(edid, &length, &idx,
+							 &ext_index);
+		if (!displayid)
+			return NULL;
 
-	idx += sizeof(struct displayid_hdr);
-	for_each_displayid_db(displayid, block, idx, length) {
-		if (block->tag == DATA_BLOCK_CTA) {
-			cea = (u8 *)block;
-			break;
+		idx += sizeof(struct displayid_hdr);
+		for_each_displayid_db(displayid, block, idx, length) {
+			if (block->tag == DATA_BLOCK_CTA)
+				return (u8 *)block;
 		}
 	}
 
-	return cea;
+	return NULL;
 }
 
 static __always_inline const struct drm_display_mode *cea_mode_for_vic(u8 vic)
@@ -5205,19 +5206,22 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
 	int num_modes = 0;
 	int ext_index = 0;
 
-	displayid = drm_find_displayid_extension(edid, &length, &idx,
-						 &ext_index);
-	if (!displayid)
-		return 0;
-
-	idx += sizeof(struct displayid_hdr);
-	for_each_displayid_db(displayid, block, idx, length) {
-		switch (block->tag) {
-		case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
-			num_modes += add_displayid_detailed_1_modes(connector, block);
+	for (;;) {
+		displayid = drm_find_displayid_extension(edid, &length, &idx,
+							 &ext_index);
+		if (!displayid)
 			break;
+
+		idx += sizeof(struct displayid_hdr);
+		for_each_displayid_db(displayid, block, idx, length) {
+			switch (block->tag) {
+			case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
+				num_modes += add_displayid_detailed_1_modes(connector, block);
+				break;
+			}
 		}
 	}
+
 	return num_modes;
 }
 
@@ -5797,8 +5801,8 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
 }
 EXPORT_SYMBOL(drm_hdmi_vendor_infoframe_from_display_mode);
 
-static int drm_parse_tiled_block(struct drm_connector *connector,
-				 const struct displayid_block *block)
+static void drm_parse_tiled_block(struct drm_connector *connector,
+				  const struct displayid_block *block)
 {
 	const struct displayid_tiled_block *tile = (struct displayid_tiled_block *)block;
 	u16 w, h;
@@ -5836,7 +5840,7 @@ static int drm_parse_tiled_block(struct drm_connector *connector,
 		tg = drm_mode_create_tile_group(connector->dev, tile->topology_id);
 	}
 	if (!tg)
-		return -ENOMEM;
+		return;
 
 	if (connector->tile_group != tg) {
 		/* if we haven't got a pointer,
@@ -5848,14 +5852,12 @@ static int drm_parse_tiled_block(struct drm_connector *connector,
 	} else
 		/* if same tile group, then release the ref we just took. */
 		drm_mode_put_tile_group(connector->dev, tg);
-	return 0;
 }
 
-static int drm_displayid_parse_tiled(struct drm_connector *connector,
-				     const u8 *displayid, int length, int idx)
+static void drm_displayid_parse_tiled(struct drm_connector *connector,
+				      const u8 *displayid, int length, int idx)
 {
 	const struct displayid_block *block;
-	int ret;
 
 	idx += sizeof(struct displayid_hdr);
 	for_each_displayid_db(displayid, block, idx, length) {
@@ -5864,16 +5866,13 @@ static int drm_displayid_parse_tiled(struct drm_connector *connector,
 
 		switch (block->tag) {
 		case DATA_BLOCK_TILED_DISPLAY:
-			ret = drm_parse_tiled_block(connector, block);
-			if (ret)
-				return ret;
+			drm_parse_tiled_block(connector, block);
 			break;
 		default:
 			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
 			break;
 		}
 	}
-	return 0;
 }
 
 void drm_update_tile_info(struct drm_connector *connector,
@@ -5882,26 +5881,19 @@ void drm_update_tile_info(struct drm_connector *connector,
 	const void *displayid = NULL;
 	int ext_index = 0;
 	int length, idx;
-	int ret;
 
 	connector->has_tile = false;
-	displayid = drm_find_displayid_extension(edid, &length, &idx,
-						 &ext_index);
-	if (!displayid) {
-		/* drop reference to any tile group we had */
-		goto out_drop_ref;
+	for (;;) {
+		displayid = drm_find_displayid_extension(edid, &length, &idx,
+							 &ext_index);
+		if (!displayid)
+			break;
+
+		drm_displayid_parse_tiled(connector, displayid, length, idx);
 	}
 
-	ret = drm_displayid_parse_tiled(connector, displayid, length, idx);
-	if (ret < 0)
-		goto out_drop_ref;
-	if (!connector->has_tile)
-		goto out_drop_ref;
-	return;
-out_drop_ref:
-	if (connector->tile_group) {
+	if (!connector->has_tile && connector->tile_group) {
 		drm_mode_put_tile_group(connector->dev, connector->tile_group);
 		connector->tile_group = NULL;
 	}
-	return;
 }
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/edid: Clean up some curly braces

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Drop some pointless curly braces, and add some across the
else when the if has them too.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index dcb23563d29d..8a951e2bfb41 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5836,22 +5836,21 @@ static void drm_parse_tiled_block(struct drm_connector *connector,
 	DRM_DEBUG_KMS("vend %c%c%c\n", tile->topology_id[0], tile->topology_id[1], tile->topology_id[2]);
 
 	tg = drm_mode_get_tile_group(connector->dev, tile->topology_id);
-	if (!tg) {
+	if (!tg)
 		tg = drm_mode_create_tile_group(connector->dev, tile->topology_id);
-	}
 	if (!tg)
 		return;
 
 	if (connector->tile_group != tg) {
 		/* if we haven't got a pointer,
 		   take the reference, drop ref to old tile group */
-		if (connector->tile_group) {
+		if (connector->tile_group)
 			drm_mode_put_tile_group(connector->dev, connector->tile_group);
-		}
 		connector->tile_group = tg;
-	} else
+	} else {
 		/* if same tile group, then release the ref we just took. */
 		drm_mode_put_tile_group(connector->dev, tg);
+	}
 }
 
 static void drm_displayid_parse_tiled(struct drm_connector *connector,
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/3] drm/edid: Allow looking for ext blocks starting from a specified index

[Souza, Jose]

On Wed, 2020-05-27 at 16:03 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Apparently EDIDs with multiple DispID ext blocks is a thing, so prepare
> for iterating through multiple ext blocks of the same type by
> passing the starting ext block index to drm_find_edid_extension(). Well
> also have drm_find_edid_extension() update the index to point to the
> next ext block on success. Thus we should be able to call
> drm_find_edid_extension() in loop.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index d8372d63851b..f2531d51dfa2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3188,7 +3188,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>  /*
>   * Search EDID for CEA extension block.
>   */
> -static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
> +static u8 *drm_find_edid_extension(const struct edid *edid,
> +				   int ext_id, int *ext_index)
>  {
>  	u8 *edid_ext = NULL;
>  	int i;
> @@ -3198,23 +3199,26 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
>  		return NULL;
>  
>  	/* Find CEA extension */
> -	for (i = 0; i < edid->extensions; i++) {
> +	for (i = *ext_index; i < edid->extensions; i++) {
>  		edid_ext = (u8 *)edid + EDID_LENGTH * (i + 1);
>  		if (edid_ext[0] == ext_id)
>  			break;
>  	}
>  
> -	if (i == edid->extensions)
> +	if (i >= edid->extensions)
>  		return NULL;
>  
> +	*ext_index = i + 1;
> +
>  	return edid_ext;
>  }
>  

I would add something like drm_find_edid_n_extension() with the implementation above and then implement drm_find_edid_extension() calling
drm_find_edid_n_extension() but it is just one caller that is not using ext_index so LGTM.

>  
>  static u8 *drm_find_displayid_extension(const struct edid *edid,
> -					int *length, int *idx)
> +					int *length, int *idx,
> +					int *ext_index)
>  {
> -	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT);
> +	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
>  	struct displayid_hdr *base;
>  	int ret;
>  
> @@ -3241,14 +3245,18 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
>  	struct displayid_block *block;
>  	u8 *cea;
>  	u8 *displayid;
> +	int ext_index;
>  
>  	/* Look for a top level CEA extension block */
> -	cea = drm_find_edid_extension(edid, CEA_EXT);
> +	ext_index = 0;

In 2 places ext_index is initialized in the variable declaration and in 2 other places is not, all of it could be done in the declaration or if you
really want to leave the context close to the users, initialize it in the "for (;;)" in the next patch.

With the change above:

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> +	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
>  	if (cea)
>  		return cea;
>  
>  	/* CEA blocks can also be found embedded in a DisplayID block */
> -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> +	ext_index = 0;
> +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> +						 &ext_index);
>  	if (!displayid)
>  		return NULL;
>  
> @@ -5195,8 +5203,10 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
>  	int length, idx;
>  	struct displayid_block *block;
>  	int num_modes = 0;
> +	int ext_index = 0;
>  
> -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> +						 &ext_index);
>  	if (!displayid)
>  		return 0;
>  
> @@ -5870,11 +5880,13 @@ void drm_update_tile_info(struct drm_connector *connector,
>  			  const struct edid *edid)
>  {
>  	const void *displayid = NULL;
> +	int ext_index = 0;
>  	int length, idx;
>  	int ret;
>  
>  	connector->has_tile = false;
> -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> +						 &ext_index);
>  	if (!displayid) {
>  		/* drop reference to any tile group we had */
>  		goto out_drop_ref;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/3] drm/edid: Iterate through all DispID ext blocks

[Souza, Jose]

On Wed, 2020-05-27 at 16:03 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Apparently there are EDIDs in the wild with multiple DispID extension
> blocks. Iterate through them all.
> 
> In one particular case the tile information is specicied in the
> second DispID ext block, and since the current parser only looks
> at the first DispID ext block we don't notice that we're dealing
> with a tiled display.
> 
> While at it change a few functions to return void since we have
> no use for the errno.

With the change in the previous patch:
Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/27
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 84 +++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index f2531d51dfa2..dcb23563d29d 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3248,6 +3248,7 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
>  	int ext_index;
>  
>  	/* Look for a top level CEA extension block */
> +	/* FIXME: make callers iterate through multiple CEA ext blocks? */
>  	ext_index = 0;
>  	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
>  	if (cea)
> @@ -3255,20 +3256,20 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
>  
>  	/* CEA blocks can also be found embedded in a DisplayID block */
>  	ext_index = 0;
> -	displayid = drm_find_displayid_extension(edid, &length, &idx,
> -						 &ext_index);
> -	if (!displayid)
> -		return NULL;
> +	for (;;) {
> +		displayid = drm_find_displayid_extension(edid, &length, &idx,
> +							 &ext_index);
> +		if (!displayid)
> +			return NULL;
>  
> -	idx += sizeof(struct displayid_hdr);
> -	for_each_displayid_db(displayid, block, idx, length) {
> -		if (block->tag == DATA_BLOCK_CTA) {
> -			cea = (u8 *)block;
> -			break;
> +		idx += sizeof(struct displayid_hdr);
> +		for_each_displayid_db(displayid, block, idx, length) {
> +			if (block->tag == DATA_BLOCK_CTA)
> +				return (u8 *)block;
>  		}
>  	}
>  
> -	return cea;
> +	return NULL;
>  }
>  
>  static __always_inline const struct drm_display_mode *cea_mode_for_vic(u8 vic)
> @@ -5205,19 +5206,22 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
>  	int num_modes = 0;
>  	int ext_index = 0;
>  
> -	displayid = drm_find_displayid_extension(edid, &length, &idx,
> -						 &ext_index);
> -	if (!displayid)
> -		return 0;
> -
> -	idx += sizeof(struct displayid_hdr);
> -	for_each_displayid_db(displayid, block, idx, length) {
> -		switch (block->tag) {
> -		case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
> -			num_modes += add_displayid_detailed_1_modes(connector, block);
> +	for (;;) {
> +		displayid = drm_find_displayid_extension(edid, &length, &idx,
> +							 &ext_index);
> +		if (!displayid)
>  			break;
> +
> +		idx += sizeof(struct displayid_hdr);
> +		for_each_displayid_db(displayid, block, idx, length) {
> +			switch (block->tag) {
> +			case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
> +				num_modes += add_displayid_detailed_1_modes(connector, block);
> +				break;
> +			}
>  		}
>  	}
> +
>  	return num_modes;
>  }
>  
> @@ -5797,8 +5801,8 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>  }
>  EXPORT_SYMBOL(drm_hdmi_vendor_infoframe_from_display_mode);
>  
> -static int drm_parse_tiled_block(struct drm_connector *connector,
> -				 const struct displayid_block *block)
> +static void drm_parse_tiled_block(struct drm_connector *connector,
> +				  const struct displayid_block *block)
>  {
>  	const struct displayid_tiled_block *tile = (struct displayid_tiled_block *)block;
>  	u16 w, h;
> @@ -5836,7 +5840,7 @@ static int drm_parse_tiled_block(struct drm_connector *connector,
>  		tg = drm_mode_create_tile_group(connector->dev, tile->topology_id);
>  	}
>  	if (!tg)
> -		return -ENOMEM;
> +		return;
>  
>  	if (connector->tile_group != tg) {
>  		/* if we haven't got a pointer,
> @@ -5848,14 +5852,12 @@ static int drm_parse_tiled_block(struct drm_connector *connector,
>  	} else
>  		/* if same tile group, then release the ref we just took. */
>  		drm_mode_put_tile_group(connector->dev, tg);
> -	return 0;
>  }
>  
> -static int drm_displayid_parse_tiled(struct drm_connector *connector,
> -				     const u8 *displayid, int length, int idx)
> +static void drm_displayid_parse_tiled(struct drm_connector *connector,
> +				      const u8 *displayid, int length, int idx)
>  {
>  	const struct displayid_block *block;
> -	int ret;
>  
>  	idx += sizeof(struct displayid_hdr);
>  	for_each_displayid_db(displayid, block, idx, length) {
> @@ -5864,16 +5866,13 @@ static int drm_displayid_parse_tiled(struct drm_connector *connector,
>  
>  		switch (block->tag) {
>  		case DATA_BLOCK_TILED_DISPLAY:
> -			ret = drm_parse_tiled_block(connector, block);
> -			if (ret)
> -				return ret;
> +			drm_parse_tiled_block(connector, block);
>  			break;
>  		default:
>  			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
>  			break;
>  		}
>  	}
> -	return 0;
>  }
>  
>  void drm_update_tile_info(struct drm_connector *connector,
> @@ -5882,26 +5881,19 @@ void drm_update_tile_info(struct drm_connector *connector,
>  	const void *displayid = NULL;
>  	int ext_index = 0;
>  	int length, idx;
> -	int ret;
>  
>  	connector->has_tile = false;
> -	displayid = drm_find_displayid_extension(edid, &length, &idx,
> -						 &ext_index);
> -	if (!displayid) {
> -		/* drop reference to any tile group we had */
> -		goto out_drop_ref;
> +	for (;;) {
> +		displayid = drm_find_displayid_extension(edid, &length, &idx,
> +							 &ext_index);
> +		if (!displayid)
> +			break;
> +
> +		drm_displayid_parse_tiled(connector, displayid, length, idx);
>  	}
>  
> -	ret = drm_displayid_parse_tiled(connector, displayid, length, idx);
> -	if (ret < 0)
> -		goto out_drop_ref;
> -	if (!connector->has_tile)
> -		goto out_drop_ref;
> -	return;
> -out_drop_ref:
> -	if (connector->tile_group) {
> +	if (!connector->has_tile && connector->tile_group) {
>  		drm_mode_put_tile_group(connector->dev, connector->tile_group);
>  		connector->tile_group = NULL;
>  	}
> -	return;
>  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/edid: Clean up some curly braces

[Souza, Jose]

On Wed, 2020-05-27 at 16:03 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Drop some pointless curly braces, and add some across the
> else when the if has them too.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index dcb23563d29d..8a951e2bfb41 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5836,22 +5836,21 @@ static void drm_parse_tiled_block(struct drm_connector *connector,
>  	DRM_DEBUG_KMS("vend %c%c%c\n", tile->topology_id[0], tile->topology_id[1], tile->topology_id[2]);
>  
>  	tg = drm_mode_get_tile_group(connector->dev, tile->topology_id);
> -	if (!tg) {
> +	if (!tg)
>  		tg = drm_mode_create_tile_group(connector->dev, tile->topology_id);
> -	}
>  	if (!tg)
>  		return;
>  
>  	if (connector->tile_group != tg) {
>  		/* if we haven't got a pointer,
>  		   take the reference, drop ref to old tile group */
> -		if (connector->tile_group) {
> +		if (connector->tile_group)
>  			drm_mode_put_tile_group(connector->dev, connector->tile_group);
> -		}
>  		connector->tile_group = tg;
> -	} else
> +	} else {
>  		/* if same tile group, then release the ref we just took. */
>  		drm_mode_put_tile_group(connector->dev, tg);
> +	}
>  }
>  
>  static void drm_displayid_parse_tiled(struct drm_connector *connector,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/3] drm/edid: Allow looking for ext blocks starting from a specified index

[Ville Syrjälä]

On Tue, Jun 30, 2020 at 11:42:36PM +0000, Souza, Jose wrote:
> On Wed, 2020-05-27 at 16:03 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Apparently EDIDs with multiple DispID ext blocks is a thing, so prepare
> > for iterating through multiple ext blocks of the same type by
> > passing the starting ext block index to drm_find_edid_extension(). Well
> > also have drm_find_edid_extension() update the index to point to the
> > next ext block on success. Thus we should be able to call
> > drm_find_edid_extension() in loop.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c | 30 +++++++++++++++++++++---------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index d8372d63851b..f2531d51dfa2 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3188,7 +3188,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
> >  /*
> >   * Search EDID for CEA extension block.
> >   */
> > -static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
> > +static u8 *drm_find_edid_extension(const struct edid *edid,
> > +				   int ext_id, int *ext_index)
> >  {
> >  	u8 *edid_ext = NULL;
> >  	int i;
> > @@ -3198,23 +3199,26 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
> >  		return NULL;
> >  
> >  	/* Find CEA extension */
> > -	for (i = 0; i < edid->extensions; i++) {
> > +	for (i = *ext_index; i < edid->extensions; i++) {
> >  		edid_ext = (u8 *)edid + EDID_LENGTH * (i + 1);
> >  		if (edid_ext[0] == ext_id)
> >  			break;
> >  	}
> >  
> > -	if (i == edid->extensions)
> > +	if (i >= edid->extensions)
> >  		return NULL;
> >  
> > +	*ext_index = i + 1;
> > +
> >  	return edid_ext;
> >  }
> >  
> 
> I would add something like drm_find_edid_n_extension() with the implementation above and then implement drm_find_edid_extension() calling
> drm_find_edid_n_extension() but it is just one caller that is not using ext_index so LGTM.
> 
> >  
> >  static u8 *drm_find_displayid_extension(const struct edid *edid,
> > -					int *length, int *idx)
> > +					int *length, int *idx,
> > +					int *ext_index)
> >  {
> > -	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT);
> > +	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
> >  	struct displayid_hdr *base;
> >  	int ret;
> >  
> > @@ -3241,14 +3245,18 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
> >  	struct displayid_block *block;
> >  	u8 *cea;
> >  	u8 *displayid;
> > +	int ext_index;
> >  
> >  	/* Look for a top level CEA extension block */
> > -	cea = drm_find_edid_extension(edid, CEA_EXT);
> > +	ext_index = 0;
> 
> In 2 places ext_index is initialized in the variable declaration and in 2 other places is not, all of it could be done in the declaration

No, in this case we need to reset it back to 0 when the start looking
for the DispID ext block (as opposed to the CEA ext block). So I figured
it's cleaner if both initialize it to 0 the same way. All the other
places need just the one initialization.

Eventually I think I'd like some kind of for_each_ext_block() to make
this stuff less crap...

> or if you
> really want to leave the context close to the users, initialize it in the "for (;;)" in the next patch.
> 
> With the change above:
> 
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> > +	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
> >  	if (cea)
> >  		return cea;
> >  
> >  	/* CEA blocks can also be found embedded in a DisplayID block */
> > -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> > +	ext_index = 0;
> > +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> > +						 &ext_index);
> >  	if (!displayid)
> >  		return NULL;
> >  
> > @@ -5195,8 +5203,10 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
> >  	int length, idx;
> >  	struct displayid_block *block;
> >  	int num_modes = 0;
> > +	int ext_index = 0;
> >  
> > -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> > +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> > +						 &ext_index);
> >  	if (!displayid)
> >  		return 0;
> >  
> > @@ -5870,11 +5880,13 @@ void drm_update_tile_info(struct drm_connector *connector,
> >  			  const struct edid *edid)
> >  {
> >  	const void *displayid = NULL;
> > +	int ext_index = 0;
> >  	int length, idx;
> >  	int ret;
> >  
> >  	connector->has_tile = false;
> > -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> > +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> > +						 &ext_index);
> >  	if (!displayid) {
> >  		/* drop reference to any tile group we had */
> >  		goto out_drop_ref;

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

Re: [Intel-gfx] [PATCH 1/3] drm/edid: Allow looking for ext blocks starting from a specified index

[Souza, Jose]

On Thu, 2020-07-02 at 17:40 +0300, Ville Syrjälä wrote:
> On Tue, Jun 30, 2020 at 11:42:36PM +0000, Souza, Jose wrote:
> > On Wed, 2020-05-27 at 16:03 +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Apparently EDIDs with multiple DispID ext blocks is a thing, so prepare
> > > for iterating through multiple ext blocks of the same type by
> > > passing the starting ext block index to drm_find_edid_extension(). Well
> > > also have drm_find_edid_extension() update the index to point to the
> > > next ext block on success. Thus we should be able to call
> > > drm_find_edid_extension() in loop.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_edid.c | 30 +++++++++++++++++++++---------
> > >  1 file changed, 21 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index d8372d63851b..f2531d51dfa2 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -3188,7 +3188,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
> > >  /*
> > >   * Search EDID for CEA extension block.
> > >   */
> > > -static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
> > > +static u8 *drm_find_edid_extension(const struct edid *edid,
> > > +				   int ext_id, int *ext_index)
> > >  {
> > >  	u8 *edid_ext = NULL;
> > >  	int i;
> > > @@ -3198,23 +3199,26 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
> > >  		return NULL;
> > >  
> > >  	/* Find CEA extension */
> > > -	for (i = 0; i < edid->extensions; i++) {
> > > +	for (i = *ext_index; i < edid->extensions; i++) {
> > >  		edid_ext = (u8 *)edid + EDID_LENGTH * (i + 1);
> > >  		if (edid_ext[0] == ext_id)
> > >  			break;
> > >  	}
> > >  
> > > -	if (i == edid->extensions)
> > > +	if (i >= edid->extensions)
> > >  		return NULL;
> > >  
> > > +	*ext_index = i + 1;
> > > +
> > >  	return edid_ext;
> > >  }
> > >  
> > 
> > I would add something like drm_find_edid_n_extension() with the implementation above and then implement drm_find_edid_extension() calling
> > drm_find_edid_n_extension() but it is just one caller that is not using ext_index so LGTM.
> > 
> > >  
> > >  static u8 *drm_find_displayid_extension(const struct edid *edid,
> > > -					int *length, int *idx)
> > > +					int *length, int *idx,
> > > +					int *ext_index)
> > >  {
> > > -	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT);
> > > +	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
> > >  	struct displayid_hdr *base;
> > >  	int ret;
> > >  
> > > @@ -3241,14 +3245,18 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
> > >  	struct displayid_block *block;
> > >  	u8 *cea;
> > >  	u8 *displayid;
> > > +	int ext_index;
> > >  
> > >  	/* Look for a top level CEA extension block */
> > > -	cea = drm_find_edid_extension(edid, CEA_EXT);
> > > +	ext_index = 0;
> > 
> > In 2 places ext_index is initialized in the variable declaration and in 2 other places is not, all of it could be done in the declaration
> 
> No, in this case we need to reset it back to 0 when the start looking
> for the DispID ext block (as opposed to the CEA ext block). So I figured
> it's cleaner if both initialize it to 0 the same way. All the other
> places need just the one initialization.
> 
> Eventually I think I'd like some kind of for_each_ext_block() to make
> this stuff less crap...

Okay makes sense.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> > or if you
> > really want to leave the context close to the users, initialize it in the "for (;;)" in the next patch.
> > 
> > With the change above:
> > 
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > > +	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
> > >  	if (cea)
> > >  		return cea;
> > >  
> > >  	/* CEA blocks can also be found embedded in a DisplayID block */
> > > -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> > > +	ext_index = 0;
> > > +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> > > +						 &ext_index);
> > >  	if (!displayid)
> > >  		return NULL;
> > >  
> > > @@ -5195,8 +5203,10 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
> > >  	int length, idx;
> > >  	struct displayid_block *block;
> > >  	int num_modes = 0;
> > > +	int ext_index = 0;
> > >  
> > > -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> > > +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> > > +						 &ext_index);
> > >  	if (!displayid)
> > >  		return 0;
> > >  
> > > @@ -5870,11 +5880,13 @@ void drm_update_tile_info(struct drm_connector *connector,
> > >  			  const struct edid *edid)
> > >  {
> > >  	const void *displayid = NULL;
> > > +	int ext_index = 0;
> > >  	int length, idx;
> > >  	int ret;
> > >  
> > >  	connector->has_tile = false;
> > > -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> > > +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> > > +						 &ext_index);
> > >  	if (!displayid) {
> > >  		/* drop reference to any tile group we had */
> > >  		goto out_drop_ref;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel