thr3ads.net - Linux Virtualization - [PATCH v4 11/13] drm/vboxvideo: Use drm_gem_vram_vmap

If this information is useful, please help other people find it:
Share via:

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

Thomas Zimmermann

2021-Jan-12 07:54 UTC

head link

[PATCH v4 11/13] drm/vboxvideo: Use drm_gem_vram_vmap_local() in cursor update

Hi

Am 11.01.21 um 18:06 schrieb Daniel Vetter:> 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.
Kind of responding to all your comment on the driver changes:

These drivers only require short-term mappings, so using vmap_local 
would be the natural choice. For SHMEM helpers, it's mostly a cosmetic 
thing. For VRAM helpers, I was hoping to remove the vmap/vunmap helpers 
entirely. One cannot really map the BOs for the long-term, so not having 
the helpers at all would make sense.

But reading all your comments on the driver patches, I'd rather not 
update the drivers here but later convert them to use 
prepare_fb/cleanup_fb in the correct way.

Best regards
Thomas
> -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
>>
> 
-- 
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:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210112/6f3c1022/attachment.sig>

Linux Virtualization - Jan 2021 - [PATCH v4 11/13] drm/vboxvideo: Use drm_gem_vram_vmap_local() in cursor update

[PATCH v4 11/13] drm/vboxvideo: Use drm_gem_vram_vmap_local() in cursor update

[PATCH v4 11/13] drm/vboxvideo: Use drm_gem_vram_vmap_local() in cursor update