[2/6] drm/i915: Reject async flips if we need to change DDB/watermarks

Commit Message

Ville Syrjälä March 20, 2024, 4:04 p.m. UTC

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

DDB/watermarks are always double buffered on the vblank, so we
can't safely change them during async flips. Currently this never
happens, but we'll be making changing between sync and async
flips a bit more flexible, in which case we can actually end up
here.

Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Arun R Murthy April 19, 2024, 4:27 a.m. UTC | #1

> -----Original Message-----
> From: Intel-gfx <[e-mail]> On Behalf Of Ville
> Syrjala
> Sent: Wednesday, March 20, 2024 9:34 PM
> To: [e-mail]
> Subject: [PATCH 2/6] drm/i915: Reject async flips if we need to change
> DDB/watermarks
> 
> From: Ville Syrjälä <[e-mail]>
> 
> DDB/watermarks are always double buffered on the vblank, so we can't safely
> change them during async flips. Currently this never happens, but we'll be
> making changing between sync and async flips a bit more flexible, in which case
> we can actually end up here.

Rather on getting wm/DDB changes should we switch from async to sync flip to honour the wm/DDB changes else might end up in underrun or flicker/corruption.
Spec is also aligned to this approach.

Thanks and Regards,
Arun R Murthy
--------------------

> 
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/skl_watermark.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index bc341abcab2f..1fa416a70d51 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -2540,6 +2540,12 @@ skl_ddb_add_affected_planes(const struct
> intel_crtc_state *old_crtc_state,
>  					&new_crtc_state-
> >wm.skl.plane_ddb_y[plane_id]))
>  			continue;
> 
> +		if (new_crtc_state->do_async_flip) {
> +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> change DDB during async flip\n",
> +				    plane->base.base.id, plane->base.name);
> +			return -EINVAL;
> +		}
> +
>  		plane_state = intel_atomic_get_plane_state(state, plane);
>  		if (IS_ERR(plane_state))
>  			return PTR_ERR(plane_state);
> @@ -2906,6 +2912,12 @@ static int skl_wm_add_affected_planes(struct
> intel_atomic_state *state,
>  						 &new_crtc_state-
> >wm.skl.optimal))
>  			continue;
> 
> +		if (new_crtc_state->do_async_flip) {
> +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> change watermarks during async flip\n",
> +				    plane->base.base.id, plane->base.name);
> +			return -EINVAL;
> +		}
> +
>  		plane_state = intel_atomic_get_plane_state(state, plane);
>  		if (IS_ERR(plane_state))
>  			return PTR_ERR(plane_state);
> --
> 2.43.2

Kulkarni, Vandita April 19, 2024, 6:31 a.m. UTC | #2

> -----Original Message-----
> From: Intel-gfx <[e-mail]> On Behalf Of Ville
> Syrjala
> Sent: Wednesday, March 20, 2024 9:34 PM
> To: [e-mail]
> Subject: [PATCH 2/6] drm/i915: Reject async flips if we need to change
> DDB/watermarks
> 
> From: Ville Syrjälä <[e-mail]>
> 
> DDB/watermarks are always double buffered on the vblank, so we can't
> safely change them during async flips. Currently this never happens, but we'll
> be making changing between sync and async flips a bit more flexible, in which
> case we can actually end up here.
> 
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
Looks good to me.
Reviewed-by: Vandita Kulkarni <[e-mail]>

>  drivers/gpu/drm/i915/display/skl_watermark.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index bc341abcab2f..1fa416a70d51 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -2540,6 +2540,12 @@ skl_ddb_add_affected_planes(const struct
> intel_crtc_state *old_crtc_state,
>  					&new_crtc_state-
> >wm.skl.plane_ddb_y[plane_id]))
>  			continue;
> 
> +		if (new_crtc_state->do_async_flip) {
> +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> change DDB during async flip\n",
> +				    plane->base.base.id, plane->base.name);
> +			return -EINVAL;
> +		}
> +
>  		plane_state = intel_atomic_get_plane_state(state, plane);
>  		if (IS_ERR(plane_state))
>  			return PTR_ERR(plane_state);
> @@ -2906,6 +2912,12 @@ static int skl_wm_add_affected_planes(struct
> intel_atomic_state *state,
>  						 &new_crtc_state-
> >wm.skl.optimal))
>  			continue;
> 
> +		if (new_crtc_state->do_async_flip) {
> +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> change watermarks during async flip\n",
> +				    plane->base.base.id, plane->base.name);
> +			return -EINVAL;
> +		}
> +
>  		plane_state = intel_atomic_get_plane_state(state, plane);
>  		if (IS_ERR(plane_state))
>  			return PTR_ERR(plane_state);
> --
> 2.43.2

