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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190311105642.44816e7b@collabora.com>
Date:   Mon, 11 Mar 2019 10:56:42 +0100
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     Helen Koike <helen.koike@...labora.com>
Cc:     dri-devel@...ts.freedesktop.org, nicholas.kazlauskas@....com,
        andrey.grodzovsky@....com, daniel.vetter@...ll.ch,
        linux-kernel@...r.kernel.org, Tomasz Figa <tfiga@...omium.org>,
        David Airlie <airlied@...ux.ie>,
        Sean Paul <seanpaul@...gle.com>, kernel@...labora.com,
        harry.wentland@....com,
        Stéphane Marchesin <marcheu@...gle.com>,
        Eric Anholt <eric@...olt.net>
Subject: Re: [PATCH 5/5] drm/vc4: fix fb references in async update

+Eric (the VC4 driver maintainer)

Hello Helen,

On Mon,  4 Mar 2019 11:49:09 -0300
Helen Koike <helen.koike@...labora.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@...r.kernel.org> # v4.19+: 25dc194b34dd: drm: Block fb changes for async plane updates
> Cc: <stable@...r.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@...labora.com>

Other than that,

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

Regards,

Boris

> Signed-off-by: Helen Koike <helen.koike@...labora.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;