[PATCH] drm: parse color format support for digital displays

[PATCH] drm: parse color format support for digital displays

[Jesse Barnes]

EDID 1.4 digital displays report the color spaces they support in the
features block.  Add support for grabbing this data and stuffing it into
the display_info struct for driver use.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/drm_edid.c |   10 ++++++++++
 include/drm/drm_crtc.h     |    6 +++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fb5ebd9..64f8b7f 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1429,6 +1429,7 @@ static void drm_add_display_info(struct edid *edid,
 
 	/* driver figures it out in this case */
 	info->bpp = 0;
+	info->color_formats = 0;
 
 	/* Only defined for 1.4 with digital displays */
 	if (edid->revision < 4)
@@ -1461,6 +1462,15 @@ static void drm_add_display_info(struct edid *edid,
 		info->bpp = 0;
 		break;
 	}
+
+	if (edid->features & DRM_EDID_FEATURE_RGB)
+		info->color_formats = DRM_COLOR_FORMAT_RGB444;
+	else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
+		info->color_formats = DRM_COLOR_FORMAT_RGB444 |
+			DRM_COLOR_FORMAT_YCRCB444;
+	else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB)
+		info->color_formats = DRM_COLOR_FORMAT_RGB444 |
+			DRM_COLOR_FORMAT_YCRCB444 | DRM_COLOR_FORMAT_YCRCB422;
 }
 
 /**
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index aaec097..5cc7008 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -183,7 +183,9 @@ enum subpixel_order {
 	SubPixelNone,
 };
 
-
+#define DRM_COLOR_FORMAT_RGB444		(1<<0)
+#define DRM_COLOR_FORMAT_YCRCB444	(1<<1)
+#define DRM_COLOR_FORMAT_YCRCB422	(1<<2)
 /*
  * Describes a given display (e.g. CRT or flat panel) and its limitations.
  */
@@ -198,8 +200,10 @@ struct drm_display_info {
 	unsigned int min_vfreq, max_vfreq;
 	unsigned int min_hfreq, max_hfreq;
 	unsigned int pixel_clock;
+	unsigned int bpp;
 
 	enum subpixel_order subpixel_order;
+	unsigned long color_formats;
 
 	char *raw_edid; /* if any */
 };
-- 
1.7.4.1

Re: [PATCH] drm: parse color format support for digital displays

[Adam Jackson]

On 4/15/11 2:40 PM, Jesse Barnes wrote:

> @@ -1461,6 +1462,15 @@ static void drm_add_display_info(struct edid *edid,
>   		info->bpp = 0;
>   		break;
>   	}
> +
> +	if (edid->features & DRM_EDID_FEATURE_RGB)
> +		info->color_formats = DRM_COLOR_FORMAT_RGB444;
> +	else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
> +		info->color_formats = DRM_COLOR_FORMAT_RGB444 |
> +			DRM_COLOR_FORMAT_YCRCB444;
> +	else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB)
> +		info->color_formats = DRM_COLOR_FORMAT_RGB444 |
> +			DRM_COLOR_FORMAT_YCRCB444 | DRM_COLOR_FORMAT_YCRCB422;

Overcomplicated (RGB is numerically 0, YCRCB is just two other values 
or'd together) and wrong (doesn't handle YCbCr 4:2:2 alone).  You want:

	info->color_formats = DRM_COLOR_FORMAT_RGB444;
	if (edid->features & DRM_EDID_FEATURE_YCRCB444)
		info->color_formats |= DRM_COLOR_FORMAT_YCBCR444;
	if (edid->features & DRM_EDID_FEATURE_YCRCB422)
		info->color_formats |= DRM_COLOR_FORMAT_YCBCR422;

... which is corrected to not include RGB uselessly in the 
DRM_EDID_FEATURE_* tokens.  I should have noticed that in your first 
patch, whoops.

- ajax

Re: [PATCH] drm: parse color format support for digital displays

[Jesse Barnes]

