| Message ID | 1435580756-20154-9-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6/29/2015 5:55 PM, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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ä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++---- > 3 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 37cc653..adaa656 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1851,7 +1851,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; > struct i915_suspend_saved_registers regfile; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index f08f729..2361347 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4580,6 +4580,9 @@ enum skl_disp_power_wells { > > #define CBR1_VLV (VLV_DISPLAY_BASE + 0x70400) > #define CBR_PND_DEADLINE_DISABLE (1<<31) > +#define CBR4_VLV (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 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index dec36a2..b862307 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1667,9 +1667,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); > + POSTING_READ(DPLL_MD(pipe)); > + } > } > > static int intel_num_dvo_pipes(struct drm_device *dev) > @@ -8065,7 +8083,11 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, > i9xx_get_pfit_config(crtc, pipe_config); > > if (INTEL_INFO(dev)->gen >= 4) { > - tmp = I915_READ(DPLL_MD(crtc->pipe)); > + /* No way to read it out on pipes B and C */ > + if (IS_CHERRYVIEW(dev) && crtc->pipe != PIPE_A) > + tmp = dev_priv->chv_dpll_md[crtc->pipe]; > + else > + tmp = I915_READ(DPLL_MD(crtc->pipe)); > pipe_config->pixel_multiplier = > ((tmp & DPLL_MD_UDI_MULTIPLIER_MASK) > >> DPLL_MD_UDI_MULTIPLIER_SHIFT) + 1; correct me if my understanding is wrong, the only place we used to read pixel_multiplier was in i9xx_get_pipe_config and we have replaced even that with dev_priv variables ? so the first time we get pipe_config will it return 0 ? i assume the right thing here is to read the CBR4_VLV and check if the current pipe is enabled, if so return the value in DPLL_MD(PIPE_B) as required. On a side note, we should calculate pixel_multiplier as part of compute_dpll instead of depending on GOP/VBIOS programmed values.
On Mon, Jul 13, 2015 at 11:44:44AM +0530, Sivakumar Thulasimani wrote: > > > On 6/29/2015 5:55 PM, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > 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ä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ > > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++---- > > 3 files changed, 36 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 37cc653..adaa656 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1851,7 +1851,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; > > struct i915_suspend_saved_registers regfile; > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index f08f729..2361347 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -4580,6 +4580,9 @@ enum skl_disp_power_wells { > > > > #define CBR1_VLV (VLV_DISPLAY_BASE + 0x70400) > > #define CBR_PND_DEADLINE_DISABLE (1<<31) > > +#define CBR4_VLV (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 > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index dec36a2..b862307 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -1667,9 +1667,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); > > + POSTING_READ(DPLL_MD(pipe)); > > + } > > } > > > > static int intel_num_dvo_pipes(struct drm_device *dev) > > @@ -8065,7 +8083,11 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, > > i9xx_get_pfit_config(crtc, pipe_config); > > > > if (INTEL_INFO(dev)->gen >= 4) { > > - tmp = I915_READ(DPLL_MD(crtc->pipe)); > > + /* No way to read it out on pipes B and C */ > > + if (IS_CHERRYVIEW(dev) && crtc->pipe != PIPE_A) > > + tmp = dev_priv->chv_dpll_md[crtc->pipe]; > > + else > > + tmp = I915_READ(DPLL_MD(crtc->pipe)); > > pipe_config->pixel_multiplier = > > ((tmp & DPLL_MD_UDI_MULTIPLIER_MASK) > > >> DPLL_MD_UDI_MULTIPLIER_SHIFT) + 1; > correct me if my understanding is wrong, the only place we used to read > pixel_multiplier was in > i9xx_get_pipe_config and we have replaced even that with dev_priv > variables ? so the first time > we get pipe_config will it return 0 ? Yes. Which means 1x multiplier. > i assume the right thing here is to read the CBR4_VLV and check if the > current pipe is enabled, > if so return the value in DPLL_MD(PIPE_B) as required. That won't work since the pixel multiplier update is a one-shot operation. We migth be able to read the value back from one pipe by looking at CBR4_VLV if the chicken bit for the pipe was left enabled after the value was set, but there's no way to read back the value for the second pipe. If both chicken bits are cleared in CBR4_VLV (which is how I implemeted it) we can't read back the value for either pipe. > On a side note, we should calculate pixel_multiplier as part of > compute_dpll instead of depending > on GOP/VBIOS programmed values. It's computed as part of encoder->compute_config().
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 37cc653..adaa656 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1851,7 +1851,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; struct i915_suspend_saved_registers regfile; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f08f729..2361347 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4580,6 +4580,9 @@ enum skl_disp_power_wells { #define CBR1_VLV (VLV_DISPLAY_BASE + 0x70400) #define CBR_PND_DEADLINE_DISABLE (1<<31) +#define CBR4_VLV (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 diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dec36a2..b862307 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1667,9 +1667,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); + POSTING_READ(DPLL_MD(pipe)); + } } static int intel_num_dvo_pipes(struct drm_device *dev) @@ -8065,7 +8083,11 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, i9xx_get_pfit_config(crtc, pipe_config); if (INTEL_INFO(dev)->gen >= 4) { - tmp = I915_READ(DPLL_MD(crtc->pipe)); + /* No way to read it out on pipes B and C */ + if (IS_CHERRYVIEW(dev) && crtc->pipe != PIPE_A) + tmp = dev_priv->chv_dpll_md[crtc->pipe]; + else + tmp = I915_READ(DPLL_MD(crtc->pipe)); pipe_config->pixel_multiplier = ((tmp & DPLL_MD_UDI_MULTIPLIER_MASK) >> DPLL_MD_UDI_MULTIPLIER_SHIFT) + 1;