[04/13] drm/i915: Clean up various indexed LUT registers

Message ID	20221123152638.20622-5-ville.syrjala@linux.intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: Ville Syrjala <ville.syrjala@linux.intel.com> To: intel-gfx@lists.freedesktop.org Date: Wed, 23 Nov 2022 17:26:29 +0200 Message-Id: <20221123152638.20622-5-ville.syrjala@linux.intel.com> In-Reply-To: <20221123152638.20622-1-ville.syrjala@linux.intel.com> References: <20221123152638.20622-1-ville.syrjala@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: [Intel-gfx] [PATCH 04/13] drm/i915: Clean up various indexed LUT registers Precedence: list Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>
Series	drm/i915: Gamma/DSB prep work \| expand [00/13] drm/i915: Gamma/DSB prep work [01/13] drm/i915: Shorten GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED a bit [02/13] drm/i915: Clean up GAMMA_MODE defines [03/13] drm/i915: Define skl+ palette anti-collision bit [04/13] drm/i915: Clean up various indexed LUT registers [05/13] drm/i915: Standardize auto-increment LUT load procedure [06/13] drm/i915: Document LUT "max" register precision [07/13] drm/i915: Move the DSB->mmio fallback into the LUT code [08/13] drm/i915: Move the DSB setup/cleaup into the color code [09/13] drm/i915: Make DSB lower level [10/13] drm/i915: Disable DSB usage specifically for LUTs [11/13] Revert "drm/i915: Disable DSB usage for now" [12/13] drm/i915: Use ilk_lut_write*() for all ilk+ gamma modes [13/13] drm/i915: Do state check for color management changes

Message ID

20221123152638.20622-5-ville.syrjala@linux.intel.com (mailing list archive)

State

New, archived

Headers

From: Ville Syrjala <ville.syrjala@linux.intel.com>
To: intel-gfx@lists.freedesktop.org
Date: Wed, 23 Nov 2022 17:26:29 +0200
Message-Id: <20221123152638.20622-5-ville.syrjala@linux.intel.com>
In-Reply-To: <20221123152638.20622-1-ville.syrjala@linux.intel.com>
References: <20221123152638.20622-1-ville.syrjala@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Subject: [Intel-gfx] [PATCH 04/13] drm/i915: Clean up various indexed LUT
 registers
Precedence: list
Errors-To: intel-gfx-bounces@lists.freedesktop.org
Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Series

drm/i915: Gamma/DSB prep work | expand

Commit Message

Ville Syrjälä Nov. 23, 2022, 3:26 p.m. UTC

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use REG_BIT() & co. for the LUT index registers, and also
use the REG_FIELD_PREP() stuff a bit more consistently when
generating the values for said registers.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 46 +++++++++++++++-------
 drivers/gpu/drm/i915/i915_reg.h            | 18 +++++----
 2 files changed, 41 insertions(+), 23 deletions(-)

Comments

Nautiyal, Ankit K Dec. 1, 2022, 5:45 a.m. UTC | #1

