[PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

Hi Leo,


On 2018-06-01 06:03 PM, sunpeng.li at amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com>
> 
> This ended up being different enough from v2 to warrant a new patchset. Per
> Michel's suggestions, there have been various optimizations and cleanups.
> Here's what's changed:
> 
> * Cache DRM color management property IDs at pre-init,
>     * instead of querying DRM each time we need to modify color properties.
> 
> * Remove drmmode_update_cm_props().
>     * Update color properties in drmmode_output_get_property() instead.
>     * This also makes old calls to update_cm_props redundant.
> 
> * Get rid of fake CRTCs.
>     * Previously, we were allocating a fake CRTC to configure color props on
>       outputs that don't have a CRTC.
>     * Instead, rr_configure_and_change_cm_property() can be easily modified to
>       accept NULL CRTCs.
> 
> * Drop patches to persist color properties across DPMS events.
>     * Kernel driver should be patched instead:
>       https://lists.freedesktop.org/archives/amd-gfx/2018-May/022744.html
>     * Color props including legacy gamma now persist across crtc dpms.
>     * Non-legacy props now persist across output dpms and hotplug, as long
>       as the same CRTC remains attached to that output.
> 
> And some smaller improvements:
> 
> * Change CTM to be 32-bit format instead of 16-bit.
>     * This requires clients to ensure that each 32-bit element is padded to be
>       long-sized, since libXrandr parses 32-bit format as long-typed.
> 
> * Optimized color management init during CRTC init.
>     * Query DRM once for the list of properties, instead of twice.

Sounds good. I'll be going through the patches in detail from now on,
but I don't know yet when I'll be able to finish the review.


Meanwhile, heads up on two issues I discovered while smoke-testing the
series (which are sort of related, but occur even without this series):


Running Xorg in depth 30[0] results in completely wrong colours
(everything has a red tint) with current kernels. I think this is
because DC now preserves the gamma LUT values, but xf86-video-amdgpu
never sets them at depth 30, so the hardware is still using values for
24-bit RGB.

Can you look into making xf86-video-amdgpu set the LUT values at depth
30 as well? Ideally in a way which doesn't require all patches in this
series, so that it could be backported for a 18.0.2 point release if
necessary. (Similarly for skipping drmmode_cursor_gamma when the kernel
supports the new colour management properties, to fix
https://bugs.freedesktop.org/106578)


Trying to run Xorg in depth 16 or 8[0] results in failure to set any mode:

[    56.138] (EE) AMDGPU(0): failed to set mode: Invalid argument
[    56.138] (WW) AMDGPU(0): Failed to set mode on CRTC 0
[    56.172] (EE) AMDGPU(0): Failed to enable any CRTC

Works fine with amdgpu.dc=0. This has been broken at least since the
4.16 development cycle.


[0] You can change Xorg's colour depth either via -depth on its command
line, or via the xorg.conf screen section:

Section "Screen"
        Identifier "Screen0"
        DefaultDepth 30 # or 16 or 8
EndSection


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer