Re: [PATCH RFC 6/5] drm/i915: Merge BDW pipe gamma and degamma table code

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH RFC 6/5] drm/i915: Merge BDW pipe gamma and degamma table code
Date: Fri, 27 Jan 2017 16:07:30 +0200	[thread overview]
Message-ID: <20170127140730.GX31595@intel.com> (raw)
In-Reply-To: <20170127091949.6618-1-ander.conselvan.de.oliveira@intel.com>

On Fri, Jan 27, 2017 at 11:19:49AM +0200, Ander Conselvan de Oliveira wrote:
> The only difference between the code loading the pipe gamma and degamma
> tables in BDW is that the gamma code also writes the registers that hold
> the maximum values. So we can use the gamma code for the degamma table,
> at the expense of writing the maximum value register twice, with
> potenttially wrong values in the first time.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
> 
> Ville, does this help with the split gamma enable/disable confusion?

I might. Certainly it avoids a bunch of duplicated code. One comment
below...

> 
> Note that I didn't test this.
> 
> Ander
> 
> ---
>  drivers/gpu/drm/i915/intel_color.c | 57 ++++++++++----------------------------
>  1 file changed, 15 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 08345e5..c686c37 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -340,54 +340,22 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
>  		hsw_enable_ips(intel_crtc);
>  }
>  
> -static void bdw_load_degamma_lut(struct drm_crtc_state *state)
> +static void bdw_load_lut(struct drm_crtc_state *state, u32 offset,
> +			 struct drm_color_lut *lut, u32 lut_size,
> +			 bool split_mode)

Maybe pass PAL_PREC_SPLIT_MODE or 0 here directly? To make the callers
even more clear we could go as far as defining a symbolic name for the 0.

>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
>  	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> -	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> -
> -	I915_WRITE(PREC_PAL_INDEX(pipe),
> -		   PAL_PREC_SPLIT_MODE | PAL_PREC_AUTO_INCREMENT);
> -
> -	if (state->degamma_lut) {
> -		struct drm_color_lut *lut =
> -			(struct drm_color_lut *) state->degamma_lut->data;
> -
> -		for (i = 0; i < lut_size; i++) {
> -			uint32_t word =
> -			drm_color_lut_extract(lut[i].red, 10) << 20 |
> -			drm_color_lut_extract(lut[i].green, 10) << 10 |
> -			drm_color_lut_extract(lut[i].blue, 10);
> -
> -			I915_WRITE(PREC_PAL_DATA(pipe), word);
> -		}
> -	} else {
> -		for (i = 0; i < lut_size; i++) {
> -			uint32_t v = (i * ((1 << 10) - 1)) / (lut_size - 1);
> -
> -			I915_WRITE(PREC_PAL_DATA(pipe),
> -				   (v << 20) | (v << 10) | v);
> -		}
> -	}
> -}
> -
> -static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> -	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> -	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	uint32_t i;
>  
>  	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
>  
>  	I915_WRITE(PREC_PAL_INDEX(pipe),
> -		   (offset ? PAL_PREC_SPLIT_MODE : 0) |
> +		   (split_mode ? PAL_PREC_SPLIT_MODE : 0) |
>  		   PAL_PREC_AUTO_INCREMENT |
>  		   offset);
>  
> -	if (state->gamma_lut) {
> -		struct drm_color_lut *lut =
> -			(struct drm_color_lut *) state->gamma_lut->data;
> -
> +	if (lut) {
>  		for (i = 0; i < lut_size; i++) {
>  			uint32_t word =
>  			(drm_color_lut_extract(lut[i].red, 10) << 20) |
> @@ -430,9 +398,12 @@ static void broadwell_load_luts(struct drm_crtc_state *state)
>  		return;
>  	}
>  
> -	bdw_load_degamma_lut(state);
> -	bdw_load_gamma_lut(state,
> -			   INTEL_INFO(dev_priv)->color.degamma_lut_size);
> +	bdw_load_lut(state, 0, (struct drm_color_lut *) state->degamma_lut,
> +		     INTEL_INFO(dev_priv)->color.degamma_lut_size, true);
> +	bdw_load_lut(state, INTEL_INFO(dev_priv)->color.degamma_lut_size,
> +		     (struct drm_color_lut *) state->gamma_lut,
> +		     INTEL_INFO(dev_priv)->color.gamma_lut_size,
> +		     true);
>  
>  	intel_state->gamma_mode = GAMMA_MODE_MODE_SPLIT;
>  	I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_SPLIT);
> @@ -489,7 +460,9 @@ static void glk_load_luts(struct drm_crtc_state *state)
>  	}
>  
>  	glk_load_degamma_lut(state);
> -	bdw_load_gamma_lut(state, 0);
> +	bdw_load_lut(state, 0, (struct drm_color_lut *) state->gamma_lut,
> +		     INTEL_INFO(dev_priv)->color.gamma_lut_size,
> +		     false);
>  
>  	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
>  	I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT);
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx