> From: Harry Wentland <harry.wentland@xxxxxxx> > Sent: Monday, November 29, 2021 8:50 PM > To: Pekka Paalanen <ppaalanen@xxxxxxxxx> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Modem, Bhanuprakash > <bhanuprakash.modem@xxxxxxxxx>; igt-dev@xxxxxxxxxxxxxxxxxxxxx; Kumar, > Mukunda Pramodh <mukunda.pramodh.kumar@xxxxxxxxx>; Juha-Pekka Heikkila > <juhapekka.heikkila@xxxxxxxxx>; Shankar, Uma <uma.shankar@xxxxxxxxx> > 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@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 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 > >
