[PATCH] drm: document userspace expectations around the Colorspace connector property

[PATCH] drm: document userspace expectations around the Colorspace connector property

[Xaver Hugl]

Signed-off-by: Xaver Hugl <xaver.hugl@kde.org>
---
 drivers/gpu/drm/drm_connector.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b0516505f7ae..01e13984629b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2158,6 +2158,14 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
  *     one supported. Sink supported colorspaces should be retrieved by
  *     userspace from EDID and driver will not explicitly expose them.
  *
+ *     As userspace can't currently know whether or not the output is using
+ *     RGB or YCC signalling, the driver must translate properties to their
+ *     relevant RGB or YCC counterparts, depending on the actually used
+ *     signalling. Property values that are only valid for either YCC or RGB
+ *     and have no equivalent for the other signalling type must not be
+ *     exposed as supported, unless the driver can guarantee it never uses
+ *     the signalling that doesn't match the property.
+ *
  *     Basically the expectation from userspace is:
  *      - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
  *        colorspace
-- 
2.43.0

Re: [PATCH] drm: document userspace expectations around the Colorspace connector property

[Sebastian Wick]

On Fri, Feb 09, 2024 at 05:53:07PM +0100, Xaver Hugl wrote:
> Signed-off-by: Xaver Hugl <xaver.hugl@kde.org>
> ---
>  drivers/gpu/drm/drm_connector.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b0516505f7ae..01e13984629b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2158,6 +2158,14 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
>   *     one supported. Sink supported colorspaces should be retrieved by
>   *     userspace from EDID and driver will not explicitly expose them.
>   *
> + *     As userspace can't currently know whether or not the output is using
> + *     RGB or YCC signalling, the driver must translate properties to their
> + *     relevant RGB or YCC counterparts, depending on the actually used
> + *     signalling. Property values that are only valid for either YCC or RGB
> + *     and have no equivalent for the other signalling type must not be
> + *     exposed as supported, unless the driver can guarantee it never uses
> + *     the signalling that doesn't match the property.
> + *
>   *     Basically the expectation from userspace is:
>   *      - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
>   *        colorspace
> -- 
> 2.43.0
>

Thank you!

Reviewed-by: Sebastian Wick <sebastian.wick@redhat.com>

Re: [PATCH] drm: document userspace expectations around the Colorspace connector property

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1432 bytes --]

On Fri,  9 Feb 2024 17:53:07 +0100
Xaver Hugl <xaver.hugl@kde.org> wrote:

> Signed-off-by: Xaver Hugl <xaver.hugl@kde.org>
> ---
>  drivers/gpu/drm/drm_connector.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b0516505f7ae..01e13984629b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2158,6 +2158,14 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
>   *     one supported. Sink supported colorspaces should be retrieved by
>   *     userspace from EDID and driver will not explicitly expose them.
>   *
> + *     As userspace can't currently know whether or not the output is using
> + *     RGB or YCC signalling, the driver must translate properties to their
> + *     relevant RGB or YCC counterparts, depending on the actually used
> + *     signalling. Property values that are only valid for either YCC or RGB
> + *     and have no equivalent for the other signalling type must not be
> + *     exposed as supported, unless the driver can guarantee it never uses
> + *     the signalling that doesn't match the property.
> + *
>   *     Basically the expectation from userspace is:
>   *      - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
>   *        colorspace

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document userspace expectations around the Colorspace connector property

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

On Fri,  9 Feb 2024 17:53:07 +0100
Xaver Hugl <xaver.hugl@kde.org> wrote:

> Signed-off-by: Xaver Hugl <xaver.hugl@kde.org>
> ---
>  drivers/gpu/drm/drm_connector.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b0516505f7ae..01e13984629b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2158,6 +2158,14 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
>   *     one supported. Sink supported colorspaces should be retrieved by
>   *     userspace from EDID and driver will not explicitly expose them.
>   *
> + *     As userspace can't currently know whether or not the output is using
> + *     RGB or YCC signalling, the driver must translate properties to their
> + *     relevant RGB or YCC counterparts, depending on the actually used
> + *     signalling. Property values that are only valid for either YCC or RGB
> + *     and have no equivalent for the other signalling type must not be
> + *     exposed as supported, unless the driver can guarantee it never uses
> + *     the signalling that doesn't match the property.
> + *
>   *     Basically the expectation from userspace is:
>   *      - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
>   *        colorspace

While this addition is good, I have another question:

Does "Colorspace" property choose also the RGB->YCbCr matrix that the
driver will use when it happens to use YCbCr signalling?

So far we have only been talking about the primaries and white point.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document userspace expectations around the Colorspace connector property

