| Message ID | 20241027075108.14273-2-vamsikrishna.brahmajosyula@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/edid: Convert cea_ext parsers to use struct cea_db * | expand |
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.
On Mon, Oct 28, 2024 at 03:45:07PM +0200, Jani Nikula wrote: > 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.) 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.
Thanks for the feedback. On Mon, Oct 28, 2024 at 8:09 PM Ville Syrjälä <ville.syrjala@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@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
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 855beafb76ff..e07e39c0caa0 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -6259,32 +6259,33 @@ 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; + const u8 *hdmi = cea_db_data(db); /* 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 (hdmi[5] & DRM_EDID_HDMI_DC_30) { dc_bpc = 10; info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_30; drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 30.\n", connector->base.id, connector->name); } - if (hdmi[6] & DRM_EDID_HDMI_DC_36) { + if (hdmi[5] & DRM_EDID_HDMI_DC_36) { dc_bpc = 12; info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_36; drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 36.\n", connector->base.id, connector->name); } - if (hdmi[6] & DRM_EDID_HDMI_DC_48) { + if (hdmi[5] & DRM_EDID_HDMI_DC_48) { dc_bpc = 16; info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_48; drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 48.\n", @@ -6302,7 +6303,7 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector, info->bpc = dc_bpc; /* YCRCB444 is optional according to spec. */ - if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) { + if (hdmi[5] & DRM_EDID_HDMI_DC_Y444) { info->edid_hdmi_ycbcr444_dc_modes = info->edid_hdmi_rgb444_dc_modes; drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does YCRCB444 in deep color.\n", connector->base.id, connector->name); @@ -6312,7 +6313,7 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector, * Spec says that if any deep color mode is supported at all, * then deep color 36 bit must be supported. */ - if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) { + if (!(hdmi[5] & DRM_EDID_HDMI_DC_36)) { drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink should do DC_36, but does not!\n", connector->base.id, connector->name); } @@ -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; if (len >= 7) - info->max_tmds_clock = db[7] * 5000; + info->max_tmds_clock = data[6] * 5000; /* * Try to infer whether the sink supports HDMI infoframes. @@ -6340,7 +6342,7 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db) * HDMI infoframe support was first added in HDMI 1.4. Assume the sink * supports infoframes if HDMI_Video_present is set. */ - if (len >= 8 && db[8] & BIT(5)) + if (len >= 8 && data[7] & BIT(5)) info->has_hdmi_infoframe = true; drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI: DVI dual %d, max TMDS clock %d kHz\n", @@ -6412,7 +6414,7 @@ static void drm_parse_cea_ext(struct drm_connector *connector, const u8 *data = (const u8 *)db; if (cea_db_is_hdmi_vsdb(db)) - drm_parse_hdmi_vsdb_video(connector, data); + drm_parse_hdmi_vsdb_video(connector, db); else if (cea_db_is_hdmi_forum_vsdb(db) || cea_db_is_hdmi_forum_scdb(db)) drm_parse_hdmi_forum_scds(connector, data);
Address the following FIXME: convert parsers to use struct cea_db in the parser drm_parse_hdmi_vsdb_video cea_db contains len and then data. Appropriately change the indices when referring to individual elements (db[n] becomes data[n-1]). Signed-off-by: Vamsi Krishna Brahmajosyula <vamsikrishna.brahmajosyula@gmail.com> --- drivers/gpu/drm/drm_edid.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)