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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4446 bytes --]

Hi

Am 12.01.21 um 10:17 schrieb Daniel Vetter:
> On Tue, Jan 12, 2021 at 08:54:02AM +0100, Thomas Zimmermann wrote:
>> 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@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.
> 
> Ack from me on this plan. I think I got all the other patches with an r-b
> or ack?

The shmem patch needs an update from my side.

Best regards
Thomas

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]