[PATCH] drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb
Thomas Zimmermann
tzimmermann at suse.de
Wed Feb 3 10:34:21 UTC 2021
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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210203/c7d0316e/attachment.sig>
More information about the dri-devel
mailing list