Ville Syrjälä April 19, 2024, 4:25 p.m. UTC | #3

On Fri, Apr 19, 2024 at 04:27:53AM +0000, Murthy, Arun R wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx <[e-mail]> On Behalf Of Ville
> > Syrjala
> > Sent: Wednesday, March 20, 2024 9:34 PM
> > To: [e-mail]
> > Subject: [PATCH 2/6] drm/i915: Reject async flips if we need to change
> > DDB/watermarks
> > 
> > From: Ville Syrjälä <[e-mail]>
> > 
> > DDB/watermarks are always double buffered on the vblank, so we can't safely
> > change them during async flips. Currently this never happens, but we'll be
> > making changing between sync and async flips a bit more flexible, in which case
> > we can actually end up here.
> 
> Rather on getting wm/DDB changes should we switch from async to sync flip to honour the wm/DDB changes else might end up in underrun or flicker/corruption.
> Spec is also aligned to this approach.

I can't really parse what you're saying.

The sequence of events that can lead us here are:
1. start in sync flip mode
2. userspace asks for an async flip (potentially asking for a
   different modifier)
   - we convert it to a sync flip and proceed
3. userspace asks for another async flip
   either:
   - the format/modifier (and thus wm/ddb) stays the same all
     is good and we do the async flip
   - the modifier changes we will now reject the request due to
     wm/ddb needing to change

We don't want to convert step 3 also to a sync flip because userspace
could just keep pingponging between two buffers with different modifiers
and we'd never actually get into proper async flip mode, and would just
keep doing sync flips. That would completely defat the purpose of async
flips.

And we do have to reject the request here in the wm code because
otherwise we'll clear the do_async_flip flag and the later
intel_async_flip_check_hw() wouldn't refuse the request even though
the modifier is changing. The other option would be to move some/all
of intel_async_flip_check_hw() into some earlier phase of
atomic_check(), but that would require some actual thought.


> Thanks and Regards,
> Arun R Murthy
> --------------------
> 
> > 
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/skl_watermark.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index bc341abcab2f..1fa416a70d51 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -2540,6 +2540,12 @@ skl_ddb_add_affected_planes(const struct
> > intel_crtc_state *old_crtc_state,
> >  					&new_crtc_state-
> > >wm.skl.plane_ddb_y[plane_id]))
> >  			continue;
> > 
> > +		if (new_crtc_state->do_async_flip) {
> > +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> > change DDB during async flip\n",
> > +				    plane->base.base.id, plane->base.name);
> > +			return -EINVAL;
> > +		}
> > +
> >  		plane_state = intel_atomic_get_plane_state(state, plane);
> >  		if (IS_ERR(plane_state))
> >  			return PTR_ERR(plane_state);
> > @@ -2906,6 +2912,12 @@ static int skl_wm_add_affected_planes(struct
> > intel_atomic_state *state,
> >  						 &new_crtc_state-
> > >wm.skl.optimal))
> >  			continue;
> > 
> > +		if (new_crtc_state->do_async_flip) {
> > +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> > change watermarks during async flip\n",
> > +				    plane->base.base.id, plane->base.name);
> > +			return -EINVAL;
> > +		}
> > +
> >  		plane_state = intel_atomic_get_plane_state(state, plane);
> >  		if (IS_ERR(plane_state))
> >  			return PTR_ERR(plane_state);
> > --
> > 2.43.2
>

Arun R Murthy April 23, 2024, 3:45 a.m. UTC | #4

