Re: [PATCH 10/24] drm/amd/display: Rework CRTC color management

From: "Kazlauskas, Nicholas" <Nicholas.Kazlauskas-5C7GfCeVMHo@public.gmane.org>
To: "Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@public.gmane.org>
Cc: "Lakha,
	Bhawanpreet" <Bhawanpreet.Lakha-5C7GfCeVMHo@public.gmane.org>,
	"amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org"
	<amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Subject: Re: [PATCH 10/24] drm/amd/display: Rework CRTC color management
Date: Fri, 7 Jun 2019 16:06:28 +0000	[thread overview]
Message-ID: <a1ffad75-d51a-5086-a0ee-9533bde8a921@amd.com> (raw)
In-Reply-To: <3e5b0c1b-0958-8c39-0030-56c5cdf53574-otUistvHUpPR7s880joybQ@public.gmane.org>

On 6/7/19 11:51 AM, Michel Dänzer wrote:
> On 2019-06-07 2:26 p.m., Kazlauskas, Nicholas wrote:
>> On 6/7/19 3:58 AM, Michel Dänzer wrote:
>>> On 2019-06-06 10:54 p.m., Bhawanpreet Lakha wrote:
>>>> From: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>>>
>>>> [Why]
>>>> To prepare for the upcoming DRM plane color management properties
>>>> we need to correct a lot of wrong behavior and assumptions made for
>>>> CRTC color management.
>>>>
>>>> The documentation added by this commit in amdgpu_dm_color explains
>>>> how the HW color pipeline works and its limitations with the DRM
>>>> interface.
>>>>
>>>> The current implementation does the following wrong:
>>>> - Implicit sRGB DGM when no CRTC DGM is set
>>>> - Implicit sRGB RGM when no CRTC RGM is set
>>>> - No way to specify a non-linear DGM matrix that produces correct output
>>>> - No way to specify a correct RGM when a linear DGM is used
>>>>
>>>> We had workarounds for passing kms_color tests but not all of the
>>>> behavior we had wrong was covered by these tests (especially when
>>>> it comes to non-linear DGM). Testing both DGM and RGM at the same time
>>>> isn't something kms_color tests well either.
>>>>
>>>> [How]
>>>> The specifics for how color management works in AMDGPU and the new
>>>> behavior can be found by reading the documentation added to
>>>> amdgpu_dm_color.c from this patch.
>>>>
>>>> All of the incorrect cases from the old implementation have been
>>>> addressed for the atomic interface, but there still a few TODOs for
>>>> the legacy one.
>>>>
>>>> Note: this does cause regressions for kms_color@pipe-a-ctm-* over HDMI.
>>>>
>>>> The result looks correct from visual inspection but the CRC no longer
>>>> matches. For reference, the test was previously doing the following:
>>>>
>>>> linear degamma -> CTM -> sRGB regamma -> RGB to YUV (709) -> ...
>>>>
>>>> Now the test is doing:
>>>>
>>>> linear degamma -> CTM -> linear regamma -> RGB to YUV (709) -> ...
>>>>
>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>>> Reviewed-by: Sun peng Li <Sunpeng.Li@amd.com>
>>>> Acked-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
>>>
>>> Does this address https://bugs.freedesktop.org/110677 ? If so, can you
>>> add a reference to the commit log?
>>
>> It unfortunately does not. There are still some remaining issues with
>> legacy gamma support that I intend to address at some point - which I
>> left in this patch as TODOs.
> 
> Note that the bug reporter is using xf86-video-amdgpu, which no longer
> uses legacy gamma with DC, unless I misunderstand what you mean by that.
> 
> 

While it can use the full LUT, my guess is that we were still getting 
256 entries in DM/DC, which we interpret as legacy at the moment.

FWIW I did actually tried reproducing the issue after applying the patch 
during development and it gave me the same results before and after for 
that specific issue.

Nicholas Kazlauskas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx