From mboxrd@z Thu Jan 1 00:00:00 1970 From: Helen Koike Subject: Re: [PATCH 5/5] drm/vc4: fix fb references in async update Date: Mon, 11 Mar 2019 20:26:34 -0300 Message-ID: References: <20190304144909.6267-1-helen.koike@collabora.com> <20190304144909.6267-6-helen.koike@collabora.com> <20190311105642.44816e7b@collabora.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20190311105642.44816e7b@collabora.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Boris Brezillon Cc: dri-devel@lists.freedesktop.org, nicholas.kazlauskas@amd.com, andrey.grodzovsky@amd.com, daniel.vetter@ffwll.ch, linux-kernel@vger.kernel.org, Tomasz Figa , David Airlie , Sean Paul , kernel@collabora.com, harry.wentland@amd.com, =?UTF-8?Q?St=c3=a9phane_Marchesin?= , Eric Anholt List-Id: dri-devel@lists.freedesktop.org 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 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: # v4.19+: 25dc194b34dd: drm: Block fb changes for async plane updates >> Cc: # 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 > > Other than that, > > Reviewed-by: Boris Brezillon Thanks! Helen > > Regards, > > Boris > >> Signed-off-by: Helen Koike >> >> --- >> 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; > >