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

[Michel Dänzer]

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?


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