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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Sun, 27 Oct 2024, Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@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.)

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?


BR,
Jani.


-- 
Jani Nikula, Intel