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

Submitted by Helen Koike on March 4, 2019, 2:49 p.m.

Details

Message ID 20190304144909.6267-6-helen.koike@collabora.com
State Accepted
Commit c16b85559dcfb5a348cc085a7b4c75ed49b05e2c
Headers show
Series "drm: Fix fb changes for async updates" ( rev: 1 ) in DRI devel

Browsing this patch as part of:
"drm: Fix fb changes for async updates" rev 1 in DRI devel
<< prev patch [5/5] next patch >>

Commit Message

Helen Koike March 4, 2019, 2:49 p.m.
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+: 25dc194b34dd: drm: Block fb changes for async plane updates
Cc: <stable@vger.kernel.org> # v4.19+: 8105bbaf9afd: drm: don't block fb changes for async plane updates
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 on the rockchip and on i915 using igt plane_cursor_legacy and
kms_cursor_legacy and I didn't see any regressions.
But I couldn't test on VC4. I have a Raspberry pi model B rev2, when
FB_SIMPLE is running I can see output on the screen, but when vc4 is
loaded my hdmi display is not detected anymore, I am still debugging
this, probably some config in the firmware, but I would appreciate if
anyone could help me testing it.

Also the Cc statble commit hash dependency needs to be updated once the
refered commit is merged.

Thanks!
Helen

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

Patch hide | download patch | download mbox

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;

Comments

On 3/4/19 11:49 AM, Helen Koike wrote:
> 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+: 25dc194b34dd: drm: Block fb changes for async plane updates
> Cc: <stable@vger.kernel.org> # v4.19+: 8105bbaf9afd: drm: don't block fb changes for async plane updates
> 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 on the rockchip and on i915 using igt plane_cursor_legacy and
> kms_cursor_legacy and I didn't see any regressions.
> But I couldn't test on VC4. I have a Raspberry pi model B rev2, when
> FB_SIMPLE is running I can see output on the screen, but when vc4 is
> loaded my hdmi display is not detected anymore, I am still debugging
> this, probably some config in the firmware, but I would appreciate if
> anyone could help me testing it.

I managed to test on VC4, and the difference between the test results
with and without the patch is:

- cursor-vs-flip-toggle -> this test was getting a timeout and now is
failing due to:
completed 64 cursor updated in a period of 30 flips, we expect to
complete approximately 480 updates, with the threshold set at 240
Subtest cursor-vs-flip-toggle failed.
- flip-vs-cursor-toggle -> this was getting a timeout and now is passing
- short-flip-after-cursor-toggle -> this was failing and now is passing
- short-flip-before-cursor-toggle -> this was failing and now is passing

You can check the tests results before the patch series [1] and after
[2] in the links below:

[1] https://people.collabora.com/~koike/vc4-results-5.0.0-rc7+vanila/html/
[2] https://people.collabora.com/~koike/vc4-results-5.0.0-rc7+fb-patch/html/

I would appreciate is someone could review this patch.

Thanks!
Helen

> 
> Also the Cc statble commit hash dependency needs to be updated once the
> refered commit is merged.
> 
> Thanks!
> Helen
> 
>  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;
>
+Eric (the VC4 driver maintainer)

Hello Helen,

On Mon,  4 Mar 2019 11:49:09 -0300
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.
> 
> 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+: 25dc194b34dd: drm: Block fb changes for async plane updates
> Cc: <stable@vger.kernel.org> # v4.19+: 8105bbaf9afd: drm: don't block fb changes for async plane updates

Hm, the commit hash you give here will change when applied to the DRM
tree. I think there's a standard way to express dependencies between
patches that needs to be applied to stable, but I'm not sure you need
to describe that since Greg picks patches in the order they appear in
Linus' tree and those patches will be applied in the right order.

Another option if you want to keep things simple is to squash all
changes in a single patch ;).

> Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates")

Nitpicking: this Fixes tag is a bit of lie since you're actually fixing a
mistake that was introduced when async update support was added to VC4.
Commit 25dc194b34dd only added a new constraint to fix the initial
problem.

So I'd suggest:

Fixes: 539c320bfa97 ("drm/vc4: update cursors asynchronously through atomic")

BTW, the same applies to other patches in this series.

> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>

Other than that,

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Regards,

Boris

> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> Hello,
> 
> As mentioned in the cover letter,
> I tested on the rockchip and on i915 using igt plane_cursor_legacy and
> kms_cursor_legacy and I didn't see any regressions.
> But I couldn't test on VC4. I have a Raspberry pi model B rev2, when
> FB_SIMPLE is running I can see output on the screen, but when vc4 is
> loaded my hdmi display is not detected anymore, I am still debugging
> this, probably some config in the firmware, but I would appreciate if
> anyone could help me testing it.
> 
> Also the Cc statble commit hash dependency needs to be updated once the
> refered commit is merged.
> 
> Thanks!
> Helen
> 
>  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;
On 3/11/19 6:56 AM, Boris Brezillon wrote:
> +Eric (the VC4 driver maintainer)
> 
> Hello Helen,
> 
> On Mon,  4 Mar 2019 11:49:09 -0300
> 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.
>>
>> 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+: 25dc194b34dd: drm: Block fb changes for async plane updates
>> Cc: <stable@vger.kernel.org> # v4.19+: 8105bbaf9afd: drm: don't block fb changes for async plane updates
> 
> Hm, the commit hash you give here will change when applied to the DRM
> tree. I think there's a standard way to express dependencies between
> patches that needs to be applied to stable, but I'm not sure you need
> to describe that since Greg picks patches in the order they appear in
> Linus' tree and those patches will be applied in the right order.

right

> 
> Another option if you want to keep things simple is to squash all
> changes in a single patch ;).

I was thinking about that, but some of them don't need to be picked by
Greg (rockchip changes won't apply to stable for example), and I think
it's easier to get tested-by/reviewed-by tags if I separate them and
send them to the proper mailing list for the respective architecture.

> 
>> Fixes: 25dc194b34dd ("drm: Block fb changes for async plane updates")
> 
> Nitpicking: this Fixes tag is a bit of lie since you're actually fixing a
> mistake that was introduced when async update support was added to VC4.
> Commit 25dc194b34dd only added a new constraint to fix the initial
> problem.
> 
> So I'd suggest:
> 
> Fixes: 539c320bfa97 ("drm/vc4: update cursors asynchronously through atomic")
> 
> BTW, the same applies to other patches in this series.
> 
>> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Other than that,
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Thanks!
Helen

> 
> Regards,
> 
> Boris
> 
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>> Hello,
>>
>> As mentioned in the cover letter,
>> I tested on the rockchip and on i915 using igt plane_cursor_legacy and
>> kms_cursor_legacy and I didn't see any regressions.
>> But I couldn't test on VC4. I have a Raspberry pi model B rev2, when
>> FB_SIMPLE is running I can see output on the screen, but when vc4 is
>> loaded my hdmi display is not detected anymore, I am still debugging
>> this, probably some config in the firmware, but I would appreciate if
>> anyone could help me testing it.
>>
>> Also the Cc statble commit hash dependency needs to be updated once the
>> refered commit is merged.
>>
>> Thanks!
>> Helen
>>
>>  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;
> 
>