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