| Message ID | 20210108094340.15290-12-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Support short-term vmap via vmap_local | expand |
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 >
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 >> >
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 >
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 --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;
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(-)