On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Use REG_BIT() & co. for the LUT index registers, and also
> use the REG_FIELD_PREP() stuff a bit more consistently when
> generating the values for said registers.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_color.c | 46 +++++++++++++++-------
>   drivers/gpu/drm/i915/i915_reg.h            | 18 +++++----
>   2 files changed, 41 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 956b221860e6..c960c2aaf328 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -910,7 +910,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
>   	enum pipe pipe = crtc->pipe;
>   
>   	for (i = 0; i < lut_size; i++) {
> -		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++);
> +		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +				  prec_index + i);
>   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
>   				  ilk_lut_10(&lut[i]));
>   	}
> @@ -919,7 +920,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
>   	 * Reset the index, otherwise it prevents the legacy palette to be
>   	 * written properly.
>   	 */
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  PAL_PREC_INDEX_VALUE(0));
>   }
>   
>   /* On BDW+ the index auto increment mode actually works */
> @@ -933,7 +935,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>   	enum pipe pipe = crtc->pipe;
>   
>   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> +			  PAL_PREC_AUTO_INCREMENT |
> +			  prec_index);
>   
>   	for (i = 0; i < lut_size; i++)
>   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
> @@ -943,7 +946,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>   	 * Reset the index, otherwise it prevents the legacy palette to be
>   	 * written properly.
>   	 */
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  PAL_PREC_INDEX_VALUE(0));
>   }
>   
>   static void ivb_load_lut_ext_max(const struct intel_crtc_state *crtc_state)
> @@ -1049,9 +1053,11 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
>   	 * ignore the index bits, so we need to reset it to index 0
>   	 * separately.
>   	 */
> -	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
>   	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> +	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
>   
>   	for (i = 0; i < lut_size; i++) {
>   		/*
> @@ -1165,7 +1171,9 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>   	 * seg2[0] being unused by the hardware.
>   	 */
>   	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> -			    PAL_PREC_AUTO_INCREMENT);
> +			    PAL_PREC_AUTO_INCREMENT |
> +			    PAL_PREC_INDEX_VALUE(0));
> +
>   	for (i = 1; i < 257; i++) {
>   		entry = &lut[i * 8];
>   		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> @@ -2756,7 +2764,8 @@ static struct drm_property_blob *ivb_read_lut_10(struct intel_crtc *crtc,
>   		ilk_lut_10_pack(&lut[i], val);
>   	}
>   
> -	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe), 0);
> +	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe),
> +			  PAL_PREC_INDEX_VALUE(0));
>   
>   	return blob;
>   }
> @@ -2811,7 +2820,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
>   	lut = blob->data;
>   
>   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> +			  PAL_PREC_AUTO_INCREMENT |
> +			  prec_index);
>   
>   	for (i = 0; i < lut_size; i++) {
>   		u32 val = intel_de_read_fw(i915, PREC_PAL_DATA(pipe));
> @@ -2819,7 +2829,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
>   		ilk_lut_10_pack(&lut[i], val);
>   	}
>   
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  PAL_PREC_INDEX_VALUE(0));
>   
>   	return blob;
>   }
> @@ -2876,9 +2887,11 @@ static struct drm_property_blob *glk_read_degamma_lut(struct intel_crtc *crtc)
>   	 * ignore the index bits, so we need to reset it to index 0
>   	 * separately.
>   	 */
> -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
>   	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
>   
>   	for (i = 0; i < lut_size; i++) {
>   		u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
> @@ -2888,7 +2901,8 @@ static struct drm_property_blob *glk_read_degamma_lut(struct intel_crtc *crtc)
>   		lut[i].blue = val;
>   	}
>   
> -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
>   
>   	return blob;
>   }
> @@ -2934,7 +2948,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
>   	lut = blob->data;
>   
>   	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			  PAL_PREC_AUTO_INCREMENT);
> +			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
> +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>   
>   	for (i = 0; i < 9; i++) {
>   		u32 ldw = intel_de_read_fw(i915, PREC_PAL_MULTI_SEG_DATA(pipe));
> @@ -2943,7 +2958,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
>   		ilk_lut_12p4_pack(&lut[i], ldw, udw);
>   	}
>   
> -	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe), 0);
> +	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>   
>   	/*
>   	 * FIXME readouts from PAL_PREC_DATA register aren't giving
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 80ac50d80af4..22fb9fd78483 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7531,11 +7531,10 @@ enum skl_power_gate {
>   #define _PAL_PREC_INDEX_A	0x4A400
>   #define _PAL_PREC_INDEX_B	0x4AC00
>   #define _PAL_PREC_INDEX_C	0x4B400
> -#define   PAL_PREC_10_12_BIT		(0 << 31)
> -#define   PAL_PREC_SPLIT_MODE		(1 << 31)
> -#define   PAL_PREC_AUTO_INCREMENT	(1 << 15)
> -#define   PAL_PREC_INDEX_VALUE_MASK	(0x3ff << 0)
> -#define   PAL_PREC_INDEX_VALUE(x)	((x) << 0)
> +#define   PAL_PREC_SPLIT_MODE		REG_BIT(31)
> +#define   PAL_PREC_AUTO_INCREMENT	REG_BIT(15)
> +#define   PAL_PREC_INDEX_VALUE_MASK	REG_GENMASK(9, 0)
> +#define   PAL_PREC_INDEX_VALUE(x)	REG_FIELD_PREP(PAL_PREC_INDEX_VALUE_MASK, (x))
>   #define _PAL_PREC_DATA_A	0x4A404
>   #define _PAL_PREC_DATA_B	0x4AC04
>   #define _PAL_PREC_DATA_C	0x4B404
> @@ -7559,7 +7558,9 @@ enum skl_power_gate {
>   #define _PRE_CSC_GAMC_INDEX_A	0x4A484
>   #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
>   #define _PRE_CSC_GAMC_INDEX_C	0x4B484
> -#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
> +#define   PRE_CSC_GAMC_AUTO_INCREMENT	REG_BIT(10)
> +#define   PRE_CSC_GAMC_INDEX_VALUE_MASK	REG_GENMASK(7, 0)


PRE_CSC_GAMC_INDEX_VALUE_MASK till TGL seem to be using bits 0:5. For 
ADL+ this seem to be 0:7 though. Would it make sense to use separate masks?


Regards,

Ankit


> +#define   PRE_CSC_GAMC_INDEX_VALUE(x)	REG_FIELD_PREP(PRE_CSC_GAMC_INDEX_VALUE_MASK, (x))
>   #define _PRE_CSC_GAMC_DATA_A	0x4A488
>   #define _PRE_CSC_GAMC_DATA_B	0x4AC88
>   #define _PRE_CSC_GAMC_DATA_C	0x4B488
> @@ -7570,8 +7571,9 @@ enum skl_power_gate {
>   /* ICL Multi segmented gamma */
>   #define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
>   #define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
> -#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT		REG_BIT(15)
> -#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
> +#define   PAL_PREC_MULTI_SEG_AUTO_INCREMENT	REG_BIT(15)
> +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
> +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE(x)	REG_FIELD_PREP(PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK, (x))
>   
>   #define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
>   #define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C

