Re: [i-g-t 00/14] Add IGT support for plane color management

[Harry Wentland <harry.wentland@amd.com>]

On 2021-11-29 04:20, Pekka Paalanen wrote:
> On Fri, 26 Nov 2021 11:54:55 -0500
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> On 2021-11-18 04:50, Pekka Paalanen wrote:
>>> On Mon, 15 Nov 2021 15:17:45 +0530
>>> Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote:
>>>   
>>>> From the Plane Color Management feature design, userspace can
>>>> take the smart blending decisions based on hardware supported
>>>> plane color features to obtain an accurate color profile.
>>>>
>>>> These IGT patches extend the existing pipe color management
>>>> tests to the plane level.
>>>>
>>>> Kernel implementation:
>>>> https://patchwork.freedesktop.org/series/90825/  
> 
> ...
> 
>>> I also found some things that looked hardware-specific in this code
>>> that to my understanding is supposed to be generic, and some concerns
>>> about UAPI as well.
>>>   
>>
>> I left some comments on intellisms in these patches.
>>
>> Do you have any specifics about your concerns about UAPI?
> 
> Yeah, the comments I left in the patches:
> 
> patch 3:
> 
>> Having to explicitly special-case index zero feels odd to me. Why does
>> it need explicit special-casing?
>>
>> To me it's a hint that the definitions of .start and .end are somehow
>> inconsistent.
> 
> patch 4 and 8:
> 
>>> +static bool is_hdr_plane(const igt_plane_t *plane)
>>> +{
>>> +	return plane->index >= 0 && plane->index < SDR_PLANE_BASE;  
>>
>> How can this be right for all KMS drivers?
>>
>> What is a HDR plane?
> 
> patch 12:
> 
>>> +struct drm_color_lut *coeffs_to_logarithmic_lut(data_t *data,
>>> +						const gamma_lut_t *gamma,
>>> +						uint32_t color_depth,
>>> +						int off)
>>> +{
>>> +	struct drm_color_lut *lut;
>>> +	int i, lut_size = gamma->size;
>>> +	/* This is the maximum value due to 16 bit precision in hardware. */  
>>
>> In whose hardware?
>>
>> Are igt tests not supposed to be generic for everything that exposes
>> the particular KMS properties?
>>
>> This also hints that the UAPI design is lacking, because userspace
>> needs to know hardware specific things out of thin air. Display servers
>> are not going to have hardware-specific code. They specialise based on
>> the existence of KMS properties instead.
> 
> ...
> 
>>> +void set_advance_gamma(data_t *data, igt_pipe_t *pipe, enum gamma_type type)
>>> +{
>>> +	igt_display_t *display = &data->display;
>>> +	gamma_lut_t *gamma_log;
>>> +	drmModePropertyPtr gamma_mode = NULL;
>>> +	segment_data_t *segment_info = NULL;
>>> +	struct drm_color_lut *lut = NULL;
>>> +	int lut_size = 0;
>>> +
>>> +	drmSetClientCap(data->drm_fd, DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES, 1);  
>>
>> Is this how we are going to do cross-software DRM-master hand-over or
>> switching compatibility in general?
>>
>> Add a new client cap for every new KMS property, and if the KMS client
>> does not set the property, the kernel will magically reset it to ensure
>> the client's expectations are met? Is that how it works?
>>
>> Or why does this exist?
> 
> ...
> 
>>> +	drmSetClientCap(data->drm_fd, DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES, 0);  
>>
>> I've never seen this done before. I did not know client caps could be
>> reset.
> 
> 
> So, patch 12 has the biggest UAPI questions, and patch 3 may need a
> UAPI change as well. The comments in patches 4 and 8 could be addressed
> with just removing that code since the concept of HDR/SDR planes does
> not exist in UAPI. Or, if that concept is needed then it's another UAPI
> problem.
> 

Thanks for reiterating your points. I missed your earlier replies and
found them in my IGT folder just now.

Looks like we're on the same page.

Harry

> 
> Thanks,
> pq
>