On 2022-01-02 23:11, Modem, Bhanuprakash wrote: >> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F90825%2F&data=04%7C01%7Charry.wentland%40amd.com%7C6d85b55209204b9b1e6108d9ce6f30f6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637767799222764784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=j%2BkGUD45bTIke%2FIeCO7GGAM1n%2FCrF2oy6CKfLc6jhzk%3D&reserved=0 >>> >>> ... >>> >>>>> 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. > No worries. > 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. > I think that's fine for AMD HW. For us all planes are the same. Harry > 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 >>> >
