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

[Modem, Bhanuprakash]

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

Thanks Pekka & Harry for the review. Apologies for late response. I thought
that everyone is in holidays 😊. Now I am re-igniting this discussion.

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.

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