[PATCH v3 3/5] drm/msm: fix fb references in async update

[PATCH v3 3/5] drm/msm: fix fb references in async update

[Helen Koike]

Async update callbacks are expected to set the old_fb in the new_state
so prepare/cleanup framebuffers are balanced.

Cc: <stable@vger.kernel.org> # v4.14+
Fixes: 224a4c970987 ("drm/msm: update cursors asynchronously through atomic")
Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Helen Koike <helen.koike@collabora.com>

---
Hello,

As mentioned in the cover letter,
But I couldn't test on MSM because I don't have the hardware and I would
appreciate if anyone could test it.

In other platforms (VC4, AMD, Rockchip), there is a hidden
drm_framebuffer_get(new_fb)/drm_framebuffer_put(old_fb) in async_update
that is wrong, but I couldn't identify those here, not sure if it is hidden
somewhere else, but if tests fail this is probably the cause.

Thanks!
Helen

Changes in v3: None
Changes in v2:
- update CC stable and Fixes tag

 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index be13140967b4..b854f471e9e5 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -502,6 +502,8 @@ static int mdp5_plane_atomic_async_check(struct drm_plane *plane,
 static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
 					   struct drm_plane_state *new_state)
 {
+	struct drm_framebuffer *old_fb = plane->state->fb;
+
 	plane->state->src_x = new_state->src_x;
 	plane->state->src_y = new_state->src_y;
 	plane->state->crtc_x = new_state->crtc_x;
@@ -524,6 +526,8 @@ static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
 
 	*to_mdp5_plane_state(plane->state) =
 		*to_mdp5_plane_state(new_state);
+
+	new_state->fb = old_fb;
 }
 
 static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = {
-- 
2.20.1

Re: [PATCH v3 3/5] drm/msm: fix fb references in async update

[Rob Clark]

On Wed, Mar 13, 2019 at 8:21 PM Helen Koike <helen.koike@collabora.com> wrote:
>
> Async update callbacks are expected to set the old_fb in the new_state
> so prepare/cleanup framebuffers are balanced.
>
> Cc: <stable@vger.kernel.org> # v4.14+
> Fixes: 224a4c970987 ("drm/msm: update cursors asynchronously through atomic")
> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>
> ---
> Hello,
>
> As mentioned in the cover letter,
> But I couldn't test on MSM because I don't have the hardware and I would
> appreciate if anyone could test it.
>
> In other platforms (VC4, AMD, Rockchip), there is a hidden
> drm_framebuffer_get(new_fb)/drm_framebuffer_put(old_fb) in async_update
> that is wrong, but I couldn't identify those here, not sure if it is hidden
> somewhere else, but if tests fail this is probably the cause.
>
> Thanks!
> Helen

Apologies, I haven't had a chance to test this yet, and it is likely
to be a few weeks before I have a chance..

That said, not refcnt'ing the fb seems suspicious.   I read the
explanation in the other patches, but kind of think it is more clear
to keep the extra refcnt'ing.. or at least have a comment.

I do a bit wonder if we hold the ref to the outgoing cursor long
enough.. I think the hw will actually continue to scan it out until
the next vblank.  But Archit was the one who added async_update
support so maybe that is handled somewhere else.  It's been a while
since I've looked at this.

BR,
-R



>
> Changes in v3: None
> Changes in v2:
> - update CC stable and Fixes tag
>
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index be13140967b4..b854f471e9e5 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -502,6 +502,8 @@ static int mdp5_plane_atomic_async_check(struct drm_plane *plane,
>  static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
>                                            struct drm_plane_state *new_state)
>  {
> +       struct drm_framebuffer *old_fb = plane->state->fb;
> +
>         plane->state->src_x = new_state->src_x;
>         plane->state->src_y = new_state->src_y;
>         plane->state->crtc_x = new_state->crtc_x;
> @@ -524,6 +526,8 @@ static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
>
>         *to_mdp5_plane_state(plane->state) =
>                 *to_mdp5_plane_state(new_state);
> +
> +       new_state->fb = old_fb;
>  }
>
>  static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = {
> --
> 2.20.1
>

Re: [PATCH v3 3/5] drm/msm: fix fb references in async update

[Helen Koike]

Hi Rob,

On 3/27/19 11:52 AM, Rob Clark wrote:
> On Wed, Mar 13, 2019 at 8:21 PM Helen Koike <helen.koike@collabora.com> wrote:
>>
>> Async update callbacks are expected to set the old_fb in the new_state
>> so prepare/cleanup framebuffers are balanced.
>>
>> Cc: <stable@vger.kernel.org> # v4.14+
>> Fixes: 224a4c970987 ("drm/msm: update cursors asynchronously through atomic")
>> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>> Hello,
>>
>> As mentioned in the cover letter,
>> But I couldn't test on MSM because I don't have the hardware and I would
>> appreciate if anyone could test it.
>>
>> In other platforms (VC4, AMD, Rockchip), there is a hidden
>> drm_framebuffer_get(new_fb)/drm_framebuffer_put(old_fb) in async_update
>> that is wrong, but I couldn't identify those here, not sure if it is hidden
>> somewhere else, but if tests fail this is probably the cause.
>>
>> Thanks!
>> Helen
> 
> Apologies, I haven't had a chance to test this yet, and it is likely
> to be a few weeks before I have a chance..
> 
> That said, not refcnt'ing the fb seems suspicious.   I read the
> explanation in the other patches, but kind of think it is more clear
> to keep the extra refcnt'ing.. or at least have a comment.


Refcnt'ing is taken care by drm_mode_cursor_universal() when calling
drm_atomic_helper_update_plane().
The other codes had a drm_atomic_set_fb_for_plane() (which get a
referent of the new fb and put the old fb), but the problem is that the
cleanup_plane will also put the ref of the old fb.
In the other drivers I saw, drm_atomic_set_fb_for_plane() shouldn't be
called, but this issue should be triggered very easily with a test.

I can add this is in the async_check() docs.

> 
> I do a bit wonder if we hold the ref to the outgoing cursor long
> enough.. I think the hw will actually continue to scan it out until
> the next vblank.  But Archit was the one who added async_update
> support so maybe that is handled somewhere else.  It's been a while
> since I've looked at this.

I don't know much about MSM, but if I understand correctly, MSM don't
support true async, is that correct?

Then I think we need to do something similar to the rockchip:

https://patchwork.kernel.org/patch/10852039/

and setup a drm_flip_work_queue().
But this seems another problem that is already there no? Not really
related to this patch. I could send a separate patch for this (but I
don't have the means to test it).

Regards,
Helen

> 
> BR,
> -R
> 
> 
> 
>>
>> Changes in v3: None
>> Changes in v2:
>> - update CC stable and Fixes tag
>>
>>  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
>> index be13140967b4..b854f471e9e5 100644
>> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
>> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
>> @@ -502,6 +502,8 @@ static int mdp5_plane_atomic_async_check(struct drm_plane *plane,
>>  static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
>>                                            struct drm_plane_state *new_state)
>>  {
>> +       struct drm_framebuffer *old_fb = plane->state->fb;
>> +
>>         plane->state->src_x = new_state->src_x;
>>         plane->state->src_y = new_state->src_y;
>>         plane->state->crtc_x = new_state->crtc_x;
>> @@ -524,6 +526,8 @@ static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
>>
>>         *to_mdp5_plane_state(plane->state) =
>>                 *to_mdp5_plane_state(new_state);
>> +
>> +       new_state->fb = old_fb;
>>  }
>>
>>  static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = {
>> --
>> 2.20.1
>>
> 

Re: [PATCH v3 3/5] drm/msm: fix fb references in async update

[Helen Koike]

Hello,

On 3/13/19 9:20 PM, Helen Koike wrote:
> Async update callbacks are expected to set the old_fb in the new_state
> so prepare/cleanup framebuffers are balanced.
> 
> Cc: <stable@vger.kernel.org> # v4.14+
> Fixes: 224a4c970987 ("drm/msm: update cursors asynchronously through atomic")
> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> Hello,
> 
> As mentioned in the cover letter,
> But I couldn't test on MSM because I don't have the hardware and I would
> appreciate if anyone could test it.

I got this tested on a dragonboard 410c, no regressions where found and
no extra warnings.

These two tests where already failing for other reasons:
flip-vs-cursor-crc-atomic
flip-vs-cursor-crc-legacy

If you want to see the full log:

https://people.collabora.com/~koike/drm-fixes-results.zip

Thanks
Helen

> 
> In other platforms (VC4, AMD, Rockchip), there is a hidden
> drm_framebuffer_get(new_fb)/drm_framebuffer_put(old_fb) in async_update
> that is wrong, but I couldn't identify those here, not sure if it is hidden
> somewhere else, but if tests fail this is probably the cause.
> 
> Thanks!
> Helen
> 
> Changes in v3: None
> Changes in v2:
> - update CC stable and Fixes tag
> 
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index be13140967b4..b854f471e9e5 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -502,6 +502,8 @@ static int mdp5_plane_atomic_async_check(struct drm_plane *plane,
>  static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
>  					   struct drm_plane_state *new_state)
>  {
> +	struct drm_framebuffer *old_fb = plane->state->fb;
> +
>  	plane->state->src_x = new_state->src_x;
>  	plane->state->src_y = new_state->src_y;
>  	plane->state->crtc_x = new_state->crtc_x;
> @@ -524,6 +526,8 @@ static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
>  
>  	*to_mdp5_plane_state(plane->state) =
>  		*to_mdp5_plane_state(new_state);
> +
> +	new_state->fb = old_fb;
>  }
>  
>  static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = {
> 

Re: [PATCH v3 3/5] drm/msm: fix fb references in async update

[Rob Clark]

On Fri, May 31, 2019 at 10:54 AM Helen Koike <helen@koikeco.de> wrote:
>
> Hello,
>
> On 3/13/19 9:20 PM, Helen Koike wrote:
> > Async update callbacks are expected to set the old_fb in the new_state
> > so prepare/cleanup framebuffers are balanced.
> >
> > Cc: <stable@vger.kernel.org> # v4.14+
> > Fixes: 224a4c970987 ("drm/msm: update cursors asynchronously through atomic")
> > Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Signed-off-by: Helen Koike <helen.koike@collabora.com>

Thanks, I'm not super happy about the refcnt'ing subtleness here
(mostly because it makes it harder to page in how things work on
kernel/display side after spending most of my time in userspace/mesa),
but I don't want to hold this up..

Acked-by: Rob Clark <robdclark@gmail.com>

> >
> > ---
> > Hello,
> >
> > As mentioned in the cover letter,
> > But I couldn't test on MSM because I don't have the hardware and I would
> > appreciate if anyone could test it.
>
> I got this tested on a dragonboard 410c, no regressions where found and
> no extra warnings.
>
> These two tests where already failing for other reasons:
> flip-vs-cursor-crc-atomic
> flip-vs-cursor-crc-legacy
>
> If you want to see the full log:
>
> https://people.collabora.com/~koike/drm-fixes-results.zip
>
> Thanks
> Helen
>
> >
> > In other platforms (VC4, AMD, Rockchip), there is a hidden
> > drm_framebuffer_get(new_fb)/drm_framebuffer_put(old_fb) in async_update
> > that is wrong, but I couldn't identify those here, not sure if it is hidden
> > somewhere else, but if tests fail this is probably the cause.
> >
> > Thanks!
> > Helen
> >
> > Changes in v3: None
> > Changes in v2:
> > - update CC stable and Fixes tag
> >
> >  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > index be13140967b4..b854f471e9e5 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > @@ -502,6 +502,8 @@ static int mdp5_plane_atomic_async_check(struct drm_plane *plane,
> >  static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
> >                                          struct drm_plane_state *new_state)
> >  {
> > +     struct drm_framebuffer *old_fb = plane->state->fb;
> > +
> >       plane->state->src_x = new_state->src_x;
> >       plane->state->src_y = new_state->src_y;
> >       plane->state->crtc_x = new_state->crtc_x;
> > @@ -524,6 +526,8 @@ static void mdp5_plane_atomic_async_update(struct drm_plane *plane,
> >
> >       *to_mdp5_plane_state(plane->state) =
> >               *to_mdp5_plane_state(new_state);
> > +
> > +     new_state->fb = old_fb;
> >  }
> >
> >  static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = {
> >

[PATCH v3 4/5] drm/vc4: fix fb references in async update

[Helen Koike]

Async update callbacks are expected to set the old_fb in the new_state
so prepare/cleanup framebuffers are balanced.

Calling drm_atomic_set_fb_for_plane() (which gets a reference of the new
fb and put the old fb) is not required, as it's taken care by
drm_mode_cursor_universal() when calling drm_atomic_helper_update_plane().

Cc: <stable@vger.kernel.org> # v4.19+
Fixes: 539c320bfa97 ("drm/vc4: update cursors asynchronously through atomic")
Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Helen Koike <helen.koike@collabora.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

---
Hello,

I tested on a Raspberry Pi model B rev2 with igt plane_cursor_legacy and
kms_cursor_legacy and I didn't see any regressions.

Changes in v3: None
Changes in v2:
- Added reviewed-by tag
- updated CC stable and Fixes tag

 drivers/gpu/drm/vc4/vc4_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 4d918d3e4858..afc80b245ea3 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -1025,7 +1025,7 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
 {
 	struct vc4_plane_state *vc4_state, *new_vc4_state;
 
-	drm_atomic_set_fb_for_plane(plane->state, state->fb);
+	swap(plane->state->fb, state->fb);
 	plane->state->crtc_x = state->crtc_x;
 	plane->state->crtc_y = state->crtc_y;
 	plane->state->crtc_w = state->crtc_w;
-- 
2.20.1

[PATCH v3 5/5] drm: don't block fb changes for async plane updates

[Helen Koike]

In the case of a normal sync update, the preparation of framebuffers (be
it calling drm_atomic_helper_prepare_planes() or doing setups with
drm_framebuffer_get()) are performed in the new_state and the respective
cleanups are performed in the old_state.

In the case of async updates, the preparation is also done in the
new_state but the cleanups are done in the new_state (because updates
are performed in place, i.e. in the current state).

The current code blocks async udpates when the fb is changed, turning
async updates into sync updates, slowing down cursor updates and
introducing regressions in igt tests with errors of type:

"CRITICAL: completed 97 cursor updated in a period of 30 flips, we
expect to complete approximately 15360 updates, with the threshold set
at 7680"

Fb changes in async updates were prevented to avoid the following scenario:

- Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1
- Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2
- Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2 (wrong)
Where we have a single call to prepare fb2 but double cleanup call to fb2.

To solve the above problems, instead of blocking async fb changes, we
place the old framebuffer in the new_state object, so when the code
performs cleanups in the new_state it will cleanup the old_fb and we
will have the following scenario instead:

- Async update, oldfb = NULL, newfb = fb1, prepare fb1, no cleanup
- Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb1
- Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2

Where calls to prepare/cleanup are balanced.

Cc: <stable@vger.kernel.org> # v4.14+
Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates")
Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Helen Koike <helen.koike@collabora.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

---
Hello,

I added a TODO in drm_atomic_helper_async_commit() regarding doing a
full state swap(), Boris and Nicholas, let me know if this is ok and if
I can keep your Reviewed-by tags)

As mentioned in the cover letter, I tested in almost all platforms with
igt plane_cursor_legacy and kms_cursor_legacy and I didn't see any
regressions. But I couldn't test on MSM and AMD because I don't have
the hardware I would appreciate if anyone could help me testing those.

Thanks!
Helen

Changes in v3:
- Add Reviewed-by tags
- Add TODO in drm_atomic_helper_async_commit()

Changes in v2:
- Change the order of the patch in the series, add this as the last one.
- Add documentation
- s/ballanced/balanced

 drivers/gpu/drm/drm_atomic_helper.c      | 22 ++++++++++++----------
 include/drm/drm_modeset_helper_vtables.h |  5 +++++
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2453678d1186..de5812c362b5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1608,15 +1608,6 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 	    old_plane_state->crtc != new_plane_state->crtc)
 		return -EINVAL;
 
-	/*
-	 * FIXME: Since prepare_fb and cleanup_fb are always called on
-	 * the new_plane_state for async updates we need to block framebuffer
-	 * changes. This prevents use of a fb that's been cleaned up and
-	 * double cleanups from occuring.
-	 */
-	if (old_plane_state->fb != new_plane_state->fb)
-		return -EINVAL;
-
 	funcs = plane->helper_private;
 	if (!funcs->atomic_async_update)
 		return -EINVAL;
@@ -1647,6 +1638,8 @@ EXPORT_SYMBOL(drm_atomic_helper_async_check);
  * drm_atomic_async_check() succeeds. Async commits are not supposed to swap
  * the states like normal sync commits, but just do in-place changes on the
  * current state.
+ *
+ * TODO: Implement full swap instead of doing in-place changes.
  */
 void drm_atomic_helper_async_commit(struct drm_device *dev,
 				    struct drm_atomic_state *state)
@@ -1657,6 +1650,9 @@ void drm_atomic_helper_async_commit(struct drm_device *dev,
 	int i;
 
 	for_each_new_plane_in_state(state, plane, plane_state, i) {
+		struct drm_framebuffer *new_fb = plane_state->fb;
+		struct drm_framebuffer *old_fb = plane->state->fb;
+
 		funcs = plane->helper_private;
 		funcs->atomic_async_update(plane, plane_state);
 
@@ -1665,11 +1661,17 @@ void drm_atomic_helper_async_commit(struct drm_device *dev,
 		 * plane->state in-place, make sure at least common
 		 * properties have been properly updated.
 		 */
-		WARN_ON_ONCE(plane->state->fb != plane_state->fb);
+		WARN_ON_ONCE(plane->state->fb != new_fb);
 		WARN_ON_ONCE(plane->state->crtc_x != plane_state->crtc_x);
 		WARN_ON_ONCE(plane->state->crtc_y != plane_state->crtc_y);
 		WARN_ON_ONCE(plane->state->src_x != plane_state->src_x);
 		WARN_ON_ONCE(plane->state->src_y != plane_state->src_y);
+
+		/*
+		 * Make sure the FBs have been swapped so that cleanups in the
+		 * new_state performs a cleanup in the old FB.
+		 */
+		WARN_ON_ONCE(plane_state->fb != old_fb);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_async_commit);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index cfb7be40bed7..ce582e8e8f2f 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1174,6 +1174,11 @@ struct drm_plane_helper_funcs {
 	 * current one with the new plane configurations in the new
 	 * plane_state.
 	 *
+	 * Drivers should also swap the framebuffers between plane state
+	 * and new_state. This is required because prepare and cleanup calls
+	 * are performed on the new_state object, then to cleanup the old
+	 * framebuffer, it needs to be placed inside the new_state object.
+	 *
 	 * FIXME:
 	 *  - It only works for single plane updates
 	 *  - Async Pageflips are not supported yet
-- 
2.20.1

Re: [PATCH v3 5/5] drm: don't block fb changes for async plane updates

[Sean Paul]

On Wed, Mar 13, 2019 at 09:20:26PM -0300, Helen Koike wrote:
> In the case of a normal sync update, the preparation of framebuffers (be
> it calling drm_atomic_helper_prepare_planes() or doing setups with
> drm_framebuffer_get()) are performed in the new_state and the respective
> cleanups are performed in the old_state.
> 
> In the case of async updates, the preparation is also done in the
> new_state but the cleanups are done in the new_state (because updates
> are performed in place, i.e. in the current state).
> 
> The current code blocks async udpates when the fb is changed, turning
> async updates into sync updates, slowing down cursor updates and
> introducing regressions in igt tests with errors of type:
> 
> "CRITICAL: completed 97 cursor updated in a period of 30 flips, we
> expect to complete approximately 15360 updates, with the threshold set
> at 7680"
> 
> Fb changes in async updates were prevented to avoid the following scenario:
> 
> - Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1
> - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2
> - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2 (wrong)
> Where we have a single call to prepare fb2 but double cleanup call to fb2.
> 
> To solve the above problems, instead of blocking async fb changes, we
> place the old framebuffer in the new_state object, so when the code
> performs cleanups in the new_state it will cleanup the old_fb and we
> will have the following scenario instead:
> 
> - Async update, oldfb = NULL, newfb = fb1, prepare fb1, no cleanup
> - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb1
> - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2
> 
> Where calls to prepare/cleanup are balanced.
> 
> Cc: <stable@vger.kernel.org> # v4.14+

I'm not convinced this should be cc: stable, seems more in the improvement
category than a bug fix.

> Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates")
> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> 
> ---
> Hello,
> 
> I added a TODO in drm_atomic_helper_async_commit() regarding doing a
> full state swap(), Boris and Nicholas, let me know if this is ok and if
> I can keep your Reviewed-by tags)
> 
> As mentioned in the cover letter, I tested in almost all platforms with
> igt plane_cursor_legacy and kms_cursor_legacy and I didn't see any
> regressions. But I couldn't test on MSM and AMD because I don't have
> the hardware I would appreciate if anyone could help me testing those.
> 
> Thanks!
> Helen
> 
> Changes in v3:
> - Add Reviewed-by tags
> - Add TODO in drm_atomic_helper_async_commit()
> 
> Changes in v2:
> - Change the order of the patch in the series, add this as the last one.
> - Add documentation
> - s/ballanced/balanced
> 
>  drivers/gpu/drm/drm_atomic_helper.c      | 22 ++++++++++++----------
>  include/drm/drm_modeset_helper_vtables.h |  5 +++++
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2453678d1186..de5812c362b5 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1608,15 +1608,6 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  	    old_plane_state->crtc != new_plane_state->crtc)
>  		return -EINVAL;
>  
> -	/*
> -	 * FIXME: Since prepare_fb and cleanup_fb are always called on
> -	 * the new_plane_state for async updates we need to block framebuffer
> -	 * changes. This prevents use of a fb that's been cleaned up and
> -	 * double cleanups from occuring.
> -	 */
> -	if (old_plane_state->fb != new_plane_state->fb)
> -		return -EINVAL;
> -
>  	funcs = plane->helper_private;
>  	if (!funcs->atomic_async_update)
>  		return -EINVAL;
> @@ -1647,6 +1638,8 @@ EXPORT_SYMBOL(drm_atomic_helper_async_check);
>   * drm_atomic_async_check() succeeds. Async commits are not supposed to swap
>   * the states like normal sync commits, but just do in-place changes on the
>   * current state.
> + *
> + * TODO: Implement full swap instead of doing in-place changes.
>   */
>  void drm_atomic_helper_async_commit(struct drm_device *dev,
>  				    struct drm_atomic_state *state)
> @@ -1657,6 +1650,9 @@ void drm_atomic_helper_async_commit(struct drm_device *dev,
>  	int i;
>  
>  	for_each_new_plane_in_state(state, plane, plane_state, i) {
> +		struct drm_framebuffer *new_fb = plane_state->fb;
> +		struct drm_framebuffer *old_fb = plane->state->fb;
> +
>  		funcs = plane->helper_private;
>  		funcs->atomic_async_update(plane, plane_state);
>  
> @@ -1665,11 +1661,17 @@ void drm_atomic_helper_async_commit(struct drm_device *dev,
>  		 * plane->state in-place, make sure at least common
>  		 * properties have been properly updated.
>  		 */
> -		WARN_ON_ONCE(plane->state->fb != plane_state->fb);
> +		WARN_ON_ONCE(plane->state->fb != new_fb);
>  		WARN_ON_ONCE(plane->state->crtc_x != plane_state->crtc_x);
>  		WARN_ON_ONCE(plane->state->crtc_y != plane_state->crtc_y);
>  		WARN_ON_ONCE(plane->state->src_x != plane_state->src_x);
>  		WARN_ON_ONCE(plane->state->src_y != plane_state->src_y);
> +
> +		/*
> +		 * Make sure the FBs have been swapped so that cleanups in the
> +		 * new_state performs a cleanup in the old FB.
> +		 */
> +		WARN_ON_ONCE(plane_state->fb != old_fb);
>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_async_commit);
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index cfb7be40bed7..ce582e8e8f2f 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1174,6 +1174,11 @@ struct drm_plane_helper_funcs {
>  	 * current one with the new plane configurations in the new
>  	 * plane_state.
>  	 *
> +	 * Drivers should also swap the framebuffers between plane state

Perhaps add "current" before plane state and then after add "(&drm_plane.state)"
so it's more clear what you're referring to here?

> +	 * and new_state. This is required because prepare and cleanup calls
> +	 * are performed on the new_state object, then to cleanup the old
> +	 * framebuffer, it needs to be placed inside the new_state object.

I'd change this bit to:

        * This is required since cleanup for async commits is performed on
        * the new state, rather than old state like for traditional commits.
        * Since we want to give up the reference on the current (old) fb instead
        * of our brand new one, swap them in the driver during the async commit.

> +	 *
>  	 * FIXME:
>  	 *  - It only works for single plane updates
>  	 *  - Async Pageflips are not supported yet
> -- 
> 2.20.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 5/5] drm: don't block fb changes for async plane updates

[Helen Koike]

On 5/7/19 5:18 PM, Sean Paul wrote:
> On Wed, Mar 13, 2019 at 09:20:26PM -0300, Helen Koike wrote:
>> In the case of a normal sync update, the preparation of framebuffers (be
>> it calling drm_atomic_helper_prepare_planes() or doing setups with
>> drm_framebuffer_get()) are performed in the new_state and the respective
>> cleanups are performed in the old_state.
>>
>> In the case of async updates, the preparation is also done in the
>> new_state but the cleanups are done in the new_state (because updates
>> are performed in place, i.e. in the current state).
>>
>> The current code blocks async udpates when the fb is changed, turning
>> async updates into sync updates, slowing down cursor updates and
>> introducing regressions in igt tests with errors of type:
>>
>> "CRITICAL: completed 97 cursor updated in a period of 30 flips, we
>> expect to complete approximately 15360 updates, with the threshold set
>> at 7680"
>>
>> Fb changes in async updates were prevented to avoid the following scenario:
>>
>> - Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1
>> - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2
>> - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2 (wrong)
>> Where we have a single call to prepare fb2 but double cleanup call to fb2.
>>
>> To solve the above problems, instead of blocking async fb changes, we
>> place the old framebuffer in the new_state object, so when the code
>> performs cleanups in the new_state it will cleanup the old_fb and we
>> will have the following scenario instead:
>>
>> - Async update, oldfb = NULL, newfb = fb1, prepare fb1, no cleanup
>> - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb1
>> - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2
>>
>> Where calls to prepare/cleanup are balanced.
>>
>> Cc: <stable@vger.kernel.org> # v4.14+
> 
> I'm not convinced this should be cc: stable, seems more in the improvement
> category than a bug fix.

I'm cc'ing to stable because the commit mentioned below inserted a
regression regarding the speed that cursors can be updated.

> 
>> Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates")
>> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
>> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>
>> ---
>> Hello,
>>
>> I added a TODO in drm_atomic_helper_async_commit() regarding doing a
>> full state swap(), Boris and Nicholas, let me know if this is ok and if
>> I can keep your Reviewed-by tags)
>>
>> As mentioned in the cover letter, I tested in almost all platforms with
>> igt plane_cursor_legacy and kms_cursor_legacy and I didn't see any
>> regressions. But I couldn't test on MSM and AMD because I don't have
>> the hardware I would appreciate if anyone could help me testing those.
>>
>> Thanks!
>> Helen
>>
>> Changes in v3:
>> - Add Reviewed-by tags
>> - Add TODO in drm_atomic_helper_async_commit()
>>
>> Changes in v2:
>> - Change the order of the patch in the series, add this as the last one.
>> - Add documentation
>> - s/ballanced/balanced
>>
>>  drivers/gpu/drm/drm_atomic_helper.c      | 22 ++++++++++++----------
>>  include/drm/drm_modeset_helper_vtables.h |  5 +++++
>>  2 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 2453678d1186..de5812c362b5 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1608,15 +1608,6 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>>  	    old_plane_state->crtc != new_plane_state->crtc)
>>  		return -EINVAL;
>>  
>> -	/*
>> -	 * FIXME: Since prepare_fb and cleanup_fb are always called on
>> -	 * the new_plane_state for async updates we need to block framebuffer
>> -	 * changes. This prevents use of a fb that's been cleaned up and
>> -	 * double cleanups from occuring.
>> -	 */
>> -	if (old_plane_state->fb != new_plane_state->fb)
>> -		return -EINVAL;
>> -
>>  	funcs = plane->helper_private;
>>  	if (!funcs->atomic_async_update)
>>  		return -EINVAL;
>> @@ -1647,6 +1638,8 @@ EXPORT_SYMBOL(drm_atomic_helper_async_check);
>>   * drm_atomic_async_check() succeeds. Async commits are not supposed to swap
>>   * the states like normal sync commits, but just do in-place changes on the
>>   * current state.
>> + *
>> + * TODO: Implement full swap instead of doing in-place changes.
>>   */
>>  void drm_atomic_helper_async_commit(struct drm_device *dev,
>>  				    struct drm_atomic_state *state)
>> @@ -1657,6 +1650,9 @@ void drm_atomic_helper_async_commit(struct drm_device *dev,
>>  	int i;
>>  
>>  	for_each_new_plane_in_state(state, plane, plane_state, i) {
>> +		struct drm_framebuffer *new_fb = plane_state->fb;
>> +		struct drm_framebuffer *old_fb = plane->state->fb;
>> +
>>  		funcs = plane->helper_private;
>>  		funcs->atomic_async_update(plane, plane_state);
>>  
>> @@ -1665,11 +1661,17 @@ void drm_atomic_helper_async_commit(struct drm_device *dev,
>>  		 * plane->state in-place, make sure at least common
>>  		 * properties have been properly updated.
>>  		 */
>> -		WARN_ON_ONCE(plane->state->fb != plane_state->fb);
>> +		WARN_ON_ONCE(plane->state->fb != new_fb);
>>  		WARN_ON_ONCE(plane->state->crtc_x != plane_state->crtc_x);
>>  		WARN_ON_ONCE(plane->state->crtc_y != plane_state->crtc_y);
>>  		WARN_ON_ONCE(plane->state->src_x != plane_state->src_x);
>>  		WARN_ON_ONCE(plane->state->src_y != plane_state->src_y);
>> +
>> +		/*
>> +		 * Make sure the FBs have been swapped so that cleanups in the
>> +		 * new_state performs a cleanup in the old FB.
>> +		 */
>> +		WARN_ON_ONCE(plane_state->fb != old_fb);
>>  	}
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_async_commit);
>> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
>> index cfb7be40bed7..ce582e8e8f2f 100644
>> --- a/include/drm/drm_modeset_helper_vtables.h
>> +++ b/include/drm/drm_modeset_helper_vtables.h
>> @@ -1174,6 +1174,11 @@ struct drm_plane_helper_funcs {
>>  	 * current one with the new plane configurations in the new
>>  	 * plane_state.
>>  	 *
>> +	 * Drivers should also swap the framebuffers between plane state
> 
> Perhaps add "current" before plane state and then after add "(&drm_plane.state)"
> so it's more clear what you're referring to here?
> 
>> +	 * and new_state. This is required because prepare and cleanup calls
>> +	 * are performed on the new_state object, then to cleanup the old
>> +	 * framebuffer, it needs to be placed inside the new_state object.
> 
> I'd change this bit to:
> 
>         * This is required since cleanup for async commits is performed on
>         * the new state, rather than old state like for traditional commits.
>         * Since we want to give up the reference on the current (old) fb instead
>         * of our brand new one, swap them in the driver during the async commit.
> 
>> +	 *
>>  	 * FIXME:
>>  	 *  - It only works for single plane updates
>>  	 *  - Async Pageflips are not supported yet
>> -- 
>> 2.20.1
>>
> 

Thanks, I'm sending this update in v4.

Helen