Shankar, Uma Dec. 7, 2022, 8:45 a.m. UTC | #2

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Nautiyal,
> Ankit K
> Sent: Thursday, December 1, 2022 11:16 AM
> To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 04/13] drm/i915: Clean up various indexed LUT
> registers
> 
> 
> On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Use REG_BIT() & co. for the LUT index registers, and also use the
> > REG_FIELD_PREP() stuff a bit more consistently when generating the
> > values for said registers.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_color.c | 46 +++++++++++++++-------
> >   drivers/gpu/drm/i915/i915_reg.h            | 18 +++++----
> >   2 files changed, 41 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > b/drivers/gpu/drm/i915/display/intel_color.c
> > index 956b221860e6..c960c2aaf328 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -910,7 +910,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
> >   	enum pipe pipe = crtc->pipe;
> >
> >   	for (i = 0; i < lut_size; i++) {
> > -		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++);
> > +		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > +				  prec_index + i);
> >   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
> >   				  ilk_lut_10(&lut[i]));
> >   	}
> > @@ -919,7 +920,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
> >   	 * Reset the index, otherwise it prevents the legacy palette to be
> >   	 * written properly.
> >   	 */
> > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > +			  PAL_PREC_INDEX_VALUE(0));
> >   }
> >
> >   /* On BDW+ the index auto increment mode actually works */ @@ -933,7
> > +935,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
> >   	enum pipe pipe = crtc->pipe;
> >
> >   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> > +			  PAL_PREC_AUTO_INCREMENT |
> > +			  prec_index);
> >
> >   	for (i = 0; i < lut_size; i++)
> >   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe), @@ -943,7 +946,8
> @@
> > static void bdw_load_lut_10(struct intel_crtc *crtc,
> >   	 * Reset the index, otherwise it prevents the legacy palette to be
> >   	 * written properly.
> >   	 */
> > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > +			  PAL_PREC_INDEX_VALUE(0));
> >   }
> >
> >   static void ivb_load_lut_ext_max(const struct intel_crtc_state
> > *crtc_state) @@ -1049,9 +1053,11 @@ static void glk_load_degamma_lut(const
> struct intel_crtc_state *crtc_state,
> >   	 * ignore the index bits, so we need to reset it to index 0
> >   	 * separately.
> >   	 */
> > -	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
> >   	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> > -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > +	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> > +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> >
> >   	for (i = 0; i < lut_size; i++) {
> >   		/*
> > @@ -1165,7 +1171,9 @@ icl_program_gamma_multi_segment(const struct
> intel_crtc_state *crtc_state)
> >   	 * seg2[0] being unused by the hardware.
> >   	 */
> >   	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> > -			    PAL_PREC_AUTO_INCREMENT);
> > +			    PAL_PREC_AUTO_INCREMENT |
> > +			    PAL_PREC_INDEX_VALUE(0));
> > +
> >   	for (i = 1; i < 257; i++) {
> >   		entry = &lut[i * 8];
> >   		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> @@
> > -2756,7 +2764,8 @@ static struct drm_property_blob *ivb_read_lut_10(struct
> intel_crtc *crtc,
> >   		ilk_lut_10_pack(&lut[i], val);
> >   	}
> >
> > -	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe), 0);
> > +	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe),
> > +			  PAL_PREC_INDEX_VALUE(0));
> >
> >   	return blob;
> >   }
> > @@ -2811,7 +2820,8 @@ static struct drm_property_blob
> *bdw_read_lut_10(struct intel_crtc *crtc,
> >   	lut = blob->data;
> >
> >   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> > +			  PAL_PREC_AUTO_INCREMENT |
> > +			  prec_index);
> >
> >   	for (i = 0; i < lut_size; i++) {
> >   		u32 val = intel_de_read_fw(i915, PREC_PAL_DATA(pipe)); @@ -
> 2819,7
> > +2829,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc
> *crtc,
> >   		ilk_lut_10_pack(&lut[i], val);
> >   	}
> >
> > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > +			  PAL_PREC_INDEX_VALUE(0));
> >
> >   	return blob;
> >   }
> > @@ -2876,9 +2887,11 @@ static struct drm_property_blob
> *glk_read_degamma_lut(struct intel_crtc *crtc)
> >   	 * ignore the index bits, so we need to reset it to index 0
> >   	 * separately.
> >   	 */
> > -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> >   	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> >
> >   	for (i = 0; i < lut_size; i++) {
> >   		u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
> @@
> > -2888,7 +2901,8 @@ static struct drm_property_blob
> *glk_read_degamma_lut(struct intel_crtc *crtc)
> >   		lut[i].blue = val;
> >   	}
> >
> > -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> > +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> >
> >   	return blob;
> >   }
> > @@ -2934,7 +2948,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
> >   	lut = blob->data;
> >
> >   	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> > -			  PAL_PREC_AUTO_INCREMENT);
> > +			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
> > +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> >
> >   	for (i = 0; i < 9; i++) {
> >   		u32 ldw = intel_de_read_fw(i915,
> PREC_PAL_MULTI_SEG_DATA(pipe));
> > @@ -2943,7 +2958,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
> >   		ilk_lut_12p4_pack(&lut[i], ldw, udw);
> >   	}
> >
> > -	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe), 0);
> > +	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> > +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> >
> >   	/*
> >   	 * FIXME readouts from PAL_PREC_DATA register aren't giving diff
> > --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 80ac50d80af4..22fb9fd78483
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7531,11 +7531,10 @@ enum skl_power_gate {
> >   #define _PAL_PREC_INDEX_A	0x4A400
> >   #define _PAL_PREC_INDEX_B	0x4AC00
> >   #define _PAL_PREC_INDEX_C	0x4B400
> > -#define   PAL_PREC_10_12_BIT		(0 << 31)
> > -#define   PAL_PREC_SPLIT_MODE		(1 << 31)
> > -#define   PAL_PREC_AUTO_INCREMENT	(1 << 15)
> > -#define   PAL_PREC_INDEX_VALUE_MASK	(0x3ff << 0)
> > -#define   PAL_PREC_INDEX_VALUE(x)	((x) << 0)
> > +#define   PAL_PREC_SPLIT_MODE		REG_BIT(31)
> > +#define   PAL_PREC_AUTO_INCREMENT	REG_BIT(15)
> > +#define   PAL_PREC_INDEX_VALUE_MASK	REG_GENMASK(9, 0)
> > +#define   PAL_PREC_INDEX_VALUE(x)
> 	REG_FIELD_PREP(PAL_PREC_INDEX_VALUE_MASK, (x))
> >   #define _PAL_PREC_DATA_A	0x4A404
> >   #define _PAL_PREC_DATA_B	0x4AC04
> >   #define _PAL_PREC_DATA_C	0x4B404
> > @@ -7559,7 +7558,9 @@ enum skl_power_gate {
> >   #define _PRE_CSC_GAMC_INDEX_A	0x4A484
> >   #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
> >   #define _PRE_CSC_GAMC_INDEX_C	0x4B484
> > -#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
> > +#define   PRE_CSC_GAMC_AUTO_INCREMENT	REG_BIT(10)
> > +#define   PRE_CSC_GAMC_INDEX_VALUE_MASK	REG_GENMASK(7, 0)
> 
> 
> PRE_CSC_GAMC_INDEX_VALUE_MASK till TGL seem to be using bits 0:5. For
> ADL+ this seem to be 0:7 though. Would it make sense to use separate masks?

We are using it mostly to reset the counter. Keeping a mask 0:7 should be a superset
and may cover the 0:5 case. Though technically it looks a bit off though.

Regards,
Uma Shankar

> 
> Regards,
> 
> Ankit
> 
> 
> > +#define   PRE_CSC_GAMC_INDEX_VALUE(x)
> 	REG_FIELD_PREP(PRE_CSC_GAMC_INDEX_VALUE_MASK, (x))
> >   #define _PRE_CSC_GAMC_DATA_A	0x4A488
> >   #define _PRE_CSC_GAMC_DATA_B	0x4AC88
> >   #define _PRE_CSC_GAMC_DATA_C	0x4B488
> > @@ -7570,8 +7571,9 @@ enum skl_power_gate {
> >   /* ICL Multi segmented gamma */
> >   #define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
> >   #define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
> > -#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT
> 	REG_BIT(15)
> > -#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4,
> 0)
> > +#define   PAL_PREC_MULTI_SEG_AUTO_INCREMENT	REG_BIT(15)
> > +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK	REG_GENMASK(4,
> 0)
> > +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE(x)
> 	REG_FIELD_PREP(PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK, (x))
> >
> >   #define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
> >   #define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C

