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

[Leo Li]

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.

> 
>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>> index 1966fd2..45457c4 100644
>> --- a/src/drmmode_display.c
>> +++ b/src/drmmode_display.c
>> @@ -61,8 +61,13 @@
>>   
>>   #define DEFAULT_NOMINAL_FRAME_RATE 60
>>   
>> +/* Forward declarations */
>> +
>>   static Bool drmmode_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height);
>>   
>> +static void drmmode_crtc_update_resources(xf86CrtcPtr crtc);
> 
> Can you move the drmmode_crtc_update_resources such that the forward
> declaration isn't necessary?
> 

Seems possible. It uses the rr_configure_and_change helper, so I'll pull
both of them up.

> 
>>   static Bool
>>   AMDGPUZaphodStringMatches(ScrnInfoPtr pScrn, const char *s, char *output_name)
>>   {
>> @@ -768,6 +773,7 @@ drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
>>   
>>   	drmModeCrtcSetGamma(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
>>   			    size, red, green, blue);
>> +	drmmode_crtc_update_resources(crtc);
>>   }
>>   
>>   Bool
>> @@ -1653,10 +1659,15 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
>>   * Configure and change the given output property through randr. Currently
>>   * ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
>>   *
>> +* @output: The output to configure and change the property on.
>> +* @pmode_prop: The driver-private property object.
> 
> These two should have been added in patch 1.

Yep, will move.

Leo

> 
>