> -----Original Message-----
> From: Ville Syrjälä <[e-mail]>
> Sent: Friday, April 19, 2024 9:56 PM
> To: Murthy, Arun R <[e-mail]>
> Cc: [e-mail]
> Subject: Re: [PATCH 2/6] drm/i915: Reject async flips if we need to change
> DDB/watermarks
> 
> On Fri, Apr 19, 2024 at 04:27:53AM +0000, Murthy, Arun R wrote:
> >
> > > -----Original Message-----
> > > From: Intel-gfx <[e-mail]> On Behalf
> > > Of Ville Syrjala
> > > Sent: Wednesday, March 20, 2024 9:34 PM
> > > To: [e-mail]
> > > Subject: [PATCH 2/6] drm/i915: Reject async flips if we need to
> > > change DDB/watermarks
> > >
> > > From: Ville Syrjälä <[e-mail]>
> > >
> > > DDB/watermarks are always double buffered on the vblank, so we can't
> > > safely change them during async flips. Currently this never happens,
> > > but we'll be making changing between sync and async flips a bit more
> > > flexible, in which case we can actually end up here.
> >
> > Rather on getting wm/DDB changes should we switch from async to sync flip
> to honour the wm/DDB changes else might end up in underrun or
> flicker/corruption.
> > Spec is also aligned to this approach.
> 
> I can't really parse what you're saying.
> 
> The sequence of events that can lead us here are:
> 1. start in sync flip mode
> 2. userspace asks for an async flip (potentially asking for a
>    different modifier)
>    - we convert it to a sync flip and proceed 3. userspace asks for another async
> flip
>    either:
>    - the format/modifier (and thus wm/ddb) stays the same all
>      is good and we do the async flip
>    - the modifier changes we will now reject the request due to
>      wm/ddb needing to change
> 
> We don't want to convert step 3 also to a sync flip because userspace could just
> keep pingponging between two buffers with different modifiers and we'd never
> actually get into proper async flip mode, and would just keep doing sync flips.
> That would completely defat the purpose of async flips.
> 
> And we do have to reject the request here in the wm code because otherwise
> we'll clear the do_async_flip flag and the later
> intel_async_flip_check_hw() wouldn't refuse the request even though the
> modifier is changing. The other option would be to move some/all of
> intel_async_flip_check_hw() into some earlier phase of atomic_check(), but that
> would require some actual thought.
> 
Even adding some/all changes in the beginning of atomic_check will eventually fail the flip, so better to send the failure as is. 
Upon seeing the failure X would fallback.

Reviewed-by: Arun R Murthy <[e-mail]>

Thanks and Regards,
Arun R Murthy
-------------------
> 
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> >
> > >
> > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/display/skl_watermark.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > index bc341abcab2f..1fa416a70d51 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > @@ -2540,6 +2540,12 @@ skl_ddb_add_affected_planes(const struct
> > > intel_crtc_state *old_crtc_state,
> > >  					&new_crtc_state-
> > > >wm.skl.plane_ddb_y[plane_id]))
> > >  			continue;
> > >
> > > +		if (new_crtc_state->do_async_flip) {
> > > +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> > > change DDB during async flip\n",
> > > +				    plane->base.base.id, plane->base.name);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > >  		plane_state = intel_atomic_get_plane_state(state, plane);
> > >  		if (IS_ERR(plane_state))
> > >  			return PTR_ERR(plane_state);
> > > @@ -2906,6 +2912,12 @@ static int skl_wm_add_affected_planes(struct
> > > intel_atomic_state *state,
> > >  						 &new_crtc_state-
> > > >wm.skl.optimal))
> > >  			continue;
> > >
> > > +		if (new_crtc_state->do_async_flip) {
> > > +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> > > change watermarks during async flip\n",
> > > +				    plane->base.base.id, plane->base.name);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > >  		plane_state = intel_atomic_get_plane_state(state, plane);
> > >  		if (IS_ERR(plane_state))
> > >  			return PTR_ERR(plane_state);
> > > --
> > > 2.43.2
> >
> 
> --
> Ville Syrjälä
> Intel

Patch

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index bc341abcab2f..1fa416a70d51 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -2540,6 +2540,12 @@  skl_ddb_add_affected_planes(const struct intel_crtc_state *old_crtc_state,
 					&new_crtc_state->wm.skl.plane_ddb_y[plane_id]))
 			continue;
 
+		if (new_crtc_state->do_async_flip) {
+			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't change DDB during async flip\n",
+				    plane->base.base.id, plane->base.name);
+			return -EINVAL;
+		}
+
 		plane_state = intel_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state))
 			return PTR_ERR(plane_state);
@@ -2906,6 +2912,12 @@  static int skl_wm_add_affected_planes(struct intel_atomic_state *state,
 						 &new_crtc_state->wm.skl.optimal))
 			continue;
 
+		if (new_crtc_state->do_async_flip) {
+			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't change watermarks during async flip\n",
+				    plane->base.base.id, plane->base.name);
+			return -EINVAL;
+		}
+
 		plane_state = intel_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state))
 			return PTR_ERR(plane_state);