diff mbox series

[04/21] drm/i915/xelpd: Define Degamma Lut range struct for HDR planes

Message ID 20210601105218.29185-5-uma.shankar@intel.com (mailing list archive)
State New, archived
Headers show
Series Add Support for Plane Color Lut and CSC features | expand

Commit Message

Shankar, Uma June 1, 2021, 10:52 a.m. UTC
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(+)

Comments

Harry Wentland June 28, 2021, 3:14 p.m. UTC | #1
On 2021-06-01 6:52 a.m., 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 dab892d2251b..c735d06a6b54 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -2093,6 +2093,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 |

Why are these using DRM_MODE_LUT_GAMMA and not DRM_MODE_LUT_DEGAMMA when
the lut_type for this LUT is LUT_TYPE_DEGAMMA?


> +			  DRM_MODE_LUT_REFLECT_NEGATIVE |
> +			  DRM_MODE_LUT_INTERPOLATE |
> +			  DRM_MODE_LUT_NON_DECREASING),
> +		.count = 128,
> +		.input_bpc = 24, .output_bpc = 16,

Why do we need more than 16 bpc for LUT? FP16 is enough to represent HDR in 
linear space. Wouldn't 16 bpc be enough?


> +		.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,

How can max be 1 << 27 if input_bpc is 24?

Harry

> +	},
> +	/* 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);
>
Shankar, Uma June 30, 2021, 11:36 a.m. UTC | #2
> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Harry
> Wentland
> Sent: Monday, June 28, 2021 8:45 PM
> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org
> Cc: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> Subject: Re: [PATCH 04/21] drm/i915/xelpd: Define Degamma Lut range struct for
> HDR planes
> 
> On 2021-06-01 6:52 a.m., 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 dab892d2251b..c735d06a6b54 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -2093,6 +2093,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 |
> 
> Why are these using DRM_MODE_LUT_GAMMA and not
> DRM_MODE_LUT_DEGAMMA when the lut_type for this LUT is
> LUT_TYPE_DEGAMMA?

Thanks Harry for the comments.

Yeah this is an oversight, will fix this.
> 
> 
> > +			  DRM_MODE_LUT_REFLECT_NEGATIVE |
> > +			  DRM_MODE_LUT_INTERPOLATE |
> > +			  DRM_MODE_LUT_NON_DECREASING),
> > +		.count = 128,
> > +		.input_bpc = 24, .output_bpc = 16,
> 
> Why do we need more than 16 bpc for LUT? FP16 is enough to represent HDR in
> linear space. Wouldn't 16 bpc be enough?

Pipe sometimes works internally on higher precision (just to take care of rounding etc.), later the
extra data gets dropped at the end of the pipe. So from source side you are right, 16bpc is enough
but the lut precision can go higher.

> 
> > +		.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,
> 
> How can max be 1 << 27 if input_bpc is 24?

This is to take care of > 1.0 section. 1.0 to 3.0 and 3.0 to 7.0.
So we have 3.24 format for Lut to take care of this. 

Also, I have an action to update the series with UAPI doc and new naming for the property.
My apologies for being late on that one. Will update and send that out soon.

Thanks & Regards,
Uma Shankar
> 
> Harry
> 
> > +	},
> > +	/* 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);
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index dab892d2251b..c735d06a6b54 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -2093,6 +2093,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);