[PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent

[Leo Li]

On 2018-04-11 04:39 AM, Michel Dänzer wrote:
> On 2018-04-10 08:02 PM, Leo Li wrote:
>> On 2018-04-09 11:03 AM, Michel Dänzer wrote:
>>> On 2018-03-26 10:00 PM, sunpeng.li at amd.com wrote:
>>>> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com>
>>>>
>>>> In cases where CRTC properties are updated without going through
>>>> RRChangeOutputProperty, we don't update the properties in user land.
>>>>
>>>> Consider setting legacy gamma. It doesn't go through
>>>> RRChangeOutputProperty, but modifies the CRTC's color management
>>>> properties. Unless they are updated, the user properties will remain
>>>> stale.
>>>
>>> Can you describe a bit more how the legacy gamma and the new properties
>>> interact?
>>>
>>
>> Sure thing, I'll include this in the message for v2:
>>
>> In kernel, the legacy set gamma interface is essentially an adapter to
>> the non-legacy set properties interface. In the end, they both set the
>> same property to a DRM property blob, which contains the gamma lookup
>> table. The key difference between them is how this blob is created.
>>
>> For legacy gamma, the kernel takes 3 arrays from user-land, and creates
>> the blob using them. Note that a blob is identified by it's blob_id.
>>
>> For non-legacy gamma, the kernel takes a blob_id from user-land that
>> references the blob. This means user-land is responsible for creating
>> the blob.
>>
>>  From the perspective of RandR, this presents some problems. Since both
>> paths modify the same property, RandR must keep the reported property
>> value up-to-date with which ever path is used:
>>
>> 1. Legacy gamma via
>> xrandr --output <output_here> --gamma x:x:x
>> 2. Non-legacy color properties via
>> xrandr --output <output_here> --set GAMMA_LUT <blob_id>
>>
>> Keeping the value up-to-date isn't a problem for 2, since RandR updates
>> it for us as part of changing output properties.
>>
>> But if 1 is used, the property blob is created within kernel, and RandR
>> is unaware of the new blob_id. To update it, we need to ask kernel about
>> it.
>>
>> --- continue with rest of message ---
>>>
>>>> Therefore, add a function to update user CRTC properties by querying
>>>> DRM,
>>>> and call it whenever legacy gamma is changed.
>>>
>>> Note that drmmode_crtc_gamma_do_set is called from
>>> drmmode_set_mode_major, i.e. on every modeset or under some
>>> circumstances when a DRI3 client stops page flipping.
>>>
>>
>> The property will have to be updated each time the legacy set gamma
>> ioctl is called, since a new blob (with a new blob_id) is created each
>> time.
>>
>> Not sure if this is a good idea, but perhaps we can have a flag that
>> explicitly enable one or the other, depending on user preference? A
>> user-only property with something like:
>>
>> 0: Use legacy gamma, calls to change non-legacy properties are ignored.
>> 1: Use non-legacy, calls to legacy gamma will be ignored.
>>
>> On 0, we can remove/disable all non-legacy properties from the property
>> list, and avoid having to update them. On 1, we'll enable the
>> properties, and won't have to update them either since legacy gamma is
>> "disabled". It has the added benefit of avoiding unexpected legacy gamma
>> sets when using non-legacy, and vice versa.
> 
> Hmm. So either legacy or non-legacy clients won't work at all, or
> they'll step on each other's toes, clobbering the HW gamma LUT from each
> other.
> 
> I'm afraid neither of those alternatives sound like a good user
> experience to me.
> 
> Consider on the one hand something like Night Light / redshift, using
> legacy APIs to adjust colour temperature to the time of day. On the
> other hand, another client using the non-legacy API for say fine-tuning
> of a display's advanced gamma capabilities.
> 
> Ideally, in this case the gamma LUTs from the legacy and non-legacy APIs
> should be combined, such that the hardware LUT reflects both the colour
> temperature set by Night Light / refshift and the fine-tuning set by the
> non-legacy client. Is that feasible? If not, can you explain a little why?
> 

Hmm, combining LUTs could be feasible, but I don't think that's the
right approach.

LUTs can be combined through composition h(x) = f(g(x)), with some
interpolation involved. Once combined, it can be set using the
non-legacy API, since that'll likely have a larger LUT size.

But I think what you've mentioned raises a bigger issue of color
management conflicts in general. It doesn't have to be redshift vs
monitor correction, what if there more than 2 applications contending to
set gamma? xrandr's brightness already conflicts with redshift, and so
does some apps running on WINE. Ultimately, any (legacy or not) gamma
set requests are unified into one CRTC property in DRM, and they will
all conflict if not managed. I don't think combining two LUTs will help
here.

For the small example at hand, the ideal solution is to have redshift
use the color transformation matrix (CTM), which will be exposed as part
of the non-legacy color API. It'll need modification of redshift, but at
least it won't conflict with anything gamma related.

----

Jumping back on some patch 1 topics:

Are there ways to get arbitrarily (no more than 4096 elements) sized
arrays from a client, to the DDX driver? Otherwise, I'm not sure if
there's another way to expose these properties, short of modifying the
RandR interface. I agree, it would definitely be nicer for clients to
not worry about DRM blobs at all.

Leo

>