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

Commit Message

Ville Syrjala April 4, 2024, 9:34 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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.

Signed-off-by: Ville Syrjälä <[e-mail]>
---
 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(-)

Comments

Murthy, Arun R April 5, 2024, 12:56 p.m. UTC | #1

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

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 <[e-mail]>

Thanks and Regards,
Arun R Murthy
--------------------
> 
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
>  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

Kulkarni, Vandita April 5, 2024, 1:42 p.m. UTC | #2

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

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ä <[e-mail]>
> ---
>  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 Syrjala April 5, 2024, 7:45 p.m. UTC | #3

On Fri, Apr 05, 2024 at 01:42:49PM +0000, Kulkarni, Vandita wrote:
> > -----Original Message-----
> > From: Intel-gfx <[e-mail]> On Behalf Of Ville
> > Syrjala
> > Sent: Friday, April 5, 2024 3:04 AM
> > To: [e-mail]
> > Subject: [PATCH v2 01/17] drm/i915: Update pipes in reverse order for
> > bigjoiner
> > 
> > From: Ville Syrjälä <[e-mail]>
> > 
> > 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 <[e-mail]>
> 
> 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?

It can happen, in which case you may observe a vertical tear.
Since we've disabled all the fancy transcoder level stuff 
(vrr/lrr/etc.) that should be the worst of it.

Ville Syrjala April 8, 2024, 12:46 p.m. UTC | #4

On Fri, Apr 05, 2024 at 12:56:52PM +0000, Murthy, Arun R wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx <[e-mail]> On Behalf Of Ville
> > Syrjala
> > Sent: Friday, April 5, 2024 3:04 AM
> > To: [e-mail]
> > Subject: [PATCH v2 01/17] drm/i915: Update pipes in reverse order for bigjoiner
> > 
> > From: Ville Syrjälä <[e-mail]>
> > 
> > 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 <[e-mail]>

Thanks. I've pushed patches 1-6.

> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
> > 
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> > ---
> >  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
>

Patch

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 && \