[Sebastian Wick]

On Mon, Feb 12, 2024 at 11:10:15AM +0200, Pekka Paalanen wrote:
> On Fri,  9 Feb 2024 17:53:07 +0100
> Xaver Hugl <xaver.hugl@kde.org> wrote:
> 
> > Signed-off-by: Xaver Hugl <xaver.hugl@kde.org>
> > ---
> >  drivers/gpu/drm/drm_connector.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index b0516505f7ae..01e13984629b 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -2158,6 +2158,14 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> >   *     one supported. Sink supported colorspaces should be retrieved by
> >   *     userspace from EDID and driver will not explicitly expose them.
> >   *
> > + *     As userspace can't currently know whether or not the output is using
> > + *     RGB or YCC signalling, the driver must translate properties to their
> > + *     relevant RGB or YCC counterparts, depending on the actually used
> > + *     signalling. Property values that are only valid for either YCC or RGB
> > + *     and have no equivalent for the other signalling type must not be
> > + *     exposed as supported, unless the driver can guarantee it never uses
> > + *     the signalling that doesn't match the property.
> > + *
> >   *     Basically the expectation from userspace is:
> >   *      - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
> >   *        colorspace
> 
> While this addition is good, I have another question:
> 
> Does "Colorspace" property choose also the RGB->YCbCr matrix that the
> driver will use when it happens to use YCbCr signalling?
> 
> So far we have only been talking about the primaries and white point.

Uh, yeah, good point. The InfoFrames do affect both the YCbCr conversion
and the transfer characteristics that the sink expects. Drivers should
do the RGB to YCbCr conversion with the new matrix. I think (and very
much hope) that drivers don't rely on the TF for any internal processing
but if they did, they also should use the one the sink expects.

Re: [PATCH] drm: document userspace expectations around the Colorspace connector property

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]

On Mon, 12 Feb 2024 17:50:36 +0100
Sebastian Wick <sebastian.wick@redhat.com> wrote:

> On Mon, Feb 12, 2024 at 11:10:15AM +0200, Pekka Paalanen wrote:
> > On Fri,  9 Feb 2024 17:53:07 +0100
> > Xaver Hugl <xaver.hugl@kde.org> wrote:
> >   
> > > Signed-off-by: Xaver Hugl <xaver.hugl@kde.org>
> > > ---
> > >  drivers/gpu/drm/drm_connector.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index b0516505f7ae..01e13984629b 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -2158,6 +2158,14 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> > >   *     one supported. Sink supported colorspaces should be retrieved by
> > >   *     userspace from EDID and driver will not explicitly expose them.
> > >   *
> > > + *     As userspace can't currently know whether or not the output is using
> > > + *     RGB or YCC signalling, the driver must translate properties to their
> > > + *     relevant RGB or YCC counterparts, depending on the actually used
> > > + *     signalling. Property values that are only valid for either YCC or RGB
> > > + *     and have no equivalent for the other signalling type must not be
> > > + *     exposed as supported, unless the driver can guarantee it never uses
> > > + *     the signalling that doesn't match the property.
> > > + *
> > >   *     Basically the expectation from userspace is:
> > >   *      - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
> > >   *        colorspace  
> > 
> > While this addition is good, I have another question:
> > 
> > Does "Colorspace" property choose also the RGB->YCbCr matrix that the
> > driver will use when it happens to use YCbCr signalling?
> > 
> > So far we have only been talking about the primaries and white point.  
> 
> Uh, yeah, good point. The InfoFrames do affect both the YCbCr conversion
> and the transfer characteristics that the sink expects. Drivers should
> do the RGB to YCbCr conversion with the new matrix. I think (and very
> much hope) that drivers don't rely on the TF for any internal processing
> but if they did, they also should use the one the sink expects.

Oh, is transfer function also somehow associated here? That would be a
third thing. Are there any "Colorspace" values that target a display
with not gamma 2.2 TF (possibly extended/mirrored)?

The only TF metadata control I've known so far is the one in
HDR_OUTPUT_METADATA.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document userspace expectations around the Colorspace connector property

[Ville Syrjälä]

