Re: [v9][PATCH 01/11] drm/i915/display: Add func to get gamma bit precision

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 30 Aug 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
> Each platform supports different gamma modes and each gamma mode
> has different bit precision. Here bit precision corresponds
> to number of bits the hw LUT supports.
>
> Add func per platform to return bit precision corresponding to gamma mode
> which will be later used as a parameter in lut comparison function
> intel_color_lut_equal().
>
> This is done for legacy, i965, ilk, glk, icl and their variant platforms.
>
> v6: -Added func intel_color_get_bit_precision() to get bit precision for
>      gamma and degamma lut readout depending upon platform and
>      corresponding to load_luts() [Ankit]
>     -Made patch11 as patch3 [Jani]
> v7: -Renamed func intel_color_get_bit_precision() to
>      intel_color_get_gamma_bit_precision()
>     -Added separate function/platform for gamma bit precision [Ville]
>     -Corrected checkpatch warnings
> v8: -Split patch 3 into 4 separate patches
> v9: -Changed commit message, gave more info [Uma]
>     -Added precision func for icl+ platform
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 99 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_color.h |  1 +
>  2 files changed, 100 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 71a0201..dcc65d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1371,6 +1371,105 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static int i9xx_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> +	if (!crtc_state->gamma_enable)
> +		return 0;
> +
> +	switch (crtc_state->gamma_mode) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		return 8;
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 16;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +static int ilk_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> +	if (!crtc_state->gamma_enable)
> +		return 0;
> +
> +	if ((crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0)
> +		return 0;
> +
> +	switch (crtc_state->gamma_mode) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		return 8;
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 10;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +static int chv_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
> +		return 10;
> +	else
> +		return i9xx_gamma_precision(crtc_state);

Why does one branch check for ->gamma_enable and the other not? See below.

> +}
> +
> +static int glk_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{
> +	if (!crtc_state->gamma_enable)
> +		return 0;
> +
> +	switch (crtc_state->gamma_mode) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		return 8;
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 10;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +static int icl_gamma_precision(const struct intel_crtc_state *crtc_state)
> +{

Why does this function not check for ->gamma_enable but the others do?
See below.

> +	if ((crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE) == 0)
> +		return 0;
> +
> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
> +	case GAMMA_MODE_MODE_8BIT:
> +		return 8;
> +	case GAMMA_MODE_MODE_10BIT:
> +		return 10;
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +		return 16;
> +	default:
> +		MISSING_CASE(crtc_state->gamma_mode);
> +		return 0;
> +	}
> +}
> +
> +int intel_color_get_gamma_bit_precision(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);
> +

Should the ->gamma_enable check be here once, instead?

With that fixed,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


BR,
Jani.


> +	if (HAS_GMCH(dev_priv)) {
> +		if (IS_CHERRYVIEW(dev_priv))
> +			return chv_gamma_precision(crtc_state);
> +		else
> +			return i9xx_gamma_precision(crtc_state);
> +	} else {
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			return icl_gamma_precision(crtc_state);
> +		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			return glk_gamma_precision(crtc_state);
> +		else if (IS_IRONLAKE(dev_priv))
> +			return ilk_gamma_precision(crtc_state);
> +	}
> +
> +	return 0;
> +}
> +
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
> index 057e8ac..0226d3a 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.h
> +++ b/drivers/gpu/drm/i915/display/intel_color.h
> @@ -14,5 +14,6 @@
>  void intel_color_commit(const struct intel_crtc_state *crtc_state);
>  void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>  void intel_color_get_config(struct intel_crtc_state *crtc_state);
> +int intel_color_get_gamma_bit_precision(const struct intel_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_COLOR_H__ */

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