Re: [PATCH xf86-video-amdgpu 10/13] Push staged color properties when DPMS state toggles On

[Leo Li <sunpeng.li-5C7GfCeVMHo@public.gmane.org>]

On 2018-05-18 04:01 AM, Michel Dänzer wrote:
> On 2018-05-17 11:44 PM, Leo Li wrote:
>> On 2018-05-16 01:10 PM, Michel Dänzer wrote:
>>> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>>
>>>> This will persist color management properties on a CRTC across DPMS
>>>> state changes.
>>>>
>>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>>>> ---
>>>>    src/drmmode_display.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>>>> index 45c582c..06ae902 100644
>>>> --- a/src/drmmode_display.c
>>>> +++ b/src/drmmode_display.c
>>>> @@ -1294,6 +1294,7 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>>>        AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
>>>>        CARD64 ust;
>>>>        int ret;
>>>> +    int i;
>>>>          if (drmmode_crtc->dpms_mode == DPMSModeOn && mode !=
>>>> DPMSModeOn) {
>>>>            uint32_t seq;
>>>> @@ -1341,6 +1342,11 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>>>                drmmode_crtc->interpolated_vblanks += delta_seq;
>>>>            }
>>>>    +        for (i = 0; i < CM_NUM_PROPS; i++) {
>>>> +            if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
>>>> +                continue;
>>>> +            drmmode_crtc_push_cm_prop(crtc, i);
>>>> +        }
>>>>        }
>>>>        drmmode_crtc->dpms_mode = mode;
>>>>    }
>>>>
>>>
>>> This and patch 11 smell like workarounds for a kernel issue. The kernel
>>> should preserve the property values regardless of DPMS state.
>>>
>>> This probably explains something I just discovered: the legacy gamma LUT
>>> becomes ineffective after turning a CRTC off and on again with DC,
>>> whereas it's preserved without DC.
>>
>> That's indeed a kernel issue, will look into it. This patch can be
>> dropped once the kernel persists the properties across dpms.
>>
>> In terms of Patch 11, which persists the properties across hotplugs, is
>> it even a valid use-case? I tested with i915 and amdgpu non-dc drivers.
>> Both don't seem to persist legacy gamma across hotplugs, or xrandr
>> --output --off/--auto.
> 
> It persists for me (without this series) with amdgpu without DC on
> xrandr --output --off/--auto, or explicitly moving an output between
> different CRTCs. Those are definitely expected.
> 
> Not sure about hotplug; it's possible that something explicitly
> re-initializes the legacy gamma in that case. But other than that, each
> CRTC should preserve its values unless they are explicitly modified.
> 

Just patched up the kernel to fix this, and the properties indeed
persist like you've described (hotplug as well). Makes sense, since the
properties don't change on the DRM states. We just have to update DC on
these events.

Thanks for your responses, moving on to v3 now.

Leo

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