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

[Harry Wentland]

On 2022-01-02 23:11, Modem, Bhanuprakash wrote:
>> From: Harry Wentland <harry.wentland at amd.com>
>> Sent: Monday, November 29, 2021 8:50 PM
>> To: Pekka Paalanen <ppaalanen at gmail.com>
>> Cc: dri-devel at lists.freedesktop.org; Modem, Bhanuprakash
>> <bhanuprakash.modem at intel.com>; igt-dev at lists.freedesktop.org; Kumar,
>> Mukunda Pramodh <mukunda.pramodh.kumar at intel.com>; Juha-Pekka Heikkila
>> <juhapekka.heikkila at gmail.com>; Shankar, Uma <uma.shankar at intel.com>
>> Subject: Re: [i-g-t 00/14] Add IGT support for plane color management
>>
>> On 2021-11-29 04:20, Pekka Paalanen wrote:
>>> On Fri, 26 Nov 2021 11:54:55 -0500
>>> Harry Wentland <harry.wentland at 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 at 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F90825%2F&data=04%7C01%7Charry.wentland%40amd.com%7C6d85b55209204b9b1e6108d9ce6f30f6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637767799222764784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=j%2BkGUD45bTIke%2FIeCO7GGAM1n%2FCrF2oy6CKfLc6jhzk%3D&reserved=0
>>>
>>> ...
>>>
>>>>> 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.
> 
> Thanks Pekka & Harry for the review. Apologies for late response. I thought
> that everyone is in holidays 😊. Now I am re-igniting this discussion.
> 

No worries.

> I have gone through all review comments and it's make sense to remove hardware
> specific stuff from the helper functions.
> 
> Patch 3:
> Intel hardware is expecting first LUT value as 0, still we can remove this logic
> from helper & handle in the subtest.
> 
> Patch 4 & 8:
> In this context, for you HDR & SDR plane stuff is just a matter of plane index.
> We are expecting to run tests on one HDR plane (index 0-3) & one SDR plane
> (index 3-6). I think we can update the logic to run tests on first & last plane.
> If you want to run on tests on all planes we need to pass an extra argument through
> command line. Please refer tests/kms_flip.c for similar implementation.
> 

I think that's fine for AMD HW. For us all planes are the same.

Harry

> Patch 12:
> It seems there is some confusion with the IGT comments & variable names, I'll
> refactor the logic & upload a new rev.
> 
> - Bhanu
> 
>>
>> Harry
>>
>>>
>>> Thanks,
>>> pq
>>>
>