Shankar, Uma Dec. 7, 2022, 10:22 a.m. UTC | #3

> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Nautiyal, Ankit K
> > Sent: Thursday, December 1, 2022 11:16 AM
> > To: Ville Syrjala <ville.syrjala@linux.intel.com>;
> > intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 04/13] drm/i915: Clean up various
> > indexed LUT registers
> >
> >
> > On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Use REG_BIT() & co. for the LUT index registers, and also use the
> > > REG_FIELD_PREP() stuff a bit more consistently when generating the
> > > values for said registers.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_color.c | 46 +++++++++++++++-------
> > >   drivers/gpu/drm/i915/i915_reg.h            | 18 +++++----
> > >   2 files changed, 41 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > > b/drivers/gpu/drm/i915/display/intel_color.c
> > > index 956b221860e6..c960c2aaf328 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > > @@ -910,7 +910,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
> > >   	enum pipe pipe = crtc->pipe;
> > >
> > >   	for (i = 0; i < lut_size; i++) {
> > > -		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++);
> > > +		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > +				  prec_index + i);
> > >   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
> > >   				  ilk_lut_10(&lut[i]));
> > >   	}
> > > @@ -919,7 +920,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
> > >   	 * Reset the index, otherwise it prevents the legacy palette to be
> > >   	 * written properly.
> > >   	 */
> > > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > +			  PAL_PREC_INDEX_VALUE(0));
> > >   }
> > >
> > >   /* On BDW+ the index auto increment mode actually works */ @@
> > > -933,7
> > > +935,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
> > >   	enum pipe pipe = crtc->pipe;
> > >
> > >   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> > > +			  PAL_PREC_AUTO_INCREMENT |
> > > +			  prec_index);
> > >
> > >   	for (i = 0; i < lut_size; i++)
> > >   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe), @@ -943,7 +946,8
> > @@
> > > static void bdw_load_lut_10(struct intel_crtc *crtc,
> > >   	 * Reset the index, otherwise it prevents the legacy palette to be
> > >   	 * written properly.
> > >   	 */
> > > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > +			  PAL_PREC_INDEX_VALUE(0));
> > >   }
> > >
> > >   static void ivb_load_lut_ext_max(const struct intel_crtc_state
> > > *crtc_state) @@ -1049,9 +1053,11 @@ static void
> > > glk_load_degamma_lut(const
> > struct intel_crtc_state *crtc_state,
> > >   	 * ignore the index bits, so we need to reset it to index 0
> > >   	 * separately.
> > >   	 */
> > > -	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
> > >   	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> > > -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > > +	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> > > +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > >
> > >   	for (i = 0; i < lut_size; i++) {
> > >   		/*
> > > @@ -1165,7 +1171,9 @@ icl_program_gamma_multi_segment(const struct
> > intel_crtc_state *crtc_state)
> > >   	 * seg2[0] being unused by the hardware.
> > >   	 */
> > >   	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> > > -			    PAL_PREC_AUTO_INCREMENT);
> > > +			    PAL_PREC_AUTO_INCREMENT |
> > > +			    PAL_PREC_INDEX_VALUE(0));
> > > +
> > >   	for (i = 1; i < 257; i++) {
> > >   		entry = &lut[i * 8];
> > >   		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> > @@
> > > -2756,7 +2764,8 @@ static struct drm_property_blob
> > > *ivb_read_lut_10(struct
> > intel_crtc *crtc,
> > >   		ilk_lut_10_pack(&lut[i], val);
> > >   	}
> > >
> > > -	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe), 0);
> > > +	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe),
> > > +			  PAL_PREC_INDEX_VALUE(0));
> > >
> > >   	return blob;
> > >   }
> > > @@ -2811,7 +2820,8 @@ static struct drm_property_blob
> > *bdw_read_lut_10(struct intel_crtc *crtc,
> > >   	lut = blob->data;
> > >
> > >   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> > > +			  PAL_PREC_AUTO_INCREMENT |
> > > +			  prec_index);
> > >
> > >   	for (i = 0; i < lut_size; i++) {
> > >   		u32 val = intel_de_read_fw(i915, PREC_PAL_DATA(pipe)); @@ -
> > 2819,7
> > > +2829,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct
> > > +intel_crtc
> > *crtc,
> > >   		ilk_lut_10_pack(&lut[i], val);
> > >   	}
> > >
> > > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > +			  PAL_PREC_INDEX_VALUE(0));
> > >
> > >   	return blob;
> > >   }
> > > @@ -2876,9 +2887,11 @@ static struct drm_property_blob
> > *glk_read_degamma_lut(struct intel_crtc *crtc)
> > >   	 * ignore the index bits, so we need to reset it to index 0
> > >   	 * separately.
> > >   	 */
> > > -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> > >   	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > > -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > > +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > > +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > >
> > >   	for (i = 0; i < lut_size; i++) {
> > >   		u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
> > @@
> > > -2888,7 +2901,8 @@ static struct drm_property_blob
> > *glk_read_degamma_lut(struct intel_crtc *crtc)
> > >   		lut[i].blue = val;
> > >   	}
> > >
> > > -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> > > +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > >
> > >   	return blob;
> > >   }
> > > @@ -2934,7 +2948,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
> > >   	lut = blob->data;
> > >
> > >   	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> > > -			  PAL_PREC_AUTO_INCREMENT);
> > > +			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
> > > +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> > >
> > >   	for (i = 0; i < 9; i++) {
> > >   		u32 ldw = intel_de_read_fw(i915,
> > PREC_PAL_MULTI_SEG_DATA(pipe));
> > > @@ -2943,7 +2958,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
> > >   		ilk_lut_12p4_pack(&lut[i], ldw, udw);
> > >   	}
> > >
> > > -	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe), 0);
> > > +	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> > > +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> > >
> > >   	/*
> > >   	 * FIXME readouts from PAL_PREC_DATA register aren't giving diff
> > > --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index 80ac50d80af4..22fb9fd78483
> > > 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7531,11 +7531,10 @@ enum skl_power_gate {
> > >   #define _PAL_PREC_INDEX_A	0x4A400
> > >   #define _PAL_PREC_INDEX_B	0x4AC00
> > >   #define _PAL_PREC_INDEX_C	0x4B400
> > > -#define   PAL_PREC_10_12_BIT		(0 << 31)
> > > -#define   PAL_PREC_SPLIT_MODE		(1 << 31)
> > > -#define   PAL_PREC_AUTO_INCREMENT	(1 << 15)
> > > -#define   PAL_PREC_INDEX_VALUE_MASK	(0x3ff << 0)
> > > -#define   PAL_PREC_INDEX_VALUE(x)	((x) << 0)
> > > +#define   PAL_PREC_SPLIT_MODE		REG_BIT(31)
> > > +#define   PAL_PREC_AUTO_INCREMENT	REG_BIT(15)
> > > +#define   PAL_PREC_INDEX_VALUE_MASK	REG_GENMASK(9, 0)
> > > +#define   PAL_PREC_INDEX_VALUE(x)
> > 	REG_FIELD_PREP(PAL_PREC_INDEX_VALUE_MASK, (x))
> > >   #define _PAL_PREC_DATA_A	0x4A404
> > >   #define _PAL_PREC_DATA_B	0x4AC04
> > >   #define _PAL_PREC_DATA_C	0x4B404
> > > @@ -7559,7 +7558,9 @@ enum skl_power_gate {
> > >   #define _PRE_CSC_GAMC_INDEX_A	0x4A484
> > >   #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
> > >   #define _PRE_CSC_GAMC_INDEX_C	0x4B484
> > > -#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
> > > +#define   PRE_CSC_GAMC_AUTO_INCREMENT	REG_BIT(10)
> > > +#define   PRE_CSC_GAMC_INDEX_VALUE_MASK	REG_GENMASK(7, 0)
> >
> >
> > PRE_CSC_GAMC_INDEX_VALUE_MASK till TGL seem to be using bits 0:5. For
> > ADL+ this seem to be 0:7 though. Would it make sense to use separate masks?
> 
> We are using it mostly to reset the counter. Keeping a mask 0:7 should be a superset
> and may cover the 0:5 case. Though technically it looks a bit off though.

Leaving to your discretion Ville. Looks good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Regards,
> Uma Shankar
> 
> >
> > Regards,
> >
> > Ankit
> >
> >
> > > +#define   PRE_CSC_GAMC_INDEX_VALUE(x)
> > 	REG_FIELD_PREP(PRE_CSC_GAMC_INDEX_VALUE_MASK, (x))
> > >   #define _PRE_CSC_GAMC_DATA_A	0x4A488
> > >   #define _PRE_CSC_GAMC_DATA_B	0x4AC88
> > >   #define _PRE_CSC_GAMC_DATA_C	0x4B488
> > > @@ -7570,8 +7571,9 @@ enum skl_power_gate {
> > >   /* ICL Multi segmented gamma */
> > >   #define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
> > >   #define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
> > > -#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT
> > 	REG_BIT(15)
> > > -#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4,
> > 0)
> > > +#define   PAL_PREC_MULTI_SEG_AUTO_INCREMENT	REG_BIT(15)
> > > +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK	REG_GENMASK(4,
> > 0)
> > > +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE(x)
> > 	REG_FIELD_PREP(PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK, (x))
> > >
> > >   #define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
> > >   #define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 956b221860e6..c960c2aaf328 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -910,7 +910,8 @@  static void ivb_load_lut_10(struct intel_crtc *crtc,
 	enum pipe pipe = crtc->pipe;
 
 	for (i = 0; i < lut_size; i++) {
-		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++);
+		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+				  prec_index + i);
 		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
 				  ilk_lut_10(&lut[i]));
 	}
