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

[Michel Dänzer]

On 2018-05-17 11:43 PM, Leo Li wrote:
> 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?)

drmmode_crtc_private_rec seems better.


>>> 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?

I think so, but my point here is mainly that whether or not the
properties exist, and the size of the LUTs should be the same regardless
of which particular CRTC you look at. So for simplicity,
drmmode_output_create_resources could even always look at
xf86_config->crtc[0] (i.e. the first CRTC).


>> 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.

Yes, I think so.


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