DRM COLOR_RANGE property

DRM COLOR_RANGE property

[Russell King - ARM Linux]

Can someone provide a deeper explanation about exactly what this
property represents please?

Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB
conversion (in other words, the range of values in the framebuffer),
or the expected output range from the conversion?

This matters, because a "limited", (iow, eg, BT601 compliant YCbCr)
framebuffer where the Y signal is between 16..235 being displayed
on a full-range RGB output would need different conversion from
needing a limited-range RGB output.

If it is indeed the output, then why is this a property of the plane?
Is that not a property of:

(a) whether the plane is being blended or overlaid onto a graphics
    plane which uses full-range RGB
(b) the properties of the output(s) to which the plane is being
    displayed.

IOW, it seems that the output of the CSC is more to do with what's
downstream of the plane than with the plane itself.

For example, take this situation:

plane 0 - graphics, full range RGB
                                  >-- CRTC --> HDMI sink only supporting
plane 1 - video, limited range YUV              limited range RGB

In order to display the graphics correctly in that scenario, the HDMI
output needs to compress the RGB 0-255 range down to 16..236 to be
compliant.  If the video is limited range, and the CSC produces a
limited range RGB output, then plane 1 gets its range further
compressed at the HDMI output, which surely is undesirable.

It would surely be better, if it's not possible to map the range of
plane 0 to limited range, to instead expand the YUV range and then
recompress it at the HDMI output to match the capabilities of the
attached source.

It also seems logical that describing the range of the RGB plane would
also be sane - if the application is limiting graphics RGB to 16..235,
then you'd want the CSC output to do the same and there'd be no need
for any range expansion or compression.

I'd personally like drm_plane_create_color_properties() to allow
creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges
being zero) until COLOR_RANGE is better defined than it is at present.

Thoughts?

I'm bringing this up, because the hardware I have has a CSC that
accepts BT601 and BT709 formats, controlled by a single bit.  Another
bit controls whether the CSC produces 0..255 output or 16..235 output.
That is then blended/overlaid with the graphics plane (0..255) and
sent to the output.  Having a "limited range" YUV plane produce
16..235 range output makes it look low-contrast compared to the
graphics, which is what would be expected - "16" is not black
compared to the black of the graphics in the same way that "235" is
not white compared to the graphics.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM COLOR_RANGE property

[Daniel Vetter]

On Tue, Jul 03, 2018 at 05:18:57PM +0100, Russell King - ARM Linux wrote:
> Can someone provide a deeper explanation about exactly what this
> property represents please?
> 
> Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB
> conversion (in other words, the range of values in the framebuffer),
> or the expected output range from the conversion?
> 
> This matters, because a "limited", (iow, eg, BT601 compliant YCbCr)
> framebuffer where the Y signal is between 16..235 being displayed
> on a full-range RGB output would need different conversion from
> needing a limited-range RGB output.
> 
> If it is indeed the output, then why is this a property of the plane?
> Is that not a property of:
> 
> (a) whether the plane is being blended or overlaid onto a graphics
>     plane which uses full-range RGB
> (b) the properties of the output(s) to which the plane is being
>     displayed.
> 
> IOW, it seems that the output of the CSC is more to do with what's
> downstream of the plane than with the plane itself.
> 
> For example, take this situation:
> 
> plane 0 - graphics, full range RGB
>                                   >-- CRTC --> HDMI sink only supporting
> plane 1 - video, limited range YUV              limited range RGB
> 
> In order to display the graphics correctly in that scenario, the HDMI
> output needs to compress the RGB 0-255 range down to 16..236 to be
> compliant.  If the video is limited range, and the CSC produces a
> limited range RGB output, then plane 1 gets its range further
> compressed at the HDMI output, which surely is undesirable.
> 
> It would surely be better, if it's not possible to map the range of
> plane 0 to limited range, to instead expand the YUV range and then
> recompress it at the HDMI output to match the capabilities of the
> attached source.
> 
> It also seems logical that describing the range of the RGB plane would
> also be sane - if the application is limiting graphics RGB to 16..235,
> then you'd want the CSC output to do the same and there'd be no need
> for any range expansion or compression.
> 
> I'd personally like drm_plane_create_color_properties() to allow
> creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges
> being zero) until COLOR_RANGE is better defined than it is at present.
> 
> Thoughts?
> 
> I'm bringing this up, because the hardware I have has a CSC that
> accepts BT601 and BT709 formats, controlled by a single bit.  Another
> bit controls whether the CSC produces 0..255 output or 16..235 output.
> That is then blended/overlaid with the graphics plane (0..255) and
> sent to the output.  Having a "limited range" YUV plane produce
> 16..235 range output makes it look low-contrast compared to the
> graphics, which is what would be expected - "16" is not black
> compared to the black of the graphics in the same way that "235" is
> not white compared to the graphics.

