On 2/13/19 5:18 PM, Nicholas Kazlauskas wrote:
> The prepare_fb call always happens on new_plane_state.
>
> The drm_atomic_helper_cleanup_planes checks to see if
> plane state pointer has changed when deciding to call cleanup_fb on
> either the new_plane_state or the old_plane_state.
>
> For a non-async atomic commit the state pointer is swapped, so this
> helper calls prepare_fb on the new_plane_state and cleanup_fb on the
> old_plane_state. This makes sense, since we want to prepare the
> framebuffer we are going to use and cleanup the the framebuffer we are
> no longer using.
>
> For the async atomic update helpers this differs. The async atomic
> update helpers perform in-place updates on the existing state. They call
> drm_atomic_helper_cleanup_planes but the state pointer is not swapped.
> This means that prepare_fb is called on the new_plane_state and
> cleanup_fb is called on the new_plane_state (not the old).
>
> In the case where old_plane_state->fb == new_plane_state->fb then
> there should be no behavioral difference between an async update
> and a non-async commit. But there are issues that arise when
> old_plane_state->fb != new_plane_state->fb.
>
> The first is that the new_plane_state->fb is immediately cleaned up
> after it has been prepared, so we're using a fb that we shouldn't
> be.
>
> The second occurs during a sequence of async atomic updates and
> non-async regular atomic commits. Suppose there are two framebuffers
> being interleaved in a double-buffering scenario, fb1 and fb2:
>
> - Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1
> - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2
> - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2
>
> We call cleanup_fb on fb2 twice in this example scenario, and any
> further use will result in use-after-free.
>
> The simple fix to this problem is to block framebuffer changes
> in the drm_atomic_helper_async_check function for now.
>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Harry Wentland <harry.wentland@xxxxxxx>
> Cc: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.14+
> Fixes: fef9df8b5945 ("drm/atomic: initial support for asynchronous plane update")
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 54e2ae614dcc..f4290f6b0c38 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1602,6 +1602,15 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> old_plane_state->crtc != new_plane_state->crtc)
> return -EINVAL;
>
> + /*
> + * FIXME: Since prepare_fb and cleanup_fb are always called on
> + * the new_plane_state for async updates we need to block framebuffer
> + * changes. This prevents use of a fb that's been cleaned up and
> + * double cleanups from occuring.
> + */
> + if (old_plane_state->fb != new_plane_state->fb)
> + return -EINVAL;
> +
> funcs = plane->helper_private;
> if (!funcs->atomic_async_update)
> return -EINVAL;
>
Hi,
Thank you for working on this.
I have two points I would like to raise:
1) cursor update is too slow:
I am working on the v8 of
https://lore.kernel.org/patchwork/patch/949264/ "drm/i915: update
cursors asynchronously through atomic".
But with this patch applied, the following tests fails:
cursor-vs-flip-atomic-transitions-varying-size
cursor-vs-flip-toggle
cursor-vs-flip-varying-size
with errors of type:
"CRITICAL: completed 97 cursor updated in a period of 30 flips, we
expect to complete approximately 15360 updates, with the threshold set
at 7680"
I guess it's because what should be async updates are being turned into
non-async updates (which is slower).
Well, I guess we need to work on a proper solution for the FIXME above,
this is just an observation/note.
2) I have another possibly related issue:
>From the tests I am making, unless I did some mistake or miss
interpretation (which is possible since I am new to drm), it seems that
there is a problem if an async update re-uses the same fb from a
previous non-async commit, for example:
- Non-async commit, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb2,
- Async commit, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1 #
double prepare fb1
The cursor through atomic patch was "solving" this by bypassing the
prepare call with a:
@@ -13495,6 +13503,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
struct drm_i915_gem_object *old_obj =
intel_fb_obj(plane->state->fb);
int ret;
+ if (new_state->state->async_update)
+ return 0;
+
if (old_obj) {
struct drm_crtc_state *crtc_state =
drm_atomic_get_new_crtc_state(new_state->state,
Which looks really wrong to me, but without it, igt tests
(plane_cursor_legacy, kms_cursor_legacy) fail, I get lots of
I915_VMA_PIN_OVERFLOW, which suggest double pinning the frame buffers.
I did this really ugly test to bypass the prepare call just if the
buffer is already pinned:
https://gitlab.collabora.com/koike/linux/commit/2d01d4af30347ff43fbac254d8ec305cd6fe4aeb
With this hack igt tests pass.
As I am new to DRM, I would appreciate if someone could confirm that
this is a real bug or please clarify if I am miss interpreting anything.
If this is a real bug, then if someone could give me pointers in how to
properly fix this it would be great.
Thanks a lot
Helen
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
