On Fri, 26 Nov 2021 11:54:55 -0500 Harry Wentland <harry.wentland@xxxxxxx> wrote: > On 2021-11-18 04:50, Pekka Paalanen wrote: > > On Mon, 15 Nov 2021 15:17:45 +0530 > > Bhanuprakash Modem <bhanuprakash.modem@xxxxxxxxx> 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:
pgpAN_mzfqrgk.pgp
Description: OpenPGP digital signature
