Thomas Zimmermann
2021-Jan-08 09:43 UTC
[PATCH v4 11/13] drm/vboxvideo: Use drm_gem_vram_vmap_local() in cursor update
Cursor updates in vboxvideo require a short-term mapping of the
source BO. Use drm_gem_vram_vmap_local() and avoid the pinning
operations.
Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
---
drivers/gpu/drm/vboxvideo/vbox_mode.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c
b/drivers/gpu/drm/vboxvideo/vbox_mode.c
index dbc0dd53c69e..215b37c78c10 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
@@ -381,7 +381,8 @@ 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]);
+ struct drm_gem_object *obj = fb->obj[0];
+ struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(obj);
u32 width = plane->state->crtc_w;
u32 height = plane->state->crtc_h;
size_t data_size, mask_size;
@@ -401,11 +402,12 @@ static void vbox_cursor_atomic_update(struct drm_plane
*plane,
vbox_crtc->cursor_enabled = true;
- ret = drm_gem_vram_vmap(gbo, &map);
+ ret = dma_resv_lock(obj->resv, NULL);
+ if (ret)
+ return;
+ ret = drm_gem_vram_vmap_local(gbo, &map);
if (ret) {
- /*
- * BUG: we should have pinned the BO in prepare_fb().
- */
+ dma_resv_unlock(obj->resv);
mutex_unlock(&vbox->hw_mutex);
DRM_WARN("Could not map cursor bo, skipping update\n");
return;
@@ -421,7 +423,8 @@ 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);
+ drm_gem_vram_vunmap_local(gbo, &map);
+ dma_resv_unlock(obj->resv);
flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE |
VBOX_MOUSE_POINTER_ALPHA;
--
2.29.2
Daniel Vetter
2021-Jan-11 17:06 UTC
[PATCH v4 11/13] drm/vboxvideo: Use drm_gem_vram_vmap_local() in cursor update
On Fri, Jan 08, 2021 at 10:43:38AM +0100, Thomas Zimmermann wrote:> Cursor updates in vboxvideo require a short-term mapping of the > source BO. Use drm_gem_vram_vmap_local() and avoid the pinning > operations. > > Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>All these drivers patches break the dma_resv_lock vs dma_fence_begin/end_signalling nesting rules, so this doesn't work. Generally this is what the prepare/cleanup_fb hooks are for, that's where mappings (including vmaps) are meant to be set up, permanently. I'm kinda not clear on why we need all these changes, I thought the locking problem is just in the fb helper paths, because it's outside of the atomic path and could conflict with an atomic update at the same time? So only that one should get the vmap_local treatment, everything else should keep the normal vmap treatment. -Daniel> --- > drivers/gpu/drm/vboxvideo/vbox_mode.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c > index dbc0dd53c69e..215b37c78c10 100644 > --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c > +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c > @@ -381,7 +381,8 @@ 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]); > + struct drm_gem_object *obj = fb->obj[0]; > + struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(obj); > u32 width = plane->state->crtc_w; > u32 height = plane->state->crtc_h; > size_t data_size, mask_size; > @@ -401,11 +402,12 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, > > vbox_crtc->cursor_enabled = true; > > - ret = drm_gem_vram_vmap(gbo, &map); > + ret = dma_resv_lock(obj->resv, NULL); > + if (ret) > + return; > + ret = drm_gem_vram_vmap_local(gbo, &map); > if (ret) { > - /* > - * BUG: we should have pinned the BO in prepare_fb(). > - */ > + dma_resv_unlock(obj->resv); > mutex_unlock(&vbox->hw_mutex); > DRM_WARN("Could not map cursor bo, skipping update\n"); > return; > @@ -421,7 +423,8 @@ 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); > + drm_gem_vram_vunmap_local(gbo, &map); > + dma_resv_unlock(obj->resv); > > flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE | > VBOX_MOUSE_POINTER_ALPHA; > -- > 2.29.2 >-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch