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

[Helen Koike]

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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> # v4.19+: 25dc194b34dd: drm: Block fb changes for async plane updates
>> Cc: <stable@xxxxxxxxxxxxxxx> # 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@xxxxxxxxxxxxx>
> 
> Other than that,
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>

Thanks!
Helen

> 
> Regards,
> 
> Boris
> 
>> Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
>>
>> ---
>> 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;
> 
>