On Tue, Mar 15, 2022 at 03:21:05PM +0000, Lee, Shawn C wrote: > On Tuesday, March 15, 2022 8:33 PM, Nikula, Jani <jani.nikula@xxxxxxxxx> wrote: > >On Mon, 14 Mar 2022, Drew Davenport <ddavenport@xxxxxxxxxxxx> wrote: > >> On Mon, Mar 14, 2022 at 10:40:47AM +0200, Jani Nikula wrote: > >>> On Sun, 13 Mar 2022, Lee Shawn C <shawn.c.lee@xxxxxxxxx> wrote: > >>> > drm_find_cea_extension() always look for a top level CEA block. > >>> > Pass ext_index from caller then this function to search next > >>> > available CEA ext block from a specific EDID block pointer. > >>> > > >>> > v2: save proper extension block index if CTA data information > >>> > was found in DispalyID block. > >>> > v3: using different parameters to store CEA and DisplayID block index. > >>> > configure DisplayID extansion block index before search available > >>> > DisplayID block. > >>> > > >>> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > >>> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> > >>> > Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> > >>> > Cc: Drew Davenport <ddavenport@xxxxxxxxxxxx> > >>> > Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx> > >>> > Signed-off-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx> > >>> > --- > >>> > drivers/gpu/drm/drm_displayid.c | 10 +++++-- > >>> > drivers/gpu/drm/drm_edid.c | 53 ++++++++++++++++++--------------- > >>> > include/drm/drm_displayid.h | 4 +-- > >>> > 3 files changed, 39 insertions(+), 28 deletions(-) > >>> > > >>> > diff --git a/drivers/gpu/drm/drm_displayid.c > >>> > b/drivers/gpu/drm/drm_displayid.c index 32da557b960f..31c3e6d7d549 > >>> > 100644 > >>> > --- a/drivers/gpu/drm/drm_displayid.c > >>> > +++ b/drivers/gpu/drm/drm_displayid.c > >>> > @@ -59,11 +59,14 @@ static const u8 > >>> > *drm_find_displayid_extension(const struct edid *edid, } > >>> > > >>> > void displayid_iter_edid_begin(const struct edid *edid, > >>> > - struct displayid_iter *iter) > >>> > + struct displayid_iter *iter, int *ext_index) > >>> > >>> Please don't do this. This just ruins the clean approach displayid > >>> iterator added. > >>> > >>> Instead of making the displayid iterator ugly, and leaking its > >>> abstractions, I'll repeat what I said should be done in reply to the > >>> very first version of this patch series [1]: > >>> > >>> "I think we're going to need abstracted EDID iteration similar to > >>> what I've done for DisplayID iteration. We can't have all places > >>> reimplementing the iteration like we have now." > >>> > >>> This isn't a problem that should be solved by having all the callers > >>> hold a bunch of local variables and pass them around to all the > >>> functions. Nobody's going to be able to keep track of this anymore. > >>> And this series, as it is, makes it harder to fix this properly later on. > >> > >> I missed your original review comment, so apologies for repeating what > >> you said there already. > >> > >> I'd agree that passing a starting index to the displayid_iter_* > >> functions is probably not the right direction here. More thoughts below. > >> > >>> > >>> > >>> BR, > >>> Jani. > >>> > >>> > >>> [1] https://lore.kernel.org/r/87czjf8dik.fsf@xxxxxxxxx > >>> > >>> > >>> > >>> > { > >>> > memset(iter, 0, sizeof(*iter)); > >>> > > >>> > iter->edid = edid; > >>> > + > >>> > + if (ext_index) > >>> > + iter->ext_index = *ext_index; > >>> > } > >>> > > >>> > static const struct displayid_block * @@ -126,7 +129,10 @@ > >>> > __displayid_iter_next(struct displayid_iter *iter) > >>> > } > >>> > } > >>> > > >>> > -void displayid_iter_end(struct displayid_iter *iter) > >>> > +void displayid_iter_end(struct displayid_iter *iter, int > >>> > +*ext_index) > >>> > { > >>> > + if (ext_index) > >>> > + *ext_index = iter->ext_index; > >>> > + > >>> > memset(iter, 0, sizeof(*iter)); > >>> > } > >>> > diff --git a/drivers/gpu/drm/drm_edid.c > >>> > b/drivers/gpu/drm/drm_edid.c index 561f53831e29..78c415aa6889 > >>> > 100644 > >>> > --- a/drivers/gpu/drm/drm_edid.c > >>> > +++ b/drivers/gpu/drm/drm_edid.c > >>> > @@ -3353,28 +3353,27 @@ const u8 *drm_find_edid_extension(const struct edid *edid, > >>> > return edid_ext; > >>> > } > >>> > > >>> > -static const u8 *drm_find_cea_extension(const struct edid *edid) > >>> > +static const u8 *drm_find_cea_extension(const struct edid *edid, > >>> > + int *cea_ext_index, int *displayid_ext_index) > >> > >> As discussed above, passing both indices into this function may not be > >> the best approach here. But I think we need to keep track of some kind > >> of state in order to know which was the last CEA block that was > >> returned, and thus this function can return the next one after that, > >> whether it's in the CEA extension block or DisplayID block. > > > >Per DisplayID v1.3 Appendix B: DisplayID as an EDID Extension, it's recommended that DisplayID extensions are exposed after all of the CEA extensions. > > > >I think it should be fine to first iterate over all CEA extensions across the EDID, followed by iterating over all the DisplayID extensions. No need to keep track of the order between CEA and DisplayID, as the former should precede the latter. > > > > Refer to "DisplayID extensions are exposed after all of the CEA extensions". It looks to me patch v5 is able to support this. And did not touch DisplayID iterator. > https://patchwork.freedesktop.org/series/100690/#rev5 Patch v5 does not iterate over all the DisplayID extensions, and it has the previously mentioned problem about resulting in a possible infinite loop when depending on ext_index to iterate over the CEA extensions. > > >> What do you think of using the pointer returned from this function as > >> that state? The caller could pass in a u8* that was returned from a > >> previous call. This function would iterate through the extension > >> blocks and skip the CEA blocks it finds until it finds the passed in > >> pointer, and then return the next one after (or NULL). > >> > >> An alternative might be to create a linked list of ptrs to the edid > >> extension blocks, and allow a caller to iterate over them in whatever > >> way they need, but I'm not sure how useful that is elsewhere. > > > >I think the main problem here is trying to hack a for loop around > >drm_find_cea_extension() and drm_find_edid_extension(), and duplicating that all over the place, without adding more structure or abstractions. > > > >Contrast with displayid_iter_edid_begin(), displayid_iter_for_each(), and displayid_iter_end() and their usage. They hide all the complexity of looping across DisplayID data blocks inside EDID extensions, and none of the users need to be aware of that complexity. All the state is hidden in struct displayid_iter, and the users don't need to look inside. No allocations, no linked lists. > > > >I think it's particularly bad to have the changes here break the abstractions in displayid_iter_*, especially because they should be usable for pure DisplayID 2.0 *without* an EDID block structure. They'll just need a displayid_iter_begin() for pure DisplayID and some internal changes. > > > >Looking at the usage, the iteration should probably be done at the CEA data block level, something like this: > > > > struct cea_iter iter; > > const struct cea_block *block; > > > > cea_iter_edid_begin(edid, &iter); > > cea_iter_for_each(block, &iter) { > > /* ... */ > > } > > cea_iter_end(&iter); > > > >This would replace cea_db_offsets() and for_each_cea_db(), and would iterate across the all CEA and DisplayID extensions in the entire EDID. > > > >This looks usable, and then you'd start figuring out how to implement that, instead of trying to retrofit all the existing usages to abstractions that don't cut it. You'll probably need an EDID extension iterator too, and then use that and __displayid_iter_next() within > >cea_iter_for_each() i.e. you'd embed the edid_iter and displayid_iter within struct cea_iter. Exhaust the former first, and then the latter. > > > > > >BR, > >Jani. > > > > Hi Jani, Drew, thank you all for your comment! Agree that edid driver may need > abstracted EDID iteration similar to what you did for DisplayID iteration. > It will need a lot of code changes in drm driver. > > But, we have a tight schedule and have to meet HDMI 2.1 compliance test requirement. > Do you think this series can be a short term solution to pass HDMI CTS test? > And we can have another long term plan to work on your opinion to modify > the iterator and make edid driver more efficient and easier to maintain. > > Best regards, > Shawn > > > > >> > >>> > { > >>> > const struct displayid_block *block; > >>> > struct displayid_iter iter; > >>> > const u8 *cea; > >>> > - int ext_index = 0; > >>> > > >>> > - /* Look for a top level CEA extension block */ > >>> > - /* FIXME: make callers iterate through multiple CEA ext blocks? */ > >>> > - cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index); > >>> > + /* Look for a CEA extension block from ext_index */ > >>> > + cea = drm_find_edid_extension(edid, CEA_EXT, cea_ext_index); > >>> > if (cea) > >>> > return cea; > >>> > > >>> > /* CEA blocks can also be found embedded in a DisplayID block */ > >>> > - displayid_iter_edid_begin(edid, &iter); > >>> > + displayid_iter_edid_begin(edid, &iter, displayid_ext_index); > >>> > displayid_iter_for_each(block, &iter) { > >>> > if (block->tag == DATA_BLOCK_CTA) { > >>> > cea = (const u8 *)block; > >>> > break; > >>> > } > >>> > } > >>> > - displayid_iter_end(&iter); > >>> > + displayid_iter_end(&iter, displayid_ext_index); > >>> > > >>> > return cea; > >>> > } > >>> > @@ -3643,10 +3642,10 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) > >>> > struct drm_device *dev = connector->dev; > >>> > struct drm_display_mode *mode, *tmp; > >>> > LIST_HEAD(list); > >>> > - int modes = 0; > >>> > + int modes = 0, cea_ext_index = 0, displayid_ext_index = 0; > >>> > > >>> > /* Don't add CEA modes if the CEA extension block is missing */ > >>> > - if (!drm_find_cea_extension(edid)) > >>> > + if (!drm_find_cea_extension(edid, &cea_ext_index, > >>> > +&displayid_ext_index)) > >>> > return 0; > >>> > > >>> > /* > >>> > @@ -4321,11 +4320,11 @@ static void > >>> > drm_parse_y420cmdb_bitmap(struct drm_connector *connector, static > >>> > int add_cea_modes(struct drm_connector *connector, struct edid > >>> > *edid) { > >>> > - const u8 *cea = drm_find_cea_extension(edid); > >>> > - const u8 *db, *hdmi = NULL, *video = NULL; > >>> > + const u8 *cea, *db, *hdmi = NULL, *video = NULL; > >>> > u8 dbl, hdmi_len, video_len = 0; > >>> > - int modes = 0; > >>> > + int modes = 0, cea_ext_index = 0, displayid_ext_index = 0; > >>> > > >>> > + cea = drm_find_cea_extension(edid, &cea_ext_index, > >>> > +&displayid_ext_index); > >>> > if (cea && cea_revision(cea) >= 3) { > >>> > int i, start, end; > >>> > > >>> > @@ -4563,6 +4562,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) > >>> > const u8 *cea; > >>> > const u8 *db; > >>> > int total_sad_count = 0; > >>> > + int cea_ext_index = 0, displayid_ext_index = 0; > >>> > int mnl; > >>> > int dbl; > >>> > > >>> > @@ -4571,7 +4571,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) > >>> > if (!edid) > >>> > return; > >>> > > >>> > - cea = drm_find_cea_extension(edid); > >>> > + cea = drm_find_cea_extension(edid, &cea_ext_index, > >>> > +&displayid_ext_index); > >>> > if (!cea) { > >>> > DRM_DEBUG_KMS("ELD: no CEA Extension found\n"); > >>> > return; > >>> > @@ -4657,9 +4657,10 @@ int drm_edid_to_sad(struct edid *edid, > >>> > struct cea_sad **sads) { > >>> > int count = 0; > >>> > int i, start, end, dbl; > >>> > + int cea_ext_index = 0, displayid_ext_index = 0; > >>> > const u8 *cea; > >>> > > >>> > - cea = drm_find_cea_extension(edid); > >>> > + cea = drm_find_cea_extension(edid, &cea_ext_index, > >>> > +&displayid_ext_index); > >>> > if (!cea) { > >>> > DRM_DEBUG_KMS("SAD: no CEA Extension found\n"); > >>> > return 0; > >>> > @@ -4719,9 +4720,10 @@ int drm_edid_to_speaker_allocation(struct > >>> > edid *edid, u8 **sadb) { > >>> > int count = 0; > >>> > int i, start, end, dbl; > >>> > + int cea_ext_index = 0, displayid_ext_index = 0; > >>> > const u8 *cea; > >>> > > >>> > - cea = drm_find_cea_extension(edid); > >>> > + cea = drm_find_cea_extension(edid, &cea_ext_index, > >>> > +&displayid_ext_index); > >>> > if (!cea) { > >>> > DRM_DEBUG_KMS("SAD: no CEA Extension found\n"); > >>> > return 0; > >>> > @@ -4815,8 +4817,9 @@ bool drm_detect_hdmi_monitor(struct edid *edid) > >>> > const u8 *edid_ext; > >>> > int i; > >>> > int start_offset, end_offset; > >>> > + int cea_ext_index = 0, displayid_ext_index = 0; > >>> > > >>> > - edid_ext = drm_find_cea_extension(edid); > >>> > + edid_ext = drm_find_cea_extension(edid, &cea_ext_index, > >>> > +&displayid_ext_index); > >>> > if (!edid_ext) > >>> > return false; > >>> > > >>> > @@ -4854,8 +4857,9 @@ bool drm_detect_monitor_audio(struct edid *edid) > >>> > int i, j; > >>> > bool has_audio = false; > >>> > int start_offset, end_offset; > >>> > + int cea_ext_index = 0, displayid_ext_index = 0; > >>> > > >>> > - edid_ext = drm_find_cea_extension(edid); > >>> > + edid_ext = drm_find_cea_extension(edid, &cea_ext_index, > >>> > +&displayid_ext_index); > >>> > if (!edid_ext) > >>> > goto end; > >>> > > >>> > @@ -5178,8 +5182,9 @@ static void drm_parse_cea_ext(struct drm_connector *connector, > >>> > struct drm_display_info *info = &connector->display_info; > >>> > const u8 *edid_ext; > >>> > int i, start, end; > >>> > + int cea_ext_index = 0, displayid_ext_index = 0; > >>> > > >>> > - edid_ext = drm_find_cea_extension(edid); > >>> > + edid_ext = drm_find_cea_extension(edid, &cea_ext_index, > >>> > +&displayid_ext_index); > >>> > if (!edid_ext) > >>> > return; > >>> > > >>> > @@ -5311,12 +5316,12 @@ static void drm_update_mso(struct drm_connector *connector, const struct edid *e > >>> > const struct displayid_block *block; > >>> > struct displayid_iter iter; > >>> > > >>> > - displayid_iter_edid_begin(edid, &iter); > >>> > + displayid_iter_edid_begin(edid, &iter, NULL); > >>> > displayid_iter_for_each(block, &iter) { > >>> > if (block->tag == DATA_BLOCK_2_VENDOR_SPECIFIC) > >>> > drm_parse_vesa_mso_data(connector, block); > >>> > } > >>> > - displayid_iter_end(&iter); > >>> > + displayid_iter_end(&iter, NULL); > >>> > } > >>> > > >>> > /* A connector has no EDID information, so we've got no EDID to > >>> > compute quirks from. Reset @@ -5516,13 +5521,13 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, > >>> > struct displayid_iter iter; > >>> > int num_modes = 0; > >>> > > >>> > - displayid_iter_edid_begin(edid, &iter); > >>> > + displayid_iter_edid_begin(edid, &iter, NULL); > >>> > displayid_iter_for_each(block, &iter) { > >>> > if (block->tag == DATA_BLOCK_TYPE_1_DETAILED_TIMING || > >>> > block->tag == DATA_BLOCK_2_TYPE_7_DETAILED_TIMING) > >>> > num_modes += add_displayid_detailed_1_modes(connector, block); > >>> > } > >>> > - displayid_iter_end(&iter); > >>> > + displayid_iter_end(&iter, NULL); > >>> > > >>> > return num_modes; > >>> > } > >>> > @@ -6164,12 +6169,12 @@ void drm_update_tile_info(struct > >>> > drm_connector *connector, > >>> > > >>> > connector->has_tile = false; > >>> > > >>> > - displayid_iter_edid_begin(edid, &iter); > >>> > + displayid_iter_edid_begin(edid, &iter, NULL); > >>> > displayid_iter_for_each(block, &iter) { > >>> > if (block->tag == DATA_BLOCK_TILED_DISPLAY) > >>> > drm_parse_tiled_block(connector, block); > >>> > } > >>> > - displayid_iter_end(&iter); > >>> > + displayid_iter_end(&iter, NULL); > >>> > > >>> > if (!connector->has_tile && connector->tile_group) { > >>> > drm_mode_put_tile_group(connector->dev, connector->tile_group); > >>> > diff --git a/include/drm/drm_displayid.h > >>> > b/include/drm/drm_displayid.h index 7ffbd9f7bfc7..15442a161c11 > >>> > 100644 > >>> > --- a/include/drm/drm_displayid.h > >>> > +++ b/include/drm/drm_displayid.h > >>> > @@ -150,11 +150,11 @@ struct displayid_iter { }; > >>> > > >>> > void displayid_iter_edid_begin(const struct edid *edid, > >>> > - struct displayid_iter *iter); > >>> > + struct displayid_iter *iter, int *ext_index); > >>> > const struct displayid_block * > >>> > __displayid_iter_next(struct displayid_iter *iter); #define > >>> > displayid_iter_for_each(__block, __iter) \ > >>> > while (((__block) = __displayid_iter_next(__iter))) -void > >>> > displayid_iter_end(struct displayid_iter *iter); > >>> > +void displayid_iter_end(struct displayid_iter *iter, int > >>> > +*ext_index); > >>> > > >>> > #endif > >>> > >>> -- > >>> Jani Nikula, Intel Open Source Graphics Center > > > >-- > >Jani Nikula, Intel Open Source Graphics Center
