Re: [PATCH v1 11/13] drm/edid: add HF-EEODB support to EDID read and allocation

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, May 24, 2022 at 01:39:33PM +0300, Jani Nikula wrote:
> HDMI 2.1 section 10.3.6 defines an HDMI Forum EDID Extension Override
> Data Block, which may contain a different extension count than the base
> block claims. Add support for reading more EDID data if available. The
> extra blocks aren't parsed yet, though.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 81 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 78 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 5e0a91da565e..ba0c880dc133 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1581,6 +1581,15 @@ static bool version_greater(const struct drm_edid *drm_edid,
>  		(edid->version == version && edid->revision > revision);
>  }
>  
> +static int edid_hfeeodb_extension_block_count(const struct edid *edid);
> +
> +static int edid_hfeeodb_block_count(const struct edid *edid)
> +{
> +	int eeodb = edid_hfeeodb_extension_block_count(edid);
> +
> +	return eeodb ? eeodb + 1 : 0;
> +}
> +
>  static int edid_extension_block_count(const struct edid *edid)
>  {
>  	return edid->extensions;
> @@ -2026,6 +2035,11 @@ static struct edid *edid_filter_invalid_blocks(struct edid *edid,
>  	struct edid *new;
>  	int i, valid_blocks = 0;
>  
> +	/*
> +	 * Note: If the EDID uses HF-EEODB, but has invalid blocks, we'll revert
> +	 * back to regular extension count here. We don't want to start
> +	 * modifying the HF-EEODB extension too.
> +	 */
>  	for (i = 0; i < edid_block_count(edid); i++) {
>  		const void *src_block = edid_block_data(edid, i);
>  
> @@ -2235,7 +2249,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>  				     size_t *size)
>  {
>  	enum edid_block_status status;
> -	int i, invalid_blocks = 0;
> +	int i, num_blocks, invalid_blocks = 0;
>  	struct edid *edid, *new;
>  	size_t alloc_size = EDID_LENGTH;
>  
> @@ -2277,7 +2291,8 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>  		goto fail;
>  	edid = new;
>  
> -	for (i = 1; i < edid_block_count(edid); i++) {
> +	num_blocks = edid_block_count(edid);
> +	for (i = 1; i < num_blocks; i++) {
>  		void *block = (void *)edid_block_data(edid, i);
>  
>  		status = edid_block_read(block, i, read_block, context);
> @@ -2288,11 +2303,31 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>  			if (status == EDID_BLOCK_READ_FAIL)
>  				goto fail;
>  			invalid_blocks++;
> +		} else if (i == 1) {
> +			/*
> +			 * If the first EDID extension is a CTA extension, and
> +			 * the first Data Block is HF-EEODB, override the
> +			 * extension block count.
> +			 *
> +			 * Note: HF-EEODB could specify a smaller extension
> +			 * count too, but we can't risk allocating a smaller
> +			 * amount.
> +			 */
> +			int eeodb = edid_hfeeodb_block_count(edid);
> +
> +			if (eeodb > num_blocks) {
> +				num_blocks = eeodb;
> +				alloc_size = edid_size_by_blocks(num_blocks);
> +				new = krealloc(edid, alloc_size, GFP_KERNEL);
> +				if (!new)
> +					goto fail;
> +				edid = new;
> +			}
>  		}
>  	}
>  
>  	if (invalid_blocks) {
> -		connector_bad_edid(connector, edid, edid_block_count(edid));
> +		connector_bad_edid(connector, edid, num_blocks);
>  
>  		edid = edid_filter_invalid_blocks(edid, &alloc_size);
>  	}
> @@ -3825,6 +3860,7 @@ static int add_detailed_modes(struct drm_connector *connector,
>  #define CTA_EXT_DB_HDR_STATIC_METADATA	6
>  #define CTA_EXT_DB_420_VIDEO_DATA	14
>  #define CTA_EXT_DB_420_VIDEO_CAP_MAP	15
> +#define CTA_EXT_DB_HF_EEODB		0x78
>  #define CTA_EXT_DB_HF_SCDB		0x79
>  
>  #define EDID_BASIC_AUDIO	(1 << 6)
> @@ -4868,6 +4904,12 @@ static bool cea_db_is_hdmi_forum_vsdb(const struct cea_db *db)
>  		cea_db_payload_len(db) >= 7;
>  }
>  
> +static bool cea_db_is_hdmi_forum_eeodb(const void *db)
> +{
> +	return cea_db_is_extended_tag(db, CTA_EXT_DB_HF_EEODB) &&
> +		cea_db_payload_len(db) >= 2;
> +}
> +
>  static bool cea_db_is_microsoft_vsdb(const struct cea_db *db)
>  {
>  	return cea_db_is_vendor(db, MICROSOFT_IEEE_OUI) &&
> @@ -4902,6 +4944,39 @@ static bool cea_db_is_hdmi_hdr_metadata_block(const struct cea_db *db)
>  		cea_db_payload_len(db) >= 3;
>  }
>  
> +/*
> + * Get the HF-EEODB override extension block count from EDID.
> + *
> + * The passed in EDID may be partially read, as long as it has at least two
> + * blocks (base block and one extension block) if EDID extension count is > 0.
> + *
> + * References:
> + * - HDMI 2.1 section 10.3.6 HDMI Forum EDID Extension Override Data Block
> + */
> +static int edid_hfeeodb_extension_block_count(const struct edid *edid)
> +{
> +	const u8 *cta;
> +
> +	/* No extensions according to base block, no HF-EEODB. */
> +	if (!edid_extension_block_count(edid))
> +		return 0;
> +
> +	/* HF-EEODB is always in the first EDID extension block only */
> +	cta = edid_extension_block_data(edid, 0);
> +	if (edid_block_tag(cta) != CEA_EXT || cea_revision(cta) < 3)
> +		return 0;
> +
> +	/*
> +	 * Sinks that include the HF-EEODB in their E-EDID shall include one and
> +	 * only one instance of the HF-EEODB in the E-EDID, occupying bytes 4
> +	 * through 6 of Block 1 of the E-EDID.
> +	 */
> +	if (!cea_db_is_hdmi_forum_eeodb(&cta[4]))
> +		return 0;

Still not a big fan of these hardcoded things. Not sure if there's any
easy way to just use the normal iterators at this point when we don't
really know the full composition of the EDID yet. If not then I guess
we'll have to use some hardcoded stuff. What we definitely seem to be
missing here are size checks, for both the whoe data block collection,
and the specific data block payload.

> +
> +	return cta[4 + 2];
> +}
> +
>  static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
>  				      const u8 *db)
>  {
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel