RE: [v7 3/5] drm/edid: read HF-EEODB ext block

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tuesday, March 15, 2022 7:03 PM, Nikula, Jani <jani.nikula@xxxxxxxxx> wrote:
>On Sun, 13 Mar 2022, Lee Shawn C <shawn.c.lee@xxxxxxxxx> wrote:
>> According to HDMI 2.1 spec.
>>
>> "The HDMI Forum EDID Extension Override Data Block (HF-EEODB) is 
>> utilized by Sink Devices to provide an alternate method to indicate an 
>> EDID Extension Block count larger than 1, while avoiding the need to 
>> present a VESA Block Map in the first E-EDID Extension Block."
>>
>> It is a mandatory for HDMI 2.1 protocol compliance as well.
>> This patch help to know how many HF_EEODB blocks report by sink and 
>> read allo HF_EEODB blocks back.
>>
>> v2: support to find CEA block, check EEODB block format, and return
>>     available block number in drm_edid_read_hf_eeodb_blk_count().
>>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
>> Cc: Drew Davenport <ddavenport@xxxxxxxxxxxx>
>> Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/drm_connector.c |  8 +++-
>>  drivers/gpu/drm/drm_edid.c      | 71 +++++++++++++++++++++++++++++++--
>>  include/drm/drm_edid.h          |  1 +
>>  3 files changed, 74 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c 
>> b/drivers/gpu/drm/drm_connector.c index a50c82bc2b2f..16011023c12e 
>> 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -2129,7 +2129,7 @@ int drm_connector_update_edid_property(struct drm_connector *connector,
>>  				       const struct edid *edid)
>>  {
>>  	struct drm_device *dev = connector->dev;
>> -	size_t size = 0;
>> +	size_t size = 0, hf_eeodb_blk_count;
>>  	int ret;
>>  	const struct edid *old_edid;
>>  
>> @@ -2137,8 +2137,12 @@ 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);
>> +		hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count(edid);
>> +		if (hf_eeodb_blk_count)
>> +			size = EDID_LENGTH * (1 + hf_eeodb_blk_count);
>
>This approach does not scale. If the number of extensions and thus the total EDID size depend on HF-EEODB, this *must* be abstracted.
>
>Consider, for example, drm_edid_duplicate(), which only looks at
>edid->extensions. A subsequent HF-EEODB aware access on an EDID
>duplicated using drm_edid_duplicate() will access beyond the allocated buffer.
>
>Yes, it's a lot of work to introduce drm_edid_size() and
>drm_edid_extension_count() or similar, and use them everywhere, but this is what we must do. It's a lot more work to try to take HF-EEODB into account everywhere. The latter is going to be riddled with bugs with everyone doing it a little different.
>

As you mentioned, we should have new functions to provide proper EDID byte size or extension counter.
And reuse them to handle/retrieve EDID more accurately in drm driver. Thanks for your comment!

>> +	}
>>  
>>  	/* 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 
>> 9fa84881fbba..5ae4e83fa5e3 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1992,6 +1992,7 @@ struct edid *drm_do_get_edid(struct 
>> drm_connector *connector,  {
>>  	int i, j = 0, valid_extensions = 0;
>>  	u8 *edid, *new;
>> +	size_t hf_eeodb_blk_count;
>>  	struct edid *override;
>>  
>>  	override = drm_get_override_edid(connector); @@ -2051,7 +2052,35 @@ 
>> struct edid *drm_do_get_edid(struct drm_connector *connector,
>>  		}
>>  
>>  		kfree(edid);
>> +		return (struct edid *)new;
>> +	}
>> +
>> +	hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count((struct edid *)edid);
>> +	if (hf_eeodb_blk_count >= 2) {
>> +		new = krealloc(edid, (hf_eeodb_blk_count + 1) * EDID_LENGTH, GFP_KERNEL);
>> +		if (!new)
>> +			goto out;
>>  		edid = new;
>> +
>> +		valid_extensions = hf_eeodb_blk_count - 1;
>> +		for (j = 2; j <= hf_eeodb_blk_count; 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 != hf_eeodb_blk_count - 1) {
>> +			DRM_ERROR("Not able to retrieve proper EDID contain HF-EEODB data.\n");
>> +			goto out;
>> +		}
>>  	}
>>  
>>  	return (struct edid *)edid;
>> @@ -3315,15 +3344,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.
>> @@ -4274,9 +4305,41 @@ 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)
>> +		return false;
>> +
>> +	if (cea_db_extended_tag(db) != EXT_VIDEO_HF_EEODB_DATA_BLOCK)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>>  #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)
>>  
>> +size_t drm_edid_read_hf_eeodb_blk_count(const struct edid *edid) {
>> +	const u8 *cea;
>> +	int i, start, end, cea_ext_index = 0, displayid_ext_index = 0;
>> +
>> +	if (edid->extensions) {
>> +		cea = drm_find_cea_extension(edid, &cea_ext_index, 
>> +&displayid_ext_index);
>> +
>> +		if (cea && !cea_db_offsets(cea, &start, &end))
>> +			for_each_cea_db(cea, i, start, end)
>> +				if (cea_db_is_hdmi_forum_eeodb(&cea[i]))
>> +					return cea[i + 2];
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(drm_edid_read_hf_eeodb_blk_count);
>
>This should be static and not exported.
>
>BR,
>Jani.
>
>> +
>>  static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
>>  				      const u8 *db)
>>  {
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 
>> 144c495b99c4..585f0ed932d4 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -593,5 +593,6 @@ 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);
>>  
>> +size_t drm_edid_read_hf_eeodb_blk_count(const struct edid *edid);
>>  
>>  #endif /* __DRM_EDID_H__ */
>
>--
>Jani Nikula, Intel Open Source Graphics Center




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux