| Message ID | 20211115094759.520955-1-bhanuprakash.modem@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add IGT support for plane color management | expand |
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/ Hi, it's really good to get these, but I am worried that the tests here may be too easy to pass when trying to ensure the KMS features work correctly and in the way real userspace is going to be using them. 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. Thanks, pq > Bhanuprakash Modem (11): > HAX: Get uapi headers to compile the IGT > lib/igt_kms: Add plane color mgmt properties > kms_color_helper: Add helper functions for plane color mgmt > tests/kms_color: New subtests for Plane gamma > tests/kms_color: New subtests for Plane degamma > tests/kms_color: New subtests for Plane CTM > tests/kms_color: New negative tests for plane level color mgmt > tests/kms_color_chamelium: New subtests for Plane gamma > tests/kms_color_chamelium: New subtests for Plane degamma > tests/kms_color_chamelium: New subtests for Plane CTM > tests/kms_color_chamelium: Extended IGT tests to support logarithmic > gamma mode > > Mukunda Pramodh Kumar (3): > lib/igt_kms: Add pipe color mgmt properties > kms_color_helper: Add helper functions to support logarithmic gamma > mode > tests/kms_color: Extended IGT tests to support logarithmic gamma mode > > include/drm-uapi/drm.h | 10 + > include/drm-uapi/drm_mode.h | 28 ++ > lib/igt_kms.c | 6 + > lib/igt_kms.h | 6 + > tests/kms_color.c | 674 +++++++++++++++++++++++++++++++++++- > tests/kms_color_chamelium.c | 588 ++++++++++++++++++++++++++++++- > tests/kms_color_helper.c | 300 ++++++++++++++++ > tests/kms_color_helper.h | 45 +++ > 8 files changed, 1648 insertions(+), 9 deletions(-) > > -- > 2.32.0 >
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/ > Are these tested and passing on any current or future Intel HW? > Hi, > > it's really good to get these, but I am worried that the tests here may > be too easy to pass when trying to ensure the KMS features work > correctly and in the way real userspace is going to be using them. > In particular per-plane color management is mainly beneficial when using HW composition, i.e. plane blending. I don't see tests for plane blending. Another thing that would be good to test is to make sure the order of operations is as documented in the KMS API, i.e. degamma -> CTM -> gamma. In order to test this it might be good to create a test case that sets these operations up in a way that only yields the expected outcome if they are implemented in this order. Last, and likely not easy to test, is the precision or accuracy of the PWL though I am unsure whether we can even test this at all with CRC. It looks like we might be able to test this with the Chamelium to some degree. > 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? Harry > > Thanks, > pq > >> Bhanuprakash Modem (11): >> HAX: Get uapi headers to compile the IGT >> lib/igt_kms: Add plane color mgmt properties >> kms_color_helper: Add helper functions for plane color mgmt >> tests/kms_color: New subtests for Plane gamma >> tests/kms_color: New subtests for Plane degamma >> tests/kms_color: New subtests for Plane CTM >> tests/kms_color: New negative tests for plane level color mgmt >> tests/kms_color_chamelium: New subtests for Plane gamma >> tests/kms_color_chamelium: New subtests for Plane degamma >> tests/kms_color_chamelium: New subtests for Plane CTM >> tests/kms_color_chamelium: Extended IGT tests to support logarithmic >> gamma mode >> >> Mukunda Pramodh Kumar (3): >> lib/igt_kms: Add pipe color mgmt properties >> kms_color_helper: Add helper functions to support logarithmic gamma >> mode >> tests/kms_color: Extended IGT tests to support logarithmic gamma mode >> >> include/drm-uapi/drm.h | 10 + >> include/drm-uapi/drm_mode.h | 28 ++ >> lib/igt_kms.c | 6 + >> lib/igt_kms.h | 6 + >> tests/kms_color.c | 674 +++++++++++++++++++++++++++++++++++- >> tests/kms_color_chamelium.c | 588 ++++++++++++++++++++++++++++++- >> tests/kms_color_helper.c | 300 ++++++++++++++++ >> tests/kms_color_helper.h | 45 +++ >> 8 files changed, 1648 insertions(+), 9 deletions(-) >> >> -- >> 2.32.0 >> >
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
On 2021-11-29 04:20, Pekka Paalanen wrote: > 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 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 >
> From: Harry Wentland <harry.wentland@amd.com> > Sent: Monday, November 29, 2021 8:50 PM > To: Pekka Paalanen <ppaalanen@gmail.com> > Cc: dri-devel@lists.freedesktop.org; Modem, Bhanuprakash > <bhanuprakash.modem@intel.com>; igt-dev@lists.freedesktop.org; Kumar, > Mukunda Pramodh <mukunda.pramodh.kumar@intel.com>; Juha-Pekka Heikkila > <juhapekka.heikkila@gmail.com>; Shankar, Uma <uma.shankar@intel.com> > 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@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 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
On 2022-01-02 23:11, Modem, Bhanuprakash wrote: >> From: Harry Wentland <harry.wentland@amd.com> >> Sent: Monday, November 29, 2021 8:50 PM >> To: Pekka Paalanen <ppaalanen@gmail.com> >> Cc: dri-devel@lists.freedesktop.org; Modem, Bhanuprakash >> <bhanuprakash.modem@intel.com>; igt-dev@lists.freedesktop.org; Kumar, >> Mukunda Pramodh <mukunda.pramodh.kumar@intel.com>; Juha-Pekka Heikkila >> <juhapekka.heikkila@gmail.com>; Shankar, Uma <uma.shankar@intel.com> >> 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@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://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
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/ Bhanuprakash Modem (11): HAX: Get uapi headers to compile the IGT lib/igt_kms: Add plane color mgmt properties kms_color_helper: Add helper functions for plane color mgmt tests/kms_color: New subtests for Plane gamma tests/kms_color: New subtests for Plane degamma tests/kms_color: New subtests for Plane CTM tests/kms_color: New negative tests for plane level color mgmt tests/kms_color_chamelium: New subtests for Plane gamma tests/kms_color_chamelium: New subtests for Plane degamma tests/kms_color_chamelium: New subtests for Plane CTM tests/kms_color_chamelium: Extended IGT tests to support logarithmic gamma mode Mukunda Pramodh Kumar (3): lib/igt_kms: Add pipe color mgmt properties kms_color_helper: Add helper functions to support logarithmic gamma mode tests/kms_color: Extended IGT tests to support logarithmic gamma mode include/drm-uapi/drm.h | 10 + include/drm-uapi/drm_mode.h | 28 ++ lib/igt_kms.c | 6 + lib/igt_kms.h | 6 + tests/kms_color.c | 674 +++++++++++++++++++++++++++++++++++- tests/kms_color_chamelium.c | 588 ++++++++++++++++++++++++++++++- tests/kms_color_helper.c | 300 ++++++++++++++++ tests/kms_color_helper.h | 45 +++ 8 files changed, 1648 insertions(+), 9 deletions(-) -- 2.32.0