(I'm sending this as an email as lowest common denominator but feel an issue on the color-and-hdr repo would be a better interface for productive discussion. Please pop over to https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/10 if you agree. Hopefully we can drive the discussion there but if there is a strong preference for email that works as well. :) )
I've wanted to start a thread to discuss the use of PWL APIs that were introduced by Uma a year ago and for which Bhanuprakash provided IGT tests. I have come to like the API but as we're getting closer to a real-world use of it I have a few questions and comments. As with a lot of complex APIs the devil is in the details. Some of those details are currently underspecified, or underdocumented and it's important that we all interpret the API the same way.
**The API**
The original patches posted by Uma:
https://patchwork.freedesktop.org/series/90822/
https://patchwork.freedesktop.org/series/90826/
The IGT tests for PWL API:
https://patchwork.freedesktop.org/series/96895/
I've rebased the kernel patches on a slightly more recent kernel, along with an AMD implementation:
https://gitlab.freedesktop.org/hwentland/linux/-/tree/color-and-hdr
I've also rebased them on an IGT tree, but that's not too up-to-date:
https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/color-and-hdr
**Why do I like the API?**
In order to allow HW composition of HDR planes in linear space we need the ability to program at least a per-CRTC regamma (inv_EOTF) to go from linear to wire format post-blending. Since userspace might want to apply corrections on top of a simple transfer function (such as PQ, BT.709, etc.) it would like a way to set a custom LUT.
The existing CRTC gamma LUT defines equally spaced entries. As Pekka shows in [1] equally-spaced LUTs have unacceptable error for regamma/inv_EOTF. Hence we need finer granularity of our entries near zero while coarse granularity works fine toward the brighter values.
[1] https://gitlab.freedesktop.org/pq/color-and-hdr/-/merge_requests/9
HW (at least AMD and Intel HW) implements this ability as segmented piece-wise linear LUTs. These define segments of equally spaced entries. These segments are constrained by the HW implementation. I like how the PWL API allows different drivers to specify the constraints imposed by different HW while allowing userspace a generic way of parsing the PWL. This also avoids complex calculations in the kernel driver, which might be required for other APIs one could envision. If anyone likes I can elaborate on some ideas for an alternate API, though all of them will require non-trivial transformations by the kernel driver in order to program them to HW.
**Nitpicks**
The API defines everything inside the segments, including flags and values that apply to the entire PWL, such as DRM_MODE_LUT_GAMMA, DRM_MODE_LUT_REFLECT_NEGATIVE, input_bpc, and output_bpc. If these don't stay constant for segments it might complicate the interpretation of segments. I suggest we consider these as effectively applying to the entire PWL. We could encode them in an overall drm_color_lut struct that includes an array of drm_color_lut_range but that's probably not necessary, hence why I called this out as a nitpick. I would just like us to be aware of this ambiguity and document that these values applies to the entire PWL.
**How to read the PWL**
Let me first give a summary for how this LUT is used in userspace. If you're familiar with this please review and comment if I got things wrong. As I mentioned, a lot of this is underspecified at the moment so you're reading my interpretation.
You can see this behavior in plane_degamma_test [2] in the kms_color.c IGT test suite. I suggest the plane_degamma_test here here instead of the test_pipe_gamma test as the latter still has Intelisms (assumptions around Intel driver/HW behavior) and will not work for other drivers.
Iterate over all enums in PLANE_DEGAMMA_MODE and find a suitable one. How do we find the suitable one? More on that below.
Once we have the right PLANE_DEGAMMA_MODE we read the blob for the blob ID associated with the PLANE_DEGAMMA_MODE enum. We interpret the blob as an array of drm_color_lut_range. See get_segment_data [3].
We can think of our LUT/PWL as f(x) = y. For a traditional equally spaced LUT with 1024 entries x would be 0, 1, 2, ..., 1023. For a PWL LUT we need to parse the segment data provided in drm_color_lut_range.
Let's look at the 2nd-last entry of the nonlinear_pwl definition for the AMD driver [4] (I've correct it here and dropped the DRM_MODE_LUT_REUSE_LAST but it's still incorrect in the link) and simplify it to 4 entries for sake of readability:
{
.flags = (DRM_MODE_LUT_GAMMA | DRM_MODE_LUT_REFLECT_NEGATIVE | DRM_MODE_LUT_INTERPOLATE | DRM_MODE_LUT_NON_DECREASING),
.count = 4,
.input_bpc = 13, .output_bpc = 18,
.start = 1 << 12, .end = 1 << 13,
.min = 0, .max = 1UL << 35
},
We see we have 16 entries in the region from (1 << 12) to (1 << 13). We see input_bpc is 13, so our 1.0 float value is 1 << 13.
(Is it sensible to use input_bpc as our 1.0 floating point reference? Why?)
Since this segment is not reusing the last entry (doesn't have DRM_MODE_LUT_REUSE_LAST) we divide the region between 1 << 12 and 1 << 13 into 4 equally spaced sections.
step_size = (segment->end - segment->start) / count
In our case our segment spans from 4096 to 8192 and yields a step_size of 1024.
Note that we need to calculate this in floating point, otherwise we're not guaranteed equal spacing.
This gives us these X entries for this particular segment:
4096, 5120, 6144, 7168
And normalized to 1 << 13 (input_bpc) for our 1.0 float value we get
0.5, 0.625, 0.75, 0.875
If the segment had the REUSE_LAST flag the values would look like
4096, 5461, 6826, 8192
and normalized to
0.5, 0.666626, 0.833252, 1
Though in the case of REUSE_LAST a more sensible definition might be with a count of 5 entries in this segment, instead of 16. But ultimatly that's up to the driver.
I attached a simple C program that parses a PWL and helps illustrate my interpretation. You'll just need to copy-paste or include your PWL definition (instead of color_gamma.h), and point the PWL define to your PWL variable. To build it you'll need to copy the drm-uapi folder from my IGT repo into the same directory as pwl-parser.c and then just build it with "gcc -Idrm-uapi pwl-parser.c".
The above entries come from running pwl-parser with the nonlinear_pwl from [5].
To illustrate these I added versions of your lut1d_error scripts that run on the SDR and HDR PWLs for AMD HW [6].
We should probably document the above in detail in the DRM/KMS API docs.
[2] https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/blob/color-and-hdr/tests/kms_color.c#L978
[3] https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/blob/color-and-hdr/tests/kms_color_helper.c#L393
[4] https://gitlab.freedesktop.org/hwentland/linux/-/blob/color-and-hdr/drivers/gpu/drm/amd/display/modules/color/color_gamma.h#L109
[5] https://gitlab.freedesktop.org/hwentland/linux/-/blob/color-and-hdr/drivers/gpu/drm/amd/display/modules/color/color_gamma.h#L38
[6] https://gitlab.freedesktop.org/hwentland/color-and-hdr/-/tree/precision/octave
**How to provide PWL entries?**
To provide the PWL values for each entry we'll have to sample our (custom) curve at the respective points specified by our PWL entries and providing those samples in a blob that is passed to PLANE_DEGAMMA_LUT. It's not much different from how we provide values for an equally spaced (legacy) LUT. As for sampling our curve, I remember seeing that Weston uses an LCMS function to sample the curve at required points. Last I checked it samples the curve at evenly spaced intervals but the LCMS function seemed to provide arbitrary sampling.
**How to pick a suitable PWL definition?**
Picking the right PWL definition out of the respective \_MODE enum isn't trivial. Without further information a userspace implementer has to understand the distribution of entries in all segments and perform a bunch of math to estimate the error for given curves.
A simpler approach might be if we defined common naming for our PWL enums. We can define the commonly expected cases. The two important parameters are within-range vs overrange entries, and linear vs non-linear outputs.
Within-range PWLs would cover [0.0, 1.0] entries and overrange [0.0, 128.0] to cover PQ and probably anything else. One could think of the within-range PWL as intended for SDR content and the over-range PWL for HDR content.
Linear PWLs would be intended for linear luminance outputs (or near-linear), and can be represented by equally spaced LUTs, such as a single-segment definitions. Non-linear PWLs would be intended for linear to non-linear transforms; Linear PWLs for non-linear to linear transforms or OOTFs.
This gives us four enums, plus one for bypass:
DRM_MODE_LUT_BYPASS
DRM_MODE_LUT_LINEAR_SDR
DRM_MODE_LUT_NONLINEAR_SDR
DRM_MODE_LUT_LINEAR_HDR
DRM_MODE_LUT_NONLINEAR_HDR
Drivers can provide appropriate PWLs based on the HW caps. Userspace can pick an appropriate one if it's available or fall back to either a sub-optimal PWL or to using a GPU transform instead.
Thoughts?
Thanks,
Harry/*
* Copyright 2022 Advanced Micro Devices, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a
* copy of this software and associated documentation files (the "Software"),
* to deal in the Software without restriction, including without limitation
* the rights to use, copy, modify, merge, publish, distribute, sublicense,
* and/or sell copies of the Software, and to permit persons to whom the
* Software is furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
* THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
* OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
* ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
* OTHER DEALINGS IN THE SOFTWARE.
*
* Authors: AMD
*
*/
#define BIT(x) (1 << x)
#include "drm_mode.h"
#include "color_gamma.h"
#include <stdio.h>
#define PWL nonlinear_hdr_pwl
#define MAX_ENTRIES_PER_SEGMENT 512
#define MAX_ENTRIES 4096
int main(int argc, char *argv[])
{
__u64 entries[MAX_ENTRIES];
unsigned pwl_size = 0;
unsigned segment_size = 0;
unsigned segment_count = 0;
unsigned i = 0;
unsigned j = 0;
unsigned overall_count = 0;
__u64 norm = 0;
pwl_size = sizeof(PWL);
segment_size = sizeof(PWL[0]);
segment_count = pwl_size / segment_size;
printf("pwl_size = %d, segment_size = %d, segment_count = %d\n",
pwl_size, segment_size, segment_count);
for (i = 0; i < segment_count; i++) {
const struct drm_color_lut_range *segment = &PWL[i];
__u64 segment_entries[MAX_ENTRIES_PER_SEGMENT];
unsigned count = 0;
__u64 step_size = 0;
printf("Segment %d\n", i);
printf("\t.flags = 0x%x\n", segment->flags);
printf("\t.count = %d\n", segment->count);
printf("\t.input_bpc = %d, .output_bpc = %d\n", segment->input_bpc, segment->output_bpc);
printf("\t.start = 0x%lx, .end = %lx\n", segment->start, segment->end);
printf("\t.min = 0x%lx, .max = %lx\n", segment->min, segment->max);
norm = segment->input_bpc;
if (segment->count == 1) {
printf("\t- steps = %d\n", segment->start);
printf("\t- normalized steps = %g\n", segment->start / ((1 << segment->input_bpc) + 0.0));
entries[overall_count++] = segment->start;
continue;
}
if (segment->flags & DRM_MODE_LUT_REUSE_LAST) {
/*
* Entries are evenly spaced in [start, end]
* including "end" as an entry.
*
* This is the same as count -1 entries when
* REUSE_LAST isn't used, with "end" added
* as the additional entry
*/
count = segment->count - 1;
} else {
/*
* entries are evenly spaced in [start, end]
* excluding "end" as an entry
*
* this will look the same as a REUSE_LAST
* distribution of count + 1 with the last
* entry omitted
*/
count = segment->count;
}
step_size = (segment->end - segment->start) / (count);
printf("\t- step_size count 0x%x, step_size 0x%x\n", count, step_size);
for (j = 0; j < count; j++) {
segment_entries[j] = segment->start + (j * step_size);
entries[overall_count++] = segment_entries[j];
}
if (segment->flags & DRM_MODE_LUT_REUSE_LAST) {
segment_entries[j] = segment->end;
entries[overall_count++] = segment_entries[j];
}
/* print all entries */
count = (segment->flags & DRM_MODE_LUT_REUSE_LAST) ? count + 1 : count;
printf("\t- steps = ");
for (j = 0; j < count; j++) {
printf("%ld", segment_entries[j]);
if (j < count-1)
printf(", ");
}
printf("\n");
printf("\t- normalized steps = ");
for (j = 0; j < count; j++) {
printf("%g", segment_entries[j] / ((1 << segment->input_bpc) + 0.0));
if (j < count-1)
printf(", ");
}
printf("\n");
}
printf("steps = ");
for (i = 0; i < overall_count; i++) {
printf("%ld", entries[i]);
if (i < overall_count-1)
printf(", ");
}
printf("\n");
printf("normalized steps = ");
for (i = 0; i < overall_count; i++) {
printf("%g", entries[i] / ((1 << norm) + 0.0));
if (i < overall_count-1)
printf(", ");
}
printf("\n");
printf("%d entries\n", overall_count);
return 0;
}
