Re: [PATCH] drm/edid: Use struct cea_db when parsing HDMI VSDB

["Joshua Peisach" <jpeisach@ubuntu.com>]

On Tue Jan 20, 2026 at 3:39 PM EST, Ville Syrjälä wrote:
> On Tue, Jan 20, 2026 at 03:01:14PM -0500, jpeisach@ubuntu.com wrote:
>> 
>> 
>> > On Jan 20, 2026, at 1:09 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > 
>> > On Tue, Jan 20, 2026 at 12:46:59PM -0500, jpeisach@ubuntu.com <mailto:jpeisach@ubuntu.com> wrote:
>> >> 
>> >> 
>> >>> On Jan 20, 2026, at 12:03 PM, Ville Syrjälä <ville.syrjala@linux.intel.com <mailto:ville.syrjala@linux.intel.com>> wrote:
>> >>> 
>> >>> On Tue, Jan 20, 2026 at 11:37:59AM -0500, jpeisach@ubuntu.com <mailto:jpeisach@ubuntu.com> <mailto:jpeisach@ubuntu.com> wrote:
>> >>>> 
>> >>>> 
>> >>>>> On Jan 20, 2026, at 10:41 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> >>>>> 
>> >>>>> On Mon, Jan 19, 2026 at 03:39:12PM +0200, Jani Nikula wrote:
>> >>>>>> On Sat, 17 Jan 2026, Joshua Peisach <jpeisach@ubuntu.com> wrote:
>> >>>>>>> drm_parse_hdmi_vsdb_video is one of the parsers that still do not use the
>> >>>>>>> cea_db struct, and currently passes a u8 pointer.
>> >>>>>>> 
>> >>>>>>> Set the correct struct type and update references to the data accordingly.
>> >>>>>>> This also makes the same change to drm_parse_hdmi_deep_color_info as
>> >>>>>>> necessary.
>> >>>>>>> 
>> >>>>>>> Signed-off-by: Joshua Peisach <jpeisach@ubuntu.com>
>> >>>>>>> ---
>> >>>>>>> drivers/gpu/drm/drm_edid.c | 26 +++++++++++++-------------
>> >>>>>>> 1 file changed, 13 insertions(+), 13 deletions(-)
>> >>>>>>> 
>> >>>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> >>>>>>> index 26bb7710a..15bd99e65 100644
>> >>>>>>> --- a/drivers/gpu/drm/drm_edid.c
>> >>>>>>> +++ b/drivers/gpu/drm/drm_edid.c
>> >>>>>>> @@ -6290,7 +6290,7 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>> >>>>>>> }
>> >>>>>>> 
>> >>>>>>> static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>> >>>>>>> -					   const u8 *hdmi)
>> >>>>>>> +					   const struct cea_db *db)
>> >>>>>>> {
>> >>>>>>> 	struct drm_display_info *info = &connector->display_info;
>> >>>>>>> 	unsigned int dc_bpc = 0;
>> >>>>>>> @@ -6298,24 +6298,24 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>> >>>>>>> 	/* HDMI supports at least 8 bpc */
>> >>>>>>> 	info->bpc = 8;
>> >>>>>>> 
>> >>>>>>> -	if (cea_db_payload_len(hdmi) < 6)
>> >>>>>>> +	if (cea_db_payload_len(db) < 6)
>> >>>>>>> 		return;
>> >>>>>>> 
>> >>>>>>> -	if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
>> >>>>>>> +	if (db->data[6] & DRM_EDID_HDMI_DC_30) {
>> >>>>>> 
>> >>>>>> That's not the same thing, but off-by-one now. Ditto everywhere that
>> >>>>>> changes from u8* to db->data[].
>> >>>>>> 
>> >>>>>> The main problem with the change (even with fixed offsets) is that the
>> >>>>>> *specs* typically use indexing from the beginning of the data block, not
>> >>>>>> from the beginning of payload data.
>> >>>>>> 
>> >>>>>> We've discussed this before with Ville (Cc'd) but I'm not sure if we
>> >>>>>> reached a conclusion.
>> >>>>> 
>> >>>>> I guess we could give up on the index matching the spec byte#.
>> >>>>> Looks like the HDMI VSDB parsing is the only place where we
>> >>>>> actually have the two match, and everwhere else it's
>> >>>>> already inconsistent.
>> >>>>> 
>> >>>>> Also maybe we should add something to also exclude the
>> >>>>> extended tag from the payload, for the blocks that use
>> >>>>> the extended tag...
>> >>>>> 
>> >>>>> -- 
>> >>>>> Ville Syrjälä
>> >>>>> Intel
>> >>>> 
>> >>>> I personally believe it is important to match the spec for consistency
>> >>>> and reference. Unless I am looking at the wrong thing, bit 6 should have
>> >>>> the correct index.
>> >>>> 
>> >>>> Also I think cea_db in the context of the function calls here are
>> >>>> just the data blocks. Unless you mean that by having the struct's first
>> >>>> member being the tag_length if offsetting everything, but I don't think
>> >>>> it is? Let me know if I'm wrong :)
>> >>> 
>> >>> The tag+length byte is:
>> >>> byte# 1 in the CTA spec
>> >>> byte# 0 in the HDMI spec
>> >>> 
>> >>> There's no super nice way to use the byte# as the index for both.
>> >>> Also the length checks end up looking somewhat confusing when
>> >>> comparing them with the corresponding index.
>> >>> 
>> >>> -- 
>> >>> Ville Syrjälä
>> >>> Intel
>> >> 
>> >> The name of the functions specifically say HDMI, so I think that's the
>> >> system to use: there are functions that say CTA in the name, like
>> >> parse_cta_y420cmdb - so that is CTA, and these functions follow HDMI.
>> > 
>> > I'm saying that there is no really sane way to deal with the CTA byte#
>> > convention. So I think it's probably best to just go for a single
>> > consistent approach for both CTA and HDMI. That way people at least
>> > won't get confused by the different convetion between the functions.
>> > And the length checks wouldn't look incorrect.
>> > 
>> 
>> I agree. I can't think of anything though, other than to assume CTA.
>> 
>> The other parsers that already use struct cea_db refer to the data
>> using the CTA spec (starting at 1). Maybe just go with that?
>
> Nothing is indexed using the CTA byte#. To do that we'd have to
> do something like 'u8 *db = start_of_db - 1'.
>
> I can more or less see these variants:
> 1. relative to the start of the data block
>    (eg. drm_parse_hdr_metadata_block(), drm_parse_vcdb())
> 2. relative to the start of the payload
>    (eg. parse_cta_vdb())
> 3. relative to the first byte past the extended tag
>    (y420* stuff)
>
> And I'm suggesting that perhaps everything should use either
> 2 or 3 depending on whether the extended tag is present.

Makes sense to me.

However, as for "drm_parse_hdmi_vsdb_video", that is not specific to
CTA. So I guess keep the function names and just correct the indexes?

--
Josh