Drivers are supposed to automatically figure this out by looking at the
edid. In i915 we also allow userspace to override this with the "Broadcast
RGB" property on the connector. Unfortunately we haven't polished that
property yet (not sure what other drivers are doing tbh), so it's only
listed in the Documentation/gpu/kms-properties.csv graveyard :-/

For the blending in-between right now the expectation is that the kernel
driver figures out a blending path that wastes the least amount of
precision. Depending upon hw that might mean blending in full RGB and
limiting later on, or converting all planes to limited RGB first.
Interactons with the CSC on the crtc for color corrections are also not
all that well defined.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM COLOR_RANGE property

[Russell King - ARM Linux]

On Wed, Jul 04, 2018 at 10:26:04AM +0200, Daniel Vetter wrote:
> On Tue, Jul 03, 2018 at 05:18:57PM +0100, Russell King - ARM Linux wrote:
> > Can someone provide a deeper explanation about exactly what this
> > property represents please?
> > 
> > Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB
> > conversion (in other words, the range of values in the framebuffer),
> > or the expected output range from the conversion?
> > 
> > This matters, because a "limited", (iow, eg, BT601 compliant YCbCr)
> > framebuffer where the Y signal is between 16..235 being displayed
> > on a full-range RGB output would need different conversion from
> > needing a limited-range RGB output.
> > 
> > If it is indeed the output, then why is this a property of the plane?
> > Is that not a property of:
> > 
> > (a) whether the plane is being blended or overlaid onto a graphics
> >     plane which uses full-range RGB
> > (b) the properties of the output(s) to which the plane is being
> >     displayed.
> > 
> > IOW, it seems that the output of the CSC is more to do with what's
> > downstream of the plane than with the plane itself.
> > 
> > For example, take this situation:
> > 
> > plane 0 - graphics, full range RGB
> >                                   >-- CRTC --> HDMI sink only supporting
> > plane 1 - video, limited range YUV              limited range RGB
> > 
> > In order to display the graphics correctly in that scenario, the HDMI
> > output needs to compress the RGB 0-255 range down to 16..236 to be
> > compliant.  If the video is limited range, and the CSC produces a
> > limited range RGB output, then plane 1 gets its range further
> > compressed at the HDMI output, which surely is undesirable.
> > 
> > It would surely be better, if it's not possible to map the range of
> > plane 0 to limited range, to instead expand the YUV range and then
> > recompress it at the HDMI output to match the capabilities of the
> > attached source.
> > 
> > It also seems logical that describing the range of the RGB plane would
> > also be sane - if the application is limiting graphics RGB to 16..235,
> > then you'd want the CSC output to do the same and there'd be no need
> > for any range expansion or compression.
> > 
> > I'd personally like drm_plane_create_color_properties() to allow
> > creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges
> > being zero) until COLOR_RANGE is better defined than it is at present.
> > 
> > Thoughts?
> > 
> > I'm bringing this up, because the hardware I have has a CSC that
> > accepts BT601 and BT709 formats, controlled by a single bit.  Another
> > bit controls whether the CSC produces 0..255 output or 16..235 output.
> > That is then blended/overlaid with the graphics plane (0..255) and
> > sent to the output.  Having a "limited range" YUV plane produce
> > 16..235 range output makes it look low-contrast compared to the
> > graphics, which is what would be expected - "16" is not black
> > compared to the black of the graphics in the same way that "235" is
> > not white compared to the graphics.
> 
> Drivers are supposed to automatically figure this out by looking at the
> edid. In i915 we also allow userspace to override this with the "Broadcast
> RGB" property on the connector. Unfortunately we haven't polished that
> property yet (not sure what other drivers are doing tbh), so it's only
> listed in the Documentation/gpu/kms-properties.csv graveyard :-/

In which case, I'd like to implement the COLOR_ENCODING property but
not the COLOR_RANGE property until COLOR_RANGE is better defined.
Unfortunately, at the moment the choices are to have either both
properties or no properties - drm_plane_create_color_properties()
doesn't support only creating the encoding property.

Implementing it without it being well defined is a recipe for having
a broken UAPI.  So, I propose:

8<=====
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: drm: allow COLOR_RANGE property to be optional

The COLOR_RANGE plane property is not well defined - it doesn't
define where in the colour conversion this control is applied.
If it's an attribute of the data in the plane, then it has the
reverse effect from if it's an attribute of the range of RGB
output from the plane colour converter.

Rather than being forced to implement this control when wanting
the COLOR_ENCODING property, make it optional instead.
---
 drivers/gpu/drm/drm_color_mgmt.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index b97e2de2c029..fc397fcf80ab 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -426,9 +426,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
 		    (supported_encodings & BIT(default_encoding)) == 0))
 		return -EINVAL;
 
