[PATCH xf86-video-amdgpu 00/13] Enabling Color Management - Round 2

[Leo Li]

On 2018-05-16 01:06 PM, Michel Dänzer wrote:
> On 2018-05-03 08:31 PM, sunpeng.li at amd.com wrote:
>>
>> 3. The three color management properties (Degamma LUT, Color Transform Matrix
>>     (CTM), and Gamma LUT) are hard-coded into the DDX driver, to be listed (as
>>     disabled) regardless of whether a CRTC is attached on the output, or whether
>>     the kernel driver supports it.
>>
>>      * If kernel driver does not support color management, the properties will
>>        remain disabled. A `xrandr --set` will then error.
> 
> Is it really useful to expose these properties to clients if the kernel
> doesn't support them?
> 

I left them exposed mainly for simplicity. I can see how it would
confuse a client.

It should be simpler to hide these properties once the color property
IDs are cached somewhere (maybe on the AMDGPUInfo struct?) during
pre_init. It'll provide a easy way to determine if there's kernel
support within other functions.

> 
>> 4. Color properties are now *staged* inside the driver-private CRTC object.
>>     This allows us to *push* the properties into kernel DRM (and consequently
>>     into hardware) whenever there is a need.
>>
>>      * Along with staging and pushing, an *update* function is used to notify
>>        RandR to update the color properties listed on outputs. This can be used
>>        when `xrandr --auto` enables a CRTC on an output, and the output need to
>>        reflect the CRTC's color properties.
> 
> I feel like some of this is a bit more complicated than necessary. This
> is how I'd envision it working:
> 
> In drmmode_crtc_init, query the properties from the kernel, more or less
> as done in this series.
> 
> In drmmode_output_create_resources, create the output properties (or
> not, per above) based on output->crtc, or if that is NULL, based on
> xf86_config->crtc[0].

What are the contents of xf86_config->crtc[0] initialized to? I'm not
too familiar with how that's setup. Does it stay the same throughout?

> 
> In drmmode_output_get_property, if output->crtc != NULL, update the
> RandR property value from it, otherwise set it to a dummy value.
> 

Feeling kind of stupid that I skipped over this :) I assumed it wasn't
useful since it's currently a no-op. This should eliminate the need for
the cm_update function, assuming it's called every time a client
requests for the properties.

This should work even if one CRTC is attached to multiple outputs,
correct? Since it would first update the RandR property with the
attached CRTC before returning it.

> In drmmode_output_set_property, if output->crtc != NULL, update its
> property value, otherwise do nothing.
> 
> Push the CRTC's property values to the kernel in
> drmmode_crtc_gamma_do_set and drmmode_output_set_property (or maybe just
> call the former from the latter when one of these properties are changed).
> 
> Is that feasible?
> 

Sounds feasible, and much better actually. Assuming it also works well
with 1 crtc on >1 outputs, and I don't see why it wouldn't. Thanks for
the pointers.

> 
> If patch 13 still exists after this, can you squash it into the earlier
> patches adding the code?
> 
> 
>> * When using libXrandr to set the CTM property, 16bit format is used. Ideally
>>    we should use 64bit format, since the CTM consists of 9x64bit fixed-point
>>    values. However, it isn't recognized by XRRChangeOutputProperty as a valid
>>    format. 32 bit could work, since the CTM values are S31.32 fixed-point.
>>    However, using this format corrupts the data once it gets to xserver. On
>>    first glance, it may be the cast to long within XRRChangeOutputProperty, in
>>    addition to compiling 64 bit (shouldn't it use int32_t instead?).
> 
> For historical reasons, Xlib uses long for 32-bit values, so you have to
> pad each 32-bit value to a long. XCB shouldn't be affected by this.
> 

Noted.

Leo

>