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

From: "Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@public.gmane.org>
To: sunpeng.li-5C7GfCeVMHo@public.gmane.org
Cc: harry.wentland-5C7GfCeVMHo@public.gmane.org,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3
Date: Thu, 7 Jun 2018 09:36:16 +0200	[thread overview]
Message-ID: <7ca3df26-5606-19c9-c289-b3bb43d51dfa@daenzer.net> (raw)
In-Reply-To: <e47b6ff4-9696-bf20-d299-687b96ab0437-otUistvHUpPR7s880joybQ@public.gmane.org>

On 2018-06-06 07: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@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@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.

For this series, the simplest solution for now is probably to continue
not setting any gamma values at depth 30, and probably not expose the
colour management properties either. We can worry about making this all
work at depth 30 later.

-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx