> -----Original Message-----
> From: Harry Wentland <harry.wentland@xxxxxxx>
> Sent: Friday, November 12, 2021 2:41 AM
> To: Shankar, Uma <uma.shankar@xxxxxxxxx>; Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> ppaalanen@xxxxxxxxx; brian.starkey@xxxxxxx; sebastian@xxxxxxxxxxxxxxxxx;
> Shashank.Sharma@xxxxxxx
> 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@xxxxxxxxxxxxxxx>
> >> Sent: Thursday, November 11, 2021 10:13 PM
> >> To: Harry Wentland <harry.wentland@xxxxxxx>
> >> Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>;
> >> intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- devel@xxxxxxxxxxxxxxxxxxxxx;
> >> ppaalanen@xxxxxxxxx; brian.starkey@xxxxxxx;
> >> sebastian@xxxxxxxxxxxxxxxxx; Shashank.Sharma@xxxxxxx
> >> 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@xxxxxxxxx>
> >>>> ---
> >>>> 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
