On Tue, Mar 10, 2015 at 03:03:59PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 10, 2015 at 02:32:34PM +0200, Ander Conselvan de Oliveira wrote:
> > Remove the global modeset resource function that would disable the
> > bifurcation bit, and instead enable/disable it when enabling or
> > disabling the pch transcoder. The checks before the mode set should
> > ensure that the configuration is valid, so it should be safe to do it
> > so.
> >
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx>
> > ---
> >
> > On Mon, 2015-03-09 at 17:21 +0100, Daniel Vetter wrote:
> > > With atomic we need to probably look at crtc_state->mode_changed. As an
> > > interim solution we have the same information available in the
> > > modeset_pipes bitmask. I think replacing the check for intel_crtc->active
> > > with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.
> >
> > I looked into that solution, but involves passing modeset_pipes way down
> > into the call stack, so I started looking for a different solution. Do you
> > see any caveat with this approach?
> >
> > Thanks,
> > Ander
> >
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++-----------------------
> > 1 file changed, 21 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4008bf4..eca5a41 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -101,6 +101,8 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
> > const struct intel_crtc_state *pipe_config);
> > static void intel_begin_crtc_commit(struct drm_crtc *crtc);
> > static void intel_finish_crtc_commit(struct drm_crtc *crtc);
> > +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> > + bool pipe_active);
> >
> > static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
> > {
> > @@ -1956,6 +1958,8 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
> > enum pipe pipe)
> > {
> > struct drm_device *dev = dev_priv->dev;
> > + struct intel_crtc *crtc =
> > + to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > uint32_t reg, val;
> >
> > /* FDI relies on the transcoder */
> > @@ -1980,6 +1984,9 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
> > val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
> > I915_WRITE(reg, val);
> > }
> > +
> > + if (IS_IVYBRIDGE(dev))
> > + ivybridge_update_fdi_bc_bifurcation(crtc, false);
> > }
> >
> > static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
> > @@ -3153,32 +3160,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
> > return crtc->base.state->enable && crtc->config->has_pch_encoder;
> > }
> >
> > -static void ivb_modeset_global_resources(struct drm_device *dev)
> > -{
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct intel_crtc *pipe_B_crtc =
> > - to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> > - struct intel_crtc *pipe_C_crtc =
> > - to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> > - uint32_t temp;
> > -
> > - /*
> > - * When everything is off disable fdi C so that we could enable fdi B
> > - * with all lanes. Note that we don't care about enabled pipes without
> > - * an enabled pch encoder.
> > - */
> > - if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> > - !pipe_has_enabled_pch(pipe_C_crtc)) {
> > - 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 = I915_READ(SOUTH_CHICKEN1);
> > - temp &= ~FDI_BC_BIFURCATION_SELECT;
> > - DRM_DEBUG_KMS("disabling fdi C rx\n");
> > - I915_WRITE(SOUTH_CHICKEN1, temp);
> > - }
> > -}
> > -
> > /* The FDI link training functions for ILK/Ibexpeak. */
> > static void ironlake_fdi_link_train(struct drm_crtc *crtc)
> > {
> > @@ -3834,41 +3815,44 @@ 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)
> > +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > uint32_t temp;
> >
> > temp = I915_READ(SOUTH_CHICKEN1);
> > - if (temp & FDI_BC_BIFURCATION_SELECT)
> > + if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
> > 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");
> > + temp &= ~FDI_BC_BIFURCATION_SELECT;
> > + if (enable)
> > + temp |= FDI_BC_BIFURCATION_SELECT;
> > +
> > + DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
> > I915_WRITE(SOUTH_CHICKEN1, temp);
> > POSTING_READ(SOUTH_CHICKEN1);
> > }
> >
> > -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> > +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> > + bool pipe_active)
> > {
> > 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);
> > + if (intel_crtc->config->fdi_lanes > 2 && pipe_active)
> > + cpt_enable_fdi_bc_bifurcation(dev, false);
> > else
> > - cpt_enable_fdi_bc_bifurcation(dev);
> > + cpt_enable_fdi_bc_bifurcation(dev, true);
> >
> > break;
> > case PIPE_C:
> > - cpt_enable_fdi_bc_bifurcation(dev);
> > + cpt_enable_fdi_bc_bifurcation(dev, pipe_active);
> >
>
> It sees could now change the bifurcation while pipe B is active (but only
> using two lanes). Not sure if the old code did that too, or if it might
> cause problems.
It shouldn't be possible and it'll anger the hw.
cpt_enable_fdi_bc_bifurcation has WARN_ONs to make sure the fdi rx for
both pch transcoder B & C are off to make sure we don't get this wrong.
> Also depending on the order you disable the pipes you might end up with
> bifurcation enabled or disabled.
>
> It seems to me that the simplest solution should be to have bifurcation
> enabled at all times, except when pipe B really needs the four lanes.
> Then the hardware state would only need to be changed when
> enabling/disabling pipe B with four lanes. Rest of the time
> crtc->enabled and fdi_lanes should be able to tell us which
> configuration is possible and which not. Or am I missing something?
>
> Well, eventually we want to tie this into the atomic state so that we
> can actaully have pipe B drop into two lane mode if pipe C needs the
> lanes (and pipe B can still operate with two lanes). But I guess that's
> still not achievable with the current code.
Checking fdi lane constraints is already done in the compute_config code.
And it assumes that it can always get the max, i.e. if pipe B is only
using 2 lanes it'll just enable pipe C. Which implies that we indeed have
to aggressive enable the bifurcate bit (since atm we don't support a
modeset on pipe B). For atomic modeset we'd just need to extend that code
to work with all pipes (like all the other compute_config code). That
shouldn't be a lot fo trouble (since we can always throw the crtc for the
other pipe into the update in atomic_check functions).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