On Fri, 15 Apr 2011 15:13:02 -0400
Adam Jackson <ajax@redhat.com> wrote:

> On 4/15/11 2:40 PM, Jesse Barnes wrote:
> 
> > @@ -1461,6 +1462,15 @@ static void drm_add_display_info(struct edid *edid,
> >   		info->bpp = 0;
> >   		break;
> >   	}
> > +
> > +	if (edid->features & DRM_EDID_FEATURE_RGB)
> > +		info->color_formats = DRM_COLOR_FORMAT_RGB444;
> > +	else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
> > +		info->color_formats = DRM_COLOR_FORMAT_RGB444 |
> > +			DRM_COLOR_FORMAT_YCRCB444;
> > +	else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB)
> > +		info->color_formats = DRM_COLOR_FORMAT_RGB444 |
> > +			DRM_COLOR_FORMAT_YCRCB444 | DRM_COLOR_FORMAT_YCRCB422;
> 
> Overcomplicated (RGB is numerically 0, YCRCB is just two other values 
> or'd together) and wrong (doesn't handle YCbCr 4:2:2 alone).  You want:

Arg, of course I have to mask out the value first, I'll fix that (my
current test display conveniently sets RGB_YCRCB so I missed this in
testing).

> 
> 	info->color_formats = DRM_COLOR_FORMAT_RGB444;
> 	if (edid->features & DRM_EDID_FEATURE_YCRCB444)
> 		info->color_formats |= DRM_COLOR_FORMAT_YCBCR444;
> 	if (edid->features & DRM_EDID_FEATURE_YCRCB422)
> 		info->color_formats |= DRM_COLOR_FORMAT_YCBCR422;
> 
> ... which is corrected to not include RGB uselessly in the 
> DRM_EDID_FEATURE_* tokens.  I should have noticed that in your first 
> patch, whoops.

I don't think EDID supports that?  The docs I have here imply that
either RGB, RGB + YCrCb444 or RGB + YCrCb444 + YCrCb422 are the only
things we can report.

Or is there a CEA block extension that allows for more granularity?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm: parse color format support for digital displays

[Jesse Barnes]

On Fri, 15 Apr 2011 12:19:31 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Fri, 15 Apr 2011 15:13:02 -0400
> Adam Jackson <ajax@redhat.com> wrote:
> > 	info->color_formats = DRM_COLOR_FORMAT_RGB444;
> > 	if (edid->features & DRM_EDID_FEATURE_YCRCB444)
> > 		info->color_formats |= DRM_COLOR_FORMAT_YCBCR444;
> > 	if (edid->features & DRM_EDID_FEATURE_YCRCB422)
> > 		info->color_formats |= DRM_COLOR_FORMAT_YCBCR422;
> > 
> > ... which is corrected to not include RGB uselessly in the 
> > DRM_EDID_FEATURE_* tokens.  I should have noticed that in your first 
> > patch, whoops.
> 
> I don't think EDID supports that?  The docs I have here imply that
> either RGB, RGB + YCrCb444 or RGB + YCrCb444 + YCrCb422 are the only
> things we can report.
> 
> Or is there a CEA block extension that allows for more granularity?

Nevermind, I even had the define correct, I just missed it when adding
the conditionals.  Will fix things to look like the above as it's nicer
than doing the switch.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm: parse color format support for digital displays

[Adam Jackson]

On 4/15/11 3:19 PM, Jesse Barnes wrote:

> Or is there a CEA block extension that allows for more granularity?

CEA has bits for the two YCbCr formats too, which we should also parse 
since there's plenty of 1.3+CEA blocks in the world thanks to HDMI.  For 
CEA blocks version 2 and up (version number in byte 2 of the CEA block, 
zero-based indexing), (1 << 5) of byte 3 is 4:4:4 and (1 << 4) is 4:2:2. 
  Same byte as what we already check for EDID_BASIC_AUDIO, if that's any 
clearer.  CEA spec contains the same language about always supporting 
RGB though.

