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;

}

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,

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;