[PATCH resend 1/2] drm: Fix EDID color fromat parsing

[PATCH resend 1/2] drm: Fix EDID color fromat parsing

[Lars-Peter Clausen]

The code should obviously check the EDID feature field for EDID feature flags
and not the color_formats field of the drm_display_info struct. Also update the
color_formats field with new modes instead of overwriting the current mode.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/gpu/drm/drm_edid.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7ee7be1..a6bb2f5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1700,10 +1700,10 @@ static void drm_add_display_info(struct edid *edid,
 	}
 
 	info->color_formats = DRM_COLOR_FORMAT_RGB444;
-	if (info->color_formats & DRM_EDID_FEATURE_RGB_YCRCB444)
-		info->color_formats = DRM_COLOR_FORMAT_YCRCB444;
-	if (info->color_formats & DRM_EDID_FEATURE_RGB_YCRCB422)
-		info->color_formats = DRM_COLOR_FORMAT_YCRCB422;
+	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
+		info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
+	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
+		info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
 
 	/* Get data from CEA blocks if present */
 	edid_ext = drm_find_cea_extension(edid);
-- 
1.7.9

[PATCH resend 2/2] drm: Parse color format information in CEA blocks

[Lars-Peter Clausen]

The CEA extension block has a field which describes which YCbCr modes are
supported by the device, use it to fill the drm_display_info color_formats
fields. Also the existence of a CEA extension block is used as indication
that the device supports RGB.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/gpu/drm/drm_edid.c |   31 +++++++++++++++++++++----------
 1 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a6bb2f5..390aeca 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1313,6 +1313,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
 #define VENDOR_BLOCK    0x03
 #define SPEAKER_BLOCK	0x04
 #define EDID_BASIC_AUDIO	(1 << 6)
+#define EDID_CEA_YCRCB444	(1 << 5)
+#define EDID_CEA_YCRCB422	(1 << 4)
 
 /**
  * Search EDID for CEA extension block.
@@ -1667,13 +1669,29 @@ static void drm_add_display_info(struct edid *edid,
 	info->bpc = 0;
 	info->color_formats = 0;
 
-	/* Only defined for 1.4 with digital displays */
-	if (edid->revision < 4)
+	if (edid->revision < 3)
 		return;
 
 	if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
 		return;
 
+	/* Get data from CEA blocks if present */
+	edid_ext = drm_find_cea_extension(edid);
+	if (edid_ext) {
+		info->cea_rev = edid_ext[1];
+
+		/* The existence of a CEA block should imply RGB support */
+		info->color_formats = DRM_COLOR_FORMAT_RGB444;
+		if (edid_ext[3] & EDID_CEA_YCRCB444)
+			info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
+		if (edid_ext[3] & EDID_CEA_YCRCB422)
+			info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
+	}
+
+	/* Only defined for 1.4 with digital displays */
+	if (edid->revision < 4)
+		return;
+
 	switch (edid->input & DRM_EDID_DIGITAL_DEPTH_MASK) {
 	case DRM_EDID_DIGITAL_DEPTH_6:
 		info->bpc = 6;
@@ -1699,18 +1717,11 @@ static void drm_add_display_info(struct edid *edid,
 		break;
 	}
 
-	info->color_formats = DRM_COLOR_FORMAT_RGB444;
+	info->color_formats |= DRM_COLOR_FORMAT_RGB444;
 	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
 		info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
 	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
 		info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
-
-	/* Get data from CEA blocks if present */
-	edid_ext = drm_find_cea_extension(edid);
-	if (!edid_ext)
-		return;
-
-	info->cea_rev = edid_ext[1];
 }
 
 /**
-- 
1.7.9

Re: [PATCH resend 1/2] drm: Fix EDID color fromat parsing

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1581 bytes --]

On Tue, 28 Feb 2012 10:21:44 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> The code should obviously check the EDID feature field for EDID feature flags
> and not the color_formats field of the drm_display_info struct. Also update the
> color_formats field with new modes instead of overwriting the current mode.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/gpu/drm/drm_edid.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7ee7be1..a6bb2f5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1700,10 +1700,10 @@ static void drm_add_display_info(struct edid *edid,
>  	}
>  
>  	info->color_formats = DRM_COLOR_FORMAT_RGB444;
> -	if (info->color_formats & DRM_EDID_FEATURE_RGB_YCRCB444)
> -		info->color_formats = DRM_COLOR_FORMAT_YCRCB444;
> -	if (info->color_formats & DRM_EDID_FEATURE_RGB_YCRCB422)
> -		info->color_formats = DRM_COLOR_FORMAT_YCRCB422;
> +	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
> +	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
>  
>  	/* Get data from CEA blocks if present */
>  	edid_ext = drm_find_cea_extension(edid);

Ah that's better.  Do you have a TV that reports these feature bits?
If so, which model?

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH resend 2/2] drm: Parse color format information in CEA blocks

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 2828 bytes --]

