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

[Kulkarni, Vandita]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Friday, April 5, 2024 3:04 AM
> To: intel-gfx at lists.freedesktop.org
> Subject: [PATCH v2 01/17] drm/i915: Update pipes in reverse order for
> bigjoiner
> 
> From: Ville Syrjälä <ville.syrjala at 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.
> 

LGTM.
Reviewed-by: Vandita Kulkarni <vandita.kulkarni at intel.com>

I had just one query though,
Will there be a case where we can get vblank between slave update and master update,
if so do you think there will be any problem due to that?

Thanks,
Vandita

> Signed-off-by: Ville Syrjälä <ville.syrjala at 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