Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Tue, 11 May 2021 12:03:30 +0200
Werner Sembach <wse@tuxedocomputers.com> wrote:

> Am 11.05.21 um 10:07 schrieb Pekka Paalanen:
> > On Mon, 10 May 2021 17:47:01 -0400
> > Alex Deucher <alexdeucher@gmail.com> wrote:
> >  
> >> On Fri, May 7, 2021 at 3:27 PM Werner Sembach <wse@tuxedocomputers.com> wrote:  
> >>> xrandr --prop and other userspace info tools have currently no way of
> >>> telling which color configuration is used on HDMI and DP ports.
> >>>
> >>> The ongoing transsition from HDMI 1.4 to 2.0 and the different bandwidth
> >>> requirements of YCbCr 4:2:0 and RGB color format raise different
> >>> incompatibilities. Having these configuration information readily
> >>> available is a useful tool in debuging washed out colors, color artefacts
> >>> on small fonts and missing refreshrate options.    
> >> I think we would ideally want these as generic connector properties
> >> rather than AMD specific ones since they are not really AMD specific.
> >> I believe there is already a generic drm property (max_bpc) for the
> >> color depth.  At this point, I think having a generic RGB vs YCbCr
> >> property would make sense.  I'm not sure about the color space.  
> 
> Problem is: amdgpu does not really use generic structs for these 3
> properties as far as I can tell. It uses own defines
> https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/amd/display/dc/dc_hw_types.h#L647
> in own structs
> https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/amd/display/dc/dc_stream.h#L141
> 
> Intel uses generic defines
> https://elixir.bootlin.com/linux/v5.13-rc1/source/include/linux/hdmi.h#L71
> https://elixir.bootlin.com/linux/v5.13-rc1/source/include/drm/drm_dp_helper.h#L1568
> split up between dp and hdmi in own structs
> https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/i915/display/intel_display_types.h#L879
> 
> So the property would need some translation from amd, intel, hdmi,
> and dp to one enumeration representing all?

If so, much better to do that in the kernel in one place per driver
that actually know what they mean, rather than in half of the Wayland
compositors by people who barely understand even the general concept.
Like me.

> > Hi,
> >
> > I believe that userspace will definitely want to know what exactly is
> > going on on the monitor cable, so I think exposing more information is
> > good, but I agree that it needs to be done with generic KMS properties
> > as much as possible. Userspace is not going to bother having explicit
> > code for driver-specific properties.
> >
> > I think a major use case will be Wayland color management, where a
> > Wayland compositor will want to make sure that the video signal
> > parameters have not changed since the monitor was last measured
> > (profiled). If the signal configuration is different, the measured
> > color profile may be invalid and therefore the end user needs to be
> > warned. See some ideas in
> > https://gitlab.freedesktop.org/wayland/weston/-/issues/467
> > under the heading "Color calibration auditing system".
> >
> > About the color space: is that something a kernel driver will decide
> > on its own? I mean in the same sense as the driver will negotiate
> > HDMI/DP link parameters, perhaps falling back to smaller requirements
> > if higher requirements signal does not seem to work.
> >
> > We only need readback properties for things that generic userspace
> > won't or cannot control explicitly, e.g. because the kernel driver has
> > room to make a choice rather than fail.  
> 
> Some explanation why I choose these 3 properties:
> 
> output color space: Mainly to see if full or limited RGB was chosen.

IOW, the driver makes the decision. Therefore userspace will need to
know what it picked. Ok.

> While in theory the driver should choose the right one automatically,
> I read articles that in some cases it doesn't (hence why the
> "Broadcast RGB" property for intel-gfx driver and "output_csc" for
> the old radeon driver exist). The next step ofc would be to bring
> over the "Broadcast RGB" property to amdgpu/make it a generic
> property. But then still: having a feedback channel to see if the
> chosen setting got correctly applied should not hurt in any way,
> shape, or form?

Feedback is good IMO, yes, when it's not clearly redundant. As long as
you don't tie the driver developers' hands in a knot or preclude
support for hardware not invented yet. But I think the option to not
expose a specific KMS property is an escape hatch enough. You might
have problems with the requirements for DRM UAPI additions though,
since they call for a proper userspace consumer.

What defines what is "the right one"?

I believe that we also need to aim for a fully known display pipeline,
so if the driver is doing compression from full range 8-bit to limited
range 8-bit, losing a little precision, userspace needs to know. This
is a long stretch and maybe not even fully feasible, but I think it
would be good to get as close as possible - within the limits of being
driver-agnostic UAPI.

> pixel encoding: Probably the most important of the 3: This should be
> accompanied with a "preferred pixel encoding" user controllable
> setting and is mainly thought as a feedback channel for that, because
> it might not always the obvious if the Display + GPU + Driver + Link
> encoder combination actually supports and therefore applies the
> selected "preferred pixel encoding". For example: I have a display
> here that can display 4k@60Hz or WQHD@120Hz, but YCbCr is only
> supported by it for 4k@60Hz (also it's not supported for 4k30Hz).

This feedback is useful, IMO.

The alternative approach would be to just let KMS atomic commit fail if
the exact given configuration does not work, but it would be really
difficult for userspace to implement the necessary fallbacks and to try
alternative configurations. Therefore I think setting preference and
then checking what the driver actually achieved seems like a good
compromise to me.

> The "preferred pixel encoding" setting is required because certain
> devices (both PC's and display's) might wrongly advertise their
> capabilities. The current fix in this case is to write a custom edid
> which is a kinda hacky solution. Examples: 1. RGB and YCbCr4:4:4 in
> theory carry the same amount of color information, but some displays
> look worse in one or the other, because they do bad internal
> conversion. 2. A laptop wants to output YCbCr4:4:4 but because of bad
> shielding of the port/the cable/the display, the screen goes black
> every few seconds. Using YCbCr4:2:0, and therefore a lower signal
> clock, stabilizes the connection without changing hardware.

That totally makes sense, and example 1 is actually a good reason why
color management needs to be interested about these properties.

> color depth: While "max bpc" can be used to change the color depth,
> there is currently no way to check which one actually gets used.
> While in theory the driver chooses the best/highest bpc within the
> max bpc setting a user might not be fully aware what his hardware is
> or isn't capable off. This is meant as a quick way to double check
> the setup.

Likewise, this is important information for color management.

I'm really happy to see these come up. The Weston CM&HDR work isn't
quite there yet to start making use of these, but some month/year I bet
it will.


Thanks,
pq

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