[PATCH v3 02/17] drm/connector: Add enum documentation to drm_colorspace

[Sebastian Wick]

On Thu, Mar 9, 2023 at 11:03 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> On Thu, 9 Mar 2023 01:56:11 +0100
> Sebastian Wick <sebastian.wick at redhat.com> wrote:
>
> > On Wed, Mar 8, 2023 at 9:59 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > >
> > > On Tue, 7 Mar 2023 10:10:52 -0500
> > > Harry Wentland <harry.wentland at amd.com> wrote:
> > >
> > > > From: Joshua Ashton <joshua at froggi.es>
> > > >
> > > > To match the other enums, and add more information about these values.
> > > >
> > > > v2:
> > > >  - Specify where an enum entry comes from
> > > >  - Clarify DEFAULT and NO_DATA behavior
> > > >  - BT.2020 CYCC is "constant luminance"
> > > >  - correct type for BT.601
> > > >
> > > > Signed-off-by: Joshua Ashton <joshua at froggi.es>
> > > > Signed-off-by: Harry Wentland <harry.wentland at amd.com>
> > > > Reviewed-by: Harry Wentland <harry.wentland at amd.com>
> > >
> > > Hi,
> > >
> > > this effort is really good, but of course I still find things to
> > > nitpick about. If there is no answer to my questions, then I would
> > > prefer the documentation to spell out the unknowns and ambiguities.
> > >
> > > > Cc: Pekka Paalanen <ppaalanen at gmail.com>
> > > > Cc: Sebastian Wick <sebastian.wick at redhat.com>
> > > > Cc: Vitaly.Prosyak at amd.com
> > > > Cc: Uma Shankar <uma.shankar at intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Cc: Joshua Ashton <joshua at froggi.es>
> > > > Cc: dri-devel at lists.freedesktop.org
> > > > Cc: amd-gfx at lists.freedesktop.org
> > > > ---
> > > >  include/drm/drm_connector.h | 67 +++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 65 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index 6d6a53a6b010..bb078666dc34 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -363,13 +363,76 @@ enum drm_privacy_screen_status {
> > > >       PRIVACY_SCREEN_ENABLED_LOCKED,
> > > >  };
> > > >
> > > > -/*
> > > > - * This is a consolidated colorimetry list supported by HDMI and
> > > > +/**
> > > > + * enum drm_colorspace - color space
> > > > + *
> > > > + * This enum is a consolidated colorimetry list supported by HDMI and
> > > >   * DP protocol standard. The respective connectors will register
> > > >   * a property with the subset of this list (supported by that
> > > >   * respective protocol). Userspace will set the colorspace through
> > > >   * a colorspace property which will be created and exposed to
> > > >   * userspace.
> > > > + *
> > > > + * DP definitions come from the DP v2.0 spec
> > > > + * HDMI definitions come from the CTA-861-H spec
> > > > + *
> > > > + * @DRM_MODE_COLORIMETRY_DEFAULT:
> > > > + *   Driver specific behavior.
> > > > + *   For DP:
> > > > + *           RGB encoded: sRGB (IEC 61966-2-1)
> > > > + *           YCbCr encoded: ITU-R BT.601 colorimetry format
> > >
> > > Does this mean that HDMI behavior is driver-specific while DP behavior
> > > is as defined?
> > >
> > > Is it intentional that YCbCr encoding also uses different RGB-primaries
> > > than RGB-encoded signal? (BT.601 vs. BT.709/sRGB)
> > >
> > > Or do you need to be more explicit on which parts of each spec apply
> > > (ColourPrimaries vs. TransferCharacteristics vs. MatrixCoefficients in
> > > CICP parlance)?
> > >
> > > E.g. BT.709/sRGB ColourPrimaries with BT.601 MatrixCoefficients.
> >
> > Yeah, just adding to this: The Default Colorspace is something well
> > defined. CTA-861 says:
> >
> > "If bits C0 and C1 are zero, the colorimetry shall correspond to the
> > default colorimetry defined in Section 5.1"
> >
> > and in Section 5.1
> >
> > "In all cases described above, the RGB color space used should be the
> > RGB color space the Sink declares in the Basic Display Parameters and
> > Feature Block of its EDID."
> >
> > If I set DRM_MODE_COLORIMETRY_DEFAULT, I expect the Colorimetry the
> > EDID reports to be in effect and not some driver specific nonsense.
>
> Does that also define the MatrixCoefficients for YCbCr signal with
> DRM_MODE_COLORIMETRY_DEFAULT?

Good question. It doesn't seem like it does, which would make
supporting YCC with the default color space impossible.

> Not that userspace would even care, since RGB-to-YCbCr is all
> driver-internal.
>
> It is interesting you point that out. I guess it means that the basic
> colorimetry from EDID is supposed to be really only the default
> colorimetry and might not have anything to do with the actual panel
> primaries.
>
>
> Thanks,
> pq