On Tue, 28 Feb 2012 10:21:45 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> The CEA extension block has a field which describes which YCbCr modes are
> supported by the device, use it to fill the drm_display_info color_formats
> fields. Also the existence of a CEA extension block is used as indication
> that the device supports RGB.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/gpu/drm/drm_edid.c |   31 +++++++++++++++++++++----------
>  1 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a6bb2f5..390aeca 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1313,6 +1313,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>  #define VENDOR_BLOCK    0x03
>  #define SPEAKER_BLOCK	0x04
>  #define EDID_BASIC_AUDIO	(1 << 6)
> +#define EDID_CEA_YCRCB444	(1 << 5)
> +#define EDID_CEA_YCRCB422	(1 << 4)
>  
>  /**
>   * Search EDID for CEA extension block.
> @@ -1667,13 +1669,29 @@ static void drm_add_display_info(struct edid *edid,
>  	info->bpc = 0;
>  	info->color_formats = 0;
>  
> -	/* Only defined for 1.4 with digital displays */
> -	if (edid->revision < 4)
> +	if (edid->revision < 3)
>  		return;
>  
>  	if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
>  		return;
>  
> +	/* Get data from CEA blocks if present */
> +	edid_ext = drm_find_cea_extension(edid);
> +	if (edid_ext) {
> +		info->cea_rev = edid_ext[1];
> +
> +		/* The existence of a CEA block should imply RGB support */
> +		info->color_formats = DRM_COLOR_FORMAT_RGB444;
> +		if (edid_ext[3] & EDID_CEA_YCRCB444)
> +			info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
> +		if (edid_ext[3] & EDID_CEA_YCRCB422)
> +			info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
> +	}
> +
> +	/* Only defined for 1.4 with digital displays */
> +	if (edid->revision < 4)
> +		return;
> +
>  	switch (edid->input & DRM_EDID_DIGITAL_DEPTH_MASK) {
>  	case DRM_EDID_DIGITAL_DEPTH_6:
>  		info->bpc = 6;
> @@ -1699,18 +1717,11 @@ static void drm_add_display_info(struct edid *edid,
>  		break;
>  	}
>  
> -	info->color_formats = DRM_COLOR_FORMAT_RGB444;
> +	info->color_formats |= DRM_COLOR_FORMAT_RGB444;
>  	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
>  		info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
>  	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
>  		info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
> -
> -	/* Get data from CEA blocks if present */
> -	edid_ext = drm_find_cea_extension(edid);
> -	if (!edid_ext)
> -		return;
> -
> -	info->cea_rev = edid_ext[1];
>  }
>  
>  /**

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH resend 1/2] drm: Fix EDID color fromat parsing

[Lars-Peter Clausen]

On 02/28/2012 07:04 PM, Jesse Barnes wrote:
> On Tue, 28 Feb 2012 10:21:44 +0100
> Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
>> The code should obviously check the EDID feature field for EDID feature flags
>> and not the color_formats field of the drm_display_info struct. Also update the
>> color_formats field with new modes instead of overwriting the current mode.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>  drivers/gpu/drm/drm_edid.c |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 7ee7be1..a6bb2f5 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1700,10 +1700,10 @@ static void drm_add_display_info(struct edid *edid,
>>  	}
>>  
>>  	info->color_formats = DRM_COLOR_FORMAT_RGB444;
>> -	if (info->color_formats & DRM_EDID_FEATURE_RGB_YCRCB444)
>> -		info->color_formats = DRM_COLOR_FORMAT_YCRCB444;
>> -	if (info->color_formats & DRM_EDID_FEATURE_RGB_YCRCB422)
>> -		info->color_formats = DRM_COLOR_FORMAT_YCRCB422;
>> +	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
>> +	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
>>  
>>  	/* Get data from CEA blocks if present */
>>  	edid_ext = drm_find_cea_extension(edid);
> 
> Ah that's better.  Do you have a TV that reports these feature bits?
> If so, which model?

No, I just stumbled upon it when implementing patch 2 of this series.

> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 

Re: [PATCH resend 1/2] drm: Fix EDID color fromat parsing

[Tormod Volden]

On Thu, Mar 1, 2012 at 8:26 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 02/28/2012 07:04 PM, Jesse Barnes wrote:
>> On Tue, 28 Feb 2012 10:21:44 +0100
>> Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>>> The code should obviously check the EDID feature field for EDID feature flags
>>> and not the color_formats field of the drm_display_info struct. Also update the
>>> color_formats field with new modes instead of overwriting the current mode.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

FWIW, there's a typo in the commit message / mail subject: fromat

Tormod

Re: [PATCH resend 1/2] drm: Fix EDID color fromat parsing

[Lars-Peter Clausen]

On 02/28/2012 07:04 PM, Jesse Barnes wrote:
> On Tue, 28 Feb 2012 10:21:44 +0100
> Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
>> The code should obviously check the EDID feature field for EDID feature flags
>> and not the color_formats field of the drm_display_info struct. Also update the
>> color_formats field with new modes instead of overwriting the current mode.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>  drivers/gpu/drm/drm_edid.c |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 7ee7be1..a6bb2f5 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1700,10 +1700,10 @@ static void drm_add_display_info(struct edid *edid,
>>  	}
>>  
>>  	info->color_formats = DRM_COLOR_FORMAT_RGB444;
>> -	if (info->color_formats & DRM_EDID_FEATURE_RGB_YCRCB444)
>> -		info->color_formats = DRM_COLOR_FORMAT_YCRCB444;
>> -	if (info->color_formats & DRM_EDID_FEATURE_RGB_YCRCB422)
>> -		info->color_formats = DRM_COLOR_FORMAT_YCRCB422;
>> +	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
>> +	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
>>  
>>  	/* Get data from CEA blocks if present */
>>  	edid_ext = drm_find_cea_extension(edid);
> 
> Ah that's better.  Do you have a TV that reports these feature bits?
> If so, which model?
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 

Hi David,

Can you take a look at these two patches and apply them if they are OK? If
you apply them it would be nice if you could fix the typo in the subject
line of this patch (s/fromat/format/).

Thanks,
- Lars