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. Harry > > Thanks, > pq >
