On Mon, 11 Mar 2024 17:06:34 +0100 Sebastian Wick <sebastian.w...@redhat.com> wrote:
> On Thu, Mar 07, 2024 at 10:29:22AM +0200, Pekka Paalanen wrote:
> > On Wed, 6 Mar 2024 17:42:09 +0100
> > Sebastian Wick <sebastian.w...@redhat.com> wrote:
> >
> > > On Wed, Mar 06, 2024 at 10:27:21AM +0200, Pekka Paalanen wrote:
> > > > On Tue, 5 Mar 2024 14:51:49 +0100
> > > > Sebastian Wick <sebastian.w...@redhat.com> wrote:
> > > >
> > > > > The initial idea of the Colorspace prop was that this maps 1:1 to
> > > > > InfoFrames/SDP but KMS does not give user space enough information nor
> > > > > control over the output format to figure out which variants can be
> > > > > used
> > > > > for a given KMS commit. At the same time, properties like Broadcast
> > > > > RGB
> > > > > expect full range quantization range being produced by user space from
> > > > > the CRTC and drivers to convert to the range expected by the sink for
> > > > > the chosen output format, mode, InfoFrames, etc.
> > > > >
> > > > > This change documents the reality of the Colorspace property. The
> > > > > Default variant unfortunately is very much driver specific and not
> > > > > reflected by the EDID. The BT2020 variants are in active use by
> > > > > generic
> > > > > compositors which have expectations from the driver about the
> > > > > conversions it has to do when selecting certain output formats.
> > > > >
> > > > > Everything else is also marked as undefined. Coming up with valid
> > > > > behavior that makes it usable from user space and consistent with
> > > > > other
> > > > > KMS properties for those variants is left as an exercise for whoever
> > > > > wants to use them.
> > > > >
> > > > > v2:
> > > > > * Talk about "pixel operation properties" that user space configures
> > > > > * Mention that user space is responsible for checking the EDID for
> > > > > sink
> > > > > support
> > > > > * Make it clear that drivers can choose between RGB and YCbCr on
> > > > > their
> > > > > own
> > > > >
> > > > > Signed-off-by: Sebastian Wick <sebastian.w...@redhat.com>
> > > > > ---
> > > > > drivers/gpu/drm/drm_connector.c | 79
> > > > > +++++++++++++++++++++++++--------
> > > > > include/drm/drm_connector.h | 8 ----
> > > > > 2 files changed, 61 insertions(+), 26 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_connector.c
> > > > > b/drivers/gpu/drm/drm_connector.c
> > > > > index b0516505f7ae..65cdcc7d22db 100644
> > > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > > @@ -2147,24 +2147,67 @@
> > > > > EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> > > > > * DOC: standard connector properties
> > > > > *
> > > > > * Colorspace:
> > > > > - * This property helps select a suitable colorspace based on the
> > > > > sink
> > > > > - * capability. Modern sink devices support wider gamut like
> > > > > BT2020.
> > > > > - * This helps switch to BT2020 mode if the BT2020 encoded video
> > > > > stream
> > > > > - * is being played by the user, same for any other colorspace.
> > > > > Thereby
> > > > > - * giving a good visual experience to users.
> > > > > - *
> > > > > - * The expectation from userspace is that it should parse the
> > > > > EDID
> > > > > - * and get supported colorspaces. Use this property and switch
> > > > > to the
> > > > > - * one supported. Sink supported colorspaces should be retrieved
> > > > > by
> > > > > - * userspace from EDID and driver will not explicitly expose
> > > > > them.
> > > > > - *
> > > > > - * Basically the expectation from userspace is:
> > > > > - * - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
> > > > > - * colorspace
> > > > > - * - Set this new property to let the sink know what it
> > > > > - * converted the CRTC output to.
> > > > > - * - This property is just to inform sink what colorspace
> > > > > - * source is trying to drive.
> > > > > + * This property is used to inform the driver about the color
> > > > > encoding
> > > > > + * user space configured the pixel operation properties to produce.
> > > > > + * The variants set the colorimetry, transfer characteristics, and
> > > > > which
> > > > > + * YCbCr conversion should be used when necessary.
> > > > > + * The transfer characteristics from HDR_OUTPUT_METADATA takes
> > > > > precedence
> > > > > + * over this property.
> > > > > + * User space always configures the pixel operation properties to
> > > > > produce
> > > > > + * full quantization range data (see the Broadcast RGB property).
> > > > > + *
> > > > > + * Drivers inform the sink about what colorimetry, transfer
> > > > > + * characteristics, YCbCr conversion, and quantization range to
> > > > > expect
> > > > > + * (this can depend on the output mode, output format and other
> > > > > + * properties). Drivers also convert the user space provided data
> > > > > to what
> > > > > + * the sink expects.
> > > >
> > > > Hi Sebastian,
> > > >
> > > > should it be more explicit that drivers are allowed to do only
> > > > RGB->YCbCr and quantization range conversions, but not TF nor gamut
> > > > conversions?
> > > >
> > > > That is, if the driver cannot pick the TF implied by "Colorspace"
> > > > property for the sink, then it cannot pick another TF for the sink and
> > > > silently convert. It think this should apply to all options including
> > > > the undefined ones. Or is that too much to guess?
> > >
> > > That's a really good point. I'll add it in the next revision.
> > >
> > > > > + *
> > > > > + * User space has to check if the sink supports all of the possible
> > > > > + * colorimetries that the driver is allowed to pick by parsing the
> > > > > EDID.
> > > >
> > > > All? Rather than at least one?
> > > >
> > > > Is this how it has been implemented for BT2020, that userspace picked
> > > > colorimetry and driver picked color model and quantization are
> > > > completely independent, and drivers do not check the combination
> > > > against EDID?
> > >
> > > AFAIK the driver exposes all Colorspace variants that it can support in
> > > the driver, independent of the sink. That means user space has to make
> > > sure that the sink supports all colorimetry variants the driver can
> > > pick.
> >
> > I didn't mean exposing but the driver could reject the atomic commit
> > that would lead to a combination not advertised as supported in EDID.
> > If drivers reject, then userspace does not need to check for all
> > driver-choosable variants, just one would be enough. Theoretically not
> > needing all might allow some cases to work that don't support all.
> > "Colorspace" property value could direct the driver's choice based on
> > what EDID claims to support.
>
> Right, this could be possible and is probably even better than what I
> wrote down but...
>
> > Of course, if drivers don't do that already, then "all" it must be.
>
> ...unfortunately that seems to be the case. Maybe we can get away with
> changing it though?
I wouldn't risk it at this point, I think :-)
It's another battle.
Thanks,
pq
> > > Would be good to get a confirmation from Harry and Ville.
> > >
> > > > If so, "all" it is. Would be good to explain this in the commit
> > > > message.
> > >
> > > Will do.
> > >
> > > > > + *
> > > > > + * For historical reasons this property exposes a number of
> > > > > variants which
> > > > > + * result in undefined behavior.
> > > > > + *
> > > > > + * Default:
> > > > > + * The behavior is driver-specific.
> > > > > + * BT2020_RGB:
> > > > > + * BT2020_YCC:
> > > > > + * User space configures the pixel operation properties to
> > > > > produce
> > > > > + * RGB content with Rec. ITU-R BT.2020 colorimetry, Rec.
> > > > > + * ITU-R BT.2020 (Table 4, RGB) transfer characteristics
> > > > > and full
> > > > > + * quantization range.
> > > > > + * User space can use the HDR_OUTPUT_METADATA property to
> > > > > set the
> > > > > + * transfer characteristics to PQ (Rec. ITU-R BT.2100
> > > > > Table 4) or
> > > > > + * HLG (Rec. ITU-R BT.2100 Table 5) in which case, user
> > > > > space
> > > > > + * configures pixel operation properties to produce
> > > > > content with
> > > > > + * the respective transfer characteristics.
> > > > > + * User space has to make sure the sink supports Rec.
> > > > > + * ITU-R BT.2020 R'G'B' and Rec. ITU-R BT.2020 Y'C'BC'R
> > > > > + * colorimetry.
> > > > > + * Drivers can configure the sink to use an RGB format,
> > > > > tell the
> > > > > + * sink to expect Rec. ITU-R BT.2020 R'G'B' colorimetry
> > > > > and convert
> > > > > + * to the appropriate quantization range.
> > > > > + * Drivers can configure the sink to use a YCbCr format,
> > > > > tell the
> > > > > + * sink to expect Rec. ITU-R BT.2020 Y'C'BC'R colorimetry,
> > > > > convert
> > > > > + * to YCbCr using the Rec. ITU-R BT.2020 non-constant
> > > > > luminance
> > > > > + * conversion matrix and convert to the appropriate
> > > > > quantization
> > > > > + * range.
> > > > > + * The variants BT2020_RGB and BT2020_YCC are equivalent
> > > > > and the
> > > > > + * driver chooses between RGB and YCbCr on its own.
> > > > > + * SMPTE_170M_YCC:
> > > > > + * BT709_YCC:
> > > > > + * XVYCC_601:
> > > > > + * XVYCC_709:
> > > > > + * SYCC_601:
> > > > > + * opYCC_601:
> > > > > + * opRGB:
> > > > > + * BT2020_CYCC:
> > > > > + * DCI-P3_RGB_D65:
> > > > > + * DCI-P3_RGB_Theater:
> > > > > + * RGB_WIDE_FIXED:
> > > > > + * RGB_WIDE_FLOAT:
> > > > > + * BT601_YCC:
> > > > > + * The behavior is undefined.
> > > > > *
> > > > > * Because between HDMI and DP have different colorspaces,
> > > > > * drm_mode_create_hdmi_colorspace_property() is used for HDMI
> > > > > connector and
> > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > index fe88d7fc6b8f..02c42b01a3a7 100644
> > > > > --- a/include/drm/drm_connector.h
> > > > > +++ b/include/drm/drm_connector.h
> > > > > @@ -437,14 +437,6 @@ enum drm_privacy_screen_status {
> > > > > *
> > > > > * DP definitions come from the DP v2.0 spec
> > > > > * HDMI definitions come from the CTA-861-H spec
> > > > > - *
> > > > > - * A note on YCC and RGB variants:
> > > > > - *
> > > > > - * Since userspace is not aware of the encoding on the wire
> > > > > - * (RGB or YCbCr), drivers are free to pick the appropriate
> > > > > - * variant, regardless of what userspace selects. E.g., if
> > > > > - * BT2020_RGB is selected by userspace a driver will pick
> > > > > - * BT2020_YCC if the encoding on the wire is YUV444 or YUV420.
> > > > > *
> > > > > * @DRM_MODE_COLORIMETRY_DEFAULT:
> > > > > * Driver specific behavior.
> > > >
> > > > This looks really good. This also makes me need to revisit the Weston
> > > > series I've been brewing that adds "Colorspace" KMS support.
> > > >
> > > > I think the two questions I had may be slightly too much in the
> > > > direction of improving rather than just documenting this property, so
> > > > I'll already give
> > > >
> > > > Reviewed-by: Pekka Paalanen <pekka.paala...@collabora.com>
> > >
> > > Thanks.
> > >
> > > >
> > > > Thanks,
> > > > pq
> > >
> > >
> >
>
>
pgpios2EdwAoe.pgp
Description: OpenPGP digital signature
