Re: [Intel-gfx] [PATCH v6 10/11] drm/i915: Add intel_update_bigjoiner handling.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 21, 2020 at 02:18:33PM -0700, Navare, Manasi wrote:
> On Mon, Sep 14, 2020 at 12:21:26PM -0700, Navare, Manasi wrote:
> > On Thu, Sep 03, 2020 at 10:23:35PM +0300, Ville Syrjälä wrote:
> > > On Wed, Jul 15, 2020 at 03:42:21PM -0700, Manasi Navare wrote:
> > > > From: Maarten Lankhorst <maarten.lankhorst@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,
> > > > so use the vblank evasion to cover both pipes.
> > > > 
> > > > This way ideally everything will be tear free, and updates are
> > > > really atomic as userspace expects it.
> > > > 
> > > > ****This needs to be checked if it still works since lot of refactoring
> > > > in skl_commit_modeset_enables
> > > > 
> > > > 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@linux.intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 120 +++++++++++++++++--
> > > >  drivers/gpu/drm/i915/display/intel_sprite.c  |  25 +++-
> > > >  drivers/gpu/drm/i915/display/intel_sprite.h  |   3 +-
> > > >  3 files changed, 129 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index a1011414da6d..00b26863ffc6 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -15656,7 +15656,7 @@ static void intel_update_crtc(struct intel_atomic_state *state,
> > > >  	else
> > > >  		i9xx_update_planes_on_crtc(state, crtc);
> > > >  
> > > > -	intel_pipe_update_end(new_crtc_state);
> > > > +	intel_pipe_update_end(new_crtc_state, NULL);
> > > >  
> > > >  	/*
> > > >  	 * We usually enable FIFO underrun interrupts as part of the
> > > > @@ -15754,6 +15754,52 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > >  	}
> > > >  }
> > > >  
> > > > +static void intel_update_bigjoiner(struct intel_crtc *crtc,
> > > > +				   struct intel_atomic_state *state,
> > > > +				   struct intel_crtc_state *old_crtc_state,
> > > > +				   struct intel_crtc_state *new_crtc_state)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	bool modeset = needs_modeset(new_crtc_state);
> > > > +	struct intel_crtc *slave = new_crtc_state->bigjoiner_linked_crtc;
> > > > +	struct intel_crtc_state *new_slave_crtc_state =
> > > > +		intel_atomic_get_new_crtc_state(state, slave);
> > > > +
> > > > +	if (modeset) {
> > > > +		/* Enable slave first */
> > > > +		intel_crtc_update_active_timings(new_slave_crtc_state);
> > > > +		dev_priv->display.crtc_enable(state, slave);
> > > > +
> > > > +		/* Then master */
> > > > +		intel_crtc_update_active_timings(new_crtc_state);
> > > > +		dev_priv->display.crtc_enable(state, crtc);
> > > > +
> > > > +		/* vblanks work again, re-enable pipe CRC. */
> > > > +		intel_crtc_enable_pipe_crc(crtc);
> > > > +
> > > > +	} else {
> > > > +		intel_pre_plane_update(state, crtc);
> > > > +		intel_pre_plane_update(state, slave);
> > > > +
> > > > +		if (new_crtc_state->update_pipe)
> > > > +			intel_encoders_update_pipe(state, crtc);
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Perform vblank evasion around commit operation, and make sure to
> > > > +	 * commit both planes simultaneously for best results.
> > > > +	 */
> > > > +	intel_pipe_update_start(new_crtc_state);
> > > > +
> > > > +	commit_pipe_config(state, crtc);
> > > > +	commit_pipe_config(state, slave);
> > > > +
> > > > +	skl_update_planes_on_crtc(state, crtc);
> > > > +	skl_update_planes_on_crtc(state, slave);
> > > > +
> > > > +	intel_pipe_update_end(new_crtc_state, new_slave_crtc_state);
> > > > +}
> > > 
> > > I think this should ideally all go away and just the normal logic
> > > in commit_modeset_enables() should deal with it. Just like it does
> > > for port sync/mst pipe dependencies.
> > >
> > 
> > Yes I think so too. Except for the intel_pipe_update_end where
> > now we send the new_slave_crtc_state() so thats still something I need to figure
> > how it will work in normal code without special bigjoiner handling.
> > 
> > I think the 2p2p transcoder ports sync code initially had a special trans port sync handling
> > function and thats why this was written this way but with the new code we just use
> > the regular modeset_enables function with no special handling
> > 
> > Manasi
> >  
> > > > +
> > > >  static void intel_commit_modeset_enables(struct intel_atomic_state *state)
> > > >  {
> > > >  	struct intel_crtc_state *new_crtc_state;
> > > > @@ -15772,15 +15818,22 @@ static void intel_commit_modeset_enables(struct intel_atomic_state *state)
> > > >  static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > -	struct intel_crtc *crtc;
> > > > +	struct intel_crtc *crtc, *slave;
> > > >  	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;
> > > > +	const struct intel_crtc_state *slave_crtc_state;
> > > >  	int i;
> > > >  
> > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > > >  		enum pipe pipe = crtc->pipe;
> > > >  
> > > > +		if (new_crtc_state->bigjoiner_slave) {
> > > > +			/* We're updated from master */
> > > > +			continue;
> > > > +		}
> > > > +
> > > >  		if (!new_crtc_state->hw.active)
> > > >  			continue;
> > > >  
> > > > @@ -15791,6 +15844,34 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > > >  		} else {
> > > >  			modeset_pipes |= BIT(pipe);
> > > >  		}
> > > > +
> > > > +		if (new_crtc_state->bigjoiner) {
> > > > +			slave = new_crtc_state->bigjoiner_linked_crtc;
> > > > +			slave_crtc_state =
> > > > +				intel_atomic_get_new_crtc_state(state,
> > > > +								slave);
> > > > +
> > > > +			/* put both entries in */
> > > > +			new_entries[i].start = new_crtc_state->wm.skl.ddb.start;
> > > > +			new_entries[i].end = slave_crtc_state->wm.skl.ddb.end;
> > > > +		} else {
> > > > +			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;
> > > > +
> > > > +		if (old_crtc_state->bigjoiner) {
> > > > +			slave = old_crtc_state->bigjoiner_linked_crtc;
> > > > +			slave_crtc_state =
> > > > +				intel_atomic_get_old_crtc_state(state, slave);
> > > > +
> > > > +			entries[i].start = old_crtc_state->wm.skl.ddb.start;
> > > > +			entries[i].end = slave_crtc_state->wm.skl.ddb.end;
> > > > +		} else {
> > > > +			entries[i] = old_crtc_state->wm.skl.ddb;
> > > > +		}
> > > 
> > > Why is this here? Can't see why the current code wouldn't work just fine
> > > for bigjoiner too.
> > >
> 
> Ville, could you provide inputs on how intel_pipe_update_end() should change so that we can use
> the current code, now this takes an additional input new_slave_crtc_state

I would not change it at all. What I would do as the first step is to
treat the pipes entirely separately, just like we do for port sync/etc.
Later we can think what would be needed to make 100% sure they update
atomically.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx