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 😊. 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