* [PATCH v2 1/5] drm/rockchip: fix fb references in async update
2019-03-12 2:21 [PATCH v2 0/5] drm: Fix fb changes for async updates Helen Koike
@ 2019-03-12 2:21 ` Helen Koike
2019-03-12 6:34 ` Boris Brezillon
[not found] ` <20190312022204.2775-1-helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Helen Koike @ 2019-03-12 2:21 UTC (permalink / raw)
To: dri-devel, nicholas.kazlauskas
Cc: andrey.grodzovsky, daniel.vetter, linux-kernel, Tomasz Figa,
boris.brezillon, David Airlie, Sean Paul, kernel, harry.wentland,
Stéphane Marchesin, Helen Koike, Sandy Huang,
linux-rockchip, Heiko Stübner, linux-arm-kernel,
Daniel Vetter
In the case of async update, modifications are done in place, i.e. in the
current plane state, so the new_state is prepared and the new_state is
cleanup up (instead of the old_state, diferrently on what happen in a
normal sync update).
To cleanup the old_fb properly, it needs to be placed in the new_state
in the end of async_update, so cleanup call will unreference the old_fb
correctly.
Also, the previous code had a:
plane_state = plane->funcs->atomic_duplicate_state(plane);
...
swap(plane_state, plane->state);
if (plane->state->fb && plane->state->fb != new_state->fb) {
...
}
Which was wrong, as the fb were just assigned to be equal, so this if
statement nevers evaluates to true.
Another details is that the function drm_crtc_vblank_get() can only be
called when vop->is_enabled is true, otherwise it has no effect and
trows a WARN_ON().
Calling drm_atomic_set_fb_for_plane() (which get a referent of the new
fb and pus the old fb) is not required, as it is taken care by
drm_mode_cursor_universal() when calling
drm_atomic_helper_update_plane().
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
Hello,
I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and
kms_cursor_legacy and I didn't see any regressions.
Changes in v2: None
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++---------
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c7d4c6073ea5..a1ee8c156a7b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
struct drm_plane_state *new_state)
{
struct vop *vop = to_vop(plane->state->crtc);
- struct drm_plane_state *plane_state;
+ struct drm_framebuffer *old_fb = plane->state->fb;
- plane_state = plane->funcs->atomic_duplicate_state(plane);
- plane_state->crtc_x = new_state->crtc_x;
- plane_state->crtc_y = new_state->crtc_y;
- plane_state->crtc_h = new_state->crtc_h;
- plane_state->crtc_w = new_state->crtc_w;
- plane_state->src_x = new_state->src_x;
- plane_state->src_y = new_state->src_y;
- plane_state->src_h = new_state->src_h;
- plane_state->src_w = new_state->src_w;
-
- if (plane_state->fb != new_state->fb)
- drm_atomic_set_fb_for_plane(plane_state, new_state->fb);
-
- swap(plane_state, plane->state);
-
- if (plane->state->fb && plane->state->fb != new_state->fb) {
+ /*
+ * A scanout can still be occurring, so we can't drop the reference to
+ * the old framebuffer. To solve this we get a reference to old_fb and
+ * set a worker to release it later.
+ */
+ if (vop->is_enabled &&
+ plane->state->fb && plane->state->fb != new_state->fb) {
drm_framebuffer_get(plane->state->fb);
WARN_ON(drm_crtc_vblank_get(plane->state->crtc) != 0);
drm_flip_work_queue(&vop->fb_unref_work, plane->state->fb);
set_bit(VOP_PENDING_FB_UNREF, &vop->pending);
}
+ plane->state->crtc_x = new_state->crtc_x;
+ plane->state->crtc_y = new_state->crtc_y;
+ plane->state->crtc_h = new_state->crtc_h;
+ plane->state->crtc_w = new_state->crtc_w;
+ plane->state->src_x = new_state->src_x;
+ plane->state->src_y = new_state->src_y;
+ plane->state->src_h = new_state->src_h;
+ plane->state->src_w = new_state->src_w;
+ plane->state->fb = new_state->fb;
+
if (vop->is_enabled) {
rockchip_drm_psr_inhibit_get_state(new_state->state);
vop_plane_atomic_update(plane, plane->state);
@@ -945,7 +946,12 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
rockchip_drm_psr_inhibit_put_state(new_state->state);
}
- plane->funcs->atomic_destroy_state(plane, plane_state);
+ /*
+ * In async update we perform inplace modifications and release the
+ * new_state. The following is required so we release the reference of
+ * the old framebuffer.
+ */
+ new_state->fb = old_fb;
}
static const struct drm_plane_helper_funcs plane_helper_funcs = {
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update
2019-03-12 2:21 ` [PATCH v2 1/5] drm/rockchip: fix fb references in async update Helen Koike
@ 2019-03-12 6:34 ` Boris Brezillon
2019-03-12 11:04 ` Daniel Vetter
2019-03-12 15:34 ` Helen Koike
0 siblings, 2 replies; 22+ messages in thread
From: Boris Brezillon @ 2019-03-12 6:34 UTC (permalink / raw)
To: Helen Koike
Cc: dri-devel, nicholas.kazlauskas, andrey.grodzovsky, daniel.vetter,
linux-kernel, Tomasz Figa, David Airlie, Sean Paul, kernel,
harry.wentland, Stéphane Marchesin, Sandy Huang,
linux-rockchip, Heiko Stübner, linux-arm-kernel,
Daniel Vetter
On Mon, 11 Mar 2019 23:21:59 -0300
Helen Koike <helen.koike@collabora.com> wrote:
> In the case of async update, modifications are done in place, i.e. in the
> current plane state, so the new_state is prepared and the new_state is
> cleanup up (instead of the old_state, diferrently on what happen in a
^ cleaned up ^ differently (but maybe
"unlike what happens" is more appropriate here).
> normal sync update).
> To cleanup the old_fb properly, it needs to be placed in the new_state
> in the end of async_update, so cleanup call will unreference the old_fb
> correctly.
>
> Also, the previous code had a:
>
> plane_state = plane->funcs->atomic_duplicate_state(plane);
> ...
> swap(plane_state, plane->state);
>
> if (plane->state->fb && plane->state->fb != new_state->fb) {
> ...
> }
>
> Which was wrong, as the fb were just assigned to be equal, so this if
> statement nevers evaluates to true.
>
> Another details is that the function drm_crtc_vblank_get() can only be
> called when vop->is_enabled is true, otherwise it has no effect and
> trows a WARN_ON().
>
> Calling drm_atomic_set_fb_for_plane() (which get a referent of the new
> fb and pus the old fb) is not required, as it is taken care by
> drm_mode_cursor_universal() when calling
> drm_atomic_helper_update_plane().
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>
> ---
> Hello,
>
> I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and
> kms_cursor_legacy and I didn't see any regressions.
>
> Changes in v2: None
>
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++---------
> 1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index c7d4c6073ea5..a1ee8c156a7b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
> struct drm_plane_state *new_state)
> {
> struct vop *vop = to_vop(plane->state->crtc);
> - struct drm_plane_state *plane_state;
> + struct drm_framebuffer *old_fb = plane->state->fb;
>
> - plane_state = plane->funcs->atomic_duplicate_state(plane);
> - plane_state->crtc_x = new_state->crtc_x;
> - plane_state->crtc_y = new_state->crtc_y;
> - plane_state->crtc_h = new_state->crtc_h;
> - plane_state->crtc_w = new_state->crtc_w;
> - plane_state->src_x = new_state->src_x;
> - plane_state->src_y = new_state->src_y;
> - plane_state->src_h = new_state->src_h;
> - plane_state->src_w = new_state->src_w;
> -
> - if (plane_state->fb != new_state->fb)
> - drm_atomic_set_fb_for_plane(plane_state, new_state->fb);
> -
> - swap(plane_state, plane->state);
> -
> - if (plane->state->fb && plane->state->fb != new_state->fb) {
> + /*
> + * A scanout can still be occurring, so we can't drop the reference to
> + * the old framebuffer. To solve this we get a reference to old_fb and
> + * set a worker to release it later.
Hm, doesn't look like an async update to me if we have to wait for the
next VBLANK to happen to get the new content on the screen. Maybe we
should reject async updates when old_fb != new_fb in the rk
->async_check() hook.
> + */
> + if (vop->is_enabled &&
> + plane->state->fb && plane->state->fb != new_state->fb) {
> drm_framebuffer_get(plane->state->fb);
> WARN_ON(drm_crtc_vblank_get(plane->state->crtc) != 0);
> drm_flip_work_queue(&vop->fb_unref_work, plane->state->fb);
> set_bit(VOP_PENDING_FB_UNREF, &vop->pending);
> }
In any case, I think this should be called after
vop_plane_atomic_update() to prevent the situation where the VBLANK
event happens between this point and the following
vop_plane_atomic_update() call.
>
> + plane->state->crtc_x = new_state->crtc_x;
> + plane->state->crtc_y = new_state->crtc_y;
> + plane->state->crtc_h = new_state->crtc_h;
> + plane->state->crtc_w = new_state->crtc_w;
> + plane->state->src_x = new_state->src_x;
> + plane->state->src_y = new_state->src_y;
> + plane->state->src_h = new_state->src_h;
> + plane->state->src_w = new_state->src_w;
> + plane->state->fb = new_state->fb;
Any reason not to use swap() here and reference plane->state->fb
instead of new_state->fb after this point?
> +
> if (vop->is_enabled) {
> rockchip_drm_psr_inhibit_get_state(new_state->state);
> vop_plane_atomic_update(plane, plane->state);
> @@ -945,7 +946,12 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
> rockchip_drm_psr_inhibit_put_state(new_state->state);
> }
>
> - plane->funcs->atomic_destroy_state(plane, plane_state);
> + /*
> + * In async update we perform inplace modifications and release the
> + * new_state. The following is required so we release the reference of
> + * the old framebuffer.
> + */
> + new_state->fb = old_fb;
> }
>
> static const struct drm_plane_helper_funcs plane_helper_funcs = {
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update
2019-03-12 6:34 ` Boris Brezillon
@ 2019-03-12 11:04 ` Daniel Vetter
2019-03-12 15:34 ` Helen Koike
1 sibling, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2019-03-12 11:04 UTC (permalink / raw)
To: Boris Brezillon
Cc: Helen Koike, dri-devel, nicholas.kazlauskas, andrey.grodzovsky,
daniel.vetter, linux-kernel, Tomasz Figa, David Airlie,
Sean Paul, kernel, harry.wentland, Stéphane Marchesin,
Sandy Huang, linux-rockchip, Heiko Stübner,
linux-arm-kernel, Daniel Vetter
On Tue, Mar 12, 2019 at 07:34:38AM +0100, Boris Brezillon wrote:
> On Mon, 11 Mar 2019 23:21:59 -0300
> Helen Koike <helen.koike@collabora.com> wrote:
>
> > In the case of async update, modifications are done in place, i.e. in the
> > current plane state, so the new_state is prepared and the new_state is
> > cleanup up (instead of the old_state, diferrently on what happen in a
>
> ^ cleaned up ^ differently (but maybe
> "unlike what happens" is more appropriate here).
>
> > normal sync update).
> > To cleanup the old_fb properly, it needs to be placed in the new_state
> > in the end of async_update, so cleanup call will unreference the old_fb
> > correctly.
> >
> > Also, the previous code had a:
> >
> > plane_state = plane->funcs->atomic_duplicate_state(plane);
> > ...
> > swap(plane_state, plane->state);
> >
> > if (plane->state->fb && plane->state->fb != new_state->fb) {
> > ...
> > }
> >
> > Which was wrong, as the fb were just assigned to be equal, so this if
> > statement nevers evaluates to true.
> >
> > Another details is that the function drm_crtc_vblank_get() can only be
> > called when vop->is_enabled is true, otherwise it has no effect and
> > trows a WARN_ON().
> >
> > Calling drm_atomic_set_fb_for_plane() (which get a referent of the new
> > fb and pus the old fb) is not required, as it is taken care by
> > drm_mode_cursor_universal() when calling
> > drm_atomic_helper_update_plane().
> >
> > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >
> > ---
> > Hello,
> >
> > I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and
> > kms_cursor_legacy and I didn't see any regressions.
> >
> > Changes in v2: None
> >
> > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++---------
> > 1 file changed, 24 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index c7d4c6073ea5..a1ee8c156a7b 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
> > struct drm_plane_state *new_state)
> > {
> > struct vop *vop = to_vop(plane->state->crtc);
> > - struct drm_plane_state *plane_state;
> > + struct drm_framebuffer *old_fb = plane->state->fb;
> >
> > - plane_state = plane->funcs->atomic_duplicate_state(plane);
> > - plane_state->crtc_x = new_state->crtc_x;
> > - plane_state->crtc_y = new_state->crtc_y;
> > - plane_state->crtc_h = new_state->crtc_h;
> > - plane_state->crtc_w = new_state->crtc_w;
> > - plane_state->src_x = new_state->src_x;
> > - plane_state->src_y = new_state->src_y;
> > - plane_state->src_h = new_state->src_h;
> > - plane_state->src_w = new_state->src_w;
> > -
> > - if (plane_state->fb != new_state->fb)
> > - drm_atomic_set_fb_for_plane(plane_state, new_state->fb);
> > -
> > - swap(plane_state, plane->state);
> > -
> > - if (plane->state->fb && plane->state->fb != new_state->fb) {
> > + /*
> > + * A scanout can still be occurring, so we can't drop the reference to
> > + * the old framebuffer. To solve this we get a reference to old_fb and
> > + * set a worker to release it later.
>
> Hm, doesn't look like an async update to me if we have to wait for the
> next VBLANK to happen to get the new content on the screen. Maybe we
> should reject async updates when old_fb != new_fb in the rk
> ->async_check() hook.
Scanning out garbage because the old buffer is unpinned too quickly is one
of the long-term "features" of async updates. At least for features.
It's another one of these things we need to fix. Which might become easier
if we switch to usual state switching, since then we can punt the
cleanup_plane phase to a worker.
Note that depending upon the gpu this might not just result in garbage but
hangs, usually when there's an iommu and the chip dies if it accesses an
unmapped page.
Probably something to fix later on in async framework.
-Daniel
>
> > + */
> > + if (vop->is_enabled &&
> > + plane->state->fb && plane->state->fb != new_state->fb) {
> > drm_framebuffer_get(plane->state->fb);
> > WARN_ON(drm_crtc_vblank_get(plane->state->crtc) != 0);
> > drm_flip_work_queue(&vop->fb_unref_work, plane->state->fb);
> > set_bit(VOP_PENDING_FB_UNREF, &vop->pending);
> > }
>
> In any case, I think this should be called after
> vop_plane_atomic_update() to prevent the situation where the VBLANK
> event happens between this point and the following
> vop_plane_atomic_update() call.
>
> >
> > + plane->state->crtc_x = new_state->crtc_x;
> > + plane->state->crtc_y = new_state->crtc_y;
> > + plane->state->crtc_h = new_state->crtc_h;
> > + plane->state->crtc_w = new_state->crtc_w;
> > + plane->state->src_x = new_state->src_x;
> > + plane->state->src_y = new_state->src_y;
> > + plane->state->src_h = new_state->src_h;
> > + plane->state->src_w = new_state->src_w;
> > + plane->state->fb = new_state->fb;
>
> Any reason not to use swap() here and reference plane->state->fb
> instead of new_state->fb after this point?
>
> > +
> > if (vop->is_enabled) {
> > rockchip_drm_psr_inhibit_get_state(new_state->state);
> > vop_plane_atomic_update(plane, plane->state);
> > @@ -945,7 +946,12 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
> > rockchip_drm_psr_inhibit_put_state(new_state->state);
> > }
> >
> > - plane->funcs->atomic_destroy_state(plane, plane_state);
> > + /*
> > + * In async update we perform inplace modifications and release the
> > + * new_state. The following is required so we release the reference of
> > + * the old framebuffer.
> > + */
> > + new_state->fb = old_fb;
> > }
> >
> > static const struct drm_plane_helper_funcs plane_helper_funcs = {
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update
2019-03-12 6:34 ` Boris Brezillon
2019-03-12 11:04 ` Daniel Vetter
@ 2019-03-12 15:34 ` Helen Koike
2019-03-12 15:52 ` Boris Brezillon
1 sibling, 1 reply; 22+ messages in thread
From: Helen Koike @ 2019-03-12 15:34 UTC (permalink / raw)
To: Boris Brezillon
Cc: Stéphane Marchesin, Sean Paul, David Airlie, daniel.vetter,
linux-kernel, dri-devel, Tomasz Figa, linux-rockchip, kernel,
nicholas.kazlauskas, linux-arm-kernel
On 3/12/19 3:34 AM, Boris Brezillon wrote:
> On Mon, 11 Mar 2019 23:21:59 -0300
> Helen Koike <helen.koike@collabora.com> wrote:
>
>> In the case of async update, modifications are done in place, i.e. in the
>> current plane state, so the new_state is prepared and the new_state is
>> cleanup up (instead of the old_state, diferrently on what happen in a
>
> ^ cleaned up ^ differently (but maybe
> "unlike what happens" is more appropriate here).
>
>> normal sync update).
>> To cleanup the old_fb properly, it needs to be placed in the new_state
>> in the end of async_update, so cleanup call will unreference the old_fb
>> correctly.
>>
>> Also, the previous code had a:
>>
>> plane_state = plane->funcs->atomic_duplicate_state(plane);
>> ...
>> swap(plane_state, plane->state);
>>
>> if (plane->state->fb && plane->state->fb != new_state->fb) {
>> ...
>> }
>>
>> Which was wrong, as the fb were just assigned to be equal, so this if
>> statement nevers evaluates to true.
>>
>> Another details is that the function drm_crtc_vblank_get() can only be
>> called when vop->is_enabled is true, otherwise it has no effect and
>> trows a WARN_ON().
>>
>> Calling drm_atomic_set_fb_for_plane() (which get a referent of the new
>> fb and pus the old fb) is not required, as it is taken care by
>> drm_mode_cursor_universal() when calling
>> drm_atomic_helper_update_plane().
>>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>> Hello,
>>
>> I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and
>> kms_cursor_legacy and I didn't see any regressions.
>>
>> Changes in v2: None
>>
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++---------
>> 1 file changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index c7d4c6073ea5..a1ee8c156a7b 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
>> struct drm_plane_state *new_state)
>> {
>> struct vop *vop = to_vop(plane->state->crtc);
>> - struct drm_plane_state *plane_state;
>> + struct drm_framebuffer *old_fb = plane->state->fb;
>>
>> - plane_state = plane->funcs->atomic_duplicate_state(plane);
>> - plane_state->crtc_x = new_state->crtc_x;
>> - plane_state->crtc_y = new_state->crtc_y;
>> - plane_state->crtc_h = new_state->crtc_h;
>> - plane_state->crtc_w = new_state->crtc_w;
>> - plane_state->src_x = new_state->src_x;
>> - plane_state->src_y = new_state->src_y;
>> - plane_state->src_h = new_state->src_h;
>> - plane_state->src_w = new_state->src_w;
>> -
>> - if (plane_state->fb != new_state->fb)
>> - drm_atomic_set_fb_for_plane(plane_state, new_state->fb);
>> -
>> - swap(plane_state, plane->state);
>> -
>> - if (plane->state->fb && plane->state->fb != new_state->fb) {
>> + /*
>> + * A scanout can still be occurring, so we can't drop the reference to
>> + * the old framebuffer. To solve this we get a reference to old_fb and
>> + * set a worker to release it later.
>
> Hm, doesn't look like an async update to me if we have to wait for the
> next VBLANK to happen to get the new content on the screen. Maybe we
> should reject async updates when old_fb != new_fb in the rk
> ->async_check() hook.
Unless I am misunderstanding this, we don't wait here, we just grab a
reference to the fb in case it is being still used by the hw, so it
doesn't get released prematurely.
>
>> + */
>> + if (vop->is_enabled &&
>> + plane->state->fb && plane->state->fb != new_state->fb) {
>> drm_framebuffer_get(plane->state->fb);
>> WARN_ON(drm_crtc_vblank_get(plane->state->crtc) != 0);
>> drm_flip_work_queue(&vop->fb_unref_work, plane->state->fb);
>> set_bit(VOP_PENDING_FB_UNREF, &vop->pending);
>> }
>
> In any case, I think this should be called after
> vop_plane_atomic_update() to prevent the situation where the VBLANK
> event happens between this point and the following
> vop_plane_atomic_update() call.
ack, I'll update it in the next version.
>
>>
>> + plane->state->crtc_x = new_state->crtc_x;
>> + plane->state->crtc_y = new_state->crtc_y;
>> + plane->state->crtc_h = new_state->crtc_h;
>> + plane->state->crtc_w = new_state->crtc_w;
>> + plane->state->src_x = new_state->src_x;
>> + plane->state->src_y = new_state->src_y;
>> + plane->state->src_h = new_state->src_h;
>> + plane->state->src_w = new_state->src_w;
>> + plane->state->fb = new_state->fb;
>
> Any reason not to use swap() here and reference plane->state->fb
> instead of new_state->fb after this point?
I had the impression I had to do this in one of my tests, but re-testing
now and re-looking at the code this doesn't seem to be necessary. I'll
use a swap() in the next version.
Thanks for your feedback.
Helen
>
>> +
>> if (vop->is_enabled) {
>> rockchip_drm_psr_inhibit_get_state(new_state->state);
>> vop_plane_atomic_update(plane, plane->state);
>> @@ -945,7 +946,12 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
>> rockchip_drm_psr_inhibit_put_state(new_state->state);
>> }
>>
>> - plane->funcs->atomic_destroy_state(plane, plane_state);
>> + /*
>> + * In async update we perform inplace modifications and release the
>> + * new_state. The following is required so we release the reference of
>> + * the old framebuffer.
>> + */
>> + new_state->fb = old_fb;
>> }
>>
>> static const struct drm_plane_helper_funcs plane_helper_funcs = {
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update
2019-03-12 15:34 ` Helen Koike
@ 2019-03-12 15:52 ` Boris Brezillon
2019-03-13 3:42 ` Tomasz Figa
0 siblings, 1 reply; 22+ messages in thread
From: Boris Brezillon @ 2019-03-12 15:52 UTC (permalink / raw)
To: Helen Koike
Cc: Stéphane Marchesin, Sean Paul, David Airlie, daniel.vetter,
linux-kernel, dri-devel, Tomasz Figa, linux-rockchip, kernel,
nicholas.kazlauskas, linux-arm-kernel
On Tue, 12 Mar 2019 12:34:45 -0300
Helen Koike <helen.koike@collabora.com> wrote:
> On 3/12/19 3:34 AM, Boris Brezillon wrote:
> > On Mon, 11 Mar 2019 23:21:59 -0300
> > Helen Koike <helen.koike@collabora.com> wrote:
> >
> >> In the case of async update, modifications are done in place, i.e. in the
> >> current plane state, so the new_state is prepared and the new_state is
> >> cleanup up (instead of the old_state, diferrently on what happen in a
> >
> > ^ cleaned up ^ differently (but maybe
> > "unlike what happens" is more appropriate here).
> >
> >> normal sync update).
> >> To cleanup the old_fb properly, it needs to be placed in the new_state
> >> in the end of async_update, so cleanup call will unreference the old_fb
> >> correctly.
> >>
> >> Also, the previous code had a:
> >>
> >> plane_state = plane->funcs->atomic_duplicate_state(plane);
> >> ...
> >> swap(plane_state, plane->state);
> >>
> >> if (plane->state->fb && plane->state->fb != new_state->fb) {
> >> ...
> >> }
> >>
> >> Which was wrong, as the fb were just assigned to be equal, so this if
> >> statement nevers evaluates to true.
> >>
> >> Another details is that the function drm_crtc_vblank_get() can only be
> >> called when vop->is_enabled is true, otherwise it has no effect and
> >> trows a WARN_ON().
> >>
> >> Calling drm_atomic_set_fb_for_plane() (which get a referent of the new
> >> fb and pus the old fb) is not required, as it is taken care by
> >> drm_mode_cursor_universal() when calling
> >> drm_atomic_helper_update_plane().
> >>
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >> Hello,
> >>
> >> I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and
> >> kms_cursor_legacy and I didn't see any regressions.
> >>
> >> Changes in v2: None
> >>
> >> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++---------
> >> 1 file changed, 24 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >> index c7d4c6073ea5..a1ee8c156a7b 100644
> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >> @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
> >> struct drm_plane_state *new_state)
> >> {
> >> struct vop *vop = to_vop(plane->state->crtc);
> >> - struct drm_plane_state *plane_state;
> >> + struct drm_framebuffer *old_fb = plane->state->fb;
> >>
> >> - plane_state = plane->funcs->atomic_duplicate_state(plane);
> >> - plane_state->crtc_x = new_state->crtc_x;
> >> - plane_state->crtc_y = new_state->crtc_y;
> >> - plane_state->crtc_h = new_state->crtc_h;
> >> - plane_state->crtc_w = new_state->crtc_w;
> >> - plane_state->src_x = new_state->src_x;
> >> - plane_state->src_y = new_state->src_y;
> >> - plane_state->src_h = new_state->src_h;
> >> - plane_state->src_w = new_state->src_w;
> >> -
> >> - if (plane_state->fb != new_state->fb)
> >> - drm_atomic_set_fb_for_plane(plane_state, new_state->fb);
> >> -
> >> - swap(plane_state, plane->state);
> >> -
> >> - if (plane->state->fb && plane->state->fb != new_state->fb) {
> >> + /*
> >> + * A scanout can still be occurring, so we can't drop the reference to
> >> + * the old framebuffer. To solve this we get a reference to old_fb and
> >> + * set a worker to release it later.
> >
> > Hm, doesn't look like an async update to me if we have to wait for the
> > next VBLANK to happen to get the new content on the screen. Maybe we
> > should reject async updates when old_fb != new_fb in the rk
> > ->async_check() hook.
>
> Unless I am misunderstanding this, we don't wait here, we just grab a
> reference to the fb in case it is being still used by the hw, so it
> doesn't get released prematurely.
I was just reacting to the comment that says the new FB should stay
around until the next VBLANK event happens. If the FB must stay around
that probably means the HW is still using, which made me wonder if this
HW actually supports async update (where async means "update now and
don't care about about tearing"). Or maybe it takes some time to switch
to the new FB and waiting for the next VBLANK to release the old FB was
an easy solution to not wait for the flip to actually happen in
->async_update() (which is kind of a combination of async+non-blocking).
Anyway, let's keep it like that.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update
2019-03-12 15:52 ` Boris Brezillon
@ 2019-03-13 3:42 ` Tomasz Figa
2019-03-13 9:58 ` Michel Dänzer
0 siblings, 1 reply; 22+ messages in thread
From: Tomasz Figa @ 2019-03-13 3:42 UTC (permalink / raw)
To: Boris Brezillon
Cc: Stéphane Marchesin, Sean Paul, David Airlie, Daniel Vetter,
Linux Kernel Mailing List, dri-devel,
open list:ARM/Rockchip SoC...,
Helen Koike, kernel, nicholas.kazlauskas,
list@263.net:IOMMU DRIVERS
<iommu@lists.linux-foundation.org>,
Joerg Roedel <joro@8bytes.org>,
On Wed, Mar 13, 2019 at 12:52 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Tue, 12 Mar 2019 12:34:45 -0300
> Helen Koike <helen.koike@collabora.com> wrote:
>
> > On 3/12/19 3:34 AM, Boris Brezillon wrote:
> > > On Mon, 11 Mar 2019 23:21:59 -0300
> > > Helen Koike <helen.koike@collabora.com> wrote:
> > >
> > >> In the case of async update, modifications are done in place, i.e. in the
> > >> current plane state, so the new_state is prepared and the new_state is
> > >> cleanup up (instead of the old_state, diferrently on what happen in a
> > >
> > > ^ cleaned up ^ differently (but maybe
> > > "unlike what happens" is more appropriate here).
> > >
> > >> normal sync update).
> > >> To cleanup the old_fb properly, it needs to be placed in the new_state
> > >> in the end of async_update, so cleanup call will unreference the old_fb
> > >> correctly.
> > >>
> > >> Also, the previous code had a:
> > >>
> > >> plane_state = plane->funcs->atomic_duplicate_state(plane);
> > >> ...
> > >> swap(plane_state, plane->state);
> > >>
> > >> if (plane->state->fb && plane->state->fb != new_state->fb) {
> > >> ...
> > >> }
> > >>
> > >> Which was wrong, as the fb were just assigned to be equal, so this if
> > >> statement nevers evaluates to true.
> > >>
> > >> Another details is that the function drm_crtc_vblank_get() can only be
> > >> called when vop->is_enabled is true, otherwise it has no effect and
> > >> trows a WARN_ON().
> > >>
> > >> Calling drm_atomic_set_fb_for_plane() (which get a referent of the new
> > >> fb and pus the old fb) is not required, as it is taken care by
> > >> drm_mode_cursor_universal() when calling
> > >> drm_atomic_helper_update_plane().
> > >>
> > >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > >>
> > >> ---
> > >> Hello,
> > >>
> > >> I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and
> > >> kms_cursor_legacy and I didn't see any regressions.
> > >>
> > >> Changes in v2: None
> > >>
> > >> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++---------
> > >> 1 file changed, 24 insertions(+), 18 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > >> index c7d4c6073ea5..a1ee8c156a7b 100644
> > >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > >> @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
> > >> struct drm_plane_state *new_state)
> > >> {
> > >> struct vop *vop = to_vop(plane->state->crtc);
> > >> - struct drm_plane_state *plane_state;
> > >> + struct drm_framebuffer *old_fb = plane->state->fb;
> > >>
> > >> - plane_state = plane->funcs->atomic_duplicate_state(plane);
> > >> - plane_state->crtc_x = new_state->crtc_x;
> > >> - plane_state->crtc_y = new_state->crtc_y;
> > >> - plane_state->crtc_h = new_state->crtc_h;
> > >> - plane_state->crtc_w = new_state->crtc_w;
> > >> - plane_state->src_x = new_state->src_x;
> > >> - plane_state->src_y = new_state->src_y;
> > >> - plane_state->src_h = new_state->src_h;
> > >> - plane_state->src_w = new_state->src_w;
> > >> -
> > >> - if (plane_state->fb != new_state->fb)
> > >> - drm_atomic_set_fb_for_plane(plane_state, new_state->fb);
> > >> -
> > >> - swap(plane_state, plane->state);
> > >> -
> > >> - if (plane->state->fb && plane->state->fb != new_state->fb) {
> > >> + /*
> > >> + * A scanout can still be occurring, so we can't drop the reference to
> > >> + * the old framebuffer. To solve this we get a reference to old_fb and
> > >> + * set a worker to release it later.
> > >
> > > Hm, doesn't look like an async update to me if we have to wait for the
> > > next VBLANK to happen to get the new content on the screen. Maybe we
> > > should reject async updates when old_fb != new_fb in the rk
> > > ->async_check() hook.
> >
> > Unless I am misunderstanding this, we don't wait here, we just grab a
> > reference to the fb in case it is being still used by the hw, so it
> > doesn't get released prematurely.
>
> I was just reacting to the comment that says the new FB should stay
> around until the next VBLANK event happens. If the FB must stay around
> that probably means the HW is still using, which made me wonder if this
> HW actually supports async update (where async means "update now and
> don't care about about tearing"). Or maybe it takes some time to switch
> to the new FB and waiting for the next VBLANK to release the old FB was
> an easy solution to not wait for the flip to actually happen in
> ->async_update() (which is kind of a combination of async+non-blocking).
The hardware switches framebuffers on vblank, so whatever framebuffer
is currently being scanned out from needs to stay there until the
hardware switches to the new one in shadow registers. If that doesn't
happen, you get IOMMU faults and the display controller stops working
since we don't have any fault handling currently, just printing a
message.
Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update
2019-03-13 3:42 ` Tomasz Figa
@ 2019-03-13 9:58 ` Michel Dänzer
2019-03-13 18:08 ` Helen Koike
0 siblings, 1 reply; 22+ messages in thread
From: Michel Dänzer @ 2019-03-13 9:58 UTC (permalink / raw)
To: Tomasz Figa, Boris Brezillon
Cc: Stéphane Marchesin, Sean Paul, David Airlie, Daniel Vetter,
Linux Kernel Mailing List, dri-devel,
open list:ARM/Rockchip SoC...,
Helen Koike, list@263.net:IOMMU DRIVERS, kernel,
nicholas.kazlauskas, linux-arm-kernel
On 2019-03-13 4:42 a.m., Tomasz Figa wrote:
> On Wed, Mar 13, 2019 at 12:52 AM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
>>
>> On Tue, 12 Mar 2019 12:34:45 -0300
>> Helen Koike <helen.koike@collabora.com> wrote:
>>
>>> On 3/12/19 3:34 AM, Boris Brezillon wrote:
>>>> On Mon, 11 Mar 2019 23:21:59 -0300
>>>> Helen Koike <helen.koike@collabora.com> wrote:
>>>>
>>>>> In the case of async update, modifications are done in place, i.e. in the
>>>>> current plane state, so the new_state is prepared and the new_state is
>>>>> cleanup up (instead of the old_state, diferrently on what happen in a
>>>>
>>>> ^ cleaned up ^ differently (but maybe
>>>> "unlike what happens" is more appropriate here).
>>>>
>>>>> normal sync update).
>>>>> To cleanup the old_fb properly, it needs to be placed in the new_state
>>>>> in the end of async_update, so cleanup call will unreference the old_fb
>>>>> correctly.
>>>>>
>>>>> Also, the previous code had a:
>>>>>
>>>>> plane_state = plane->funcs->atomic_duplicate_state(plane);
>>>>> ...
>>>>> swap(plane_state, plane->state);
>>>>>
>>>>> if (plane->state->fb && plane->state->fb != new_state->fb) {
>>>>> ...
>>>>> }
>>>>>
>>>>> Which was wrong, as the fb were just assigned to be equal, so this if
>>>>> statement nevers evaluates to true.
>>>>>
>>>>> Another details is that the function drm_crtc_vblank_get() can only be
>>>>> called when vop->is_enabled is true, otherwise it has no effect and
>>>>> trows a WARN_ON().
>>>>>
>>>>> Calling drm_atomic_set_fb_for_plane() (which get a referent of the new
>>>>> fb and pus the old fb) is not required, as it is taken care by
>>>>> drm_mode_cursor_universal() when calling
>>>>> drm_atomic_helper_update_plane().
>>>>>
>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>>
>>>>> ---
>>>>> Hello,
>>>>>
>>>>> I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and
>>>>> kms_cursor_legacy and I didn't see any regressions.
>>>>>
>>>>> Changes in v2: None
>>>>>
>>>>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++---------
>>>>> 1 file changed, 24 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> index c7d4c6073ea5..a1ee8c156a7b 100644
>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
>>>>> struct drm_plane_state *new_state)
>>>>> {
>>>>> struct vop *vop = to_vop(plane->state->crtc);
>>>>> - struct drm_plane_state *plane_state;
>>>>> + struct drm_framebuffer *old_fb = plane->state->fb;
>>>>>
>>>>> - plane_state = plane->funcs->atomic_duplicate_state(plane);
>>>>> - plane_state->crtc_x = new_state->crtc_x;
>>>>> - plane_state->crtc_y = new_state->crtc_y;
>>>>> - plane_state->crtc_h = new_state->crtc_h;
>>>>> - plane_state->crtc_w = new_state->crtc_w;
>>>>> - plane_state->src_x = new_state->src_x;
>>>>> - plane_state->src_y = new_state->src_y;
>>>>> - plane_state->src_h = new_state->src_h;
>>>>> - plane_state->src_w = new_state->src_w;
>>>>> -
>>>>> - if (plane_state->fb != new_state->fb)
>>>>> - drm_atomic_set_fb_for_plane(plane_state, new_state->fb);
>>>>> -
>>>>> - swap(plane_state, plane->state);
>>>>> -
>>>>> - if (plane->state->fb && plane->state->fb != new_state->fb) {
>>>>> + /*
>>>>> + * A scanout can still be occurring, so we can't drop the reference to
>>>>> + * the old framebuffer. To solve this we get a reference to old_fb and
>>>>> + * set a worker to release it later.
>>>>
>>>> Hm, doesn't look like an async update to me if we have to wait for the
>>>> next VBLANK to happen to get the new content on the screen. Maybe we
>>>> should reject async updates when old_fb != new_fb in the rk
>>>> ->async_check() hook.
>>>
>>> Unless I am misunderstanding this, we don't wait here, we just grab a
>>> reference to the fb in case it is being still used by the hw, so it
>>> doesn't get released prematurely.
>>
>> I was just reacting to the comment that says the new FB should stay
>> around until the next VBLANK event happens. If the FB must stay around
>> that probably means the HW is still using, which made me wonder if this
>> HW actually supports async update (where async means "update now and
>> don't care about about tearing"). Or maybe it takes some time to switch
>> to the new FB and waiting for the next VBLANK to release the old FB was
>> an easy solution to not wait for the flip to actually happen in
>> ->async_update() (which is kind of a combination of async+non-blocking).
>
> The hardware switches framebuffers on vblank, so whatever framebuffer
> is currently being scanned out from needs to stay there until the
> hardware switches to the new one in shadow registers. If that doesn't
> happen, you get IOMMU faults and the display controller stops working
> since we don't have any fault handling currently, just printing a
> message.
Sounds like your hardware doesn't actually support async flips. It's
probably better for the driver not to pretend otherwise.
--
Earthling Michel Dänzer | https://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update
2019-03-13 9:58 ` Michel Dänzer
@ 2019-03-13 18:08 ` Helen Koike
[not found] ` <ed44e1f4-07da-b1cb-b5cb-d34d29758502-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
0 siblings, 1 reply; 22+ messages in thread
From: Helen Koike @ 2019-03-13 18:08 UTC (permalink / raw)
To: Michel Dänzer, Tomasz Figa, Boris Brezillon
Cc: Stéphane Marchesin, Sean Paul, David Airlie, Daniel Vetter,
Linux Kernel Mailing List, dri-devel,
open list:ARM/Rockchip SoC...,
kernel, nicholas.kazlauskas, list@263.net:IOMMU DRIVERS,
Joerg Roedel, linux-arm-kernel
On 3/13/19 6:58 AM, Michel Dänzer wrote:
> On 2019-03-13 4:42 a.m., Tomasz Figa wrote:
>> On Wed, Mar 13, 2019 at 12:52 AM Boris Brezillon
>> <boris.brezillon@collabora.com> wrote:
>>>
>>> On Tue, 12 Mar 2019 12:34:45 -0300
>>> Helen Koike <helen.koike@collabora.com> wrote:
>>>
>>>> On 3/12/19 3:34 AM, Boris Brezillon wrote:
>>>>> On Mon, 11 Mar 2019 23:21:59 -0300
>>>>> Helen Koike <helen.koike@collabora.com> wrote:
>>>>>
>>>>>> In the case of async update, modifications are done in place, i.e. in the
>>>>>> current plane state, so the new_state is prepared and the new_state is
>>>>>> cleanup up (instead of the old_state, diferrently on what happen in a
>>>>>
>>>>> ^ cleaned up ^ differently (but maybe
>>>>> "unlike what happens" is more appropriate here).
>>>>>
>>>>>> normal sync update).
>>>>>> To cleanup the old_fb properly, it needs to be placed in the new_state
>>>>>> in the end of async_update, so cleanup call will unreference the old_fb
>>>>>> correctly.
>>>>>>
>>>>>> Also, the previous code had a:
>>>>>>
>>>>>> plane_state = plane->funcs->atomic_duplicate_state(plane);
>>>>>> ...
>>>>>> swap(plane_state, plane->state);
>>>>>>
>>>>>> if (plane->state->fb && plane->state->fb != new_state->fb) {
>>>>>> ...
>>>>>> }
>>>>>>
>>>>>> Which was wrong, as the fb were just assigned to be equal, so this if
>>>>>> statement nevers evaluates to true.
>>>>>>
>>>>>> Another details is that the function drm_crtc_vblank_get() can only be
>>>>>> called when vop->is_enabled is true, otherwise it has no effect and
>>>>>> trows a WARN_ON().
>>>>>>
>>>>>> Calling drm_atomic_set_fb_for_plane() (which get a referent of the new
>>>>>> fb and pus the old fb) is not required, as it is taken care by
>>>>>> drm_mode_cursor_universal() when calling
>>>>>> drm_atomic_helper_update_plane().
>>>>>>
>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>>>
>>>>>> ---
>>>>>> Hello,
>>>>>>
>>>>>> I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and
>>>>>> kms_cursor_legacy and I didn't see any regressions.
>>>>>>
>>>>>> Changes in v2: None
>>>>>>
>>>>>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++---------
>>>>>> 1 file changed, 24 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>> index c7d4c6073ea5..a1ee8c156a7b 100644
>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>> @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
>>>>>> struct drm_plane_state *new_state)
>>>>>> {
>>>>>> struct vop *vop = to_vop(plane->state->crtc);
>>>>>> - struct drm_plane_state *plane_state;
>>>>>> + struct drm_framebuffer *old_fb = plane->state->fb;
>>>>>>
>>>>>> - plane_state = plane->funcs->atomic_duplicate_state(plane);
>>>>>> - plane_state->crtc_x = new_state->crtc_x;
>>>>>> - plane_state->crtc_y = new_state->crtc_y;
>>>>>> - plane_state->crtc_h = new_state->crtc_h;
>>>>>> - plane_state->crtc_w = new_state->crtc_w;
>>>>>> - plane_state->src_x = new_state->src_x;
>>>>>> - plane_state->src_y = new_state->src_y;
>>>>>> - plane_state->src_h = new_state->src_h;
>>>>>> - plane_state->src_w = new_state->src_w;
>>>>>> -
>>>>>> - if (plane_state->fb != new_state->fb)
>>>>>> - drm_atomic_set_fb_for_plane(plane_state, new_state->fb);
>>>>>> -
>>>>>> - swap(plane_state, plane->state);
>>>>>> -
>>>>>> - if (plane->state->fb && plane->state->fb != new_state->fb) {
>>>>>> + /*
>>>>>> + * A scanout can still be occurring, so we can't drop the reference to
>>>>>> + * the old framebuffer. To solve this we get a reference to old_fb and
>>>>>> + * set a worker to release it later.
>>>>>
>>>>> Hm, doesn't look like an async update to me if we have to wait for the
>>>>> next VBLANK to happen to get the new content on the screen. Maybe we
>>>>> should reject async updates when old_fb != new_fb in the rk
>>>>> ->async_check() hook.
>>>>
>>>> Unless I am misunderstanding this, we don't wait here, we just grab a
>>>> reference to the fb in case it is being still used by the hw, so it
>>>> doesn't get released prematurely.
>>>
>>> I was just reacting to the comment that says the new FB should stay
>>> around until the next VBLANK event happens. If the FB must stay around
>>> that probably means the HW is still using, which made me wonder if this
>>> HW actually supports async update (where async means "update now and
>>> don't care about about tearing"). Or maybe it takes some time to switch
>>> to the new FB and waiting for the next VBLANK to release the old FB was
>>> an easy solution to not wait for the flip to actually happen in
>>> ->async_update() (which is kind of a combination of async+non-blocking).
>>
>> The hardware switches framebuffers on vblank, so whatever framebuffer
>> is currently being scanned out from needs to stay there until the
>> hardware switches to the new one in shadow registers. If that doesn't
>> happen, you get IOMMU faults and the display controller stops working
>> since we don't have any fault handling currently, just printing a
>> message.
>
> Sounds like your hardware doesn't actually support async flips. It's
> probably better for the driver not to pretend otherwise.
>
>
I think wee need to clarify the meaning of the async_update callback
(and we should clarify it in the docs).
The way I understand what the async_update callback should do is: don't
block (i.e. don't wait for the next vblank), and update the hw state at
some point with the latest state from the last call to async_update.
Which means that: any driver can implement the async_update callback,
independently if it supports changing its state right away or not.
If hw supports, async_update can change the hw state right away, if not,
then changes will be applied in the next vblank (it can even amend the
pending commit if there is one).
With this, we can remove all the legacy cursor code to use the
async_update callback, since async_update can be called 100 times before
the next vblank, and the latest state will be set to the hw without
waiting 100 vblanks.
Please, let me know if this is your understanding as well. If not, then
we need to remodel things.
Thanks,
Helen
^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <20190312022204.2775-1-helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>]
* [PATCH v2 2/5] drm/amd: fix fb references in async update
[not found] ` <20190312022204.2775-1-helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
@ 2019-03-12 2:22 ` Helen Koike
0 siblings, 0 replies; 22+ messages in thread
From: Helen Koike @ 2019-03-12 2:22 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
nicholas.kazlauskas-5C7GfCeVMHo
Cc: andrey.grodzovsky-5C7GfCeVMHo, Stéphane Marchesin,
Sean Paul, David (ChunMing) Zhou, David Airlie,
daniel.vetter-/w4YWyX8dFk, David Francis, Mikita Lipski,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Tomasz Figa,
Bhawanpreet Lakha, Leo Li, Helen Koike,
boris.brezillon-ZGY8ohtN/8qB+jHODAdFcQ, Daniel Vetter,
Alex Deucher, kernel-ZGY8ohtN/8qB+jHODAdFcQ,
harry.wentland-5C7GfCeVMHo, Christian König, Anthony Koo
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().
Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Helen Koike <helen.koike@collabora.com>
Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
Changes in v2:
- added reviewed-by tag
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 3a6f595f295e..bc02800254bf 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3760,8 +3760,7 @@ static void dm_plane_atomic_async_update(struct drm_plane *plane,
struct drm_plane_state *old_state =
drm_atomic_get_old_plane_state(new_state->state, plane);
- if (plane->state->fb != new_state->fb)
- drm_atomic_set_fb_for_plane(plane->state, new_state->fb);
+ swap(plane->state->fb, new_state->fb);
plane->state->src_x = new_state->src_x;
plane->state->src_y = new_state->src_y;
--
2.20.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 3/5] drm/msm: fix fb references in async update
2019-03-12 2:21 [PATCH v2 0/5] drm: Fix fb changes for async updates Helen Koike
2019-03-12 2:21 ` [PATCH v2 1/5] drm/rockchip: fix fb references in async update Helen Koike
[not found] ` <20190312022204.2775-1-helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
@ 2019-03-12 2:22 ` Helen Koike
2019-03-12 2:22 ` [PATCH v2 4/5] drm/vc4: " Helen Koike
2019-03-12 2:22 ` [PATCH v2 5/5] drm: don't block fb changes for async plane updates Helen Koike
4 siblings, 0 replies; 22+ messages in thread
From: Helen Koike @ 2019-03-12 2:22 UTC (permalink / raw)
To: dri-devel, nicholas.kazlauskas
Cc: andrey.grodzovsky, daniel.vetter, linux-kernel, Tomasz Figa,
boris.brezillon, David Airlie, Sean Paul, kernel, harry.wentland,
Stéphane Marchesin, Helen Koike, stable, linux-arm-msm,
Ville Syrjälä,
Russell King, Mamta Shukla, robdclark, Maarten Lankhorst,
freedreno, Sean Paul, Daniel Vetter
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 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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 4/5] drm/vc4: fix fb references in async update
2019-03-12 2:21 [PATCH v2 0/5] drm: Fix fb changes for async updates Helen Koike
` (2 preceding siblings ...)
2019-03-12 2:22 ` [PATCH v2 3/5] drm/msm: " Helen Koike
@ 2019-03-12 2:22 ` Helen Koike
2019-03-25 0:38 ` Sasha Levin
2019-03-12 2:22 ` [PATCH v2 5/5] drm: don't block fb changes for async plane updates Helen Koike
4 siblings, 1 reply; 22+ messages in thread
From: Helen Koike @ 2019-03-12 2:22 UTC (permalink / raw)
To: dri-devel, nicholas.kazlauskas
Cc: andrey.grodzovsky, daniel.vetter, linux-kernel, Tomasz Figa,
boris.brezillon, David Airlie, Sean Paul, kernel, harry.wentland,
Stéphane Marchesin, Helen Koike, stable, eric,
Daniel Vetter
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 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 1babfeca0c92..1235e53b22a3 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -968,7 +968,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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/vc4: fix fb references in async update
2019-03-12 2:22 ` [PATCH v2 4/5] drm/vc4: " Helen Koike
@ 2019-03-25 0:38 ` Sasha Levin
0 siblings, 0 replies; 22+ messages in thread
From: Sasha Levin @ 2019-03-25 0:38 UTC (permalink / raw)
To: Sasha Levin, Helen Koike, dri-devel, nicholas.kazlauskas
Cc: stable, daniel.vetter
Hi,
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 539c320bfa97 drm/vc4: update cursors asynchronously through atomic.
The bot has tested the following trees: v5.0.3, v4.19.30.
v5.0.3: Build OK!
v4.19.30: Failed to apply! Possible dependencies:
1d4118ca165e ("drm/vc4: Rework the async update logic")
5a43911fd256 ("drm/vc4: Fix NULL pointer dereference in the async update path")
How should we proceed with this patch?
--
Thanks,
Sasha
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 5/5] drm: don't block fb changes for async plane updates
2019-03-12 2:21 [PATCH v2 0/5] drm: Fix fb changes for async updates Helen Koike
` (3 preceding siblings ...)
2019-03-12 2:22 ` [PATCH v2 4/5] drm/vc4: " Helen Koike
@ 2019-03-12 2:22 ` Helen Koike
2019-03-12 6:44 ` Boris Brezillon
4 siblings, 1 reply; 22+ messages in thread
From: Helen Koike @ 2019-03-12 2:22 UTC (permalink / raw)
To: dri-devel, nicholas.kazlauskas
Cc: andrey.grodzovsky, daniel.vetter, linux-kernel, Tomasz Figa,
boris.brezillon, David Airlie, Sean Paul, kernel, harry.wentland,
Stéphane Marchesin, Helen Koike, stable, Sean Paul,
Sandy Huang, linux-rockchip, linux-arm-msm, eric, robdclark,
amd-gfx, Heiko Stübner, Maarten Lankhorst, Daniel Vetter,
freedreno
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>
---
Hello,
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 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 | 20 ++++++++++----------
include/drm/drm_modeset_helper_vtables.h | 5 +++++
2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 540a77a2ade9..e7eb96f1efc2 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;
@@ -1657,6 +1648,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 +1659,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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] drm: don't block fb changes for async plane updates
2019-03-12 2:22 ` [PATCH v2 5/5] drm: don't block fb changes for async plane updates Helen Koike
@ 2019-03-12 6:44 ` Boris Brezillon
2019-03-12 12:49 ` Kazlauskas, Nicholas
0 siblings, 1 reply; 22+ messages in thread
From: Boris Brezillon @ 2019-03-12 6:44 UTC (permalink / raw)
To: Helen Koike
Cc: dri-devel, nicholas.kazlauskas, andrey.grodzovsky, daniel.vetter,
linux-kernel, Tomasz Figa, David Airlie, Sean Paul, kernel,
harry.wentland, Stéphane Marchesin, stable, Sean Paul,
Sandy Huang, linux-rockchip, linux-arm-msm, eric, robdclark,
amd-gfx, Heiko Stübner, Maarten Lankhorst, Daniel Vetter,
freedreno, linux-arm-kernel
On Mon, 11 Mar 2019 23:22:03 -0300
Helen Koike <helen.koike@collabora.com> 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+
> 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>
>
> ---
> Hello,
>
> 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 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 | 20 ++++++++++----------
> include/drm/drm_modeset_helper_vtables.h | 5 +++++
> 2 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 540a77a2ade9..e7eb96f1efc2 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;
> @@ -1657,6 +1648,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 +1659,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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] drm: don't block fb changes for async plane updates
2019-03-12 6:44 ` Boris Brezillon
@ 2019-03-12 12:49 ` Kazlauskas, Nicholas
0 siblings, 0 replies; 22+ messages in thread
From: Kazlauskas, Nicholas @ 2019-03-12 12:49 UTC (permalink / raw)
To: Boris Brezillon, Helen Koike
Cc: Sean Paul, David Airlie, daniel.vetter, dri-devel, kernel,
Maxime Ripard, amd-gfx, linux-rockchip, linux-arm-msm, Sean Paul,
linux-arm-kernel, Stéphane Marchesin, linux-kernel, stable,
Tomasz Figa, freedreno
On 3/12/19 2:44 AM, Boris Brezillon wrote:
> On Mon, 11 Mar 2019 23:22:03 -0300
> Helen Koike <helen.koike@collabora.com> 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+
>> 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>
I was thinking that the comment could go in async_commit or async_check,
but I guess it works there too. Maybe it needs a FIXME or a TODO for a
full state swap, but these are just nitpicks.
Nicholas Kazlauskas
>
>>
>> ---
>> Hello,
>>
>> 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 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 | 20 ++++++++++----------
>> include/drm/drm_modeset_helper_vtables.h | 5 +++++
>> 2 files changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 540a77a2ade9..e7eb96f1efc2 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;
>> @@ -1657,6 +1648,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 +1659,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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread