Re: [v3 2/3] drm/i915/icl: Enable ICL Pipe CSC block

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

 



On Thu, Nov 29, 2018 at 08:21:42PM +0530, Uma Shankar wrote:
> Enable ICL pipe csc hardware. CSC block is enabled
> in CSC_MODE register instead of PLANE_COLOR_CTL.
> 
> v2: Addressed Maarten's review comments.
> 
> v3: Addressed Matt's review comments. Removed rmw patterns
> as suggested by Matt.
> 
> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  2 ++
>  drivers/gpu/drm/i915/intel_color.c | 28 +++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h   |  3 +++
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b0147bf..6470008 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9824,6 +9824,8 @@ enum skl_power_gate {
>  #define _PIPE_A_CSC_COEFF_RV_GV	0x49020
>  #define _PIPE_A_CSC_COEFF_BV	0x49024
>  #define _PIPE_A_CSC_MODE	0x49028
> +#define   CSC_ENABLE			(1 << 31)
> +#define   OUTPUT_CSC_ENABLE		(1 << 30)
>  #define   CSC_BLACK_SCREEN_OFFSET	(1 << 2)
>  #define   CSC_POSITION_BEFORE_GAMMA	(1 << 1)
>  #define   CSC_MODE_YUV_TO_RGB		(1 << 0)
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 7c8c996..414e1ca 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -108,10 +108,14 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
>  	return result;
>  }
>  
> -static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
> +static void ilk_load_ycbcr_conversion_matrix(struct drm_crtc_state *crtc_state)

It's generally preferred to use Intel types for these lower-level
functions rather than DRM types.  It looks like a lot of the preexisting
color management code breaks from this convention, so we might want to
also clean that up in the preexisting code as a separate patch.


>  {
> +	struct drm_crtc *crtc = crtc_state->crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
> -	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> +	struct intel_crtc_state *intel_crtc_state =
> +					to_intel_crtc_state(crtc_state);
>  
>  	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
>  	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> @@ -129,7 +133,11 @@ static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
>  	I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
>  	I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME);
>  	I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
> -	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		intel_crtc_state->csc_mode |=  CSC_ENABLE;

Shouldn't fixed RGB->YUV be done with OUTPUT_CSC_ENABLE now that we have
two CSC's to work with instead of just one?  I.e., I think we can leave
the first pipe CSC for the userspace-defined transformation (or identity
if none is provided) and then enable the output CSC if we need to
convert to YUV and/or compress to limited range.


Matt

> +	else
> +		intel_crtc_state->csc_mode = 0;
>  }
>  
>  static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
> @@ -151,7 +159,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
>  
>  	if (intel_crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
>  	    intel_crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
> -		ilk_load_ycbcr_conversion_matrix(intel_crtc);
> +		ilk_load_ycbcr_conversion_matrix(crtc_state);
>  		return;
>  	} else if (crtc_state->ctm) {
>  		struct drm_color_ctm *ctm = crtc_state->ctm->data;
> @@ -239,7 +247,10 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
>  		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
>  		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
>  
> -		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			intel_crtc_state->csc_mode |=  CSC_ENABLE;
> +		else
> +			intel_crtc_state->csc_mode = 0;
>  	} else {
>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>  
> @@ -305,9 +316,15 @@ void intel_color_set_csc(struct drm_crtc_state *crtc_state)
>  {
>  	struct drm_device *dev = crtc_state->crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->crtc);
> +	struct intel_crtc_state *intel_crtc_state =
> +				to_intel_crtc_state(crtc_state);
> +	enum pipe pipe = intel_crtc->pipe;
>  
>  	if (dev_priv->display.load_csc_matrix)
>  		dev_priv->display.load_csc_matrix(crtc_state);
> +
> +	I915_WRITE(PIPE_CSC_MODE(pipe), intel_crtc_state->csc_mode);
>  }
>  
>  /* Loads the legacy palette/gamma unit for the CRTC. */
> @@ -734,6 +751,7 @@ void intel_color_init(struct drm_crtc *crtc)
>  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = glk_load_luts;
>  	} else if (IS_ICELAKE(dev_priv)) {
> +		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = icl_load_luts;
>  	} else {
>  		dev_priv->display.load_luts = i9xx_load_luts;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a62d77b..e7b77c1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -922,6 +922,9 @@ struct intel_crtc_state {
>  	/* Gamma mode programmed on the pipe */
>  	uint32_t gamma_mode;
>  
> +	/* CSC mode programmed on the pipe */
> +	uint32_t csc_mode;
> +
>  	/* bitmask of visible planes (enum plane_id) */
>  	u8 active_planes;
>  	u8 nv12_planes;
> -- 
> 1.9.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
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