On 2021-11-05 08:59, Ville Syrjälä wrote:
On Wed, Nov 03, 2021 at 11:10:37AM -0400, Harry Wentland wrote:
On 2021-09-06 17:38, Uma Shankar wrote:
Define the structure with XE_LPD degamma lut ranges. HDR and SDR planes have different capabilities, implemented respective structure for the HDR planes.
Signed-off-by: Uma Shankar uma.shankar@intel.com
drivers/gpu/drm/i915/display/intel_color.c | 52 ++++++++++++++++++++++ 1 file changed, 52 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index afcb4bf3826c..6403bd74324b 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -2092,6 +2092,58 @@ static void icl_read_luts(struct intel_crtc_state *crtc_state) } }
- /* FIXME input bpc? */
+__maybe_unused +static const struct drm_color_lut_range d13_degamma_hdr[] = {
- /* segment 1 */
- {
.flags = (DRM_MODE_LUT_GAMMA |DRM_MODE_LUT_REFLECT_NEGATIVE |DRM_MODE_LUT_INTERPOLATE |DRM_MODE_LUT_NON_DECREASING),.count = 128,Is the distribution of the 128 entries uniform?
I guess this is the plane gamma thing despite being in intel_color.c, so yeah I think that's correct.
If so, is a uniform distribution of 128 points across most of the LUT good enough for HDR with 128 entries?
No idea how good this actually is. It is .24 so at least it does have a fair bit of precision.
Precision is good but you also need enough samples. Though that's probably less my concern and more your concern and should become apparent once its used.
.input_bpc = 24, .output_bpc = 16,.start = 0, .end = (1 << 24) - 1,.min = 0, .max = (1 << 24) - 1,- },
- /* segment 2 */
- {
.flags = (DRM_MODE_LUT_GAMMA |DRM_MODE_LUT_REFLECT_NEGATIVE |DRM_MODE_LUT_INTERPOLATE |DRM_MODE_LUT_REUSE_LAST |DRM_MODE_LUT_NON_DECREASING),.count = 1,.input_bpc = 24, .output_bpc = 16,.start = (1 << 24) - 1, .end = 1 << 24,.start and .end are only a single entry apart. Is this correct?
One think I wanted to do is simplify this stuff by getting rid of .end entirely. So I think this should just be '.start=1<<24' (or whatever way we decide to specify the input precision, which is I think another slightly open question).
So for this thing we could just have: { .count = 128, .min = 0, .max = (1 << 24) - 1, .start = 0 }, { .count = 1, .min = 0, .max = (7 << 24) - 1, .start = 1 << 24 }, { .count = 1, .min = 0, .max = (7 << 24) - 1, .start = 3 << 24 }, { .count = 1, .min = 0, .max = (7 << 24) - 1, .start = 7 << 24 },
- flags/etc. which I left out for brevity.
Makes sense. I like this.
So that is trying to indicate that the first 129 entries are equally spaced, and would be used to interpolate for input values [0.0,1.0). Input values [1.0,3.0) would interpolate between entry 128 and 129, and [3.0,7.0) would interpolate between entry 129 and 130.
What in the segment definition defines the 1.0 mark? In your example it seems to be at (1 << 24) but then we would have values that go beyond the input_bpc for the last three segments.
How about output_bpc? Would output_bpc somehow limit the U32.32 (or S31.32) entries, and if so, how?
Or should we treat input_/output_bpc only as capability reporting, so userspace can calculate the possible error when programming the LUT? Again, this leaves us with the question what the input_/output_bpc means for our PWL entries.
Harry