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

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Tue, 13 Dec 2022 11:32:01 -0500
Harry Wentland <harry.wentland@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@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 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@amd.com>
> >> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> >> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> >> Cc: Vitaly.Prosyak@amd.com
> >> Cc: Uma Shankar <uma.shankar@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Joshua Ashton <joshua@froggi.es>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: amd-gfx@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(-)
> >>

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