diff mbox series

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

Message ID 20210108094340.15290-12-tzimmermann@suse.de
State New
Headers show
Series drm: Support short-term vmap via vmap_local | expand

Commit Message

Thomas Zimmermann Jan. 8, 2021, 9:43 a.m. UTC
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>
---
 drivers/gpu/drm/vboxvideo/vbox_mode.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Daniel Vetter Jan. 11, 2021, 5:06 p.m. UTC | #1
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.
-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 Jan. 12, 2021, 7:54 a.m. UTC | #2
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.

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
>>
>
Daniel Vetter Jan. 12, 2021, 9:17 a.m. UTC | #3
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?
-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 Jan. 12, 2021, 9:53 a.m. UTC | #4
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
>>
> 
> 
> 
>
diff mbox series

Patch

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;