-	if (WARN_ON(supported_ranges == 0 ||
-		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
-		    (supported_ranges & BIT(default_range)) == 0))
+	if (WARN_ON(supported_ranges != 0 &&
+		    ((supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
+		     (supported_ranges & BIT(default_range)) == 0)))
 		return -EINVAL;
 
 	len = 0;
@@ -450,6 +450,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
 	if (plane->state)
 		plane->state->color_encoding = default_encoding;
 
+	if (supported_ranges == 0)
+		return 0;
+
 	len = 0;
 	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
 		if ((supported_ranges & BIT(i)) == 0)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM COLOR_RANGE property

[Daniel Vetter]

On Wed, Jul 04, 2018 at 10:05:41AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 04, 2018 at 10:26:04AM +0200, Daniel Vetter wrote:
> > On Tue, Jul 03, 2018 at 05:18:57PM +0100, Russell King - ARM Linux wrote:
> > > Can someone provide a deeper explanation about exactly what this
> > > property represents please?
> > > 
> > > Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB
> > > conversion (in other words, the range of values in the framebuffer),
> > > or the expected output range from the conversion?
> > > 
> > > This matters, because a "limited", (iow, eg, BT601 compliant YCbCr)
> > > framebuffer where the Y signal is between 16..235 being displayed
> > > on a full-range RGB output would need different conversion from
> > > needing a limited-range RGB output.
> > > 
> > > If it is indeed the output, then why is this a property of the plane?
> > > Is that not a property of:
> > > 
> > > (a) whether the plane is being blended or overlaid onto a graphics
> > >     plane which uses full-range RGB
> > > (b) the properties of the output(s) to which the plane is being
> > >     displayed.
> > > 
> > > IOW, it seems that the output of the CSC is more to do with what's
> > > downstream of the plane than with the plane itself.
> > > 
> > > For example, take this situation:
> > > 
> > > plane 0 - graphics, full range RGB
> > >                                   >-- CRTC --> HDMI sink only supporting
> > > plane 1 - video, limited range YUV              limited range RGB
> > > 
> > > In order to display the graphics correctly in that scenario, the HDMI
> > > output needs to compress the RGB 0-255 range down to 16..236 to be
> > > compliant.  If the video is limited range, and the CSC produces a
> > > limited range RGB output, then plane 1 gets its range further
> > > compressed at the HDMI output, which surely is undesirable.
> > > 
> > > It would surely be better, if it's not possible to map the range of
> > > plane 0 to limited range, to instead expand the YUV range and then
> > > recompress it at the HDMI output to match the capabilities of the
> > > attached source.
> > > 
> > > It also seems logical that describing the range of the RGB plane would
> > > also be sane - if the application is limiting graphics RGB to 16..235,
> > > then you'd want the CSC output to do the same and there'd be no need
> > > for any range expansion or compression.
> > > 
> > > I'd personally like drm_plane_create_color_properties() to allow
> > > creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges
> > > being zero) until COLOR_RANGE is better defined than it is at present.
> > > 
> > > Thoughts?
> > > 
> > > I'm bringing this up, because the hardware I have has a CSC that
> > > accepts BT601 and BT709 formats, controlled by a single bit.  Another
> > > bit controls whether the CSC produces 0..255 output or 16..235 output.
> > > That is then blended/overlaid with the graphics plane (0..255) and
> > > sent to the output.  Having a "limited range" YUV plane produce
> > > 16..235 range output makes it look low-contrast compared to the
> > > graphics, which is what would be expected - "16" is not black
> > > compared to the black of the graphics in the same way that "235" is
> > > not white compared to the graphics.
> > 
> > Drivers are supposed to automatically figure this out by looking at the
> > edid. In i915 we also allow userspace to override this with the "Broadcast
> > RGB" property on the connector. Unfortunately we haven't polished that
> > property yet (not sure what other drivers are doing tbh), so it's only
> > listed in the Documentation/gpu/kms-properties.csv graveyard :-/
> 
> In which case, I'd like to implement the COLOR_ENCODING property but
> not the COLOR_RANGE property until COLOR_RANGE is better defined.
> Unfortunately, at the moment the choices are to have either both
> properties or no properties - drm_plane_create_color_properties()
> doesn't support only creating the encoding property.
> 
> Implementing it without it being well defined is a recipe for having
> a broken UAPI.  So, I propose:

Hm maybe I misunderstood, but I thought the COLOR_RANGE is on the input
side. And you need to adjust your CSC to make sure the output range is
whatever makes things Just Work. Probably better to poke Ville on these
details, and then clarify the docs.

The commit adding the color stuff also doesn't point at the userspace, and
I'm too lazy to dig it out, so no idea how it's supposed to work. Ideally
we'll end up with a patch to add a nice graph to the docs.
-Daniel