@@ -919,7 +920,8 @@  static void ivb_load_lut_10(struct intel_crtc *crtc,
 	 * Reset the index, otherwise it prevents the legacy palette to be
 	 * written properly.
 	 */
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
+	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+			  PAL_PREC_INDEX_VALUE(0));
 }
 
 /* On BDW+ the index auto increment mode actually works */
@@ -933,7 +935,8 @@  static void bdw_load_lut_10(struct intel_crtc *crtc,
 	enum pipe pipe = crtc->pipe;
 
 	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
-			  prec_index | PAL_PREC_AUTO_INCREMENT);
+			  PAL_PREC_AUTO_INCREMENT |
+			  prec_index);
 
 	for (i = 0; i < lut_size; i++)
 		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
@@ -943,7 +946,8 @@  static void bdw_load_lut_10(struct intel_crtc *crtc,
 	 * Reset the index, otherwise it prevents the legacy palette to be
 	 * written properly.
 	 */
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
+	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+			  PAL_PREC_INDEX_VALUE(0));
 }
 
 static void ivb_load_lut_ext_max(const struct intel_crtc_state *crtc_state)
@@ -1049,9 +1053,11 @@  static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
 	 * ignore the index bits, so we need to reset it to index 0
 	 * separately.
 	 */
-	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
 	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
