On Tue, Oct 29, 2013 at 02:46:10PM +0200, Ville Syrjälä wrote:
> On Tue, Oct 29, 2013 at 12:04:08PM +0100, Daniel Vetter wrote:
> > Originally I've thought that this is leftover hw state dirt from the
> > BIOS. But after way too much helpless flailing around on my part I've
> > noticed that the actual bug is when we change the state of an already
> > active pipe.
> >
> > For example when we change the fdi lines from 2 to 3 without switching
> > off outputs in-between we'll never see the crucial on->off transition
> > in the ->modeset_global_resources hook the current logic relies on.
> >
> > Patch version 2 got this right by instead also checking whether the
> > pipe is indeed active. But that in turn broke things when pipes have
> > been turned off through dpms since the bifurcate enabling is done in
> > the ->crtc_mode_set callback.
> >
> > To address this issues discussed with Ville in the patch review move
> > the setting of the bifurcate bit into the ->crtc_enable hook. That way
> > we won't wreak havoc with this state when userspace puts all other
> > outputs into dpms off state. This also moves us forward with our
> > overall goal to unify the modeset and dpms on paths (which we need to
> > have to allow runtime pm in the dpms off state).
> >
> > Unfortunately this requires us to move the bifurcate helpers around a
> > bit.
> >
> > Also update the commit message, I've misanalyzed the bug rather badly.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70507
> > Tested-by: Jan-Michael Brummer <jan.brum...@tabos.org>
> > Cc: sta...@vger.kernel.org
> > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
>
> Reviewed-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> And since it now calls cpt_enable_fdi_bc_bifurcation() only when
> has_pch_encoder==true it should also fix the following scenario:
>
> 1. setcrtc pipe B -> PCH w/ fdi_lanes>2
> 2. setcrtc pipe C -> eDP
>
> Previously the pipe C .mode_set() whould have called
> cpt_enable_fdi_bc_bifurcation() unconditionally, which
> would have killed pipe B.
Geez, did that take a while for me to sink in. Somehow I've thought I've
fixed this a long time ago so that cpu edp on pipe C allows us to fully
use the fdi links on pipes A/B. But reading through the code again I've
only updated the logic in the compute_config stage, not here.
Thanks for your review, patch merged to -fixes.
-Daniel
>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 95
> > ++++++++++++++++++------------------
> > 1 file changed, 48 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 8c3bf8a89cb7..509762c85d2e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2421,9 +2421,10 @@ static void intel_fdi_normal_train(struct drm_crtc
> > *crtc)
> > FDI_FE_ERRC_ENABLE);
> > }
> >
> > -static bool pipe_has_enabled_pch(struct intel_crtc *intel_crtc)
> > +static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
> > {
> > - return intel_crtc->base.enabled && intel_crtc->config.has_pch_encoder;
> > + return crtc->base.enabled && crtc->active &&
> > + crtc->config.has_pch_encoder;
> > }
> >
> > static void ivb_modeset_global_resources(struct drm_device *dev)
> > @@ -3074,6 +3075,48 @@ static void
> > ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
> > I915_READ(VSYNCSHIFT(cpu_transcoder)));
> > }
> >
> > +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + uint32_t temp;
> > +
> > + temp = I915_READ(SOUTH_CHICKEN1);
> > + if (temp & FDI_BC_BIFURCATION_SELECT)
> > + return;
> > +
> > + WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> > + WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> > +
> > + temp |= FDI_BC_BIFURCATION_SELECT;
> > + DRM_DEBUG_KMS("enabling fdi C rx\n");
> > + I915_WRITE(SOUTH_CHICKEN1, temp);
> > + POSTING_READ(SOUTH_CHICKEN1);
> > +}
> > +
> > +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc
> > *intel_crtc)
> > +{
> > + struct drm_device *dev = intel_crtc->base.dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > + switch (intel_crtc->pipe) {
> > + case PIPE_A:
> > + break;
> > + case PIPE_B:
> > + if (intel_crtc->config.fdi_lanes > 2)
> > + WARN_ON(I915_READ(SOUTH_CHICKEN1) &
> > FDI_BC_BIFURCATION_SELECT);
> > + else
> > + cpt_enable_fdi_bc_bifurcation(dev);
> > +
> > + break;
> > + case PIPE_C:
> > + cpt_enable_fdi_bc_bifurcation(dev);
> > +
> > + break;
> > + default:
> > + BUG();
> > + }
> > +}
> > +
> > /*
> > * Enable PCH resources required for PCH ports:
> > * - PCH PLLs
> > @@ -3092,6 +3135,9 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
> >
> > assert_pch_transcoder_disabled(dev_priv, pipe);
> >
> > + if (IS_IVYBRIDGE(dev))
> > + ivybridge_update_fdi_bc_bifurcation(intel_crtc);
> > +
> > /* Write the TU size bits before fdi link training, so that error
> > * detection works. */
> > I915_WRITE(FDI_RX_TUSIZE1(pipe),
> > @@ -5849,48 +5895,6 @@ static bool ironlake_compute_clocks(struct drm_crtc
> > *crtc,
> > return true;
> > }
> >
> > -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> > -{
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - uint32_t temp;
> > -
> > - temp = I915_READ(SOUTH_CHICKEN1);
> > - if (temp & FDI_BC_BIFURCATION_SELECT)
> > - return;
> > -
> > - WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> > - WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> > -
> > - temp |= FDI_BC_BIFURCATION_SELECT;
> > - DRM_DEBUG_KMS("enabling fdi C rx\n");
> > - I915_WRITE(SOUTH_CHICKEN1, temp);
> > - POSTING_READ(SOUTH_CHICKEN1);
> > -}
> > -
> > -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc
> > *intel_crtc)
> > -{
> > - struct drm_device *dev = intel_crtc->base.dev;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> > - switch (intel_crtc->pipe) {
> > - case PIPE_A:
> > - break;
> > - case PIPE_B:
> > - if (intel_crtc->config.fdi_lanes > 2)
> > - WARN_ON(I915_READ(SOUTH_CHICKEN1) &
> > FDI_BC_BIFURCATION_SELECT);
> > - else
> > - cpt_enable_fdi_bc_bifurcation(dev);
> > -
> > - break;
> > - case PIPE_C:
> > - cpt_enable_fdi_bc_bifurcation(dev);
> > -
> > - break;
> > - default:
> > - BUG();
> > - }
> > -}
> > -
> > int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
> > {
> > /*
> > @@ -6079,9 +6083,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc
> > *crtc,
> > &intel_crtc->config.fdi_m_n);
> > }
> >
> > - if (IS_IVYBRIDGE(dev))
> > - ivybridge_update_fdi_bc_bifurcation(intel_crtc);
> > -
> > ironlake_set_pipeconf(crtc);
> >
> > /* Set up the display plane register */
> > --
> > 1.8.4.rc3
>
> --
> Ville Syrjälä
> Intel OTC
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
