diff mbox series

drm: document userspace expectations around the Colorspace connector property

Message ID 20240209165307.29856-1-xaver.hugl@kde.org (mailing list archive)
State New, archived
Headers show
Series drm: document userspace expectations around the Colorspace connector property | expand

Commit Message

Xaver Hugl Feb. 9, 2024, 4:53 p.m. UTC
Signed-off-by: Xaver Hugl <xaver.hugl@kde.org>
---
 drivers/gpu/drm/drm_connector.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Sebastian Wick Feb. 9, 2024, 8:36 p.m. UTC | #1
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>
Pekka Paalanen Feb. 12, 2024, 8:46 a.m. UTC | #2
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
Pekka Paalanen Feb. 12, 2024, 9:10 a.m. UTC | #3
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.
Sebastian Wick Feb. 12, 2024, 4:50 p.m. UTC | #4
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.
Pekka Paalanen Feb. 13, 2024, 8:54 a.m. UTC | #5
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
Ville Syrjala Feb. 13, 2024, 9:26 a.m. UTC | #6
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
Sebastian Wick Feb. 19, 2024, 1:22 p.m. UTC | #7
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
>
diff mbox series

Patch

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