-			  PRE_CSC_GAMC_AUTO_INCREMENT);
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
+	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
+			  PRE_CSC_GAMC_AUTO_INCREMENT |
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
 
 	for (i = 0; i < lut_size; i++) {
 		/*
@@ -1165,7 +1171,9 @@  icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 	 * seg2[0] being unused by the hardware.
 	 */
 	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
-			    PAL_PREC_AUTO_INCREMENT);
+			    PAL_PREC_AUTO_INCREMENT |
+			    PAL_PREC_INDEX_VALUE(0));
+
 	for (i = 1; i < 257; i++) {
 		entry = &lut[i * 8];
 		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
@@ -2756,7 +2764,8 @@  static struct drm_property_blob *ivb_read_lut_10(struct intel_crtc *crtc,
 		ilk_lut_10_pack(&lut[i], val);
 	}
 
-	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe), 0);
+	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe),
+			  PAL_PREC_INDEX_VALUE(0));
 
 	return blob;
 }
@@ -2811,7 +2820,8 @@  static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
 	lut = blob->data;
 
 	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
-			  prec_index | PAL_PREC_AUTO_INCREMENT);
+			  PAL_PREC_AUTO_INCREMENT |
+			  prec_index);
 
 	for (i = 0; i < lut_size; i++) {
 		u32 val = intel_de_read_fw(i915, PREC_PAL_DATA(pipe));
@@ -2819,7 +2829,8 @@  static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
 		ilk_lut_10_pack(&lut[i], val);
 	}
 
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
+	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+			  PAL_PREC_INDEX_VALUE(0));
 
 	return blob;
 }
