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

[Michel Dänzer]

On 2018-06-08 12:21 AM, Leo Li wrote:
> 
> 
> On 2018-06-06 01:03 PM, Michel Dänzer wrote:
>> On 2018-06-06 06:01 PM, Michel Dänzer wrote:
>>> 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.
>>
>> Actually, looks like I made a mistake in my testing before; this issue
>> only occurs as of patch 6 of this series.
>>
> 
> It seems to be caused by legacy gamma being disabled on depth 30. When
> that's the case, the gamma_set() hook is set to null. RandR won't
> compute the legacy LUT, causing LUT interpolation/composition to spit
> out an invalid LUT. I verified this by commenting out the `pScrn->depth
> == 30` conditionals guarding the legacy gamma features (in pre_init, and
> mode_major).
> 
> I'm not certain of the best way to fix this. Would it make sense to
> enable legacy gamma on 30bpp if non-legacy is supported? We can do that
> since legacy gamma gets interpolated/composed to non-legacy.
> 
> However, it doesn't make sense when you look at the LUT size, since
> legacy gamma supports only 256 elements (should be 1024 on depth 30?)
> In which case we can skip interpolation/composition altogether.

Right, we'll probably need to increase the RandR LUT size as well at
depth 30.


Anyway, let's not hold up this patch series over this. Simply keep all
this code disabled (i.e. same as if the kernel didn't expose the
properties) at depth 30 for now.


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