> 
> 8<=====
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: drm: allow COLOR_RANGE property to be optional
> 
> The COLOR_RANGE plane property is not well defined - it doesn't
> define where in the colour conversion this control is applied.
> If it's an attribute of the data in the plane, then it has the
> reverse effect from if it's an attribute of the range of RGB
> output from the plane colour converter.
> 
> Rather than being forced to implement this control when wanting
> the COLOR_ENCODING property, make it optional instead.
> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index b97e2de2c029..fc397fcf80ab 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -426,9 +426,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  		    (supported_encodings & BIT(default_encoding)) == 0))
>  		return -EINVAL;
>  
> -	if (WARN_ON(supported_ranges == 0 ||
> -		    (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
> -		    (supported_ranges & BIT(default_range)) == 0))
> +	if (WARN_ON(supported_ranges != 0 &&
> +		    ((supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 ||
> +		     (supported_ranges & BIT(default_range)) == 0)))
>  		return -EINVAL;
>  
>  	len = 0;
> @@ -450,6 +450,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
>  	if (plane->state)
>  		plane->state->color_encoding = default_encoding;
>  
> +	if (supported_ranges == 0)
> +		return 0;
> +
>  	len = 0;
>  	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
>  		if ((supported_ranges & BIT(i)) == 0)
> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
> According to speedtest.net: 13Mbps down 490kbps up

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM COLOR_RANGE property

[Russell King - ARM Linux]

On Wed, Jul 04, 2018 at 11:24:10AM +0200, Daniel Vetter wrote:
> On Wed, Jul 04, 2018 at 10:05:41AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jul 04, 2018 at 10:26:04AM +0200, Daniel Vetter wrote:
> > > On Tue, Jul 03, 2018 at 05:18:57PM +0100, Russell King - ARM Linux wrote:
> > > > Can someone provide a deeper explanation about exactly what this
> > > > property represents please?
> > > > 
> > > > Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB
> > > > conversion (in other words, the range of values in the framebuffer),
> > > > or the expected output range from the conversion?
> > > > 
> > > > This matters, because a "limited", (iow, eg, BT601 compliant YCbCr)
> > > > framebuffer where the Y signal is between 16..235 being displayed
> > > > on a full-range RGB output would need different conversion from
> > > > needing a limited-range RGB output.
> > > > 
> > > > If it is indeed the output, then why is this a property of the plane?
> > > > Is that not a property of:
> > > > 
> > > > (a) whether the plane is being blended or overlaid onto a graphics
> > > >     plane which uses full-range RGB
> > > > (b) the properties of the output(s) to which the plane is being
> > > >     displayed.
> > > > 
> > > > IOW, it seems that the output of the CSC is more to do with what's
> > > > downstream of the plane than with the plane itself.
> > > > 
> > > > For example, take this situation:
> > > > 
> > > > plane 0 - graphics, full range RGB
> > > >                                   >-- CRTC --> HDMI sink only supporting
> > > > plane 1 - video, limited range YUV              limited range RGB
> > > > 
> > > > In order to display the graphics correctly in that scenario, the HDMI
> > > > output needs to compress the RGB 0-255 range down to 16..236 to be
> > > > compliant.  If the video is limited range, and the CSC produces a
> > > > limited range RGB output, then plane 1 gets its range further
> > > > compressed at the HDMI output, which surely is undesirable.
> > > > 
> > > > It would surely be better, if it's not possible to map the range of
> > > > plane 0 to limited range, to instead expand the YUV range and then
> > > > recompress it at the HDMI output to match the capabilities of the
> > > > attached source.
> > > > 
> > > > It also seems logical that describing the range of the RGB plane would
> > > > also be sane - if the application is limiting graphics RGB to 16..235,
> > > > then you'd want the CSC output to do the same and there'd be no need
> > > > for any range expansion or compression.
> > > > 
> > > > I'd personally like drm_plane_create_color_properties() to allow
> > > > creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges
> > > > being zero) until COLOR_RANGE is better defined than it is at present.
> > > > 
> > > > Thoughts?
> > > > 
> > > > I'm bringing this up, because the hardware I have has a CSC that
> > > > accepts BT601 and BT709 formats, controlled by a single bit.  Another
> > > > bit controls whether the CSC produces 0..255 output or 16..235 output.
> > > > That is then blended/overlaid with the graphics plane (0..255) and
> > > > sent to the output.  Having a "limited range" YUV plane produce
> > > > 16..235 range output makes it look low-contrast compared to the
> > > > graphics, which is what would be expected - "16" is not black
> > > > compared to the black of the graphics in the same way that "235" is
> > > > not white compared to the graphics.
> > > 
> > > Drivers are supposed to automatically figure this out by looking at the
> > > edid. In i915 we also allow userspace to override this with the "Broadcast
> > > RGB" property on the connector. Unfortunately we haven't polished that
> > > property yet (not sure what other drivers are doing tbh), so it's only
> > > listed in the Documentation/gpu/kms-properties.csv graveyard :-/
> > 
> > In which case, I'd like to implement the COLOR_ENCODING property but
> > not the COLOR_RANGE property until COLOR_RANGE is better defined.
> > Unfortunately, at the moment the choices are to have either both
> > properties or no properties - drm_plane_create_color_properties()
> > doesn't support only creating the encoding property.
> > 
> > Implementing it without it being well defined is a recipe for having
> > a broken UAPI.  So, I propose:
> 
> Hm maybe I misunderstood, but I thought the COLOR_RANGE is on the input
> side.

If that's the case, I should force it to only indicate support for
limited range, while programming the CSC to produce full range RGB
on its output (see below).

> And you need to adjust your CSC to make sure the output range is
> whatever makes things Just Work. Probably better to poke Ville on these
> details, and then clarify the docs.

It's not possible to arbitarily program the CSC - I have two bits:

- one bit selects whether BT601 or BT709 YUV encoding is used on the CSC
  input.
- one bit selects whether limited or full range RGB is produced on the
  CSC output.

Given that I have to assume that the primary plane contains full range
RGB values (DRM has no means to communicate from userspace anything
else at present) I have to always select full range RGB at the CSC
output to be blended with the primary plane.

I then need some way to know that the TDA998x is being fed by a full
range RGB (for the time being, I'm going to assume that's the case)
and convert to limited range in the TDA998x if the sink doesn't
support full range.  I'm proposing to implement your i915 "broadcast
rgb" control in TDA998x to allow that to be overridden - maybe that
should become a standardised control for HDMI outputs?

As an optimisation, if the primary plane is disabled, then it would
be possible (in theory) to have the plane CSC produce limited range
and feed that straight to the TDA998x with no conversion there - but
it's probably not feasible to switch both modes without some artifacts
being generated, especially as we're talking over I2C to the TDA998x.
Also, as mentioned above, the TDA998x has no way to know the format
from the upstream CRTC, since it can't assume what DRM driver it is
being used with, and the DRM driver can't really assume that the
down-stream device is a TDA998x.  (I have the same problem with the
bus format used between the TDA998x and CRTC, and I notice comments
in DRM talking about inter-bridge bus formats also being a problem -
it's all the same issue really.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM COLOR_RANGE property

[Daniel Vetter]

On Wed, Jul 4, 2018 at 11:58 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jul 04, 2018 at 11:24:10AM +0200, Daniel Vetter wrote:
>> On Wed, Jul 04, 2018 at 10:05:41AM +0100, Russell King - ARM Linux wrote:
>> > On Wed, Jul 04, 2018 at 10:26:04AM +0200, Daniel Vetter wrote:
>> > > On Tue, Jul 03, 2018 at 05:18:57PM +0100, Russell King - ARM Linux wrote:
>> > > > Can someone provide a deeper explanation about exactly what this
>> > > > property represents please?
>> > > >
>> > > > Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB
>> > > > conversion (in other words, the range of values in the framebuffer),
>> > > > or the expected output range from the conversion?
>> > > >
>> > > > This matters, because a "limited", (iow, eg, BT601 compliant YCbCr)
>> > > > framebuffer where the Y signal is between 16..235 being displayed
>> > > > on a full-range RGB output would need different conversion from
>> > > > needing a limited-range RGB output.
>> > > >
>> > > > If it is indeed the output, then why is this a property of the plane?
>> > > > Is that not a property of:
>> > > >
>> > > > (a) whether the plane is being blended or overlaid onto a graphics
>> > > >     plane which uses full-range RGB
>> > > > (b) the properties of the output(s) to which the plane is being
>> > > >     displayed.
>> > > >
>> > > > IOW, it seems that the output of the CSC is more to do with what's
>> > > > downstream of the plane than with the plane itself.
>> > > >
>> > > > For example, take this situation:
>> > > >
>> > > > plane 0 - graphics, full range RGB
>> > > >                                   >-- CRTC --> HDMI sink only supporting
>> > > > plane 1 - video, limited range YUV              limited range RGB
>> > > >
>> > > > In order to display the graphics correctly in that scenario, the HDMI
>> > > > output needs to compress the RGB 0-255 range down to 16..236 to be
>> > > > compliant.  If the video is limited range, and the CSC produces a
>> > > > limited range RGB output, then plane 1 gets its range further
>> > > > compressed at the HDMI output, which surely is undesirable.
>> > > >
>> > > > It would surely be better, if it's not possible to map the range of
>> > > > plane 0 to limited range, to instead expand the YUV range and then
>> > > > recompress it at the HDMI output to match the capabilities of the
>> > > > attached source.
>> > > >
>> > > > It also seems logical that describing the range of the RGB plane would
>> > > > also be sane - if the application is limiting graphics RGB to 16..235,
>> > > > then you'd want the CSC output to do the same and there'd be no need
>> > > > for any range expansion or compression.
>> > > >
>> > > > I'd personally like drm_plane_create_color_properties() to allow
>> > > > creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges
>> > > > being zero) until COLOR_RANGE is better defined than it is at present.
>> > > >
>> > > > Thoughts?
>> > > >
>> > > > I'm bringing this up, because the hardware I have has a CSC that
>> > > > accepts BT601 and BT709 formats, controlled by a single bit.  Another
>> > > > bit controls whether the CSC produces 0..255 output or 16..235 output.
>> > > > That is then blended/overlaid with the graphics plane (0..255) and
>> > > > sent to the output.  Having a "limited range" YUV plane produce
>> > > > 16..235 range output makes it look low-contrast compared to the
>> > > > graphics, which is what would be expected - "16" is not black
>> > > > compared to the black of the graphics in the same way that "235" is
>> > > > not white compared to the graphics.
>> > >
>> > > Drivers are supposed to automatically figure this out by looking at the
>> > > edid. In i915 we also allow userspace to override this with the "Broadcast
>> > > RGB" property on the connector. Unfortunately we haven't polished that
>> > > property yet (not sure what other drivers are doing tbh), so it's only
>> > > listed in the Documentation/gpu/kms-properties.csv graveyard :-/
>> >
>> > In which case, I'd like to implement the COLOR_ENCODING property but
>> > not the COLOR_RANGE property until COLOR_RANGE is better defined.
>> > Unfortunately, at the moment the choices are to have either both
>> > properties or no properties - drm_plane_create_color_properties()
>> > doesn't support only creating the encoding property.
>> >
>> > Implementing it without it being well defined is a recipe for having
>> > a broken UAPI.  So, I propose:
>>
>> Hm maybe I misunderstood, but I thought the COLOR_RANGE is on the input
>> side.
>
> If that's the case, I should force it to only indicate support for
> limited range, while programming the CSC to produce full range RGB
> on its output (see below).

Agreed (as far as my understanding of all this goes at least).

>> And you need to adjust your CSC to make sure the output range is
>> whatever makes things Just Work. Probably better to poke Ville on these
>> details, and then clarify the docs.
>
> It's not possible to arbitarily program the CSC - I have two bits:
>
> - one bit selects whether BT601 or BT709 YUV encoding is used on the CSC
>   input.
> - one bit selects whether limited or full range RGB is produced on the
>   CSC output.
>
> Given that I have to assume that the primary plane contains full range
> RGB values (DRM has no means to communicate from userspace anything
> else at present) I have to always select full range RGB at the CSC
> output to be blended with the primary plane.

You could attach the color range stuff also to the primary plane, but
I don't think any current userspace is prepared to use it.

> I then need some way to know that the TDA998x is being fed by a full
> range RGB (for the time being, I'm going to assume that's the case)
> and convert to limited range in the TDA998x if the sink doesn't
> support full range.  I'm proposing to implement your i915 "broadcast
> rgb" control in TDA998x to allow that to be overridden - maybe that
> should become a standardised control for HDMI outputs?

Yup that sounds like a very good idea, plus documenting it in

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#hdmi-specific-connector-properties

> As an optimisation, if the primary plane is disabled, then it would
> be possible (in theory) to have the plane CSC produce limited range
> and feed that straight to the TDA998x with no conversion there - but
> it's probably not feasible to switch both modes without some artifacts
> being generated, especially as we're talking over I2C to the TDA998x.
> Also, as mentioned above, the TDA998x has no way to know the format
> from the upstream CRTC, since it can't assume what DRM driver it is
> being used with, and the DRM driver can't really assume that the
> down-stream device is a TDA998x.  (I have the same problem with the
> bus format used between the TDA998x and CRTC, and I notice comments
> in DRM talking about inter-bridge bus formats also being a problem -
> it's all the same issue really.)

Yeah atm the entire "what exactly is being feed from the crtc to the
bridge" question is rather open and unsolved. Even more so if you have
multiple bridges in a chain. There's ideas floating around to add a
drm_bridge_state (we have support for adding arbitrary internal state
structures now), but nothing came of those efforts yet. For now just
hacking around with baked-in assumptions like "tda998x always gets
full range on its input side" seems to be good enough still.

There's some high inertia to fix this since you need to get all m
bridge drivers and all n drm drivers to align on something, or
otherwise it won't really work. And yes the bus format is the exact
same issue. There's also been issues around mode polarity and exact
timings (if you e.g. have a buffer in your bridge capable of limited
timing transformations).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM COLOR_RANGE property

[Ville Syrjälä]

On Tue, Jul 03, 2018 at 05:18:57PM +0100, Russell King - ARM Linux wrote:
> Can someone provide a deeper explanation about exactly what this
> property represents please?
> 
> Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB
> conversion (in other words, the range of values in the framebuffer),
> or the expected output range from the conversion?

The color_range/encoding props are about the input. The plane output
is generally full range RGB, but presumably some driver could choose
to use some other internal format as well.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM COLOR_RANGE property

[Jyri Sarha]

On 03/07/18 19:18, Russell King - ARM Linux wrote:
> Can someone provide a deeper explanation about exactly what this
> property represents please?
> 
> Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB
> conversion (in other words, the range of values in the framebuffer),
> or the expected output range from the conversion?
> 

Yes, it to describe the YCbCr color encoding used in the buffer given to
the plane, nothing more. It should then be up to the driver and display
HW to decide what to do with the information. But of course since there
is currently no way to tell the range of the RGB buffers used, the
working assumption is that the RGB buffers are all full range and the
blending is done accordingly.

It should be easy enough to extend the existing enums, or to create
completely new ones, for information about the RGB buffers range.
Extending has a downside of adding nasty dependency between the
framebuffer format and the encoding properties.

The output side is then another matter.

Best regards,
Jyri

> This matters, because a "limited", (iow, eg, BT601 compliant YCbCr)
> framebuffer where the Y signal is between 16..235 being displayed
> on a full-range RGB output would need different conversion from
> needing a limited-range RGB output.
> 
> If it is indeed the output, then why is this a property of the plane?
> Is that not a property of:
> 
> (a) whether the plane is being blended or overlaid onto a graphics
>     plane which uses full-range RGB
> (b) the properties of the output(s) to which the plane is being
>     displayed.
> 
> IOW, it seems that the output of the CSC is more to do with what's
> downstream of the plane than with the plane itself.
> 
> For example, take this situation:
> 
> plane 0 - graphics, full range RGB
>                                   >-- CRTC --> HDMI sink only supporting
> plane 1 - video, limited range YUV              limited range RGB
> 
> In order to display the graphics correctly in that scenario, the HDMI
> output needs to compress the RGB 0-255 range down to 16..236 to be
> compliant.  If the video is limited range, and the CSC produces a
> limited range RGB output, then plane 1 gets its range further
> compressed at the HDMI output, which surely is undesirable.
> 
> It would surely be better, if it's not possible to map the range of
> plane 0 to limited range, to instead expand the YUV range and then
> recompress it at the HDMI output to match the capabilities of the
> attached source.
> 
> It also seems logical that describing the range of the RGB plane would
> also be sane - if the application is limiting graphics RGB to 16..235,
> then you'd want the CSC output to do the same and there'd be no need
> for any range expansion or compression.
> 
> I'd personally like drm_plane_create_color_properties() to allow
> creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges
> being zero) until COLOR_RANGE is better defined than it is at present.
> 
> Thoughts?
> 
> I'm bringing this up, because the hardware I have has a CSC that
> accepts BT601 and BT709 formats, controlled by a single bit.  Another
> bit controls whether the CSC produces 0..255 output or 16..235 output.
> That is then blended/overlaid with the graphics plane (0..255) and
> sent to the output.  Having a "limited range" YUV plane produce
> 16..235 range output makes it look low-contrast compared to the
> graphics, which is what would be expected - "16" is not black
> compared to the black of the graphics in the same way that "235" is
> not white compared to the graphics.
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM COLOR_RANGE property

[Jyri Sarha]

On 04/07/18 12:58, Russell King - ARM Linux wrote:
>> Hm maybe I misunderstood, but I thought the COLOR_RANGE is on the input
>> side.
> If that's the case, I should force it to only indicate support for
> limited range, while programming the CSC to produce full range RGB
> on its output (see below).
> 

That should need no forcing. Just set supported_ranges parameter
to BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) in
drm_plane_create_color_properties() call to create an enum property with
only the allowed value.

