| Message ID | 20200527130310.27099-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/edid: Allow looking for ext blocks starting from a specified index | expand |
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;
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;
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;
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;