[Intel-gfx] [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept
Conselvan De Oliveira, Ander
ander.conselvan.de.oliveira at intel.com
Wed Mar 11 06:23:33 PDT 2015
On Wed, 2015-03-11 at 15:10 +0200, Ville Syrjälä wrote:
> On Wed, Mar 11, 2015 at 01:35:43PM +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 the pch
> > transcoder. The mode set consistency check should prevent us from
> > disabling the bit if pipe C is enabled so the change should be safe.
> >
> > Note that this doens't affect the logic that prevents the bit being
> > set while a pipe is active, since the patch retains the behavior of
> > only chaging the bit if necessary. Because of the checks during mode
> > set, the first change would necessarily happen with both pipes B and
> > C disabled, and any subsequent write would be skipped.
> >
> > v2: Only change the bit during pch trancoder enable. (Ville)
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
> > 1 file changed, 10 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4008bf4..bfbd829 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3153,32 +3153,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,20 +3808,23 @@ 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_set_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);
> > }
> > @@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> > 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);
> > + cpt_set_fdi_bc_bifurcation(dev, false);
>
> So we just replace the globla_resources enforced assumed state with an
> explicit state change here. Should be perfectly fine since pipe is not
> active at this point.
>
>
> What really confuses me about the whole FDI bifurcation code is
> ironlake_check_fdi_lanes(). First of all I would rewrite it a bit like
> so:
>
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5615,14 +5615,13 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
> }
> return true;
> case PIPE_C:
> - if (!pipe_has_enabled_pch(pipe_B_crtc) ||
> - pipe_B_crtc->config->fdi_lanes <= 2) {
> - if (pipe_config->fdi_lanes > 2) {
> - DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %c: %i lanes\n",
> - pipe_name(pipe), pipe_config->fdi_lanes);
> - return false;
> - }
> - } else {
> + if (pipe_config->fdi_lanes > 2) {
> + DRM_DEBUG_KMS("only 2 lanes on pipe %c: required %i lanes\n",
> + pipe_name(pipe), pipe_config->fdi_lanes);
> + return false;
> + }
> + if (pipe_has_enabled_pch(pipe_B_crtc) &&
> + pipe_B_crtc->config->fdi_lanes > 2) {
> DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n");
> return false;
> }
>
> And the next thing confusing me is why we use pipe_has_enabled_pch()
> to check pipe B state, but just pipe->enabled to check pipe C state?
>
> Consider the following scenario:
> 1. enable pipe B with PCH using >2 FDI lanes
> 2. set pipe B to DPMS off
> 3. enable pipe C with PCH
>
> The crtc->active check in pipe_has_enabled_pch() will indicate that pipe
> B does not need FDI, so step 3 will succeed. Or am I missing something
> subtle here?
>
> Also if we do things this way:
> 1. enable pipe C with eDP
> 2. enable pipe B with PCH using >2 FDI lanes
>
> Now step 2 will fail even though pipe C doesn't need any FDI lanes.
>
> So to fix that it would seem that we should remove the crtc->active
> check from pipe_has_enabled_pch() and use that to check for conlicts
> in both cases. Now that .global_resources() is no longer in the picture
> the crtc->active check not needed anyway, I think.
This is actually the motivation for this patch. I tried to fix that in
http://patchwork.freedesktop.org/patch/44281/ ,
but then it became clear that patch broke the case of pipe B going from
a >2 lanes mode to a <=2 mode. But the combination of the two patches
works for both cases.
I haven't thought about the eDP on pipe C case before, but it seems we
want the change you mentioned above too to fix that.
Ander
>
> > else
> > - cpt_enable_fdi_bc_bifurcation(dev);
> > + cpt_set_fdi_bc_bifurcation(dev, true);
> >
> > break;
> > case PIPE_C:
> > - cpt_enable_fdi_bc_bifurcation(dev);
> > + cpt_set_fdi_bc_bifurcation(dev, true);
> >
> > break;
> > default:
> > @@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
> > } else if (IS_IVYBRIDGE(dev)) {
> > /* FIXME: detect B0+ stepping and use auto training */
> > dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> > - dev_priv->display.modeset_global_resources =
> > - ivb_modeset_global_resources;
> > } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> > } else if (IS_VALLEYVIEW(dev)) {
> > --
> > 2.1.0
>
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
More information about the Intel-gfx
mailing list