BR,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM COLOR_RANGE property

[Ville Syrjälä]

On Wed, Jul 04, 2018 at 01:06:42PM +0200, Daniel Vetter wrote:
> On Wed, Jul 4, 2018 at 11:58 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Wed, Jul 04, 2018 at 11:24:10AM +0200, Daniel Vetter wrote:
> >> On Wed, Jul 04, 2018 at 10:05:41AM +0100, Russell King - ARM Linux wrote:
> >> > On Wed, Jul 04, 2018 at 10:26:04AM +0200, Daniel Vetter wrote:
> >> > > On Tue, Jul 03, 2018 at 05:18:57PM +0100, Russell King - ARM Linux wrote:
> >> > > > Can someone provide a deeper explanation about exactly what this
> >> > > > property represents please?
> >> > > >
> >> > > > Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB
> >> > > > conversion (in other words, the range of values in the framebuffer),
> >> > > > or the expected output range from the conversion?
> >> > > >
> >> > > > This matters, because a "limited", (iow, eg, BT601 compliant YCbCr)
> >> > > > framebuffer where the Y signal is between 16..235 being displayed
> >> > > > on a full-range RGB output would need different conversion from
> >> > > > needing a limited-range RGB output.
> >> > > >
> >> > > > If it is indeed the output, then why is this a property of the plane?
> >> > > > Is that not a property of:
> >> > > >
> >> > > > (a) whether the plane is being blended or overlaid onto a graphics
> >> > > >     plane which uses full-range RGB
> >> > > > (b) the properties of the output(s) to which the plane is being
> >> > > >     displayed.
> >> > > >
> >> > > > IOW, it seems that the output of the CSC is more to do with what's
> >> > > > downstream of the plane than with the plane itself.
> >> > > >
> >> > > > For example, take this situation:
> >> > > >
> >> > > > plane 0 - graphics, full range RGB
> >> > > >                                   >-- CRTC --> HDMI sink only supporting
> >> > > > plane 1 - video, limited range YUV              limited range RGB
> >> > > >
> >> > > > In order to display the graphics correctly in that scenario, the HDMI
> >> > > > output needs to compress the RGB 0-255 range down to 16..236 to be
> >> > > > compliant.  If the video is limited range, and the CSC produces a
> >> > > > limited range RGB output, then plane 1 gets its range further
> >> > > > compressed at the HDMI output, which surely is undesirable.
> >> > > >
> >> > > > It would surely be better, if it's not possible to map the range of
> >> > > > plane 0 to limited range, to instead expand the YUV range and then
> >> > > > recompress it at the HDMI output to match the capabilities of the
> >> > > > attached source.
> >> > > >
> >> > > > It also seems logical that describing the range of the RGB plane would
> >> > > > also be sane - if the application is limiting graphics RGB to 16..235,
> >> > > > then you'd want the CSC output to do the same and there'd be no need
> >> > > > for any range expansion or compression.
> >> > > >
> >> > > > I'd personally like drm_plane_create_color_properties() to allow
> >> > > > creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges
> >> > > > being zero) until COLOR_RANGE is better defined than it is at present.
> >> > > >
> >> > > > Thoughts?
> >> > > >
> >> > > > I'm bringing this up, because the hardware I have has a CSC that
> >> > > > accepts BT601 and BT709 formats, controlled by a single bit.  Another
> >> > > > bit controls whether the CSC produces 0..255 output or 16..235 output.
> >> > > > That is then blended/overlaid with the graphics plane (0..255) and
> >> > > > sent to the output.  Having a "limited range" YUV plane produce
> >> > > > 16..235 range output makes it look low-contrast compared to the
> >> > > > graphics, which is what would be expected - "16" is not black
> >> > > > compared to the black of the graphics in the same way that "235" is
> >> > > > not white compared to the graphics.
> >> > >
> >> > > Drivers are supposed to automatically figure this out by looking at the
> >> > > edid. In i915 we also allow userspace to override this with the "Broadcast
> >> > > RGB" property on the connector. Unfortunately we haven't polished that
> >> > > property yet (not sure what other drivers are doing tbh), so it's only
> >> > > listed in the Documentation/gpu/kms-properties.csv graveyard :-/
> >> >
> >> > In which case, I'd like to implement the COLOR_ENCODING property but
> >> > not the COLOR_RANGE property until COLOR_RANGE is better defined.
> >> > Unfortunately, at the moment the choices are to have either both
> >> > properties or no properties - drm_plane_create_color_properties()
> >> > doesn't support only creating the encoding property.
> >> >
> >> > Implementing it without it being well defined is a recipe for having
> >> > a broken UAPI.  So, I propose:
> >>
> >> Hm maybe I misunderstood, but I thought the COLOR_RANGE is on the input
> >> side.
> >
> > If that's the case, I should force it to only indicate support for
> > limited range, while programming the CSC to produce full range RGB
> > on its output (see below).
> 
> Agreed (as far as my understanding of all this goes at least).
> 
> >> And you need to adjust your CSC to make sure the output range is
> >> whatever makes things Just Work. Probably better to poke Ville on these
> >> details, and then clarify the docs.
> >
> > It's not possible to arbitarily program the CSC - I have two bits:
> >
> > - one bit selects whether BT601 or BT709 YUV encoding is used on the CSC
> >   input.
> > - one bit selects whether limited or full range RGB is produced on the
> >   CSC output.
> >
> > Given that I have to assume that the primary plane contains full range
> > RGB values (DRM has no means to communicate from userspace anything
> > else at present) I have to always select full range RGB at the CSC
> > output to be blended with the primary plane.
> 
> You could attach the color range stuff also to the primary plane, but
> I don't think any current userspace is prepared to use it.
> 
> > I then need some way to know that the TDA998x is being fed by a full
> > range RGB (for the time being, I'm going to assume that's the case)
> > and convert to limited range in the TDA998x if the sink doesn't
> > support full range.  I'm proposing to implement your i915 "broadcast
> > rgb" control in TDA998x to allow that to be overridden - maybe that
> > should become a standardised control for HDMI outputs?
> 
> Yup that sounds like a very good idea, plus documenting it in
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#hdmi-specific-connector-properties

Note that at some point we'll have to come up with some new props for
specifying the full output colorimetry information and I have a
feeling the broadcast rgb prop may get slightly in the way. But
we'll have to cross that bridge in i915 anyway so I guess it wouldn't
complicate the situation any more if other drivers have this prop as
well.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel