| Message ID | 20220222063819.5279-1-shawn.c.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/edid: parse multiple CEA extension block | expand |
On Tue, Feb 22, 2022 at 02:38:17PM +0800, Lee Shawn C wrote: > Try to find and parse more CEA ext blocks if edid->extensions > is greater than one. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> > --- > drivers/gpu/drm/drm_edid.c | 75 +++++++++++++++++++++++--------------- > 1 file changed, 45 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 12893e7be89b..3d5dbbeca7f9 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -4313,43 +4313,58 @@ 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; > u8 dbl, hdmi_len, video_len = 0; > - int modes = 0; > + int modes = 0, j; > > - if (cea && cea_revision(cea) >= 3) { > - int i, start, end; > + if (!cea) > + return 0; > > - if (cea_db_offsets(cea, &start, &end)) > - return 0; > + for (j = (cea - (u8 *)edid) / EDID_LENGTH; j <= edid->extensions;) { That looks rather illegible. I think we want a drm_find_cea_extension(const struct edid *edid, int *ext_index) and then just loop until it stops giving us stuff. There are also several other callers of drm_find_cea_extension(). Why don't they require the same treatment? > + if (cea && cea_revision(cea) >= 3) { > + int i, start, end; > > - for_each_cea_db(cea, i, start, end) { > - db = &cea[i]; > - dbl = cea_db_payload_len(db); > + if (cea_db_offsets(cea, &start, &end)) > + continue; > > - if (cea_db_tag(db) == VIDEO_BLOCK) { > - video = db + 1; > - video_len = dbl; > - modes += do_cea_modes(connector, video, dbl); > - } else if (cea_db_is_hdmi_vsdb(db)) { > - hdmi = db; > - hdmi_len = dbl; > - } else if (cea_db_is_y420vdb(db)) { > - const u8 *vdb420 = &db[2]; > - > - /* Add 4:2:0(only) modes present in EDID */ > - modes += do_y420vdb_modes(connector, > - vdb420, > - dbl - 1); > + for_each_cea_db(cea, i, start, end) { > + db = &cea[i]; > + dbl = cea_db_payload_len(db); > + > + if (cea_db_tag(db) == VIDEO_BLOCK) { > + video = db + 1; > + video_len = dbl; > + modes += do_cea_modes(connector, video, dbl); > + } else if (cea_db_is_hdmi_vsdb(db)) { > + hdmi = db; > + hdmi_len = dbl; > + } else if (cea_db_is_y420vdb(db)) { > + const u8 *vdb420 = &db[2]; > + > + /* Add 4:2:0(only) modes present in EDID */ > + modes += do_y420vdb_modes(connector, > + vdb420, > + dbl - 1); > + } > } > } > - } > > - /* > - * We parse the HDMI VSDB after having added the cea modes as we will > - * be patching their flags when the sink supports stereo 3D. > - */ > - if (hdmi) > - modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, > - video_len); > + /* > + * We parse the HDMI VSDB after having added the cea modes as we will > + * be patching their flags when the sink supports stereo 3D. > + */ > + if (hdmi) { > + modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, > + video_len); > + hdmi = NULL; > + video = NULL; > + hdmi_len = 0; > + video_len = 0; > + } > + > + /* move to next CEA extension block */ > + cea = drm_find_edid_extension(edid, CEA_EXT, &j); > + if (!cea) > + break; > + } > > return modes; > } > -- > 2.17.1
On Tue, Feb 22, 2022 at 03:28:17PM +0800, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >On Tue, Feb 22, 2022 at 02:38:17PM +0800, Lee Shawn C wrote: >> Try to find and parse more CEA ext blocks if edid->extensions is >> greater than one. >> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> >> --- >> drivers/gpu/drm/drm_edid.c | 75 >> +++++++++++++++++++++++--------------- >> 1 file changed, 45 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index 12893e7be89b..3d5dbbeca7f9 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -4313,43 +4313,58 @@ 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; >> u8 dbl, hdmi_len, video_len = 0; >> - int modes = 0; >> + int modes = 0, j; >> >> - if (cea && cea_revision(cea) >= 3) { >> - int i, start, end; >> + if (!cea) >> + return 0; >> >> - if (cea_db_offsets(cea, &start, &end)) >> - return 0; >> + for (j = (cea - (u8 *)edid) / EDID_LENGTH; j <= edid->extensions;) { > >That looks rather illegible. I think we want a drm_find_cea_extension(const struct edid *edid, int *ext_index) and then just loop until it stops giving us stuff. > I will modify drm_find_cea_extension() to find out next CEA from *ext_index. >There are also several other callers of drm_find_cea_extension(). >Why don't they require the same treatment? My suspicion is the original design judge edid->extension would be zero or one. And only one extension block would available. Best regards, Shawn > >> + if (cea && cea_revision(cea) >= 3) { >> + int i, start, end; >> >> - for_each_cea_db(cea, i, start, end) { >> - db = &cea[i]; >> - dbl = cea_db_payload_len(db); >> + if (cea_db_offsets(cea, &start, &end)) >> + continue; >> >> - if (cea_db_tag(db) == VIDEO_BLOCK) { >> - video = db + 1; >> - video_len = dbl; >> - modes += do_cea_modes(connector, video, dbl); >> - } else if (cea_db_is_hdmi_vsdb(db)) { >> - hdmi = db; >> - hdmi_len = dbl; >> - } else if (cea_db_is_y420vdb(db)) { >> - const u8 *vdb420 = &db[2]; >> - >> - /* Add 4:2:0(only) modes present in EDID */ >> - modes += do_y420vdb_modes(connector, >> - vdb420, >> - dbl - 1); >> + for_each_cea_db(cea, i, start, end) { >> + db = &cea[i]; >> + dbl = cea_db_payload_len(db); >> + >> + if (cea_db_tag(db) == VIDEO_BLOCK) { >> + video = db + 1; >> + video_len = dbl; >> + modes += do_cea_modes(connector, video, dbl); >> + } else if (cea_db_is_hdmi_vsdb(db)) { >> + hdmi = db; >> + hdmi_len = dbl; >> + } else if (cea_db_is_y420vdb(db)) { >> + const u8 *vdb420 = &db[2]; >> + >> + /* Add 4:2:0(only) modes present in EDID */ >> + modes += do_y420vdb_modes(connector, >> + vdb420, >> + dbl - 1); >> + } >> } >> } >> - } >> >> - /* >> - * We parse the HDMI VSDB after having added the cea modes as we will >> - * be patching their flags when the sink supports stereo 3D. >> - */ >> - if (hdmi) >> - modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, >> - video_len); >> + /* >> + * We parse the HDMI VSDB after having added the cea modes as we will >> + * be patching their flags when the sink supports stereo 3D. >> + */ >> + if (hdmi) { >> + modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, >> + video_len); >> + hdmi = NULL; >> + video = NULL; >> + hdmi_len = 0; >> + video_len = 0; >> + } >> + >> + /* move to next CEA extension block */ >> + cea = drm_find_edid_extension(edid, CEA_EXT, &j); >> + if (!cea) >> + break; >> + } >> >> return modes; >> } >> -- >> 2.17.1 > >-- >Ville Syrjälä >Intel >
On Tue, 22 Feb 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Feb 22, 2022 at 02:38:17PM +0800, Lee Shawn C wrote: >> Try to find and parse more CEA ext blocks if edid->extensions >> is greater than one. >> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> >> --- >> drivers/gpu/drm/drm_edid.c | 75 +++++++++++++++++++++++--------------- >> 1 file changed, 45 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index 12893e7be89b..3d5dbbeca7f9 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -4313,43 +4313,58 @@ 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; >> u8 dbl, hdmi_len, video_len = 0; >> - int modes = 0; >> + int modes = 0, j; >> >> - if (cea && cea_revision(cea) >= 3) { >> - int i, start, end; >> + if (!cea) >> + return 0; >> >> - if (cea_db_offsets(cea, &start, &end)) >> - return 0; >> + for (j = (cea - (u8 *)edid) / EDID_LENGTH; j <= edid->extensions;) { > > That looks rather illegible. I think we want a > drm_find_cea_extension(const struct edid *edid, int *ext_index) > and then just loop until it stops giving us stuff. Neither approach takes multiple CEA blocks within DisplayID extension into account. Or some blocks outside and some inside DisplayID extension. 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. BR, Jani. > > There are also several other callers of drm_find_cea_extension(). > Why don't they require the same treatment? > >> + if (cea && cea_revision(cea) >= 3) { >> + int i, start, end; >> >> - for_each_cea_db(cea, i, start, end) { >> - db = &cea[i]; >> - dbl = cea_db_payload_len(db); >> + if (cea_db_offsets(cea, &start, &end)) >> + continue; >> >> - if (cea_db_tag(db) == VIDEO_BLOCK) { >> - video = db + 1; >> - video_len = dbl; >> - modes += do_cea_modes(connector, video, dbl); >> - } else if (cea_db_is_hdmi_vsdb(db)) { >> - hdmi = db; >> - hdmi_len = dbl; >> - } else if (cea_db_is_y420vdb(db)) { >> - const u8 *vdb420 = &db[2]; >> - >> - /* Add 4:2:0(only) modes present in EDID */ >> - modes += do_y420vdb_modes(connector, >> - vdb420, >> - dbl - 1); >> + for_each_cea_db(cea, i, start, end) { >> + db = &cea[i]; >> + dbl = cea_db_payload_len(db); >> + >> + if (cea_db_tag(db) == VIDEO_BLOCK) { >> + video = db + 1; >> + video_len = dbl; >> + modes += do_cea_modes(connector, video, dbl); >> + } else if (cea_db_is_hdmi_vsdb(db)) { >> + hdmi = db; >> + hdmi_len = dbl; >> + } else if (cea_db_is_y420vdb(db)) { >> + const u8 *vdb420 = &db[2]; >> + >> + /* Add 4:2:0(only) modes present in EDID */ >> + modes += do_y420vdb_modes(connector, >> + vdb420, >> + dbl - 1); >> + } >> } >> } >> - } >> >> - /* >> - * We parse the HDMI VSDB after having added the cea modes as we will >> - * be patching their flags when the sink supports stereo 3D. >> - */ >> - if (hdmi) >> - modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, >> - video_len); >> + /* >> + * We parse the HDMI VSDB after having added the cea modes as we will >> + * be patching their flags when the sink supports stereo 3D. >> + */ >> + if (hdmi) { >> + modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, >> + video_len); >> + hdmi = NULL; >> + video = NULL; >> + hdmi_len = 0; >> + video_len = 0; >> + } >> + >> + /* move to next CEA extension block */ >> + cea = drm_find_edid_extension(edid, CEA_EXT, &j); >> + if (!cea) >> + break; >> + } >> >> return modes; >> } >> -- >> 2.17.1
On Tue, Feb 22, 2022 at 11:19:15AM +0200, Jani Nikula wrote: > On Tue, 22 Feb 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Tue, Feb 22, 2022 at 02:38:17PM +0800, Lee Shawn C wrote: > >> Try to find and parse more CEA ext blocks if edid->extensions > >> is greater than one. > >> > >> Cc: Jani Nikula <jani.nikula@linux.intel.com> > >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > >> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> > >> --- > >> drivers/gpu/drm/drm_edid.c | 75 +++++++++++++++++++++++--------------- > >> 1 file changed, 45 insertions(+), 30 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > >> index 12893e7be89b..3d5dbbeca7f9 100644 > >> --- a/drivers/gpu/drm/drm_edid.c > >> +++ b/drivers/gpu/drm/drm_edid.c > >> @@ -4313,43 +4313,58 @@ 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; > >> u8 dbl, hdmi_len, video_len = 0; > >> - int modes = 0; > >> + int modes = 0, j; > >> > >> - if (cea && cea_revision(cea) >= 3) { > >> - int i, start, end; > >> + if (!cea) > >> + return 0; > >> > >> - if (cea_db_offsets(cea, &start, &end)) > >> - return 0; > >> + for (j = (cea - (u8 *)edid) / EDID_LENGTH; j <= edid->extensions;) { > > > > That looks rather illegible. I think we want a > > drm_find_cea_extension(const struct edid *edid, int *ext_index) > > and then just loop until it stops giving us stuff. > > Neither approach takes multiple CEA blocks within DisplayID extension > into account. Or some blocks outside and some inside DisplayID > extension. > > 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. Aye. We need so many layers of iteration in various places that the whole thing is starting to resemble a Russian doll. Following a common form should probably make that a lot more manageable. I've been already thinking about introducing an iterator for the cea db stuff. But the EDID ext blocks is definitely another target we need to look at. And someone is going to have to figure out what are all the ways these need to nest. I suppose the high level code should only have to care about the deepest layer of stuff and the iterators should take care to iterate through all the potential containers? Eg. if the high level code wants to look at cea dbs then it just iterates those and shouldn't have to care at all where they are stored.
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 12893e7be89b..3d5dbbeca7f9 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4313,43 +4313,58 @@ 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; u8 dbl, hdmi_len, video_len = 0; - int modes = 0; + int modes = 0, j; - if (cea && cea_revision(cea) >= 3) { - int i, start, end; + if (!cea) + return 0; - if (cea_db_offsets(cea, &start, &end)) - return 0; + for (j = (cea - (u8 *)edid) / EDID_LENGTH; j <= edid->extensions;) { + if (cea && cea_revision(cea) >= 3) { + int i, start, end; - for_each_cea_db(cea, i, start, end) { - db = &cea[i]; - dbl = cea_db_payload_len(db); + if (cea_db_offsets(cea, &start, &end)) + continue; - if (cea_db_tag(db) == VIDEO_BLOCK) { - video = db + 1; - video_len = dbl; - modes += do_cea_modes(connector, video, dbl); - } else if (cea_db_is_hdmi_vsdb(db)) { - hdmi = db; - hdmi_len = dbl; - } else if (cea_db_is_y420vdb(db)) { - const u8 *vdb420 = &db[2]; - - /* Add 4:2:0(only) modes present in EDID */ - modes += do_y420vdb_modes(connector, - vdb420, - dbl - 1); + for_each_cea_db(cea, i, start, end) { + db = &cea[i]; + dbl = cea_db_payload_len(db); + + if (cea_db_tag(db) == VIDEO_BLOCK) { + video = db + 1; + video_len = dbl; + modes += do_cea_modes(connector, video, dbl); + } else if (cea_db_is_hdmi_vsdb(db)) { + hdmi = db; + hdmi_len = dbl; + } else if (cea_db_is_y420vdb(db)) { + const u8 *vdb420 = &db[2]; + + /* Add 4:2:0(only) modes present in EDID */ + modes += do_y420vdb_modes(connector, + vdb420, + dbl - 1); + } } } - } - /* - * We parse the HDMI VSDB after having added the cea modes as we will - * be patching their flags when the sink supports stereo 3D. - */ - if (hdmi) - modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, - video_len); + /* + * We parse the HDMI VSDB after having added the cea modes as we will + * be patching their flags when the sink supports stereo 3D. + */ + if (hdmi) { + modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, + video_len); + hdmi = NULL; + video = NULL; + hdmi_len = 0; + video_len = 0; + } + + /* move to next CEA extension block */ + cea = drm_find_edid_extension(edid, CEA_EXT, &j); + if (!cea) + break; + } return modes; }
Try to find and parse more CEA ext blocks if edid->extensions is greater than one. Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> --- drivers/gpu/drm/drm_edid.c | 75 +++++++++++++++++++++++--------------- 1 file changed, 45 insertions(+), 30 deletions(-)