Re: [PATCH 14/15] drm/i915: skl nv12 workarounds

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chandra Konduru <chandra.konduru@intel.com>
Cc: daniel.vetter@intel.com, intel-gfx@lists.freedesktop.org,
	ville.syrjala@intel.com
Subject: Re: [PATCH 14/15] drm/i915: skl nv12 workarounds
Date: Fri, 4 Sep 2015 14:26:04 +0300	[thread overview]
Message-ID: <20150904112604.GJ29811@intel.com> (raw)
In-Reply-To: <1440032556-9920-15-git-send-email-chandra.konduru@intel.com>

On Wed, Aug 19, 2015 at 06:02:35PM -0700, Chandra Konduru wrote:
> Adding driver workarounds for nv12.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_csr.c     |    2 +-
>  drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  drivers/gpu/drm/i915/intel_sprite.c  |    7 +++++++
>  5 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c4d732f..3192837 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5354,6 +5354,26 @@ enum skl_disp_power_wells {
>  #define PLANE_NV12_BUF_CFG(pipe, plane)	\
>  	_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
>  
> +/*
> + * Skylake Chicken registers
> + */
> +#define _CHICKEN_PIPESL_A          0x420B0
> +#define _CHICKEN_PIPESL_B          0x420B4
> +#define _CHICKEN_PIPESL_C          0x420B8
> +#define  DISABLE_STREAMER_FIX      (1 << 22)
> +#define CHICKEN_PIPESL(pipe) _PIPE(pipe, _CHICKEN_PIPESL_A, _CHICKEN_PIPESL_B)
> +
> +#define CHICKEN_DCPR_1             0x46430
> +#define IDLE_WAKEMEM_MASK          (1 << 13)
> +
> +#define CLKGATE_DIS_PSL_A        0x46520
> +#define CLKGATE_DIS_PSL_B        0x46524
> +#define CLKGATE_DIS_PSL_C        0x46528
> +#define DUPS1_GATING_DIS         (1 << 15)
> +#define DUPS2_GATING_DIS         (1 << 19)
> +#define DUPS3_GATING_DIS         (1 << 23)
> +#define CLKGATE_DIS_PSL(pipe)  _PIPE(pipe, CLKGATE_DIS_PSL_A, CLKGATE_DIS_PSL_B)
> +
>  /* SKL new cursor registers */
>  #define _CUR_BUF_CFG_A				0x7017c
>  #define _CUR_BUF_CFG_B				0x7117c
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index ba1ae03..559a7f5 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -181,7 +181,7 @@ static const struct stepping_info skl_stepping_info[] = {
>  		{'G', '0'}, {'H', '0'}, {'I', '0'}
>  };
>  
> -static char intel_get_stepping(struct drm_device *dev)
> +char intel_get_stepping(struct drm_device *dev)
>  {
>  	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
>  			ARRAY_SIZE(skl_stepping_info)))
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 419660d..2158b8f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3196,6 +3196,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
>  	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 | aux_x_offset);
>  
> +	DRM_DEBUG_KMS("KCM: is_skl = %d is_bxt = %d\n",
> +		IS_SKYLAKE(dev), IS_BROXTON(dev));
> +
> +	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
> +		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
> +		fb->pixel_format == DRM_FORMAT_NV12) {
> +			I915_WRITE(CHICKEN_PIPESL(pipe),
> +				I915_READ(CHICKEN_PIPESL(pipe)) | DISABLE_STREAMER_FIX);
> +	}

According to Bspec this would need to be disabled for render
compression. And to do that we'd need to add some vblank waits to make
sure we don't disable it too soon. But since it's pre-production
hardware anyway I guess we might not care too much.

I would probably drop SKL from these since I'd assume almost everyone
has D+ by now. And maybe just stuff it in init_clock_gating for BXT
since we're going to eliminate it soonish anyway.

> +
>  	if (scaler_id >= 0) {
>  		uint32_t ps_ctrl = 0;
>  
> @@ -5004,6 +5014,21 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
>  	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
>  }
>  
> +
> +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> +	int pipe, int enable)
> +{
> +	if (pipe == PIPE_A || pipe == PIPE_B) {
> +		if (enable)
> +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> +				DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> +		else
> +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> +				I915_READ(CLKGATE_DIS_PSL(pipe) &
> +				~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)));
> +	}
> +}
> +
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -5094,6 +5119,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
>  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
>  	}
> +
> +	/* workaround for NV12 */
> +	skl_wa_clkgate(dev_priv, pipe, 1);

I wonder what's the cost of having this
a) always enabled
b) enabled when the pipe is enabled
c) enabled only when NV12 is used
?

>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -5211,6 +5239,9 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
> +
> +	/* workaround for NV12 */
> +	skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0);
>  }
>  
>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d50b8cb..63750d5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1158,6 +1158,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
>  					enum csr_state state);
>  void intel_csr_load_program(struct drm_device *dev);
>  void intel_csr_ucode_fini(struct drm_device *dev);
> +char intel_get_stepping(struct drm_device *dev);
>  void assert_csr_loaded(struct drm_i915_private *dev_priv);
>  
>  /* intel_dp.c */
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0ea9273..9d1c5b9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -278,6 +278,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	I915_WRITE(PLANE_AUX_DIST(pipe, plane), aux_dist | aux_stride);
>  	I915_WRITE(PLANE_AUX_OFFSET(pipe, plane), aux_y_offset<<16 | aux_x_offset);
>  
> +	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
> +		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
> +		fb->pixel_format == DRM_FORMAT_NV12) {
> +			I915_WRITE(CHICKEN_PIPESL(pipe),
> +				I915_READ(CHICKEN_PIPESL(pipe)) | DISABLE_STREAMER_FIX);
> +	}
> +
>  	/* program plane scaler */
>  	if (scaler_id >= 0) {
>  		uint32_t ps_ctrl = 0;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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