On Thu, 11 Nov 2021 21:58:35 +0000 "Shankar, Uma" wrote: > > -----Original Message----- > > From: Harry Wentland > > Sent: Friday, November 12, 2021 2:41 AM > > To: Shankar, Uma ; Ville Syrjälä > > > > 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ä > > >> Sent: Thursday, November 11, 2021 10:13 PM > > >> To: Harry Wentland > > >> Cc: Shankar, Uma ; > > >> 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 > > >>>> --- > > >>>> 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