On Mon, Feb 12, 2024 at 05:50:36PM +0100, Sebastian Wick wrote:
> On Mon, Feb 12, 2024 at 11:10:15AM +0200, Pekka Paalanen wrote:
> > On Fri,  9 Feb 2024 17:53:07 +0100
> > Xaver Hugl <xaver.hugl@kde.org> wrote:
> > 
> > > Signed-off-by: Xaver Hugl <xaver.hugl@kde.org>
> > > ---
> > >  drivers/gpu/drm/drm_connector.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index b0516505f7ae..01e13984629b 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -2158,6 +2158,14 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> > >   *     one supported. Sink supported colorspaces should be retrieved by
> > >   *     userspace from EDID and driver will not explicitly expose them.
> > >   *
> > > + *     As userspace can't currently know whether or not the output is using
> > > + *     RGB or YCC signalling, the driver must translate properties to their
> > > + *     relevant RGB or YCC counterparts, depending on the actually used
> > > + *     signalling. Property values that are only valid for either YCC or RGB
> > > + *     and have no equivalent for the other signalling type must not be
> > > + *     exposed as supported, unless the driver can guarantee it never uses
> > > + *     the signalling that doesn't match the property.
> > > + *
> > >   *     Basically the expectation from userspace is:
> > >   *      - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
> > >   *        colorspace
> > 
> > While this addition is good, I have another question:
> > 
> > Does "Colorspace" property choose also the RGB->YCbCr matrix that the
> > driver will use when it happens to use YCbCr signalling?
> > 
> > So far we have only been talking about the primaries and white point.
> 
> Uh, yeah, good point. The InfoFrames do affect both the YCbCr conversion
> and the transfer characteristics that the sink expects. Drivers should
> do the RGB to YCbCr conversion with the new matrix. I think (and very
> much hope) that drivers don't rely on the TF for any internal processing
> but if they did, they also should use the one the sink expects.

What we need is:
- list of the property values that are still allowd (the ones that still
  make sense with the changed semantics)
- mark all the other ones as deprecated
- filter/reject the deprecated values when creating the property
- document each valid enum value fully (for both RGB and YCbCr output):
  * what exacly is signalled to the sink via infoframe/msa/vsc sdp
  * what processing is the driver expected to perform on the actual data

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: document userspace expectations around the Colorspace connector property

[Sebastian Wick]

On Tue, Feb 13, 2024 at 11:26:44AM +0200, Ville Syrjälä wrote:
> On Mon, Feb 12, 2024 at 05:50:36PM +0100, Sebastian Wick wrote:
> > On Mon, Feb 12, 2024 at 11:10:15AM +0200, Pekka Paalanen wrote:
> > > On Fri,  9 Feb 2024 17:53:07 +0100
> > > Xaver Hugl <xaver.hugl@kde.org> wrote:
> > > 
> > > > Signed-off-by: Xaver Hugl <xaver.hugl@kde.org>
> > > > ---
> > > >  drivers/gpu/drm/drm_connector.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > > index b0516505f7ae..01e13984629b 100644
> > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > @@ -2158,6 +2158,14 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> > > >   *     one supported. Sink supported colorspaces should be retrieved by
> > > >   *     userspace from EDID and driver will not explicitly expose them.
> > > >   *
> > > > + *     As userspace can't currently know whether or not the output is using
> > > > + *     RGB or YCC signalling, the driver must translate properties to their
> > > > + *     relevant RGB or YCC counterparts, depending on the actually used
> > > > + *     signalling. Property values that are only valid for either YCC or RGB
> > > > + *     and have no equivalent for the other signalling type must not be
> > > > + *     exposed as supported, unless the driver can guarantee it never uses
> > > > + *     the signalling that doesn't match the property.
> > > > + *
> > > >   *     Basically the expectation from userspace is:
> > > >   *      - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
> > > >   *        colorspace
> > > 
> > > While this addition is good, I have another question:
> > > 
> > > Does "Colorspace" property choose also the RGB->YCbCr matrix that the
> > > driver will use when it happens to use YCbCr signalling?
> > > 
> > > So far we have only been talking about the primaries and white point.
> > 
> > Uh, yeah, good point. The InfoFrames do affect both the YCbCr conversion
> > and the transfer characteristics that the sink expects. Drivers should
> > do the RGB to YCbCr conversion with the new matrix. I think (and very
> > much hope) that drivers don't rely on the TF for any internal processing
> > but if they did, they also should use the one the sink expects.
> 
> What we need is:
> - list of the property values that are still allowd (the ones that still
>   make sense with the changed semantics)
> - mark all the other ones as deprecated
> - filter/reject the deprecated values when creating the property
> - document each valid enum value fully (for both RGB and YCbCr output):
>   * what exacly is signalled to the sink via infoframe/msa/vsc sdp
>   * what processing is the driver expected to perform on the actual data

I'm starting to agree that this is a better idea than coming up with a
number of rules. We only rely on the default colorimetry as indicated
by EDID and BT.2020.

Xaver, are you going to update the patch accordingly?

> -- 
> Ville Syrjälä
> Intel
>