[PATCH v2 11/21] drm/i915/dp: Add way to get active pipes with syncing commits

Fri Feb 23 22:13:04 UTC 2024

On Sat, Feb 24, 2024 at 12:09:41AM +0200, Imre Deak wrote:
> On Fri, Feb 23, 2024 at 11:11:45PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 20, 2024 at 11:18:31PM +0200, Imre Deak wrote:
> > > Add a way to get the active pipes through a given DP port by syncing
> > > against a related pending non-blocking commit. Atm
> > > intel_dp_get_active_pipes() will only try to sync a given pipe and if
> > > that would block ignore the pipe. A follow-up change enabling the DP
> > > tunnel BW allocation mode will need to ensure that all active pipes are
> > > returned.
> > > 
> > > This change will use intel_crtc_state::uapi.commit instead of the
> > > corresponding commit in the connector state. This shouldn't make a
> > > difference, since the two commit objects match for an active pipe.
> > 
> > There is a slight difference here in that a non-modeset/fastset commit
> > will not have the connector inluded in the state and thus
> > connector->state.commit will be updated.
> > 
> > I think the original idea of the code was to just skip anything that
> > looks like it's already undergoing a full modeset. With this we might
> > skip the retrain if there happens to be any kind of commit happening
> > on the crtc. Althoguh it seems that the original code is already
> > broken in the same way when there's a fastset happening in parallel.
> 
> Ok, didn't think of it. Are you ok to sync instead of try-sync the pipes
> in case of link re-training?

Yeah, I guess that should be safer option, and we do recheck the
condition after the sync anyway so it shouldn't cause too much
extra stuff to happen. We can think about optimizing it correctly
later if necessary.

> 
> > > A follow-up patchset will remove syncing during TC port reset, which
> > > should reset a port/pipe even if syncing against a commit would block.
> > > Syncing OTOH is not needed there, since the commit used for the reset
> > > implies a sync already. For now add a TODO comment for this.
> > > 
> > > v2:
> > > - Add a separate function to try-sync the pipes. (Ville)
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_crtc.c | 27 +++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_crtc.h |  1 +
> > >  drivers/gpu/drm/i915/display/intel_dp.c   |  6 ++---
> > >  drivers/gpu/drm/i915/display/intel_tc.c   |  7 ++++++
> > >  4 files changed, 37 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > index 25593f6aae7de..17ed2e62cc66a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > @@ -654,3 +654,30 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
> > >  out:
> > >  	intel_psr_unlock(new_crtc_state);
> > >  }
> > > +
> > > +/**
> > > + * intel_crtc_try_sync_pipes - Try syncing pending commits on a set of pipes
> > > + * @i915: i915 device object
> > > + * @pipe_mask: Mask of pipes to sync
> > > + *
> > > + * Try to sync a pending non-blocking commit for the provided pipes in
> > > + * @pipe_mask. The commit won't be synced if this would block.
> > > + *
> > > + * Return a mask of the pipes that got synced or didn't need syncing.
> > > + */
> > > +u32 intel_crtc_try_sync_pipes(struct drm_i915_private *i915, u32 pipe_mask)
> > > +{
> > > +	struct intel_crtc *crtc;
> > > +	u32 synced = 0;
> > > +
> > > +	for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) {
> > > +		const struct intel_crtc_state *crtc_state =
> > > +			to_intel_crtc_state(crtc->base.state);
> > > +
> > > +		if (!crtc_state->uapi.commit ||
> > > +		    try_wait_for_completion(&crtc_state->uapi.commit->hw_done))
> > > +			synced |= BIT(crtc->pipe);
> > > +	}
> > > +
> > > +	return synced;
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h
> > > index 22d7993d1f0ba..71a5b93166da7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_crtc.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.h
> > > @@ -47,5 +47,6 @@ struct intel_crtc *intel_crtc_for_pipe(struct drm_i915_private *i915,
> > >  void intel_wait_for_vblank_if_active(struct drm_i915_private *i915,
> > >  				     enum pipe pipe);
> > >  void intel_crtc_wait_for_next_vblank(struct intel_crtc *crtc);
> > > +u32 intel_crtc_try_sync_pipes(struct drm_i915_private *i915, u32 pipe_mask);
> > >  
> > >  #endif
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index d9e75922ff8f5..d0452d3e534a7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -5048,10 +5048,6 @@ int intel_dp_get_active_pipes(struct intel_dp *intel_dp,
> > >  		if (!crtc_state->hw.active)
> > >  			continue;
> > >  
> > > -		if (conn_state->commit &&
> > > -		    !try_wait_for_completion(&conn_state->commit->hw_done))
> > > -			continue;
> > > -
> > >  		*pipe_mask |= BIT(crtc->pipe);
> > >  	}
> > >  	drm_connector_list_iter_end(&conn_iter);
> > > @@ -5091,6 +5087,8 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	pipe_mask &= intel_crtc_try_sync_pipes(dev_priv, pipe_mask);
> > > +
> > >  	if (pipe_mask == 0)
> > >  		return 0;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > index 6b374d481cd9e..14d17903a81f5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > @@ -7,6 +7,7 @@
> > >  #include "i915_reg.h"
> > >  #include "intel_atomic.h"
> > >  #include "intel_cx0_phy_regs.h"
> > > +#include "intel_crtc.h"
> > >  #include "intel_ddi.h"
> > >  #include "intel_de.h"
> > >  #include "intel_display.h"
> > > @@ -1663,6 +1664,12 @@ static int reset_link_commit(struct intel_tc_port *tc,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	/*
> > > +	 * TODO: remove the following, since an output must be reset
> > > +	 * even if we had to wait for a non-blocking commit on a pipe.
> > > +	 */
> > > +	pipe_mask &= intel_crtc_try_sync_pipes(i915, pipe_mask);
> > > +
> > >  	if (!pipe_mask)
> > >  		return 0;
> > >  
> > > -- 
> > > 2.39.2
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel