* [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
@ 2021-01-27 14:05 Thomas Zimmermann
2021-02-03 10:29 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2021-01-27 14:05 UTC (permalink / raw)
To: hdegoede, airlied, daniel; +Cc: Thomas Zimmermann, dri-devel
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>
---
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
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
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2021-02-03 10:29 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: airlied, hdegoede, dri-devel
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?
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
2021-02-03 10:29 ` Daniel Vetter
@ 2021-02-03 10:34 ` Thomas Zimmermann
2021-02-03 10:44 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2021-02-03 10:34 UTC (permalink / raw)
To: Daniel Vetter; +Cc: airlied, hdegoede, dri-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 7473 bytes --]
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.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I'll certainly send out an updated patch.
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
[-- 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
2021-02-03 10:34 ` Thomas Zimmermann
@ 2021-02-03 10:44 ` Daniel Vetter
2021-02-03 11:14 ` Thomas Zimmermann
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2021-02-03 10:44 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: airlied, hdegoede, dri-devel
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.
> >
> > 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
2021-02-03 10:44 ` Daniel Vetter
@ 2021-02-03 11:14 ` Thomas Zimmermann
2021-02-03 11:50 ` Hans de Goede
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2021-02-03 11:14 UTC (permalink / raw)
To: Daniel Vetter; +Cc: airlied, hdegoede, dri-devel
[-- 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
2021-02-03 11:14 ` Thomas Zimmermann
@ 2021-02-03 11:50 ` Hans de Goede
2021-02-03 11:56 ` Thomas Zimmermann
2021-02-08 13:54 ` Thomas Zimmermann
0 siblings, 2 replies; 8+ messages in thread
From: Hans de Goede @ 2021-02-03 11:50 UTC (permalink / raw)
To: Thomas Zimmermann, Daniel Vetter; +Cc: airlied, dri-devel
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@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.
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
2021-02-03 11:50 ` Hans de Goede
@ 2021-02-03 11:56 ` Thomas Zimmermann
2021-02-08 13:54 ` Thomas Zimmermann
1 sibling, 0 replies; 8+ messages in thread
From: Thomas Zimmermann @ 2021-02-03 11:56 UTC (permalink / raw)
To: Hans de Goede, Daniel Vetter; +Cc: airlied, dri-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 10760 bytes --]
Hi
Am 03.02.21 um 12:50 schrieb Hans de Goede:
> 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@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.
>
> 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 ?
Yes, no point in testing ATM. There'll be a v2.
Best regards
Thomas
>
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
2021-02-03 11:50 ` Hans de Goede
2021-02-03 11:56 ` Thomas Zimmermann
@ 2021-02-08 13:54 ` Thomas Zimmermann
1 sibling, 0 replies; 8+ messages in thread
From: Thomas Zimmermann @ 2021-02-08 13:54 UTC (permalink / raw)
To: Hans de Goede, Daniel Vetter; +Cc: airlied, dri-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 9192 bytes --]
Hi Hans
Am 03.02.21 um 12:50 schrieb Hans de Goede:
> 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 ?
I just sent out v2 of the patchset. [1] It's now build upon DRM helpers,
so vboxvideo got slightly smaller. For testing, the latest drm-tip or
drm-misc-next is required.
Best regards
Thomas
[1]
https://lore.kernel.org/dri-devel/20210208135044.27827-1-tzimmermann@suse.de/T/#t
>
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-08 13:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2021-02-03 11:50 ` Hans de Goede
2021-02-03 11:56 ` Thomas Zimmermann
2021-02-08 13:54 ` Thomas Zimmermann
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).