| Message ID | 20190304144909.6267-6-helen.koike@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fix fb changes for async updates | expand |
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; > >
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;
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(-)