[PATCH 1/5] drm/edid: convert drm_parse_hdmi_vsdb_video to use struct cea_db *

[Vamsi Krishna Brahmajosyula]

Thanks for the feedback.

On Mon, Oct 28, 2024 at 8:09 PM Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
>
> On Mon, Oct 28, 2024 at 03:45:07PM +0200, Jani Nikula wrote:
> > On Sun, 27 Oct 2024, Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula at gmail.com> wrote:
> > > @@ -6320,19 +6321,20 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> > >
> > >  /* HDMI Vendor-Specific Data Block (HDMI VSDB, H14b-VSDB) */
> > >  static void
> > > -drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
> > > +drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const struct cea_db *db)
> > >  {
> > >     struct drm_display_info *info = &connector->display_info;
> > >     u8 len = cea_db_payload_len(db);
> > > +   const u8 *data = cea_db_data(db);
> > >
> > >     info->is_hdmi = true;
> > >
> > > -   info->source_physical_address = (db[4] << 8) | db[5];
> > > +   info->source_physical_address = (data[3] << 8) | data[4];
> > >
> > >     if (len >= 6)
> > > -           info->dvi_dual = db[6] & 1;
> > > +           info->dvi_dual = data[5] & 1;
> >
> > Just commenting on one hunk, because it's a good example of the whole
> > series I think.
> >
> > The above is nice, because it improves the offset vs. length
> > comparisons. Many of the old checks like above look like off-by-ones,
> > when indexing from the beginning of the data block, not from the
> > beginning of payload, and cea_db_payload_len() excludes the first byte.
> >
> > The main problem is that the specs are written with indexing from the
> > beginning of the data block. For example, HDMI 1.4 table 8-16 defining
> > the HDMI VSDB says source physical address is at byte offsets 4 and 5,
> > and dvi dual flag at byte offset 6. That will no longer be the case in
> > code. It gets tricky to review when you have to keep adjusting the
> > offsets in your head. (I don't remember if there are specs that specify
> > the offsets starting from the "actual" payload after all the meta stuff
> > has been removed.)
>
> IIRC there was some off-by-one indexing difference between
> some of the specs. But I don't remember which ones use what.
>
> >
> > Now, if we accept having to do that mental acrobatics, why stop there?
> > You also have extended tags (first payload byte is the tag), as well as
> > vendor tags (first three payload bytes are the OUI). It begs the
> > question whether there should be higher level data and length helpers
> > that identify and remove the tags (including extended tags and OUI
> > stuff). For example, the actual data for HDMI VSDB starts at payload
> > offset 3, as the first three bytes are the HDMI OUI.
> >
> > What to do? Ville, thoughts?
>
> So just different *_{data,len}() for the different indexing variants
> (as defined by the relevant spec)? That seems like a reasonable
> apporach as then the len vs. index checks might actually make sense.
>
Please let me know if the below snippet matches the feedback.

const u8 *vsdb_data = cea_db_to_vsdb_data(db); // skips indexes for HDMI OUI
...
info->source_physical_address = (vsdb_data[0] << 8) | vsdb_data[1];
...
info->dvi_dual = vsdb_data[2] & 1;
> --
> Ville Syrjälä
> Intel

Thanks,
Vamsi