| Message ID | 1517915206-30731-1-git-send-email-vidya.srinivas@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Feb 06, 2018 at 04:36:42PM +0530, Vidya Srinivas wrote: > From: Chandra Konduru <chandra.konduru@intel.com> > > Display WA #0827: > Switching the plane format from NV12 to RGB and leaving system idle results > in display underrun and corruption. WA: Set the bit 15 & bit 19 to 1b > in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane is enabled. > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 8f36023..c4af05e 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3822,6 +3822,9 @@ enum { > #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 DPF_GATING_DIS (1 << 10) > #define DPF_RAM_GATING_DIS (1 << 9) > #define DPFR_GATING_DIS (1 << 8) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 551c970..94faf3e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5495,6 +5495,20 @@ static void glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv, > I915_WRITE(CLKGATE_DIS_PSL(pipe), val); > } > > +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 intel_crtc_state *pipe_config, > struct drm_atomic_state *old_state) > { > @@ -5599,6 +5613,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); > } > + skl_wa_clkgate(dev_priv, pipe, 1); > } > > static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force) > @@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state, > intel_ddi_disable_pipe_clock(intel_crtc->config); > > intel_encoders_post_disable(crtc, old_crtc_state, old_state); > + skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0); > } Unless I'm misreading the context of this patch you're applying a workaround, that by name seems to be for Skylake only, to: Haswell, Broadwell, and gen9+. Either the name is incorrect, or the application of it. As per BSpec the workaround seems to be for all of gen9 and only A-stepping of gen10. I don't see it listed for Haswell or Broadwell. Cross-referencing the WA-database with Bspec, based on the HSD link, it seems that this issue *might* be WaDups1GatingDisableClockGatingForMPO; if this is the case it might make sense to include that WA name too. At the very least there should always be a comment mentioning the workaround name/number and the platform(s) it applies to. Also, according to the WA database, if the above mentioned issue really is the same, the WA is *NOT* necessary on GLK (seeing as GLK uses gen10 display this might make sense, though the WA database sometimes contains mistakes). Regards, David > static void i9xx_pfit_enable(struct intel_crtc *crtc) > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sorry, my bad. This was a wrong push from my end. I have changed the tag to Not applicable. Apologies. Have sent out the NV12 series separately. Regards Vidya > -----Original Message----- > From: David Weinehall [mailto:david.weinehall@linux.intel.com] > Sent: Tuesday, February 6, 2018 5:34 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Display WA #0827 for NV12 to > RGB switch > > On Tue, Feb 06, 2018 at 04:36:42PM +0530, Vidya Srinivas wrote: > > From: Chandra Konduru <chandra.konduru@intel.com> > > > > Display WA #0827: > > Switching the plane format from NV12 to RGB and leaving system idle > > results in display underrun and corruption. WA: Set the bit 15 & bit > > 19 to 1b in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane > is enabled. > > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > > drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h index 8f36023..c4af05e 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3822,6 +3822,9 @@ enum { > > #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 DPF_GATING_DIS (1 << 10) > > #define DPF_RAM_GATING_DIS (1 << 9) > > #define DPFR_GATING_DIS (1 << 8) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 551c970..94faf3e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5495,6 +5495,20 @@ static void > glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv, > > I915_WRITE(CLKGATE_DIS_PSL(pipe), val); } > > > > +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 intel_crtc_state *pipe_config, > > struct drm_atomic_state *old_state) { @@ > -5599,6 +5613,7 @@ > > static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); > > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); > > } > > + skl_wa_clkgate(dev_priv, pipe, 1); > > } > > > > static void ironlake_pfit_disable(struct intel_crtc *crtc, bool > > force) @@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct > intel_crtc_state *old_crtc_state, > > intel_ddi_disable_pipe_clock(intel_crtc->config); > > > > intel_encoders_post_disable(crtc, old_crtc_state, old_state); > > + skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0); > > } > > Unless I'm misreading the context of this patch you're applying a > workaround, that by name seems to be for Skylake only, to: Haswell, > Broadwell, and gen9+. > > Either the name is incorrect, or the application of it. > > As per BSpec the workaround seems to be for all of gen9 and only A- > stepping of gen10. > I don't see it listed for Haswell or Broadwell. > > Cross-referencing the WA-database with Bspec, based on the HSD link, it > seems that this issue *might* be > WaDups1GatingDisableClockGatingForMPO; if this is the case it might make > sense to include that WA name too. At the very least there should always be > a comment mentioning the workaround name/number and the platform(s) > it applies to. > > Also, according to the WA database, if the above mentioned issue really is > the same, the WA is *NOT* necessary on GLK (seeing as GLK uses gen10 > display this might make sense, though the WA database sometimes > contains mistakes). > > > Regards, David > > > static void i9xx_pfit_enable(struct intel_crtc *crtc) > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Regards Shashank On 2/6/2018 4:36 PM, Vidya Srinivas wrote: > From: Chandra Konduru <chandra.konduru@intel.com> > > Display WA #0827: > Switching the plane format from NV12 to RGB and leaving system idle results > in display underrun and corruption. WA: Set the bit 15 & bit 19 to 1b > in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane is enabled. > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 8f36023..c4af05e 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3822,6 +3822,9 @@ enum { > #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) Bit definition should be aligned by one extra space (like below), also the bit sequence should be high -> low (so 23,19 and then 15) > #define DPF_GATING_DIS (1 << 10) > #define DPF_RAM_GATING_DIS (1 << 9) > #define DPFR_GATING_DIS (1 << 8) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 551c970..94faf3e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5495,6 +5495,20 @@ static void glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv, > I915_WRITE(CLKGATE_DIS_PSL(pipe), val); > } > > +static void skl_wa_clkgate(struct drm_i915_private *dev_priv, > + int pipe, int enable) Do we need an int ? or bool enable ? also This line should be aligned to opening brace '(' above. > +{ > + if (pipe == PIPE_A || pipe == PIPE_B) { > + if (enable) > + I915_WRITE(CLKGATE_DIS_PSL(pipe), > + DUPS1_GATING_DIS | DUPS2_GATING_DIS); Alignment, also we are overwriting all other bits here, this should be I915_WRITE(CLKGATE_DIS_PSL(pipe), I915_READ(CLKGATE_DIS_PSL(pipe)) |= (DUPS1_GATING_DIS | DUPS2_GATING_DIS) ) > + else > + I915_WRITE(CLKGATE_DIS_PSL(pipe), > + I915_READ(CLKGATE_DIS_PSL(pipe)) & This line should be aligned to the '(' above > + ~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)); > + } > +} > + > static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > struct drm_atomic_state *old_state) > { > @@ -5599,6 +5613,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); > } > + skl_wa_clkgate(dev_priv, pipe, 1); send true from here (instead of 1) > } > > static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force) > @@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state, > intel_ddi_disable_pipe_clock(intel_crtc->config); > > intel_encoders_post_disable(crtc, old_crtc_state, old_state); > + skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0); send false from here (instead of 0) > } > > static void i9xx_pfit_enable(struct intel_crtc *crtc)
Apologies. This patch was pushed wrongly. Not a part of the 16 patch series of NV12. Regards Vidya > -----Original Message----- > From: Sharma, Shashank > Sent: Thursday, February 8, 2018 6:08 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: maarten.lankhorst@linux.intel.com; Kamath, Sunil > <sunil.kamath@intel.com>; Shankar, Uma <uma.shankar@intel.com>; > Konduru, Chandra <chandra.konduru@intel.com> > Subject: Re: [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch > > Regards > > Shashank > > > On 2/6/2018 4:36 PM, Vidya Srinivas wrote: > > From: Chandra Konduru <chandra.konduru@intel.com> > > > > Display WA #0827: > > Switching the plane format from NV12 to RGB and leaving system idle > > results in display underrun and corruption. WA: Set the bit 15 & bit > > 19 to 1b in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane > is enabled. > > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > > drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h index 8f36023..c4af05e 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3822,6 +3822,9 @@ enum { > > #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) > Bit definition should be aligned by one extra space (like below), also the bit > sequence should be high -> low (so 23,19 and then 15) > > #define DPF_GATING_DIS (1 << 10) > > #define DPF_RAM_GATING_DIS (1 << 9) > > #define DPFR_GATING_DIS (1 << 8) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 551c970..94faf3e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5495,6 +5495,20 @@ static void > glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv, > > I915_WRITE(CLKGATE_DIS_PSL(pipe), val); > > } > > > > +static void skl_wa_clkgate(struct drm_i915_private *dev_priv, > > + int pipe, int enable) > Do we need an int ? or bool enable ? also This line should be aligned to > opening brace '(' above. > > +{ > > + if (pipe == PIPE_A || pipe == PIPE_B) { > > + if (enable) > > + I915_WRITE(CLKGATE_DIS_PSL(pipe), > > + DUPS1_GATING_DIS | DUPS2_GATING_DIS); > Alignment, also we are overwriting all other bits here, this should be > I915_WRITE(CLKGATE_DIS_PSL(pipe), I915_READ(CLKGATE_DIS_PSL(pipe)) > |= (DUPS1_GATING_DIS | DUPS2_GATING_DIS) ) > > + else > > + I915_WRITE(CLKGATE_DIS_PSL(pipe), > > + I915_READ(CLKGATE_DIS_PSL(pipe)) & > This line should be aligned to the '(' above > > + ~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)); > > + } > > +} > > + > > static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > > struct drm_atomic_state *old_state) > > { > > @@ -5599,6 +5613,7 @@ static void haswell_crtc_enable(struct > intel_crtc_state *pipe_config, > > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); > > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); > > } > > + skl_wa_clkgate(dev_priv, pipe, 1); > send true from here (instead of 1) > > } > > > > static void ironlake_pfit_disable(struct intel_crtc *crtc, bool > > force) @@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct > intel_crtc_state *old_crtc_state, > > intel_ddi_disable_pipe_clock(intel_crtc->config); > > > > intel_encoders_post_disable(crtc, old_crtc_state, old_state); > > + skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0); > send false from here (instead of 0) > > } > > > > static void i9xx_pfit_enable(struct intel_crtc *crtc)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8f36023..c4af05e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3822,6 +3822,9 @@ enum { #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 DPF_GATING_DIS (1 << 10) #define DPF_RAM_GATING_DIS (1 << 9) #define DPFR_GATING_DIS (1 << 8) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 551c970..94faf3e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5495,6 +5495,20 @@ static void glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv, I915_WRITE(CLKGATE_DIS_PSL(pipe), val); } +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 intel_crtc_state *pipe_config, struct drm_atomic_state *old_state) { @@ -5599,6 +5613,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); intel_wait_for_vblank(dev_priv, hsw_workaround_pipe); } + skl_wa_clkgate(dev_priv, pipe, 1); } static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force) @@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state, intel_ddi_disable_pipe_clock(intel_crtc->config); intel_encoders_post_disable(crtc, old_crtc_state, old_state); + skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0); } static void i9xx_pfit_enable(struct intel_crtc *crtc)