From: Thomas Zimmermann <tzimmermann@suse.de>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: airlied@linux.ie, hdegoede@redhat.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
Date: Wed, 3 Feb 2021 12:14:38 +0100 [thread overview]
Message-ID: <74307294-2220-29d6-9d59-a73c2b8f43c6@suse.de> (raw)
In-Reply-To: <YBp+jxbjkdcpN1eo@phenom.ffwll.local>
[-- Attachment #1.1.1: Type: text/plain, Size: 8952 bytes --]
Hi
Am 03.02.21 um 11:44 schrieb Daniel Vetter:
> On Wed, Feb 03, 2021 at 11:34:21AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 03.02.21 um 11:29 schrieb Daniel Vetter:
>>> On Wed, Jan 27, 2021 at 03:05:03PM +0100, Thomas Zimmermann wrote:
>>>> Functions in the atomic commit tail are not allowed to acquire the
>>>> dmabuf's reservation lock. So we cannot legally call the GEM object's
>>>> vmap functionality in atomic_update.
>>>>
>>>> Instead vmap the cursor BO in prepare_fb and vunmap it in cleanup_fb.
>>>> The cursor plane state stores the mapping's address. The pinning of the
>>>> BO is implicitly done by vmap.
>>>>
>>>> As an extra benefit, there's no source of runtime errors left in
>>>> atomic_update.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>
>>> Did you test this with my dma_fence_signalling annotations patches?
>>
>> Not with vbox. I did test similar code in my recent ast patchset. But I
>> think there's still a bug here, as there's no custom plane-reset function.
>
> Do right, KASAN should complain when you load the driver because the first
> state is a bit too small. But probably still within the kmalloc'ed block
> by sheer luck. Worth confirming that KASAN can catch this.
I have KASAN enabled and I might just have missed the error message. I
later saw the error with another driver after I already posted the vbox
and ast patches.
If you have the time, a look at the first half of the ast patchset might
still be worth it. It removes the cursor-code abstraction and shouldn't
be affected by the issue.
Best regards
Thomas
>
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> I'll certainly send out an updated patch.
>
> I wonder whether it's worth to have a runtime check that when you
> overwrite one, you have to overwrite all of them or it's clearly buggy?
> -Daniel
>
>>
>> Best regards
>> Thomas
>>
>>>
>>>> ---
>>>> drivers/gpu/drm/vboxvideo/vbox_mode.c | 102 +++++++++++++++++++++-----
>>>> 1 file changed, 82 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
>>>> index dbc0dd53c69e..b5625a7d6cef 100644
>>>> --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
>>>> +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
>>>> @@ -324,6 +324,19 @@ static void vbox_primary_atomic_disable(struct drm_plane *plane,
>>>> old_state->src_y >> 16);
>>>> }
>>>> +struct vbox_cursor_plane_state {
>>>> + struct drm_plane_state base;
>>>> +
>>>> + /* Transitional state - do not export or duplicate */
>>>> +
>>>> + struct dma_buf_map map;
>>>> +};
>>>> +
>>>> +static struct vbox_cursor_plane_state *to_vbox_cursor_plane_state(struct drm_plane_state *state)
>>>> +{
>>>> + return container_of(state, struct vbox_cursor_plane_state, base);
>>>> +}
>>>> +
>>>> static int vbox_cursor_atomic_check(struct drm_plane *plane,
>>>> struct drm_plane_state *new_state)
>>>> {
>>>> @@ -381,14 +394,13 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
>>>> container_of(plane->dev, struct vbox_private, ddev);
>>>> struct vbox_crtc *vbox_crtc = to_vbox_crtc(plane->state->crtc);
>>>> struct drm_framebuffer *fb = plane->state->fb;
>>>> - struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(fb->obj[0]);
>>>> u32 width = plane->state->crtc_w;
>>>> u32 height = plane->state->crtc_h;
>>>> + struct vbox_cursor_plane_state *vbox_state = to_vbox_cursor_plane_state(plane->state);
>>>> + struct dma_buf_map map = vbox_state->map;
>>>> + u8 *src = map.vaddr; /* TODO: Use mapping abstraction properly */
>>>> size_t data_size, mask_size;
>>>> u32 flags;
>>>> - struct dma_buf_map map;
>>>> - int ret;
>>>> - u8 *src;
>>>> /*
>>>> * VirtualBox uses the host windowing system to draw the cursor so
>>>> @@ -401,17 +413,6 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
>>>> vbox_crtc->cursor_enabled = true;
>>>> - ret = drm_gem_vram_vmap(gbo, &map);
>>>> - if (ret) {
>>>> - /*
>>>> - * BUG: we should have pinned the BO in prepare_fb().
>>>> - */
>>>> - mutex_unlock(&vbox->hw_mutex);
>>>> - DRM_WARN("Could not map cursor bo, skipping update\n");
>>>> - return;
>>>> - }
>>>> - src = map.vaddr; /* TODO: Use mapping abstraction properly */
>>>> -
>>>> /*
>>>> * The mask must be calculated based on the alpha
>>>> * channel, one bit per ARGB word, and must be 32-bit
>>>> @@ -421,7 +422,6 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
>>>> data_size = width * height * 4 + mask_size;
>>>> copy_cursor_image(src, vbox->cursor_data, width, height, mask_size);
>>>> - drm_gem_vram_vunmap(gbo, &map);
>>>> flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE |
>>>> VBOX_MOUSE_POINTER_ALPHA;
>>>> @@ -458,6 +458,43 @@ static void vbox_cursor_atomic_disable(struct drm_plane *plane,
>>>> mutex_unlock(&vbox->hw_mutex);
>>>> }
>>>> +static int vbox_cursor_prepare_fb(struct drm_plane *plane, struct drm_plane_state *new_state)
>>>> +{
>>>> + struct vbox_cursor_plane_state *new_vbox_state = to_vbox_cursor_plane_state(new_state);
>>>> + struct drm_framebuffer *fb = new_state->fb;
>>>> + struct drm_gem_vram_object *gbo;
>>>> + struct dma_buf_map map;
>>>> + int ret;
>>>> +
>>>> + if (!fb)
>>>> + return 0;
>>>> +
>>>> + gbo = drm_gem_vram_of_gem(fb->obj[0]);
>>>> +
>>>> + ret = drm_gem_vram_vmap(gbo, &map);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + new_vbox_state->map = map;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void vbox_cursor_cleanup_fb(struct drm_plane *plane, struct drm_plane_state *old_state)
>>>> +{
>>>> + struct vbox_cursor_plane_state *old_vbox_state = to_vbox_cursor_plane_state(old_state);
>>>> + struct drm_framebuffer *fb = old_state->fb;
>>>> + struct dma_buf_map map = old_vbox_state->map;
>>>> + struct drm_gem_vram_object *gbo;
>>>> +
>>>> + if (!fb)
>>>> + return;
>>>> +
>>>> + gbo = drm_gem_vram_of_gem(fb->obj[0]);
>>>> +
>>>> + drm_gem_vram_vunmap(gbo, &map);
>>>> +}
>>>> +
>>>> static const u32 vbox_cursor_plane_formats[] = {
>>>> DRM_FORMAT_ARGB8888,
>>>> };
>>>> @@ -466,17 +503,42 @@ static const struct drm_plane_helper_funcs vbox_cursor_helper_funcs = {
>>>> .atomic_check = vbox_cursor_atomic_check,
>>>> .atomic_update = vbox_cursor_atomic_update,
>>>> .atomic_disable = vbox_cursor_atomic_disable,
>>>> - .prepare_fb = drm_gem_vram_plane_helper_prepare_fb,
>>>> - .cleanup_fb = drm_gem_vram_plane_helper_cleanup_fb,
>>>> + .prepare_fb = vbox_cursor_prepare_fb,
>>>> + .cleanup_fb = vbox_cursor_cleanup_fb,
>>>> };
>>>> +static struct drm_plane_state *vbox_cursor_atomic_duplicate_state(struct drm_plane *plane)
>>>> +{
>>>> + struct vbox_cursor_plane_state *new_vbox_state;
>>>> + struct drm_device *dev = plane->dev;
>>>> +
>>>> + if (drm_WARN_ON(dev, !plane->state))
>>>> + return NULL;
>>>> +
>>>> + new_vbox_state = kzalloc(sizeof(*new_vbox_state), GFP_KERNEL);
>>>> + if (!new_vbox_state)
>>>> + return NULL;
>>>> + __drm_atomic_helper_plane_duplicate_state(plane, &new_vbox_state->base);
>>>> +
>>>> + return &new_vbox_state->base;
>>>> +}
>>>> +
>>>> +static void vbox_cursor_atomic_destroy_state(struct drm_plane *plane,
>>>> + struct drm_plane_state *state)
>>>> +{
>>>> + struct vbox_cursor_plane_state *vbox_state = to_vbox_cursor_plane_state(state);
>>>> +
>>>> + __drm_atomic_helper_plane_destroy_state(&vbox_state->base);
>>>> + kfree(vbox_state);
>>>> +}
>>>> +
>>>> static const struct drm_plane_funcs vbox_cursor_plane_funcs = {
>>>> .update_plane = drm_atomic_helper_update_plane,
>>>> .disable_plane = drm_atomic_helper_disable_plane,
>>>> .destroy = drm_primary_helper_destroy,
>>>> .reset = drm_atomic_helper_plane_reset,
>>>> - .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>>>> - .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>>>> + .atomic_duplicate_state = vbox_cursor_atomic_duplicate_state,
>>>> + .atomic_destroy_state = vbox_cursor_atomic_destroy_state,
>>>> };
>>>> static const u32 vbox_primary_plane_formats[] = {
>>>>
>>>> base-commit: 3836b7fdfad40e2bac5dc882332f42bed6942cf4
>>>> prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
>>>> --
>>>> 2.30.0
>>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
>
>
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-02-03 11:14 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-27 14:05 [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb Thomas Zimmermann
2021-02-03 10:29 ` Daniel Vetter
2021-02-03 10:34 ` Thomas Zimmermann
2021-02-03 10:44 ` Daniel Vetter
2021-02-03 11:14 ` Thomas Zimmermann [this message]
2021-02-03 11:50 ` Hans de Goede
2021-02-03 11:56 ` Thomas Zimmermann
2021-02-08 13:54 ` Thomas Zimmermann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=74307294-2220-29d6-9d59-a73c2b8f43c6@suse.de \
--to=tzimmermann@suse.de \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=hdegoede@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).