[PATCH 06/16] drm/connector: Allow drivers to pass list of supported colorspaces

[Alex Deucher]

On Wed, Dec 14, 2022 at 3:56 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> On Tue, 13 Dec 2022 11:32:01 -0500
> Harry Wentland <harry.wentland at amd.com> wrote:
>
> > On 12/13/22 05:23, Pekka Paalanen wrote:
> > > On Mon, 12 Dec 2022 13:21:27 -0500
> > > Harry Wentland <harry.wentland at amd.com> wrote:
> > >
> > >> Drivers might not support all colorspaces defined in
> > >> dp_colorspaces and hdmi_colorspaces. This results in
> > >> undefined behavior when userspace is setting an
> > >> unsupported colorspace.
> > >>
> > >> Allow drivers to pass the list of supported colorspaces
> > >> when creating the colorspace property.
> > >
> > > Hi Harry,
> > >
> > > what is there for drivers to support? Isn't this just infoframe data
> > > that shall be sent down to the sink as-is with no other effect?
> > >
> >
> > You have a good point.
> >
> > Right now the supported colorspaces de-facto depend on driver implementations
> > as you can see in [1] for i915 and [2] for amdgpu. The amdgpu driver will
> > also program the MSA [3] for DP connections, and a bunch of other things which
> > are deeper in the driver.
> >
> > [1] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/i915/display/intel_dp.c#L1741
> > [2] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5155
> > [3] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_stream_encoder.c#L368
> >
> > I don't know why the DP VSC stuff needs to be in drivers. It should be
> > common. The MSA packing would likely have to be driver specific since the
> > mechanism of packing it is HW specific.
>
> What's MSA?

I think it's Main Stream Attribute data.  Part of DP.  See slide 31 of
this document:
https://www.vesa.org/wp-content/uploads/2011/01/ICCE-Presentation-on-VESA-DisplayPort.pdf

Alex

>
> I don't see it in
> https://www.kernel.org/doc/html/latest/gpu/amdgpu/display/dc-glossary.html
> or anywhere under Documentation/gpu or in CTA-861-H.
>
> > I'll have a closer look and see if we can eliminate the "driver supported"
> > bit. If we can't we'll probably need to describe the reasoning better.
>
> That would be nice, thanks!
>
> > Will it be a problem if the list of supported colorspaces differs between
> > drivers?
>
> I do not think so. It's just normal KMS UAPI that one must always
> inspect an enumeration to see what values are possible. Userspace
> cannot use a header with pre-defined numerical values, they always need
> to be introspected first like everything else about KMS properties.
>
> I know there were some opinions about hard-coding enum numerical values
> in headers, but I think in the end everyone agreed to the introspection
> even if it didn't seem useful at the time.
>
> Besides, if a driver never supported a given value but misbehaved or
> refused, I don't think that counts as a kernel regression?
>
>
> Thanks,
> pq
>
> >
> > Harry
> >
> > > Is the driver confusing colorimetry with color-representation (the
> > > RGB-YCbCr conversion)? Or is this property defining both?
> > >
> > > I feel that the documentation of "Colorspace" KMS connector property
> > > needs clarification, and a list of potentially available values with
> > > explanations, more than just a reference to CTA-816-H which it does not
> > > even do yet.
> > >
> > > Perhaps a table, where for each enum drm_colorspace entry has a row explaining:
> > >
> > >
> > > Thanks,
> > > pq
> > >
> > >
> > >> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
> > >> 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
> > >> ---
> > >>  drivers/gpu/drm/drm_connector.c               | 140 +++++++++---------
> > >>  .../gpu/drm/i915/display/intel_connector.c    |   4 +-
> > >>  drivers/gpu/drm/vc4/vc4_hdmi.c                |   2 +-
> > >>  include/drm/drm_connector.h                   |   8 +-
> > >>  4 files changed, 83 insertions(+), 71 deletions(-)
> > >>