Re: [PATCH v2 01/17] drm/i915: Update pipes in reverse order for bigjoiner

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

On Fri, Apr 05, 2024 at 12:56:52PM +0000, Murthy, Arun R wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Friday, April 5, 2024 3:04 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [PATCH v2 01/17] drm/i915: Update pipes in reverse order for bigjoiner
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > With bigjoiner the master crtc is the one that will send out the uapi event/etc.
> > We want that to happen after all the slaves are done, so let's try to do the
> > commits in reverse order so that the master comes last.
> > 
> > Even worse, the modeset helper will simply complete the commit on the slave
> > pipe immediately as it consider the crtc to be inactive (it can't see our
> > crtc_state->hw.active/etc.).
> > 
> > With regular sync updates this generally doesn't matter all that much as the
> > slave pipe should typically finish its work during the same frame as the master
> > pipe. However in case the slave pipe's commit slips into the next frame we end
> > up in a bit of trouble. This is most visible with either async flips (currently
> > disabled with bigjoiner exactly for this reason), and DSB gamma updates. With
> > DSB the problem happens because the DSB itself will wait until the next start
> > vblank before starting to execute. So if the master pipe already finished its
> > commit and the DSB on the slave pipe is still waiting for the next vblank we will
> > assume the DSB as gotten stuck and terminate it.
> > 
> > Reversing the commit order should ameliarate this for the most part as the
> > master pipe is guaranteed to start its commit after the slave pipe started. The
> > one thing that can still screw us over is the fact that we aren't necessarily going
> > to commit the pipes in the reverse order as the actual order is dictated by the
> > DDB overlap avoidance.
> > But that can only happen while other pipes are being enabled/disabled, and so
> > in the normal steady state we should be safe.
> > 
> > The full fix will involve making the commit machinery aware of the slave pipes
> > and not finish their commits prematurely. But that will involve a bit more work
> > than this. And this commit order reversal will still be beneficial to avoid
> > userspace getting an -EBUSY from the following page flip if the second pipe's
> > commit does stretch into the next frame.
> Can there be a possibility of seeing a flicker/corruption in that case?
> Also should this be added a TODO in the driver so that it will not be missed out?

I have something typed up for this already. Just waiting for better
testing coverage to actually exercise it properly.

> 
> Above comment is for clarification and if a TODO is required, can be taken up while merging the patch. Remaining logic looks good to me.
> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>

Thanks. I've pushed patches 1-6.

> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 14 +++++++++++---
> > drivers/gpu/drm/i915/display/intel_display.h |  8 ++++++++
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index a481c9218138..0086a7422e86 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6956,8 +6956,12 @@ static void skl_commit_modeset_enables(struct
> > intel_atomic_state *state)
> >  	intel_dbuf_mbus_pre_ddb_update(state);
> > 
> >  	while (update_pipes) {
> > -		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > -						    new_crtc_state, i) {
> > +		/*
> > +		 * Commit in reverse order to make bigjoiner master
> > +		 * send the uapi events after slaves are done.
> > +		 */
> > +		for_each_oldnew_intel_crtc_in_state_reverse(state, crtc,
> > old_crtc_state,
> > +							    new_crtc_state, i) {
> >  			enum pipe pipe = crtc->pipe;
> > 
> >  			if ((update_pipes & BIT(pipe)) == 0) @@ -7036,7
> > +7040,11 @@ static void skl_commit_modeset_enables(struct
> > intel_atomic_state *state)
> >  		intel_pre_update_crtc(state, crtc);
> >  	}
> > 
> > -	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > +	/*
> > +	 * Commit in reverse order to make bigjoiner master
> > +	 * send the uapi events after slaves are done.
> > +	 */
> > +	for_each_new_intel_crtc_in_state_reverse(state, crtc, new_crtc_state,
> > +i) {
> >  		enum pipe pipe = crtc->pipe;
> > 
> >  		if ((update_pipes & BIT(pipe)) == 0)
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> > b/drivers/gpu/drm/i915/display/intel_display.h
> > index 986ec77490de..423074d6947a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -344,6 +344,14 @@ enum phy_fia {
> >  	     (__i)++) \
> >  		for_each_if(crtc)
> > 
> > +#define for_each_new_intel_crtc_in_state_reverse(__state, crtc,
> > new_crtc_state, __i) \
> > +	for ((__i) = (__state)->base.dev->mode_config.num_crtc - 1; \
> > +	     (__i) >= 0  && \
> > +	     ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
> > +	      (new_crtc_state) = to_intel_crtc_state((__state)-
> > >base.crtcs[__i].new_state), 1); \
> > +	     (__i)--) \
> > +		for_each_if(crtc)
> > +
> >  #define for_each_oldnew_intel_plane_in_state(__state, plane, old_plane_state,
> > new_plane_state, __i) \
> >  	for ((__i) = 0; \
> >  	     (__i) < (__state)->base.dev->mode_config.num_total_plane && \
> > --
> > 2.43.2
> 

-- 
Ville Syrjälä
Intel