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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3455 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]