Re: [Intel-gfx] [PATCH 03/16] drm/i915: Implement WaPixelRepeatModeFixForC0:chv

2016-03-19 Thread Ville Syrjälä
On Wed, Mar 16, 2016 at 11:27:05AM +0200, Jani Nikula wrote:
> On Tue, 15 Mar 2016, ville.syrj...@linux.intel.com wrote:
> > [ text/plain ]
> > From: Ville Syrjälä 
> >
> > DPLL_MD(PIPE_C) is AWOL on CHV. Instead of fixing it someone added
> > chicken bits to propagate the pixel multiplier from DPLL_MD(PIPE_B)
> > to either pipe B or C. So do that to make pixel repeat work on pipes
> > B and C. Pipe A is fine without any tricks.
> >
> > Fortunately the pixel repeat propagation appears to be a oneshot
> > operation, so once the value has been written we can clear the
> > chicken bits. So it is still possible to drive pipe B and C with
> > different pixel multipliers simultaneosly.
> >
> > Looks like DPLL_VGA_MODE_DIS must also be set in DPLL(PIPE_B)
> > for this to work. But since we keep that bit always set in all
> > DPLLs there's no problem.
> >
> > This of course means we can't reliably read out the pixel multiplier
> > for pipes B and C. That would make the state checker unhappy, so I
> > added shadow copies of those registers in to dev_priv. The other
> > option would have been to skip pixel multiplier, dpll_md an dotclock
> > checks entirely on CHV, but that feels like a serious loss of cross
> > checking, so just pretending that we have working DPLL MD registers
> > seemed better. Obviously with the shadow copies we can't detect if
> > the pixel multiplier was properly configured, nor can we take over
> > its state from the BIOS, but hopefully people won't have displays
> > that would be limitd to such crappy modes.
> >
> > There is one strange flicker still remaining. It's visible on
> > pipe C/HDMID when HDMIB is enabled while driven by pipe B.
> > It doesn't occur if pipe A drives HDMIB, nor is there any glitch
> > on pipe B/HDMIB when port C/HDMID starts up. I don't have a board
> > with HDMIC so not sure if it happens there too. So I'm not sure
> > if it's somehow tied in with this strange linkage between pipe B
> > and C. Sadly I was unable to find an enable sequence that would
> > avoid the glitch, but at least it's not fatal ie. the output
> > recovers afterwards.
> >
> > Signed-off-by: Ville Syrjälä 
> 
> I didn't dig up the specs, but the code seems to do what the commit
> message says.

The "spec" wouldn't help much either, apart from showing where
the chicken bits live. I might have a powerpoint somewhere about
this, but it IIRC it didn't really tell you how to do things in
practice. As far as I can remember, I just used the standard
trial and error method to implement this.

