[Intel-gfx] [PATCH v10 10/11] drm/i915: Ensure correct master/slave enable/disable sequence
Navare, Manasi
manasi.d.navare at intel.com
Mon Oct 19 23:05:00 UTC 2020
On Mon, Oct 19, 2020 at 07:26:19PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 08, 2020 at 02:45:34PM -0700, Manasi Navare wrote:
> > From: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >
> > Enabling is done in a special sequence and so should plane updates
> > be. Ideally the end user never notices the second pipe is used.
> >
> > This way ideally everything will be tear free, and updates are
> > really atomic as userspace expects it.
> >
> > This uses generic modeset_enables() calls like trans port sync
> > but still has special handling for disable since for slave we
> > should not disable things like encoder, plls that are not enabled
> > for slave.
> >
> > v3:
> > * Fixes in enable and disable sequence from testing (Manasi)
> > v2:
> > * Manual Rebase (Manasi)
> > * Refactoring on intel_update_crtc and enable_crtc and removing
> > special trans_port_sync_update (Manasi)
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 55 ++++++++++++++------
> > drivers/gpu/drm/i915/display/intel_sprite.c | 5 +-
> > 2 files changed, 43 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 357cc2bce300..101ddd0b48ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15878,6 +15878,9 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> >
> > dev_priv->display.crtc_enable(state, crtc);
> >
> > + if (new_crtc_state->bigjoiner_slave)
> > + return;
> > +
> > /* vblanks work again, re-enable pipe CRC. */
> > intel_crtc_enable_pipe_crc(crtc);
> > }
> > @@ -15914,9 +15917,7 @@ static void intel_update_crtc(struct intel_atomic_state *state,
> >
> > commit_pipe_config(state, crtc);
> >
> > - if (new_crtc_state->bigjoiner) {
> > - /* Not supported yet */
> > - } else if (INTEL_GEN(dev_priv) >= 9)
> > + if (INTEL_GEN(dev_priv) >= 9)
> > skl_update_planes_on_crtc(state, crtc);
> > else
> > i9xx_update_planes_on_crtc(state, crtc);
> > @@ -15945,9 +15946,17 @@ static void intel_old_crtc_state_disables(struct intel_atomic_state *state,
> > drm_WARN_ON(&dev_priv->drm, old_crtc_state->bigjoiner_slave);
> >
> > intel_crtc_disable_planes(state, crtc);
> > - if (old_crtc_state->bigjoiner)
> > +
> > + /*
> > + * We still need special handling for disabling bigjoiner master
> > + * and slaves since for slave we do not have encoder or plls
> > + * so we dont need to disable those.
> > + */
> > + if (old_crtc_state->bigjoiner) {
> > intel_crtc_disable_planes(state,
> > old_crtc_state->bigjoiner_linked_crtc);
> > + old_crtc_state->bigjoiner_linked_crtc->active = false;
> > + }
> >
> > /*
> > * We need to disable pipe CRC before disabling the pipe,
> > @@ -15977,7 +15986,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > /* Only disable port sync and MST slaves */
> > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > new_crtc_state, i) {
> > - if (!needs_modeset(new_crtc_state) || old_crtc_state->bigjoiner_slave)
> > + if (!needs_modeset(new_crtc_state) || old_crtc_state->bigjoiner)
> > continue;
> >
> > if (!old_crtc_state->hw.active)
> > @@ -16040,6 +16049,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > struct intel_crtc *crtc;
> > struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> > struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> > + struct skl_ddb_entry new_entries[I915_MAX_PIPES] = {};
> > u8 update_pipes = 0, modeset_pipes = 0;
> > int i;
> >
> > @@ -16056,6 +16066,14 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > } else {
> > modeset_pipes |= BIT(pipe);
> > }
> > +
> > + new_entries[i] = new_crtc_state->wm.skl.ddb;
> > +
> > + /* ignore allocations for crtc's that have been turned off during modeset. */
> > + if (needs_modeset(new_crtc_state))
> > + continue;
> > +
> > + entries[i] = old_crtc_state->wm.skl.ddb;
> > }
> >
> > /*
> > @@ -16071,28 +16089,28 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > new_crtc_state, i) {
> > enum pipe pipe = crtc->pipe;
> > + bool ddb_changed;
> >
> > if ((update_pipes & BIT(pipe)) == 0)
> > continue;
> >
> > - if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > + if (skl_ddb_allocation_overlaps(&new_entries[pipe],
> > entries, I915_MAX_PIPES, pipe))
> > continue;
> >
> > - entries[pipe] = new_crtc_state->wm.skl.ddb;
> > + ddb_changed = !skl_ddb_entry_equal(&new_entries[pipe], &entries[pipe]);
> > + entries[pipe] = new_entries[pipe];
> > update_pipes &= ~BIT(pipe);
> >
> > - intel_update_crtc(state, crtc);
> > -
> > /*
> > * If this is an already active pipe, it's DDB changed,
> > * and this isn't the last pipe that needs updating
> > * then we need to wait for a vblank to pass for the
> > * new ddb allocation to take effect.
> > */
> > - if (!skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb,
> > - &old_crtc_state->wm.skl.ddb) &&
> > - (update_pipes | modeset_pipes))
> > + intel_update_crtc(state, crtc);
> > +
> > + if (ddb_changed && (update_pipes | modeset_pipes))
> > intel_wait_for_vblank(dev_priv, pipe);
>
> What are these ddb changes trying to achieve?
Maarten had these in the attempt of making sure we account for master and slave ddb entries.
Do we jus remove these changes for now since we are treating them like 2p2p now?
>
> > }
> > }
> > @@ -16110,7 +16128,8 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > continue;
> >
> > if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > - is_trans_port_sync_master(new_crtc_state))
> > + is_trans_port_sync_master(new_crtc_state) ||
> > + (new_crtc_state->bigjoiner && !new_crtc_state->bigjoiner_slave))
> > continue;
> >
> > modeset_pipes &= ~BIT(pipe);
> > @@ -16120,7 +16139,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >
> > /*
> > * Then we enable all remaining pipes that depend on other
> > - * pipes: MST slaves and port sync masters.
> > + * pipes: MST slaves and port sync masters, big joiner master
> > */
> > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > enum pipe pipe = crtc->pipe;
> > @@ -16128,6 +16147,10 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > if ((modeset_pipes & BIT(pipe)) == 0)
> > continue;
> >
> > + WARN_ON(skl_ddb_allocation_overlaps(&new_entries[pipe],
> > + entries, I915_MAX_PIPES, pipe));
> > +
> > + entries[pipe] = new_entries[pipe];
> > modeset_pipes &= ~BIT(pipe);
> >
> > intel_enable_crtc(state, crtc);
> > @@ -16142,10 +16165,10 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > if ((update_pipes & BIT(pipe)) == 0)
> > continue;
> >
> > - drm_WARN_ON(&dev_priv->drm, skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > + drm_WARN_ON(&dev_priv->drm, skl_ddb_allocation_overlaps(&new_entries[pipe],
> > entries, I915_MAX_PIPES, pipe));
> >
> > - entries[pipe] = new_crtc_state->wm.skl.ddb;
> > + entries[pipe] = new_entries[pipe];
> > update_pipes &= ~BIT(pipe);
> >
> > intel_update_crtc(state, crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index 9e235210adc7..1c740a22a8d7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -103,6 +103,8 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> >
> > /* FIXME needs to be calibrated sensibly */
> > min = vblank_start - intel_usecs_to_scanlines(adjusted_mode,
> > + new_crtc_state->bigjoiner ?
> > + 2 * VBLANK_EVASION_TIME_US :
> > VBLANK_EVASION_TIME_US);
> > max = vblank_start - 1;
> >
> > @@ -227,7 +229,8 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
> >
> > spin_lock(&crtc->base.dev->event_lock);
> > drm_crtc_arm_vblank_event(&crtc->base,
> > - new_crtc_state->uapi.event);
> > + new_crtc_state->uapi.event);
> > +
>
> Spurious whitespace change.
Yes will fix that, everything else here looks good?
Manasi
>
> > spin_unlock(&crtc->base.dev->event_lock);
> >
> > new_crtc_state->uapi.event = NULL;
> > --
> > 2.19.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
More information about the Intel-gfx
mailing list