I don't have a good answer for what to do if you have a 1.4+CEA block 
and the two bitfields are inconsistent, besides violence against the 
monitor vendor.

- ajax

Re: [PATCH] drm: parse color format support for digital displays

[Jesse Barnes]

On Fri, 15 Apr 2011 15:36:22 -0400
Adam Jackson <ajax@redhat.com> wrote:

> On 4/15/11 3:19 PM, Jesse Barnes wrote:
> 
> > Or is there a CEA block extension that allows for more granularity?
> 
> CEA has bits for the two YCbCr formats too, which we should also parse 
> since there's plenty of 1.3+CEA blocks in the world thanks to HDMI.  For 
> CEA blocks version 2 and up (version number in byte 2 of the CEA block, 
> zero-based indexing), (1 << 5) of byte 3 is 4:4:4 and (1 << 4) is 4:2:2. 
>   Same byte as what we already check for EDID_BASIC_AUDIO, if that's any 
> clearer.  CEA spec contains the same language about always supporting 
> RGB though.

Ok, sounds good.  I can add that a separate function that runs after we
fill out display_info from the input & feature bits.

> I don't have a good answer for what to do if you have a 1.4+CEA block 
> and the two bitfields are inconsistent, besides violence against the 
> monitor vendor.

Hm I guess we'd take the CEA block instead as it's probably fresher at
least.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm: parse color format support for digital displays

[Jesse Barnes]

EDID 1.4 digital displays report the color spaces they support in the
features block.  Add support for grabbing this data and stuffing it into
the display_info struct for driver use.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/drm_edid.c |    6 ++++++
 include/drm/drm_crtc.h     |    5 ++++-
 include/drm/drm_edid.h     |    8 ++++++++
 3 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3518e1e..0a9357c 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1462,6 +1462,12 @@ static void drm_add_display_info(struct edid *edid,
 		info->bpc = 0;
 		break;
 	}
+
+	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;
 }
 
 /**
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 4d8fbb0..b786a24 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -183,7 +183,9 @@ enum subpixel_order {
 	SubPixelNone,
 };
 
-
+#define DRM_COLOR_FORMAT_RGB444		(1<<0)
+#define DRM_COLOR_FORMAT_YCRCB444	(1<<1)
+#define DRM_COLOR_FORMAT_YCRCB422	(1<<2)
 /*
  * Describes a given display (e.g. CRT or flat panel) and its limitations.
  */
@@ -201,6 +203,7 @@ struct drm_display_info {
 	unsigned int bpc;
 
 	enum subpixel_order subpixel_order;
+	u32 color_formats;
 
 	char *raw_edid; /* if any */
 };
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 9b9bf94..eacb415 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -175,7 +175,15 @@ struct detailed_timing {
 #define DRM_EDID_FEATURE_DEFAULT_GTF      (1 << 0)
 #define DRM_EDID_FEATURE_PREFERRED_TIMING (1 << 1)
 #define DRM_EDID_FEATURE_STANDARD_COLOR   (1 << 2)
+/* If analog */
 #define DRM_EDID_FEATURE_DISPLAY_TYPE     (3 << 3) /* 00=mono, 01=rgb, 10=non-rgb, 11=unknown */
+/* If digital */
+#define DRM_EDID_FEATURE_COLOR_MASK	  (3 << 3)
+#define DRM_EDID_FEATURE_RGB		  (0 << 3)
+#define DRM_EDID_FEATURE_RGB_YCRCB444	  (1 << 3)
+#define DRM_EDID_FEATURE_RGB_YCRCB422	  (2 << 3)
+#define DRM_EDID_FEATURE_RGB_YCRCB	  (3 << 3) /* both 4:4:4 and 4:2:2 */
+
 #define DRM_EDID_FEATURE_PM_ACTIVE_OFF    (1 << 5)
 #define DRM_EDID_FEATURE_PM_SUSPEND       (1 << 6)
 #define DRM_EDID_FEATURE_PM_STANDBY       (1 << 7)
-- 
1.7.4.1