| Message ID | 20210906213904.27918-6-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Support for Plane Color Lut and CSC features | expand |
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? If so, is a uniform distribution of 128 points across most of the LUT good enough for HDR with 128 entries? > + .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? Harry > + .min = 0, .max = (1 << 27) - 1, > + }, > + /* Segment 3 */ > + { > + .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, .end = 3 << 24, > + .min = 0, .max = (1 << 27) - 1, > + }, > + /* Segment 4 */ > + { > + .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 = 3 << 24, .end = 7 << 24, > + .min = 0, .max = (1 << 27) - 1, > + }, > +}; > + > void intel_color_init(struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >
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. > > > + .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. 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.
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
On Tue, Nov 09, 2021 at 03:19:47PM -0500, Harry Wentland wrote: > 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. Yeah, for pipe gamma we have a few different variants with non-uniform spacing of the samples. But not here AFAICS for whatever reason. > > >> > >>> + .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. Yes, input_bpc would define the precision of the input values (.start). so 1.0 would be at 1<<input_bpc. Tne range of input values is allowed to extend outside the 0.0-1.0 range. > > How about output_bpc? Would output_bpc somehow limit the U32.32 (or > S31.32) entries, and if so, how? output_bpc would define the actual precision of the output values, so again 1.0 would be 1<<output_bpc, and .min and .max define the min/max values (which can extend outside the 0.0-1.0 range). The alternative I guess would be to not have .output_bpc at all and just have .min/.max be s32.32 values. Though then you can't tell what the actual precision is. Same could be done for .input_bpc I suppose. > > 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. Yeah, I mostly thought they might be interesting if userspace wants to know the exact precision. But not strictly necessary if you want just to go generate a "close enough" curve. What's PWL?
On 2021-11-09 16:45, Ville Syrjälä wrote: > On Tue, Nov 09, 2021 at 03:19:47PM -0500, Harry Wentland wrote: >> 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. > > Yeah, for pipe gamma we have a few different variants with > non-uniform spacing of the samples. But not here AFAICS for > whatever reason. > >> >>>> >>>>> + .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. > > Yes, input_bpc would define the precision of the input values (.start). > so 1.0 would be at 1<<input_bpc. Tne range of input values is allowed to > extend outside the 0.0-1.0 range. > >> >> How about output_bpc? Would output_bpc somehow limit the U32.32 (or >> S31.32) entries, and if so, how? > > output_bpc would define the actual precision of the output values, > so again 1.0 would be 1<<output_bpc, and .min and .max define the > min/max values (which can extend outside the 0.0-1.0 range). The > alternative I guess would be to not have .output_bpc at all and > just have .min/.max be s32.32 values. Though then you can't tell > what the actual precision is. Same could be done for .input_bpc > I suppose. > >> >> 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. > > Yeah, I mostly thought they might be interesting if userspace wants > to know the exact precision. But not strictly necessary if you want > just to go generate a "close enough" curve. > > What's PWL? > Got it, I think. Piece-wise linear LUT, i.e. a (usually segmented) LUT that linearly interpolates in between entries. Harry
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, > + .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, > + .min = 0, .max = (1 << 27) - 1, > + }, > + /* Segment 3 */ > + { > + .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, .end = 3 << 24, > + .min = 0, .max = (1 << 27) - 1, > + }, > + /* Segment 4 */ > + { > + .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 = 3 << 24, .end = 7 << 24, > + .min = 0, .max = (1 << 27) - 1, > + }, > +}; If I understand this right, userspace would need this definition in order to populate the degamma blob. Should this sit in a UAPI header? Harry > + > void intel_color_init(struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >
On Thu, Nov 11, 2021 at 10:17:17AM -0500, 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, > > + .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, > > + .min = 0, .max = (1 << 27) - 1, > > + }, > > + /* Segment 3 */ > > + { > > + .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, .end = 3 << 24, > > + .min = 0, .max = (1 << 27) - 1, > > + }, > > + /* Segment 4 */ > > + { > > + .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 = 3 << 24, .end = 7 << 24, > > + .min = 0, .max = (1 << 27) - 1, > > + }, > > +}; > > If I understand this right, userspace would need this definition in order > to populate the degamma blob. Should this sit in a UAPI header? My original idea (not sure it's fully realized in this series) is to have a new GAMMA_MODE/etc. enum property on each crtc (or plane) for which each enum value points to a kernel provided blob that contains one of these LUT descriptors. Userspace can then query them dynamically and pick the best one for its current use case. The algorithm for choosing the best one might be something like: - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc - prefer interpolated vs. direct lookup based on current needs (eg. X could prefer direct lookup to get directcolor visuals). - prefer one with extended range values if needed - for HDR prefer smaller step size in dark tones, for SDR perhaps prefer a more uniform step size Or maybe we should include some kind of usage hints as well? And I was thinking of even adding a new property type (eg. ENUM_BLOB) just for this sort of usecase. That could let us have a bit more generic code to do all the validation around the property values and whatnot. The one nagging concern I really have with GAMMA_MODE is how a mix of old and new userspace would work. Though that is more of a generic issue with any new property really.
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Thursday, November 11, 2021 10:13 PM > To: Harry Wentland <harry.wentland@amd.com> > Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri- > devel@lists.freedesktop.org; ppaalanen@gmail.com; brian.starkey@arm.com; > sebastian@sebastianwick.net; Shashank.Sharma@amd.com > Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for > HDR planes > > On Thu, Nov 11, 2021 at 10:17:17AM -0500, 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, > > > + .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, > > > + .min = 0, .max = (1 << 27) - 1, > > > + }, > > > + /* Segment 3 */ > > > + { > > > + .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, .end = 3 << 24, > > > + .min = 0, .max = (1 << 27) - 1, > > > + }, > > > + /* Segment 4 */ > > > + { > > > + .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 = 3 << 24, .end = 7 << 24, > > > + .min = 0, .max = (1 << 27) - 1, > > > + }, > > > +}; > > > > If I understand this right, userspace would need this definition in > > order to populate the degamma blob. Should this sit in a UAPI header? Hi Harry, Pekka and Ville, Sorry for being a bit late on the replies, got side tracked with various issues. I am back on this. Apologies for delay. > My original idea (not sure it's fully realized in this series) is to have a new > GAMMA_MODE/etc. enum property on each crtc (or plane) for which each enum > value points to a kernel provided blob that contains one of these LUT descriptors. > Userspace can then query them dynamically and pick the best one for its current use > case. We have this as part of the series Ville. Patch 3 of this series creates a DEGAMMA_MODE property just for this. With that userspace can just query the blob_id's and will get the various degamma mode possible and the respective segment and lut distributions. This will be generic, so for userspace it should just be able to query this and parse and get the lut distribution and segment ranges. > The algorithm for choosing the best one might be something like: > - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc > - prefer interpolated vs. direct lookup based on current needs (eg. X > could prefer direct lookup to get directcolor visuals). > - prefer one with extended range values if needed > - for HDR prefer smaller step size in dark tones, > for SDR perhaps prefer a more uniform step size > > Or maybe we should include some kind of usage hints as well? I think the segment range and distribution of lut should be enough for a userspace to pick the right ones, but we can add some examples in UAPI descriptions as hints. > And I was thinking of even adding a new property type (eg. > ENUM_BLOB) just for this sort of usecase. That could let us have a bit more generic > code to do all the validation around the property values and whatnot. > > The one nagging concern I really have with GAMMA_MODE is how a mix of old and > new userspace would work. Though that is more of a generic issue with any new > property really. For plane properties getting added newly, old userspace will not get it so I think this should be ok. Newer userspace will implement this and get the new functionality. Problem will be in extending this to crtc where we have a legacy baggage, the client caps approach may help us there. Have it as part of separate series just to not mix it with this new plane stuff, though the idea remains same based on your design. Series below for reference: https://patchwork.freedesktop.org/series/90821/ Regards, Uma Shankar > -- > Ville Syrjälä > Intel
On 2021-11-11 15:42, Shankar, Uma wrote: > > >> -----Original Message----- >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Sent: Thursday, November 11, 2021 10:13 PM >> To: Harry Wentland <harry.wentland@amd.com> >> Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri- >> devel@lists.freedesktop.org; ppaalanen@gmail.com; brian.starkey@arm.com; >> sebastian@sebastianwick.net; Shashank.Sharma@amd.com >> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for >> HDR planes >> >> On Thu, Nov 11, 2021 at 10:17:17AM -0500, 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, >>>> + .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, >>>> + .min = 0, .max = (1 << 27) - 1, >>>> + }, >>>> + /* Segment 3 */ >>>> + { >>>> + .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, .end = 3 << 24, >>>> + .min = 0, .max = (1 << 27) - 1, >>>> + }, >>>> + /* Segment 4 */ >>>> + { >>>> + .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 = 3 << 24, .end = 7 << 24, >>>> + .min = 0, .max = (1 << 27) - 1, >>>> + }, >>>> +}; >>> >>> If I understand this right, userspace would need this definition in >>> order to populate the degamma blob. Should this sit in a UAPI header? > > Hi Harry, Pekka and Ville, > Sorry for being a bit late on the replies, got side tracked with various issues. > I am back on this. Apologies for delay. > >> My original idea (not sure it's fully realized in this series) is to have a new >> GAMMA_MODE/etc. enum property on each crtc (or plane) for which each enum >> value points to a kernel provided blob that contains one of these LUT descriptors. >> Userspace can then query them dynamically and pick the best one for its current use >> case. > > We have this as part of the series Ville. Patch 3 of this series creates a DEGAMMA_MODE > property just for this. With that userspace can just query the blob_id's and will get the > various degamma mode possible and the respective segment and lut distributions. > > This will be generic, so for userspace it should just be able to query this and parse and get > the lut distribution and segment ranges. > Thanks for the explanation. Uma, have you had a chance to sketch some of this out in IGT? I'm trying to see how userspace would do this in practice and will try to sketch an IGT test for this myself, but if you have it already we could share the effort. >> The algorithm for choosing the best one might be something like: >> - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc >> - prefer interpolated vs. direct lookup based on current needs (eg. X >> could prefer direct lookup to get directcolor visuals). >> - prefer one with extended range values if needed >> - for HDR prefer smaller step size in dark tones, >> for SDR perhaps prefer a more uniform step size >> >> Or maybe we should include some kind of usage hints as well? > > I think the segment range and distribution of lut should be enough for a userspace > to pick the right ones, but we can add some examples in UAPI descriptions as hints. > The range might be enough, but we're already parsing hints like "GAMMA" or "DEGAMMA". I wonder if it would make sense to add a flag for "HDR" or "SDR" as well. >> And I was thinking of even adding a new property type (eg. >> ENUM_BLOB) just for this sort of usecase. That could let us have a bit more generic >> code to do all the validation around the property values and whatnot. >> >> The one nagging concern I really have with GAMMA_MODE is how a mix of old and >> new userspace would work. Though that is more of a generic issue with any new >> property really. > > For plane properties getting added newly, old userspace will not get it so I think this should be ok. > Newer userspace will implement this and get the new functionality. > Problem will be in extending this to crtc where we have a legacy baggage, the client caps approach > may help us there. Have it as part of separate series just to not mix it with this new plane stuff, though > the idea remains same based on your design. Series below for reference: > https://patchwork.freedesktop.org/series/90821/>> Could we just assume we do a uniform LUT if userspace doesn't set a _MODE enum value for the respective gamma? Maybe the _MODE should have a default enum value that means a uniform (legacy) LUT. Harry > Regards, > Uma Shankar > >> -- >> Ville Syrjälä >> Intel
> -----Original Message----- > From: Harry Wentland <harry.wentland@amd.com> > Sent: Friday, November 12, 2021 2:41 AM > To: Shankar, Uma <uma.shankar@intel.com>; Ville Syrjälä > <ville.syrjala@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > ppaalanen@gmail.com; brian.starkey@arm.com; sebastian@sebastianwick.net; > Shashank.Sharma@amd.com > Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for > HDR planes > > > > On 2021-11-11 15:42, Shankar, Uma wrote: > > > > > >> -----Original Message----- > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Sent: Thursday, November 11, 2021 10:13 PM > >> To: Harry Wentland <harry.wentland@amd.com> > >> Cc: Shankar, Uma <uma.shankar@intel.com>; > >> intel-gfx@lists.freedesktop.org; dri- devel@lists.freedesktop.org; > >> ppaalanen@gmail.com; brian.starkey@arm.com; > >> sebastian@sebastianwick.net; Shashank.Sharma@amd.com > >> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range > >> struct for HDR planes > >> > >> On Thu, Nov 11, 2021 at 10:17:17AM -0500, 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, > >>>> + .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, > >>>> + .min = 0, .max = (1 << 27) - 1, > >>>> + }, > >>>> + /* Segment 3 */ > >>>> + { > >>>> + .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, .end = 3 << 24, > >>>> + .min = 0, .max = (1 << 27) - 1, > >>>> + }, > >>>> + /* Segment 4 */ > >>>> + { > >>>> + .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 = 3 << 24, .end = 7 << 24, > >>>> + .min = 0, .max = (1 << 27) - 1, > >>>> + }, > >>>> +}; > >>> > >>> If I understand this right, userspace would need this definition in > >>> order to populate the degamma blob. Should this sit in a UAPI header? > > > > Hi Harry, Pekka and Ville, > > Sorry for being a bit late on the replies, got side tracked with various issues. > > I am back on this. Apologies for delay. > > > >> My original idea (not sure it's fully realized in this series) is to > >> have a new GAMMA_MODE/etc. enum property on each crtc (or plane) for > >> which each enum value points to a kernel provided blob that contains one of > these LUT descriptors. > >> Userspace can then query them dynamically and pick the best one for > >> its current use case. > > > > We have this as part of the series Ville. Patch 3 of this series > > creates a DEGAMMA_MODE property just for this. With that userspace can > > just query the blob_id's and will get the various degamma mode possible and the > respective segment and lut distributions. > > > > This will be generic, so for userspace it should just be able to query > > this and parse and get the lut distribution and segment ranges. > > > > Thanks for the explanation. > > Uma, have you had a chance to sketch some of this out in IGT? I'm trying to see how > userspace would do this in practice and will try to sketch an IGT test for this myself, > but if you have it already we could share the effort. Yes Harry, we do have some sample IGT's to test this. Will send those out and will copy you and all the stakeholders. > >> The algorithm for choosing the best one might be something like: > >> - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc > >> - prefer interpolated vs. direct lookup based on current needs (eg. X > >> could prefer direct lookup to get directcolor visuals). > >> - prefer one with extended range values if needed > >> - for HDR prefer smaller step size in dark tones, > >> for SDR perhaps prefer a more uniform step size > >> > >> Or maybe we should include some kind of usage hints as well? > > > > I think the segment range and distribution of lut should be enough for > > a userspace to pick the right ones, but we can add some examples in UAPI > descriptions as hints. > > > > The range might be enough, but we're already parsing hints like "GAMMA" > or "DEGAMMA". I wonder if it would make sense to add a flag for "HDR" or "SDR" as > well. On Intel hardware, we differentiate this with precision and have HDR planes (they have extra Lut precision and samples) separately called out. We could add SDR/HDR FLAG as well. > >> And I was thinking of even adding a new property type (eg. > >> ENUM_BLOB) just for this sort of usecase. That could let us have a > >> bit more generic code to do all the validation around the property values and > whatnot. > >> > >> The one nagging concern I really have with GAMMA_MODE is how a mix of > >> old and new userspace would work. Though that is more of a generic > >> issue with any new property really. > > > > For plane properties getting added newly, old userspace will not get it so I think > this should be ok. > > Newer userspace will implement this and get the new functionality. > > Problem will be in extending this to crtc where we have a legacy > > baggage, the client caps approach may help us there. Have it as part > > of separate series just to not mix it with this new plane stuff, though the idea > remains same based on your design. Series below for reference: > > https://patchwork.freedesktop.org/series/90821/>> > > Could we just assume we do a uniform LUT if userspace doesn't set a _MODE enum > value for the respective gamma? > > Maybe the _MODE should have a default enum value that means a uniform (legacy) > LUT. Yeah we could have this and document the behavior in UAPI description. Regards, Uma Shankar > Harry > > > Regards, > > Uma Shankar > > > >> -- > >> Ville Syrjälä > >> Intel
On Thu, 11 Nov 2021 21:58:35 +0000 "Shankar, Uma" <uma.shankar@intel.com> wrote: > > -----Original Message----- > > From: Harry Wentland <harry.wentland@amd.com> > > Sent: Friday, November 12, 2021 2:41 AM > > To: Shankar, Uma <uma.shankar@intel.com>; Ville Syrjälä > > <ville.syrjala@linux.intel.com> > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > > ppaalanen@gmail.com; brian.starkey@arm.com; sebastian@sebastianwick.net; > > Shashank.Sharma@amd.com > > Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for > > HDR planes > > > > > > > > On 2021-11-11 15:42, Shankar, Uma wrote: > > > > > > > > >> -----Original Message----- > > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> Sent: Thursday, November 11, 2021 10:13 PM > > >> To: Harry Wentland <harry.wentland@amd.com> > > >> Cc: Shankar, Uma <uma.shankar@intel.com>; > > >> intel-gfx@lists.freedesktop.org; dri- devel@lists.freedesktop.org; > > >> ppaalanen@gmail.com; brian.starkey@arm.com; > > >> sebastian@sebastianwick.net; Shashank.Sharma@amd.com > > >> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range > > >> struct for HDR planes > > >> > > >> On Thu, Nov 11, 2021 at 10:17:17AM -0500, 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, > > >>>> + .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, > > >>>> + .min = 0, .max = (1 << 27) - 1, > > >>>> + }, > > >>>> + /* Segment 3 */ > > >>>> + { > > >>>> + .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, .end = 3 << 24, > > >>>> + .min = 0, .max = (1 << 27) - 1, > > >>>> + }, > > >>>> + /* Segment 4 */ > > >>>> + { > > >>>> + .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 = 3 << 24, .end = 7 << 24, > > >>>> + .min = 0, .max = (1 << 27) - 1, > > >>>> + }, > > >>>> +}; > > >>> > > >>> If I understand this right, userspace would need this definition in > > >>> order to populate the degamma blob. Should this sit in a UAPI header? Are you asking whether 'struct drm_color_lut_range` is defined in any userspace visible header? It seems to be in patch 2. > > > > > > Hi Harry, Pekka and Ville, > > > Sorry for being a bit late on the replies, got side tracked with various issues. > > > I am back on this. Apologies for delay. > > > > > >> My original idea (not sure it's fully realized in this series) is to > > >> have a new GAMMA_MODE/etc. enum property on each crtc (or plane) for > > >> which each enum value points to a kernel provided blob that contains one of > > these LUT descriptors. > > >> Userspace can then query them dynamically and pick the best one for > > >> its current use case. > > > > > > We have this as part of the series Ville. Patch 3 of this series > > > creates a DEGAMMA_MODE property just for this. With that userspace can > > > just query the blob_id's and will get the various degamma mode possible and the > > respective segment and lut distributions. > > > > > > This will be generic, so for userspace it should just be able to query > > > this and parse and get the lut distribution and segment ranges. > > > > > > > Thanks for the explanation. > > > > Uma, have you had a chance to sketch some of this out in IGT? I'm trying to see how > > userspace would do this in practice and will try to sketch an IGT test for this myself, > > but if you have it already we could share the effort. > > Yes Harry, we do have some sample IGT's to test this. Will send those out and will copy > you and all the stakeholders. > > > >> The algorithm for choosing the best one might be something like: > > >> - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc > > >> - prefer interpolated vs. direct lookup based on current needs (eg. X > > >> could prefer direct lookup to get directcolor visuals). > > >> - prefer one with extended range values if needed > > >> - for HDR prefer smaller step size in dark tones, > > >> for SDR perhaps prefer a more uniform step size > > >> > > >> Or maybe we should include some kind of usage hints as well? > > > > > > I think the segment range and distribution of lut should be enough for > > > a userspace to pick the right ones, but we can add some examples in UAPI > > descriptions as hints. > > > > > > > The range might be enough, but we're already parsing hints like "GAMMA" > > or "DEGAMMA". I wonder if it would make sense to add a flag for "HDR" or "SDR" as > > well. What hints are GAMMA or DEGAMMA and who's parsing them? I thought they are just arbitrary names to identify the element's position in the abstract pipeline. > > On Intel hardware, we differentiate this with precision and have HDR planes (they have extra > Lut precision and samples) separately called out. We could add SDR/HDR FLAG as well. What about wide color gamut SDR? That probably needs more precision than "normal" SDR but is not HDR. I can't think of how SDR/HDR flags would work or what they would mean. Feels a bit too simple for practice. Maybe that concept should be created by a hypothetical userspace helper library instead. Thanks, pq
On Thu, Nov 11, 2021 at 04:10:41PM -0500, Harry Wentland wrote: > > > On 2021-11-11 15:42, Shankar, Uma wrote: > > > > > >> -----Original Message----- > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Sent: Thursday, November 11, 2021 10:13 PM > >> To: Harry Wentland <harry.wentland@amd.com> > >> Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri- > >> devel@lists.freedesktop.org; ppaalanen@gmail.com; brian.starkey@arm.com; > >> sebastian@sebastianwick.net; Shashank.Sharma@amd.com > >> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for > >> HDR planes > >> > >> On Thu, Nov 11, 2021 at 10:17:17AM -0500, 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, > >>>> + .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, > >>>> + .min = 0, .max = (1 << 27) - 1, > >>>> + }, > >>>> + /* Segment 3 */ > >>>> + { > >>>> + .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, .end = 3 << 24, > >>>> + .min = 0, .max = (1 << 27) - 1, > >>>> + }, > >>>> + /* Segment 4 */ > >>>> + { > >>>> + .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 = 3 << 24, .end = 7 << 24, > >>>> + .min = 0, .max = (1 << 27) - 1, > >>>> + }, > >>>> +}; > >>> > >>> If I understand this right, userspace would need this definition in > >>> order to populate the degamma blob. Should this sit in a UAPI header? > > > > Hi Harry, Pekka and Ville, > > Sorry for being a bit late on the replies, got side tracked with various issues. > > I am back on this. Apologies for delay. > > > >> My original idea (not sure it's fully realized in this series) is to have a new > >> GAMMA_MODE/etc. enum property on each crtc (or plane) for which each enum > >> value points to a kernel provided blob that contains one of these LUT descriptors. > >> Userspace can then query them dynamically and pick the best one for its current use > >> case. > > > > We have this as part of the series Ville. Patch 3 of this series creates a DEGAMMA_MODE > > property just for this. With that userspace can just query the blob_id's and will get the > > various degamma mode possible and the respective segment and lut distributions. > > > > This will be generic, so for userspace it should just be able to query this and parse and get > > the lut distribution and segment ranges. > > > > Thanks for the explanation. > > Uma, have you had a chance to sketch some of this out in IGT? I'm trying > to see how userspace would do this in practice and will try to sketch an > IGT test for this myself, but if you have it already we could share the > effort. > > >> The algorithm for choosing the best one might be something like: > >> - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc > >> - prefer interpolated vs. direct lookup based on current needs (eg. X > >> could prefer direct lookup to get directcolor visuals). > >> - prefer one with extended range values if needed > >> - for HDR prefer smaller step size in dark tones, > >> for SDR perhaps prefer a more uniform step size > >> > >> Or maybe we should include some kind of usage hints as well? > > > > I think the segment range and distribution of lut should be enough for a userspace > > to pick the right ones, but we can add some examples in UAPI descriptions as hints. > > > > The range might be enough, but we're already parsing hints like "GAMMA" > or "DEGAMMA". I wonder if it would make sense to add a flag for "HDR" or > "SDR" as well. > > >> And I was thinking of even adding a new property type (eg. > >> ENUM_BLOB) just for this sort of usecase. That could let us have a bit more generic > >> code to do all the validation around the property values and whatnot. > >> > >> The one nagging concern I really have with GAMMA_MODE is how a mix of old and > >> new userspace would work. Though that is more of a generic issue with any new > >> property really. > > > > For plane properties getting added newly, old userspace will not get it so I think this should be ok. > > Newer userspace will implement this and get the new functionality. > > Problem will be in extending this to crtc where we have a legacy baggage, the client caps approach > > may help us there. Have it as part of separate series just to not mix it with this new plane stuff, though > > the idea remains same based on your design. Series below for reference: > > https://patchwork.freedesktop.org/series/90821/>> > > Could we just assume we do a uniform LUT if userspace doesn't > set a _MODE enum value for the respective gamma? > > Maybe the _MODE should have a default enum value that means > a uniform (legacy) LUT. Yeah, it definitely needs a default like that. But the problem arises when new userspace sets it to something else and then hands the reins over to some old userspace that doesn't know how to reset it back to default.
On Fri, 12 Nov 2021 16:54:35 +0200 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Nov 11, 2021 at 04:10:41PM -0500, Harry Wentland wrote: > > > > > > On 2021-11-11 15:42, Shankar, Uma wrote: > > > > > > > > >> -----Original Message----- > > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> Sent: Thursday, November 11, 2021 10:13 PM > > >> To: Harry Wentland <harry.wentland@amd.com> > > >> Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri- > > >> devel@lists.freedesktop.org; ppaalanen@gmail.com; brian.starkey@arm.com; > > >> sebastian@sebastianwick.net; Shashank.Sharma@amd.com > > >> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for > > >> HDR planes > > >> > > >> On Thu, Nov 11, 2021 at 10:17:17AM -0500, 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, > > >>>> + .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, > > >>>> + .min = 0, .max = (1 << 27) - 1, > > >>>> + }, > > >>>> + /* Segment 3 */ > > >>>> + { > > >>>> + .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, .end = 3 << 24, > > >>>> + .min = 0, .max = (1 << 27) - 1, > > >>>> + }, > > >>>> + /* Segment 4 */ > > >>>> + { > > >>>> + .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 = 3 << 24, .end = 7 << 24, > > >>>> + .min = 0, .max = (1 << 27) - 1, > > >>>> + }, > > >>>> +}; > > >>> > > >>> If I understand this right, userspace would need this definition in > > >>> order to populate the degamma blob. Should this sit in a UAPI header? > > > > > > Hi Harry, Pekka and Ville, > > > Sorry for being a bit late on the replies, got side tracked with various issues. > > > I am back on this. Apologies for delay. > > > > > >> My original idea (not sure it's fully realized in this series) is to have a new > > >> GAMMA_MODE/etc. enum property on each crtc (or plane) for which each enum > > >> value points to a kernel provided blob that contains one of these LUT descriptors. > > >> Userspace can then query them dynamically and pick the best one for its current use > > >> case. > > > > > > We have this as part of the series Ville. Patch 3 of this series creates a DEGAMMA_MODE > > > property just for this. With that userspace can just query the blob_id's and will get the > > > various degamma mode possible and the respective segment and lut distributions. > > > > > > This will be generic, so for userspace it should just be able to query this and parse and get > > > the lut distribution and segment ranges. > > > > > > > Thanks for the explanation. > > > > Uma, have you had a chance to sketch some of this out in IGT? I'm trying > > to see how userspace would do this in practice and will try to sketch an > > IGT test for this myself, but if you have it already we could share the > > effort. > > > > >> The algorithm for choosing the best one might be something like: > > >> - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc > > >> - prefer interpolated vs. direct lookup based on current needs (eg. X > > >> could prefer direct lookup to get directcolor visuals). > > >> - prefer one with extended range values if needed > > >> - for HDR prefer smaller step size in dark tones, > > >> for SDR perhaps prefer a more uniform step size > > >> > > >> Or maybe we should include some kind of usage hints as well? > > > > > > I think the segment range and distribution of lut should be enough for a userspace > > > to pick the right ones, but we can add some examples in UAPI descriptions as hints. > > > > > > > The range might be enough, but we're already parsing hints like "GAMMA" > > or "DEGAMMA". I wonder if it would make sense to add a flag for "HDR" or > > "SDR" as well. > > > > >> And I was thinking of even adding a new property type (eg. > > >> ENUM_BLOB) just for this sort of usecase. That could let us have a bit more generic > > >> code to do all the validation around the property values and whatnot. > > >> > > >> The one nagging concern I really have with GAMMA_MODE is how a mix of old and > > >> new userspace would work. Though that is more of a generic issue with any new > > >> property really. > > > > > > For plane properties getting added newly, old userspace will not get it so I think this should be ok. > > > Newer userspace will implement this and get the new functionality. > > > Problem will be in extending this to crtc where we have a legacy baggage, the client caps approach > > > may help us there. Have it as part of separate series just to not mix it with this new plane stuff, though > > > the idea remains same based on your design. Series below for reference: > > > https://patchwork.freedesktop.org/series/90821/>> > > > > Could we just assume we do a uniform LUT if userspace doesn't > > set a _MODE enum value for the respective gamma? > > > > Maybe the _MODE should have a default enum value that means > > a uniform (legacy) LUT. > > Yeah, it definitely needs a default like that. But the problem arises > when new userspace sets it to something else and then hands the reins > over to some old userspace that doesn't know how to reset it back to > default. This very problem is the one where I have been suggesting that userspace that supports temporarily handing DRM-master to something else needs to be prepared to save and restore also unrecognized KMS properties. We've also had talk about a "reset" switch in KMS, but I forget the conclusion. Both ideas lack the people working on them. I don't think we can design each new KMS property ad hoc to somehow magically be compatible with old vs. new client interoperation. In fact, the problem exists already with e.g. the old GAMMA etc. properties. OTOH, when a userspace client is reported to misbehave because something else left KMS in a funny state, it is just too easy to simply patch the innocent but misbehaving client to understand whatever new property the other one was using, particularly if it's just to reset it to some hardcoded expected value. So it's unclear if this problem even needs a solution. Thanks, pq
On 2021-11-12 03:37, Pekka Paalanen wrote: > On Thu, 11 Nov 2021 21:58:35 +0000 > "Shankar, Uma" <uma.shankar@intel.com> wrote: > >>> -----Original Message----- >>> From: Harry Wentland <harry.wentland@amd.com> >>> Sent: Friday, November 12, 2021 2:41 AM >>> To: Shankar, Uma <uma.shankar@intel.com>; Ville Syrjälä >>> <ville.syrjala@linux.intel.com> >>> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; >>> ppaalanen@gmail.com; brian.starkey@arm.com; sebastian@sebastianwick.net; >>> Shashank.Sharma@amd.com >>> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for >>> HDR planes >>> >>> >>> >>> On 2021-11-11 15:42, Shankar, Uma wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>> Sent: Thursday, November 11, 2021 10:13 PM >>>>> To: Harry Wentland <harry.wentland@amd.com> >>>>> Cc: Shankar, Uma <uma.shankar@intel.com>; >>>>> intel-gfx@lists.freedesktop.org; dri- devel@lists.freedesktop.org; >>>>> ppaalanen@gmail.com; brian.starkey@arm.com; >>>>> sebastian@sebastianwick.net; Shashank.Sharma@amd.com >>>>> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range >>>>> struct for HDR planes >>>>> >>>>> On Thu, Nov 11, 2021 at 10:17:17AM -0500, 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, >>>>>>> + .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, >>>>>>> + .min = 0, .max = (1 << 27) - 1, >>>>>>> + }, >>>>>>> + /* Segment 3 */ >>>>>>> + { >>>>>>> + .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, .end = 3 << 24, >>>>>>> + .min = 0, .max = (1 << 27) - 1, >>>>>>> + }, >>>>>>> + /* Segment 4 */ >>>>>>> + { >>>>>>> + .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 = 3 << 24, .end = 7 << 24, >>>>>>> + .min = 0, .max = (1 << 27) - 1, >>>>>>> + }, >>>>>>> +}; >>>>>> >>>>>> If I understand this right, userspace would need this definition in >>>>>> order to populate the degamma blob. Should this sit in a UAPI header? > > Are you asking whether 'struct drm_color_lut_range` is defined in any > userspace visible header? > > It seems to be in patch 2. > >>>> >>>> Hi Harry, Pekka and Ville, >>>> Sorry for being a bit late on the replies, got side tracked with various issues. >>>> I am back on this. Apologies for delay. >>>> >>>>> My original idea (not sure it's fully realized in this series) is to >>>>> have a new GAMMA_MODE/etc. enum property on each crtc (or plane) for >>>>> which each enum value points to a kernel provided blob that contains one of >>> these LUT descriptors. >>>>> Userspace can then query them dynamically and pick the best one for >>>>> its current use case. >>>> >>>> We have this as part of the series Ville. Patch 3 of this series >>>> creates a DEGAMMA_MODE property just for this. With that userspace can >>>> just query the blob_id's and will get the various degamma mode possible and the >>> respective segment and lut distributions. >>>> >>>> This will be generic, so for userspace it should just be able to query >>>> this and parse and get the lut distribution and segment ranges. >>>> >>> >>> Thanks for the explanation. >>> >>> Uma, have you had a chance to sketch some of this out in IGT? I'm trying to see how >>> userspace would do this in practice and will try to sketch an IGT test for this myself, >>> but if you have it already we could share the effort. >> >> Yes Harry, we do have some sample IGT's to test this. Will send those out and will copy >> you and all the stakeholders. >> Thanks. The set is on my list of items to review. >>>>> The algorithm for choosing the best one might be something like: >>>>> - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc >>>>> - prefer interpolated vs. direct lookup based on current needs (eg. X >>>>> could prefer direct lookup to get directcolor visuals). >>>>> - prefer one with extended range values if needed >>>>> - for HDR prefer smaller step size in dark tones, >>>>> for SDR perhaps prefer a more uniform step size >>>>> >>>>> Or maybe we should include some kind of usage hints as well? >>>> >>>> I think the segment range and distribution of lut should be enough for >>>> a userspace to pick the right ones, but we can add some examples in UAPI >>> descriptions as hints. >>>> >>> >>> The range might be enough, but we're already parsing hints like "GAMMA" >>> or "DEGAMMA". I wonder if it would make sense to add a flag for "HDR" or "SDR" as >>> well. > > What hints are GAMMA or DEGAMMA and who's parsing them? I thought they > are just arbitrary names to identify the element's position in the > abstract pipeline. > They are provided with the segment definitions, e.g. in https://patchwork.freedesktop.org/patch/452589/?series=90826&rev=2 I believe they are indicating whether a segment definition is intended for degamma (linearization) use or for gamma (delinearization) use. >> >> On Intel hardware, we differentiate this with precision and have HDR planes (they have extra >> Lut precision and samples) separately called out. We could add SDR/HDR FLAG as well. > > What about wide color gamut SDR? That probably needs more precision > than "normal" SDR but is not HDR. > > I can't think of how SDR/HDR flags would work or what they would mean. > Feels a bit too simple for practice. Maybe that concept should be > created by a hypothetical userspace helper library instead. > Maybe this is a decision best left up to compositors. A compositor will know best what precision and range it needs. Harry > > Thanks, > pq >
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, + .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, + .min = 0, .max = (1 << 27) - 1, + }, + /* Segment 3 */ + { + .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, .end = 3 << 24, + .min = 0, .max = (1 << 27) - 1, + }, + /* Segment 4 */ + { + .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 = 3 << 24, .end = 7 << 24, + .min = 0, .max = (1 << 27) - 1, + }, +}; + void intel_color_init(struct intel_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
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(+)