> 
> Reviewed-by: Jani Nikula 
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  7 +++
> >  drivers/gpu/drm/i915/i915_reg.h  |  4 
> >  drivers/gpu/drm/i915/intel_display.c | 30 ++
> >  3 files changed, 37 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 80b14f1ba302..31689e1b19e6 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1871,7 +1871,14 @@ struct drm_i915_private {
> >  
> > u32 fdi_rx_config;
> >  
> > +   /* Shadow for DISPLAY_PHY_CONTROL which can't be safely read */
> > u32 chv_phy_control;
> > +   /*
> > +* Shadows for CHV DPLL_MD regs to keep the state
> > +* checker somewhat working in the presence hardware
> > +* crappiness (can't read out DPLL_MD for pipes B & C).
> > +*/
> > +   u32 chv_dpll_md[I915_MAX_PIPES];
> >  
> > u32 suspend_count;
> > bool suspended_to_idle;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 7dfc4007f3fa..f138588a88cd 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4778,6 +4778,10 @@ enum skl_disp_power_wells {
> >  #define  CBR_PND_DEADLINE_DISABLE  (1<<31)
> >  #define  CBR_PWM_CLOCK_MUX_SELECT  (1<<30)
> >  
> > +#define CBR4_VLV   _MMIO(VLV_DISPLAY_BASE + 0x70450)
> > +#define  CBR_DPLLBMD_PIPE_C(1<<29)
> > +#define  CBR_DPLLBMD_PIPE_B(1<<18)
> > +
> >  /* FIFO watermark sizes etc */
> >  #define G4X_FIFO_LINE_SIZE 64
> >  #define I915_FIFO_LINE_SIZE64
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 414ed5007e60..18158b0324ed 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1635,9 +1635,27 @@ static void chv_enable_pll(struct intel_crtc *crtc,
> > if (wait_for(((I915_READ(DPLL(pipe)) & DPLL_LOCK_VLV) == 
> > DPLL_LOCK_VLV), 1))
> > DRM_ERROR("PLL %d failed to lock\n", pipe);
> >  
> > -   /* not sure when this should be written */
> > -   I915_WRITE(DPLL_MD(pipe), pipe_config->dpll_hw_state.dpll_md);
> > -   POSTING_READ(DPLL_MD(pipe));
> > +   if (pipe != PIPE_A) {
> > +   /*
> > +

Re: [Intel-gfx] [PATCH 03/16] drm/i915: Implement WaPixelRepeatModeFixForC0:chv

2016-03-16 Thread Jani Nikula
On Tue, 15 Mar 2016, ville.syrj...@linux.intel.com wrote:
> [ text/plain ]
> From: Ville Syrjälä 
>
> DPLL_MD(PIPE_C) is AWOL on CHV. Instead of fixing it someone added
> chicken bits to propagate the pixel multiplier from DPLL_MD(PIPE_B)
> to either pipe B or C. So do that to make pixel repeat work on pipes
> B and C. Pipe A is fine without any tricks.
>
> Fortunately the pixel repeat propagation appears to be a oneshot
> operation, so once the value has been written we can clear the
> chicken bits. So it is still possible to drive pipe B and C with
> different pixel multipliers simultaneosly.
>
> Looks like DPLL_VGA_MODE_DIS must also be set in DPLL(PIPE_B)
> for this to work. But since we keep that bit always set in all
> DPLLs there's no problem.
>
> This of course means we can't reliably read out the pixel multiplier
> for pipes B and C. That would make the state checker unhappy, so I
> added shadow copies of those registers in to dev_priv. The other
> option would have been to skip pixel multiplier, dpll_md an dotclock
> checks entirely on CHV, but that feels like a serious loss of cross
> checking, so just pretending that we have working DPLL MD registers
> seemed better. Obviously with the shadow copies we can't detect if
> the pixel multiplier was properly configured, nor can we take over
> its state from the BIOS, but hopefully people won't have displays
> that would be limitd to such crappy modes.
>
> There is one strange flicker still remaining. It's visible on
> pipe C/HDMID when HDMIB is enabled while driven by pipe B.
> It doesn't occur if pipe A drives HDMIB, nor is there any glitch
> on pipe B/HDMIB when port C/HDMID starts up. I don't have a board
> with HDMIC so not sure if it happens there too. So I'm not sure
> if it's somehow tied in with this strange linkage between pipe B
> and C. Sadly I was unable to find an enable sequence that would
> avoid the glitch, but at least it's not fatal ie. the output
> recovers afterwards.
>
> Signed-off-by: Ville Syrjälä 

I didn't dig up the specs, but the code seems to do what the commit
message says.

Reviewed-by: Jani Nikula 

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  7 +++
>  drivers/gpu/drm/i915/i915_reg.h  |  4 
>  drivers/gpu/drm/i915/intel_display.c | 30 ++
>  3 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 80b14f1ba302..31689e1b19e6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1871,7 +1871,14 @@ struct drm_i915_private {
>  
>   u32 fdi_rx_config;
>  
> + /* Shadow for DISPLAY_PHY_CONTROL which can't be safely read */
>   u32 chv_phy_control;
> + /*
> +  * Shadows for CHV DPLL_MD regs to keep the state
> +  * checker somewhat working in the presence hardware
> +  * crappiness (can't read out DPLL_MD for pipes B & C).
> +  */
> + u32 chv_dpll_md[I915_MAX_PIPES];
>  
>   u32 suspend_count;
>   bool suspended_to_idle;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7dfc4007f3fa..f138588a88cd 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4778,6 +4778,10 @@ enum skl_disp_power_wells {
>  #define  CBR_PND_DEADLINE_DISABLE(1<<31)
>  #define  CBR_PWM_CLOCK_MUX_SELECT(1<<30)
>  
> +#define CBR4_VLV _MMIO(VLV_DISPLAY_BASE + 0x70450)
> +#define  CBR_DPLLBMD_PIPE_C  (1<<29)
> +#define  CBR_DPLLBMD_PIPE_B  (1<<18)
> +
>  /* FIFO watermark sizes etc */
>  #define G4X_FIFO_LINE_SIZE   64
>  #define I915_FIFO_LINE_SIZE  64
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 414ed5007e60..18158b0324ed 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1635,9 +1635,27 @@ static void chv_enable_pll(struct intel_crtc *crtc,
>   if (wait_for(((I915_READ(DPLL(pipe)) & DPLL_LOCK_VLV) == 
> DPLL_LOCK_VLV), 1))
>   DRM_ERROR("PLL %d failed to lock\n", pipe);
>  
> - /* not sure when this should be written */
> - I915_WRITE(DPLL_MD(pipe), pipe_config->dpll_hw_state.dpll_md);
> - POSTING_READ(DPLL_MD(pipe));
> + if (pipe != PIPE_A) {
> + /*
> +  * WaPixelRepeatModeFixForC0:chv
> +  *
> +  * DPLLCMD is AWOL. Use chicken bits to propagate
> +  * the value from DPLLBMD to either pipe B or C.
> +  */
> + I915_WRITE(CBR4_VLV, pipe == PIPE_B ? CBR_DPLLBMD_PIPE_B : 
> CBR_DPLLBMD_PIPE_C);
> + I915_WRITE(DPLL_MD(PIPE_B), pipe_config->dpll_hw_state.dpll_md);
> + I915_WRITE(CBR4_VLV, 0);
> + dev_priv->chv_dpll_md[pipe] = 
> pipe_config->dpll_hw_state.dpll_md;
> +
> + /*
> +  

[Intel-gfx] [PATCH 03/16] drm/i915: Implement WaPixelRepeatModeFixForC0:chv

2016-03-15 Thread ville . syrjala
From: Ville Syrjälä 

DPLL_MD(PIPE_C) is AWOL on CHV. Instead of fixing it someone added
chicken bits to propagate the pixel multiplier from DPLL_MD(PIPE_B)
to either pipe B or C. So do that to make pixel repeat work on pipes
B and C. Pipe A is fine without any tricks.

Fortunately the pixel repeat propagation appears to be a oneshot
operation, so once the value has been written we can clear the
chicken bits. So it is still possible to drive pipe B and C with
different pixel multipliers simultaneosly.

Looks like DPLL_VGA_MODE_DIS must also be set in DPLL(PIPE_B)
for this to work. But since we keep that bit always set in all
DPLLs there's no problem.

This of course means we can't reliably read out the pixel multiplier
for pipes B and C. That would make the state checker unhappy, so I
added shadow copies of those registers in to dev_priv. The other
option would have been to skip pixel multiplier, dpll_md an dotclock
checks entirely on CHV, but that feels like a serious loss of cross
checking, so just pretending that we have working DPLL MD registers
seemed better. Obviously with the shadow copies we can't detect if
the pixel multiplier was properly configured, nor can we take over
its state from the BIOS, but hopefully people won't have displays
that would be limitd to such crappy modes.

There is one strange flicker still remaining. It's visible on
pipe C/HDMID when HDMIB is enabled while driven by pipe B.
It doesn't occur if pipe A drives HDMIB, nor is there any glitch
on pipe B/HDMIB when port C/HDMID starts up. I don't have a board
with HDMIC so not sure if it happens there too. So I'm not sure
if it's somehow tied in with this strange linkage between pipe B
and C. Sadly I was unable to find an enable sequence that would
avoid the glitch, but at least it's not fatal ie. the output
recovers afterwards.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_drv.h  |  7 +++
 drivers/gpu/drm/i915/i915_reg.h  |  4 
 drivers/gpu/drm/i915/intel_display.c | 30 ++
 3 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 80b14f1ba302..31689e1b19e6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1871,7 +1871,14 @@ struct drm_i915_private {
 
u32 fdi_rx_config;
 
+   /* Shadow for DISPLAY_PHY_CONTROL which can't be safely read */
u32 chv_phy_control;
+   /*
+* Shadows for CHV DPLL_MD regs to keep the state
+* checker somewhat working in the presence hardware
+* crappiness (can't read out DPLL_MD for pipes B & C).
+*/
+   u32 chv_dpll_md[I915_MAX_PIPES];
 
u32 suspend_count;
bool suspended_to_idle;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7dfc4007f3fa..f138588a88cd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4778,6 +4778,10 @@ enum skl_disp_power_wells {
 #define  CBR_PND_DEADLINE_DISABLE  (1<<31)
 #define  CBR_PWM_CLOCK_MUX_SELECT  (1<<30)
 
+#define CBR4_VLV   _MMIO(VLV_DISPLAY_BASE + 0x70450)
+#define  CBR_DPLLBMD_PIPE_C(1<<29)
+#define  CBR_DPLLBMD_PIPE_B(1<<18)
+
 /* FIFO watermark sizes etc */
 #define G4X_FIFO_LINE_SIZE 64
 #define I915_FIFO_LINE_SIZE64
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 414ed5007e60..18158b0324ed 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1635,9 +1635,27 @@ static void chv_enable_pll(struct intel_crtc *crtc,
if (wait_for(((I915_READ(DPLL(pipe)) & DPLL_LOCK_VLV) == 
DPLL_LOCK_VLV), 1))
DRM_ERROR("PLL %d failed to lock\n", pipe);
 
-   /* not sure when this should be written */
-   I915_WRITE(DPLL_MD(pipe), pipe_config->dpll_hw_state.dpll_md);
-   POSTING_READ(DPLL_MD(pipe));
+   if (pipe != PIPE_A) {
+   /*
+* WaPixelRepeatModeFixForC0:chv
+*
+* DPLLCMD is AWOL. Use chicken bits to propagate
+* the value from DPLLBMD to either pipe B or C.
+*/
+   I915_WRITE(CBR4_VLV, pipe == PIPE_B ? CBR_DPLLBMD_PIPE_B : 
CBR_DPLLBMD_PIPE_C);
+   I915_WRITE(DPLL_MD(PIPE_B), pipe_config->dpll_hw_state.dpll_md);
+   I915_WRITE(CBR4_VLV, 0);
+   dev_priv->chv_dpll_md[pipe] = 
pipe_config->dpll_hw_state.dpll_md;
+
+   /*
+* DPLLB VGA mode also seems to cause problems.
+* We should always have it disabled.
+*/
+   WARN_ON((I915_READ(DPLL(PIPE_B)) & DPLL_VGA_MODE_DIS) == 0);
+   } else {
+   I915_WRITE(DPLL_MD(pipe), pipe_config->dpll_hw_state.dpll_md);
+