@@ -2876,9 +2887,11 @@  static struct drm_property_blob *glk_read_degamma_lut(struct intel_crtc *crtc)
 	 * ignore the index bits, so we need to reset it to index 0
 	 * separately.
 	 */
-	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
 	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
-			  PRE_CSC_GAMC_AUTO_INCREMENT);
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
+	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
+			  PRE_CSC_GAMC_AUTO_INCREMENT |
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
 
 	for (i = 0; i < lut_size; i++) {
 		u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
@@ -2888,7 +2901,8 @@  static struct drm_property_blob *glk_read_degamma_lut(struct intel_crtc *crtc)
 		lut[i].blue = val;
 	}
 
-	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
+	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
 
 	return blob;
 }
@@ -2934,7 +2948,8 @@  icl_read_lut_multi_segment(struct intel_crtc *crtc)
 	lut = blob->data;
 
 	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
-			  PAL_PREC_AUTO_INCREMENT);
+			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
+			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
 
 	for (i = 0; i < 9; i++) {
 		u32 ldw = intel_de_read_fw(i915, PREC_PAL_MULTI_SEG_DATA(pipe));
@@ -2943,7 +2958,8 @@  icl_read_lut_multi_segment(struct intel_crtc *crtc)
 		ilk_lut_12p4_pack(&lut[i], ldw, udw);
 	}
 
