Re: [PATCH 03/11] [v4] drm/i915: Extract i9xx_get_color_config()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 17, 2019 at 07:33:25PM +0530, Swati Sharma wrote:
> v4: -No need to initialize *blob [Jani]
>     -Removed right shifts [Jani]
>     -Dropped dev local var [Jani]
> 
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  3 +++
>  drivers/gpu/drm/i915/intel_color.c | 50 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9c206e8..8f2ae8a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7175,6 +7175,9 @@ enum {
>  /* legacy palette */
>  #define _LGC_PALETTE_A           0x4a000
>  #define _LGC_PALETTE_B           0x4a800
> +#define LGC_PALETTE_RED_MASK     REG_GENMASK(23, 16)
> +#define LGC_PALETTE_GREEN_MASK   REG_GENMASK(15, 8)
> +#define LGC_PALETTE_BLUE_MASK    REG_GENMASK(7, 0)
>  #define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4)
>  
>  /* ilk/snb precision palette */
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 309f714..33223e0 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -1229,6 +1229,55 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +/* convert hw value with given bit_precision to lut property val */
> +static u32 intel_color_lut_pack(u32 val, u32 bit_precision)
> +{
> +	u32 max = 0xffff >> (16 - bit_precision);
> +
> +	val = clamp_val(val, 0, max);
> +
> +	if (bit_precision < 16)
> +		val <<= 16 - bit_precision;
> +
> +	return val;
> +}
> +
> +static void i9xx_get_config_internal(struct intel_crtc_state *crtc_state)

The naming of these functions isn't particularly descriptive. I would
probably go for eg. i9xx_read_lut_8(), chv_read_cgm_gamma_lut(), etc.
AFAICS the series is also missing readout for degamma LUTs entirely.

We could also make these functions just return the blob instead of
assigning it internally. That way we can have the higher level
.get_color_config() implementation assign each gamma/degamma lut
apporpriately. Eg. might looks something like this for bdw:

bdw_get_color_config()
{
	if (gamma_mode == 8bit) {
		crtc_state->gamma_lut = i9xx_read_lut_8();
	} else if (gamma_mode == split) {
		crtc_state->degamma_lut = bdw_read_lut_10(SPLIT | 0);
		crtc_state->gamma_lut = bdw_read_lut_10(SPLIT | 512);
	} else if (csc_mode == gamma_before_csc) {
		crtc_state->degamma_lut = bdw_read_lut_10(0);
	} else {
		crtc_state->gamma_lut = bdw_read_lut_10(0);
	}
}

The tricky part will be to figure out how the gamma vs. degamma gets
assigned and/or checked since the choice depends on the presence of
the ctm, rgb limit range, and rgb->ycbcr conversion.

Oh, and most (all maybe?) patches seem to be missing a proper commit
message.

> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	struct drm_property_blob *blob;
> +	struct drm_color_lut *blob_data;
> +	u32 i, val;
> +
> +	blob = drm_property_create_blob(&dev_priv->drm,
> +					sizeof(struct drm_color_lut) * 256,
> +					NULL);
> +	if (IS_ERR(blob))
> +		return;
> +
> +	blob_data = blob->data;
> +
> +	for (i = 0; i < 256; i++) {
> +		if (HAS_GMCH(dev_priv))
> +			val = I915_READ(PALETTE(pipe, i));
> +		else
> +			val = I915_READ(LGC_PALETTE(pipe, i));
> +
> +		blob_data[i].red = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val), 8);
> +		blob_data[i].green = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val), 8);
> +		blob_data[i].blue = intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);
> +	}
> +
> +	crtc_state->base.gamma_lut = blob;
> +}
> +
> +static void i9xx_get_color_config(struct intel_crtc_state *crtc_state)
> +{
> +	i9xx_get_config_internal(crtc_state);
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -1249,6 +1298,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  			dev_priv->display.color_check = i9xx_color_check;
>  			dev_priv->display.color_commit = i9xx_color_commit;
>  			dev_priv->display.load_luts = i9xx_load_luts;
> +			dev_priv->display.get_color_config = i9xx_get_color_config;
>  		}
>  	} else {
>  		if (INTEL_GEN(dev_priv) >= 11)
> -- 
> 1.9.1

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