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