On Saturday, February 26, 2022 2:09 AM, Ville Syrjälä wrote:
>On Thu, Feb 24, 2022 at 10:16:24PM +0800, Lee Shawn C wrote:
>> Support to read HF_EEODB block that request by HDMI 2.1 specification.
>
>Please spell out what that thing is and why it exists.
>
>>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_connector.c | 5 ++-
>> drivers/gpu/drm/drm_edid.c | 76 ++++++++++++++++++++++++++++++---
>> include/drm/drm_edid.h | 2 +
>> 3 files changed, 77 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c index a50c82bc2b2f..0f9e3ef00be7
>> 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -2137,8 +2137,11 @@ int drm_connector_update_edid_property(struct drm_connector *connector,
>> if (connector->override_edid)
>> return 0;
>>
>> - if (edid)
>> + if (edid) {
>> size = EDID_LENGTH * (1 + edid->extensions);
>> + if (drm_edid_is_hf_eeodb_blk_available(edid))
>> + size = EDID_LENGTH * (1 + drm_edid_read_hf_eeodb_blk_size(edid));
>
>I think we just want a small helper function that gives us the number of ext blocks whether it be from the base block or from this thing.
>
>> + }
>>
>> /* Set the display info, using edid if available, otherwise
>> * resetting the values to defaults. This duplicates the work diff
>> --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index
>> 19426f28b411..056e735ef932 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1991,7 +1991,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>> void *data)
>> {
>> int i, j = 0, valid_extensions = 0;
>> - u8 *edid, *new;
>> + u8 *edid, *new, ext_eeodb_blk_size;
>> struct edid *override;
>>
>> override = drm_get_override_edid(connector); @@ -2051,7 +2051,40 @@
>> struct edid *drm_do_get_edid(struct drm_connector *connector,
>> }
>>
>> kfree(edid);
>> + return (struct edid *)new;
>> + }
>> +
>> + if (drm_edid_is_hf_eeodb_blk_available((struct edid *)edid)) {
>> + ext_eeodb_blk_size = drm_edid_read_hf_eeodb_blk_size((struct edid
>> +*)edid);
>> +
>> + // no more ext blk wait for read
>> + if (ext_eeodb_blk_size <= 1)
>> + return (struct edid *)edid;
>> +
>> + new = krealloc(edid, (ext_eeodb_blk_size + 1) * EDID_LENGTH, GFP_KERNEL);
>> + if (!new)
>> + goto out;
>> edid = new;
>> +
>> + valid_extensions = ext_eeodb_blk_size - 1;
>> + for (j = 2; j <= ext_eeodb_blk_size; j++) {
>> + u8 *block = edid + j * EDID_LENGTH;
>> +
>> + for (i = 0; i < 4; i++) {
>> + if (get_edid_block(data, block, j, EDID_LENGTH))
>> + goto out;
>> + if (drm_edid_block_valid(block, j, false, NULL))
>> + break;
>> + }
>> +
>> + if (i == 4)
>> + valid_extensions--;
>> + }
>> +
>> + if (valid_extensions != ext_eeodb_blk_size - 1) {
>> + DRM_ERROR("Not able to retrieve proper EDID contain HF-EEODB data.\n");
>> + goto out;
>> + }
>> }
>>
>> return (struct edid *)edid;
>> @@ -3315,15 +3348,17 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>> #define VIDEO_BLOCK 0x02
>> #define VENDOR_BLOCK 0x03
>> #define SPEAKER_BLOCK 0x04
>> -#define HDR_STATIC_METADATA_BLOCK 0x6
>> -#define USE_EXTENDED_TAG 0x07
>> -#define EXT_VIDEO_CAPABILITY_BLOCK 0x00
>> +#define EXT_VIDEO_CAPABILITY_BLOCK 0x00
>> +#define HDR_STATIC_METADATA_BLOCK 0x06
>> +#define USE_EXTENDED_TAG 0x07
>> #define EXT_VIDEO_DATA_BLOCK_420 0x0E
>> -#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F
>> +#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F
>> +#define EXT_VIDEO_HF_EEODB_DATA_BLOCK 0x78
>> #define EDID_BASIC_AUDIO (1 << 6)
>> #define EDID_CEA_YCRCB444 (1 << 5)
>> #define EDID_CEA_YCRCB422 (1 << 4)
>> #define EDID_CEA_VCDB_QS (1 << 6)
>> +#define HF_EEODB_LENGTH 2
>>
>> /*
>> * Search EDID for CEA extension block.
>> @@ -4273,6 +4308,37 @@ static bool cea_db_is_y420vdb(const u8 *db)
>> return true;
>> }
>>
>> +static bool cea_db_is_hdmi_forum_eeodb(const u8 *db) {
>> + if (cea_db_tag(db) != USE_EXTENDED_TAG)
>> + return false;
>> +
>> + if (cea_db_payload_len(db) != HF_EEODB_LENGTH)
>
>We don't have any defines for the length of other blocks. Don't see why this one should be different.
>
>Also, the spec says
>"The Sources that are compliant to This Specification will not be adversely affected by the presence of the additional bytes within the Length field range."
>
Below is what I got from spec. That's why I added definition and check its value in block header.
". Length [5 bits] Total length of data block, not including this byte. For this Data Block, Sinks that follow This Specification shall set the Length field to 2."
>> + return false;
>> +
>> + if (cea_db_extended_tag(db) != EXT_VIDEO_HF_EEODB_DATA_BLOCK)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +bool drm_edid_is_hf_eeodb_blk_available(const struct edid *edid) {
>> + const u8 *eeodb_header = (u8 *)edid + EDID_LENGTH + 4;
>> +
>> + if (!edid->extensions)
>> + return false;
>> +
>> + return cea_db_is_hdmi_forum_eeodb(eeodb_header);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_edid_is_hf_eeodb_blk_available);
>> +
>> +u8 drm_edid_read_hf_eeodb_blk_size(const struct edid *edid) {
>> + return ((u8 *)edid)[EDID_LENGTH + 6];
>
>Not a big fan of these hardcoded offsets and stuff. Seems too easy to screw up.
>
My opinion to add cea_db_is_hdmi_forum_eeodb() here to make sure it is EEODB. If so, then allow this function return [EDID_LENGTH + 6] byte value.
Please let me know if any other comments. Thanks!
Best regards,
Shawn
>> +}
>> +EXPORT_SYMBOL_GPL(drm_edid_read_hf_eeodb_blk_size);
>> +
>> #define for_each_cea_db(cea, i, start, end) \
>> for ((i) = (start); (i) < (end) && (i) +
>> cea_db_payload_len(&(cea)[(i)]) < (end); (i) +=
>> cea_db_payload_len(&(cea)[(i)]) + 1)
>>
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index
>> 144c495b99c4..22872d9731a8 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -593,5 +593,7 @@ drm_display_mode_from_cea_vic(struct drm_device
>> *dev, const u8 *drm_find_edid_extension(const struct edid *edid,
>> int ext_id, int *ext_index);
>>
>> +bool drm_edid_is_hf_eeodb_blk_available(const struct edid *edid);
>> +u8 drm_edid_read_hf_eeodb_blk_size(const struct edid *edid);
>>
>> #endif /* __DRM_EDID_H__ */
>> --
>> 2.17.1
>
>--
>Ville Syrjälä
>Intel
>
