Re: [PATCH v4 08/10] drm/i915/dsb: Enable gamma lut programming using DSB.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 30 Aug 2019, Animesh Manna <animesh.manna@xxxxxxxxx> wrote:
> Gamma lut programming can be programmed using DSB
> where bulk register programming can be done using indexed
> register write which takes number of data and the mmio offset
> to be written.
>
> v1: Initial version.
> v2: Directly call dsb-api at callsites. (Jani)
>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 64 ++++++++++++++--------
>  drivers/gpu/drm/i915/i915_drv.h            |  2 +
>  2 files changed, 43 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 71a0201437a9..e4090d8e4547 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>  	const struct drm_color_lut *lut = blob->data;
>  	int i, lut_size = drm_color_lut_size(blob);
>  	enum pipe pipe = crtc->pipe;
> +	struct intel_dsb *dsb = dev_priv->dsb;

No. Don't put dsb in dev_priv. You have 12 DSB engines, 3 per pipe. You
have intel_crtc for that.

Was this not clear from my previous review?

You have tons of functions here that will never have a DSB engine, it
makes no sense to convert all of them to use the DSB.

>  
> -	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
> -		   PAL_PREC_AUTO_INCREMENT);
> +	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe),
> +			    prec_index | PAL_PREC_AUTO_INCREMENT);
>  
>  	for (i = 0; i < hw_lut_size; i++) {
>  		/* We discard half the user entries in split gamma mode */
>  		const struct drm_color_lut *entry =
>  			&lut[i * (lut_size - 1) / (hw_lut_size - 1)];
>  
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_10(entry));
>  	}
>  
>  	/*
>  	 * Reset the index, otherwise it prevents the legacy palette to be
>  	 * written properly.
>  	 */
> -	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> +	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0);
>  }
>  
>  static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_dsb *dsb = dev_priv->dsb;
>  	enum pipe pipe = crtc->pipe;
>  
>  	/* Program the max register to clamp values > 1.0. */
> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
> +
>  
>  	/*
>  	 * Program the gc max 2 register to clamp values > 1.0.
> @@ -624,9 +628,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>  	 * from 3.0 to 7.0
>  	 */
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0),
> +				    1 << 16);
> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1),
> +				    1 << 16);
> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2),
> +				    1 << 16);
>  	}
>  }
>  
> @@ -788,12 +795,13 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state,
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_dsb *dsb = dev_priv->dsb;
>  	enum pipe pipe = crtc->pipe;
>  
>  	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red);
> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green);
> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue);
> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red);
> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green);
> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue);
>  }
>  
>  static void
> @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>  	const struct drm_color_lut *lut = blob->data;
> +	struct intel_dsb *dsb = dev_priv->dsb;
>  	enum pipe pipe = crtc->pipe;
>  	u32 i;
>  
> @@ -813,15 +822,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>  	 * Superfine segment has 9 entries, corresponding to values
>  	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>  	 */
> -	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> +	intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			    PAL_PREC_AUTO_INCREMENT);
>  
>  	for (i = 0; i < 9; i++) {
>  		const struct drm_color_lut *entry = &lut[i];
>  
> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> -			   ilk_lut_12p4_ldw(entry));
> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> -			   ilk_lut_12p4_udw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
> +					    ilk_lut_12p4_ldw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
> +					    ilk_lut_12p4_udw(entry));
>  	}
>  }
>  
> @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>  	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>  	const struct drm_color_lut *lut = blob->data;
>  	const struct drm_color_lut *entry;
> +	struct intel_dsb *dsb = dev_priv->dsb;
>  	enum pipe pipe = crtc->pipe;
>  	u32 i;
>  
> @@ -847,11 +858,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>  	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>  	 * with seg2[0] being unused by the hardware.
>  	 */
> -	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> +	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>  	for (i = 1; i < 257; i++) {
>  		entry = &lut[i * 8];
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_12p4_ldw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_12p4_udw(entry));
>  	}
>  
>  	/*
> @@ -868,8 +881,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>  	 */
>  	for (i = 0; i < 256; i++) {
>  		entry = &lut[i * 8 * 128];
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_12p4_ldw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_12p4_udw(entry));
>  	}
>  
>  	/* The last entry in the LUT is to be programmed in GCMAX */
> @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>  
> +	dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc));

No, don't do this. Don't store crtc specific info in a device global
structure.

>  	dev_priv->display.load_luts(crtc_state);
> +	intel_dsb_commit(dev_priv->dsb);
> +	intel_dsb_put(dev_priv->dsb);

Please localize the use of DSB where needed. Move the gets and puts
within the relevant platform specific ->load_luts hooks.

>  }
>  
>  void intel_color_commit(const struct intel_crtc_state *crtc_state)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7aa11e3dd413..98f546c4ad49 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1760,6 +1760,8 @@ struct drm_i915_private {
>  	/* Mutex to protect the above hdcp component related values. */
>  	struct mutex hdcp_comp_mutex;
>  
> +	struct intel_dsb *dsb;
> +
>  	/*
>  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>  	 * will be rejected. Instead look for a better place.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux