| Message ID | 1440032556-9920-15-git-send-email-chandra.konduru@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) I guess we should have a new home for this now that it's used outside of intel_csr.c Plus kerneldoc, as usual. > { > 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)); > + Wa comments are missing here. Also generally we do 1 wa per commit. -Daniel > + 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); > + } > + > 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); > } > > 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
> > -static char intel_get_stepping(struct drm_device *dev) > > +char intel_get_stepping(struct drm_device *dev) > > I guess we should have a new home for this now that it's used outside of > intel_csr.c Plus kerneldoc, as usual. Will add kerneldoc header and respun, but where do you want to move to? > > > { > > 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)); > > + > > Wa comments are missing here. Also generally we do 1 wa per commit. > -Daniel Sure, will split and respun.
On Thu, Aug 27, 2015 at 01:44:06AM +0000, Konduru, Chandra wrote: > > > -static char intel_get_stepping(struct drm_device *dev) > > > +char intel_get_stepping(struct drm_device *dev) > > > > I guess we should have a new home for this now that it's used outside of > > intel_csr.c Plus kerneldoc, as usual. > > Will add kerneldoc header and respun, but where do you want to move to? If you want my dice-roll, I'd shovel it into intel_uncore.c for lack of better home. -Daniel > > > > > > { > > > 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)); > > > + > > > > Wa comments are missing here. Also generally we do 1 wa per commit. > > -Daniel > > Sure, will split and respun. >
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Wednesday, September 02, 2015 1:02 AM > To: Konduru, Chandra > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville > Subject: Re: [Intel-gfx] [PATCH 14/15] drm/i915: skl nv12 workarounds > > On Thu, Aug 27, 2015 at 01:44:06AM +0000, Konduru, Chandra wrote: > > > > -static char intel_get_stepping(struct drm_device *dev) > > > > +char intel_get_stepping(struct drm_device *dev) > > > > > > I guess we should have a new home for this now that it's used outside of > > > intel_csr.c Plus kerneldoc, as usual. > > > > Will add kerneldoc header and respun, but where do you want to move to? > > If you want my dice-roll, I'd shovel it into intel_uncore.c for lack of > better home. > -Daniel I sent updated patch series couple days ago moving to intel_display.c. I hope that is fine. By the way, can you start giving RB tags for the patches you reviewed? > > > > > > > > > > { > > > > 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)); > > > > + > > > > > > Wa comments are missing here. Also generally we do 1 wa per commit. > > > -Daniel > > > > Sure, will split and respun. > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Sep 03, 2015 at 06:33:25PM +0000, Konduru, Chandra wrote: > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > > Sent: Wednesday, September 02, 2015 1:02 AM > > To: Konduru, Chandra > > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville > > Subject: Re: [Intel-gfx] [PATCH 14/15] drm/i915: skl nv12 workarounds > > > > On Thu, Aug 27, 2015 at 01:44:06AM +0000, Konduru, Chandra wrote: > > > > > -static char intel_get_stepping(struct drm_device *dev) > > > > > +char intel_get_stepping(struct drm_device *dev) > > > > > > > > I guess we should have a new home for this now that it's used outside of > > > > intel_csr.c Plus kerneldoc, as usual. > > > > > > Will add kerneldoc header and respun, but where do you want to move to? > > > > If you want my dice-roll, I'd shovel it into intel_uncore.c for lack of > > better home. > > -Daniel > I sent updated patch series couple days ago moving to intel_display.c. > I hope that is fine. Imo intel_display.c is the wrong place since this isn't display-specific really - stepping is for the entire device. intel_uncore.c, i915_drv.c or i915_dma.c would all be suitable places. > By the way, can you start giving RB tags for the patches you reviewed? This isn't a full review, just a small thing I spotted while reading patches. -Daniel
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
> > + > > + 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. Render compression related checks will be coming as part of that patch. > > 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. Last checked some folks are still using, so keep it intact for completeness. > > +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 > ? Initially optimized to enable only when nv12 is used, but there are some corner cases when planes switch to and from nv12 to non-nv12 and SV recommendation is to enable always; and SV evaluated cost, and it isn't a big concern.
> > > On Thu, Aug 27, 2015 at 01:44:06AM +0000, Konduru, Chandra wrote: > > > > > > -static char intel_get_stepping(struct drm_device *dev) > > > > > > +char intel_get_stepping(struct drm_device *dev) > > > > > > > > > > I guess we should have a new home for this now that it's used outside of > > > > > intel_csr.c Plus kerneldoc, as usual. > > > > > > > > Will add kerneldoc header and respun, but where do you want to move to? > > > > > > If you want my dice-roll, I'd shovel it into intel_uncore.c for lack of > > > better home. > > > -Daniel > > I sent updated patch series couple days ago moving to intel_display.c. > > I hope that is fine. > > Imo intel_display.c is the wrong place since this isn't display-specific > really - stepping is for the entire device. intel_uncore.c, i915_drv.c or > i915_dma.c would all be suitable places. Moved to i915_drv.c. Sending out updated patches shortly. > > > By the way, can you start giving RB tags for the patches you reviewed? > > This isn't a full review, just a small thing I spotted while reading > patches. OK, Ville did/is doing full review. So covered.
On Sat, Sep 05, 2015 at 01:28:56AM +0000, Konduru, Chandra wrote: > > > + > > > + 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. > > Render compression related checks will be coming as part of that patch. > > > > > 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. > > Last checked some folks are still using, so keep it intact for completeness. > > > > +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 > > ? > > Initially optimized to enable only when nv12 is used, > but there are some corner cases when planes switch to and > from nv12 to non-nv12 and SV recommendation is to enable > always; and SV evaluated cost, and it isn't a big concern. So, based on that we could just stuff it into init_clock_gating and forget about it. But we'll run into problems as soon as render compression enters the picure. But I don't have a problem leaving it up to the render compression patches to solve.
> > > > +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 > > > ? > > > > Initially optimized to enable only when nv12 is used, > > but there are some corner cases when planes switch to and > > from nv12 to non-nv12 and SV recommendation is to enable > > always; and SV evaluated cost, and it isn't a big concern. > > So, based on that we could just stuff it into init_clock_gating and > forget about it. Couldn't include into init_clock_gating because this requires a pipe based check. By the way, so far 4 patches got RB tags. In the respun series (http://lists.freedesktop.org/archives/intel-gfx/2015-September/075235.html addressed your feedback), those 4 tags goes to 1, 3, 5 and 8 of 15. Can you check updated patches and issue R-B tags for remaining ones? > > But we'll run into problems as soon as render compression enters the > picure. But I don't have a problem leaving it up to the render > compression patches to solve. > > -- > Ville Syrjälä > Intel OTC
On Wed, Sep 09, 2015 at 02:51:58AM +0300, Konduru, Chandra wrote: > > > > > +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 > > > > ? > > > > > > Initially optimized to enable only when nv12 is used, > > > but there are some corner cases when planes switch to and > > > from nv12 to non-nv12 and SV recommendation is to enable > > > always; and SV evaluated cost, and it isn't a big concern. > > > > So, based on that we could just stuff it into init_clock_gating and > > forget about it. > > Couldn't include into init_clock_gating because this requires > a pipe based check. init_clock_gating() { ... enable for pipe A enable for pipe B ... } or for_each_pipe(pipe) if (pipe != C) enable w/a > > By the way, so far 4 patches got RB tags. > In the respun series (http://lists.freedesktop.org/archives/intel-gfx/2015-September/075235.html > addressed your feedback), those 4 tags goes to 1, 3, 5 and 8 of 15. > Can you check updated patches and issue R-B tags for remaining ones? I think your trigger finger is a bit overly sensitive :) We still had open issues in this series so posting another one makes things somewhat messy.
> > > > > > + > > > > > > + /* 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 > > > > > ? > > > > > > > > Initially optimized to enable only when nv12 is used, > > > > but there are some corner cases when planes switch to and > > > > from nv12 to non-nv12 and SV recommendation is to enable > > > > always; and SV evaluated cost, and it isn't a big concern. > > > > > > So, based on that we could just stuff it into init_clock_gating and > > > forget about it. > > > > Couldn't include into init_clock_gating because this requires > > a pipe based check. > > init_clock_gating() > { > ... > enable for pipe A > enable for pipe B > ... > } > > or > > for_each_pipe(pipe) > if (pipe != C) > enable w/a > Oh yeah, it can be done that way. OK, will move it there. > > > > > By the way, so far 4 patches got RB tags. > > In the respun series (http://lists.freedesktop.org/archives/intel-gfx/2015- > September/075235.html > > addressed your feedback), those 4 tags goes to 1, 3, 5 and 8 of 15. > > Can you check updated patches and issue R-B tags for remaining ones? > > I think your trigger finger is a bit overly sensitive :) We still had > open issues in this series so posting another one makes things somewhat > messy. There is one other open relate to tile-Yf Y, UV plane pitch/stride check. I just responded to that one. > > -- > Ville Syrjälä > Intel OTC
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); + } + 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); } 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;
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(-)