-	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe), 0);
+	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
+			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
 
 	/*
 	 * FIXME readouts from PAL_PREC_DATA register aren't giving
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 80ac50d80af4..22fb9fd78483 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7531,11 +7531,10 @@  enum skl_power_gate {
 #define _PAL_PREC_INDEX_A	0x4A400
 #define _PAL_PREC_INDEX_B	0x4AC00
 #define _PAL_PREC_INDEX_C	0x4B400
-#define   PAL_PREC_10_12_BIT		(0 << 31)
-#define   PAL_PREC_SPLIT_MODE		(1 << 31)
-#define   PAL_PREC_AUTO_INCREMENT	(1 << 15)
-#define   PAL_PREC_INDEX_VALUE_MASK	(0x3ff << 0)
-#define   PAL_PREC_INDEX_VALUE(x)	((x) << 0)
+#define   PAL_PREC_SPLIT_MODE		REG_BIT(31)
+#define   PAL_PREC_AUTO_INCREMENT	REG_BIT(15)
+#define   PAL_PREC_INDEX_VALUE_MASK	REG_GENMASK(9, 0)
+#define   PAL_PREC_INDEX_VALUE(x)	REG_FIELD_PREP(PAL_PREC_INDEX_VALUE_MASK, (x))
 #define _PAL_PREC_DATA_A	0x4A404
 #define _PAL_PREC_DATA_B	0x4AC04
 #define _PAL_PREC_DATA_C	0x4B404
@@ -7559,7 +7558,9 @@  enum skl_power_gate {
 #define _PRE_CSC_GAMC_INDEX_A	0x4A484
 #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
 #define _PRE_CSC_GAMC_INDEX_C	0x4B484
-#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
+#define   PRE_CSC_GAMC_AUTO_INCREMENT	REG_BIT(10)
+#define   PRE_CSC_GAMC_INDEX_VALUE_MASK	REG_GENMASK(7, 0)
+#define   PRE_CSC_GAMC_INDEX_VALUE(x)	REG_FIELD_PREP(PRE_CSC_GAMC_INDEX_VALUE_MASK, (x))
 #define _PRE_CSC_GAMC_DATA_A	0x4A488
 #define _PRE_CSC_GAMC_DATA_B	0x4AC88
 #define _PRE_CSC_GAMC_DATA_C	0x4B488
@@ -7570,8 +7571,9 @@  enum skl_power_gate {
 /* ICL Multi segmented gamma */
 #define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
 #define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
-#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT		REG_BIT(15)
-#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
+#define   PAL_PREC_MULTI_SEG_AUTO_INCREMENT	REG_BIT(15)
+#define   PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
+#define   PAL_PREC_MULTI_SEG_INDEX_VALUE(x)	REG_FIELD_PREP(PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK, (x))
 
 #define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
 #define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C

[04/13] drm/i915: Clean up various indexed LUT registers

Commit Message

Comments

Patch