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? Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel