diff mbox series

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

Message ID 7451127fb889f9d2ed723a6d83e033d6a5196354.1653381821.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/edid: expand on struct drm_edid usage | expand

Commit Message

Jani Nikula May 24, 2022, 10:39 a.m. UTC
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(-)

Comments

Ville Syrjala June 3, 2022, 5:13 p.m. UTC | #1
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
Jani Nikula June 7, 2022, 11:26 a.m. UTC | #2
On Fri, 03 Jun 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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.

I don't like it either, but it's a chicken and egg problem wrt how far
to iterate. Taking this into account in the iterators looks like making
the iterators harder to understand, so I prefer the hardcoded hack here
in one place. And the spec specifically says where this data block must
be.

The data block collection size check is an oversight, but
cea_db_is_hdmi_forum_eeodb() does check for minimum payload length.

BR,
Jani.

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

Patch

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;
+
+	return cta[4 + 2];
+}
+
 static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
 				      const u8 *db)
 {