Re: [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 2/3/21 12:14 PM, Thomas Zimmermann wrote:
> 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@xxxxxxx>
>>>>
>>>> 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.

I must say I'm not entirely following this thread.

If I understand things correctly, there is some memory corruption
introduced by this patch, so there will be a v2 fixing this ?

The reason why I'm asking is because I always try to test vboxvideo patches
inside a vbox vm, but if a v2 is coming, there is not much use in me testing
this version, correct ?

Regards,

Hans



>>>>> ---
>>>>>    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
>>>
>>
>>
>>
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux