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

Helen Koike
2019-03-11T23:26:48Z

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;
>
>