Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

20 participants
2966 discussions

Re: [Linaro-mm-sig] [PATCH v4 04/13] drm/shmem-helper: Provide a vmap function for short-term mappings

by Daniel Vetter

On Tue, Jan 12, 2021 at 02:11:24PM +0100, Thomas Zimmermann wrote: > Hi > > Am 11.01.21 um 17:50 schrieb Daniel Vetter: > > On Fri, Jan 08, 2021 at 10:43:31AM +0100, Thomas Zimmermann wrote: > > > Implementations of the vmap/vunmap GEM callbacks may perform pinning > > > of the BO and may acquire the associated reservation object's lock. > > > Callers that only require a mapping of the contained memory can thus > > > interfere with other tasks that require exact pinning, such as scanout. > > > This is less of an issue with private SHMEM buffers, but may happen > > > with imported ones. > > > > > > Therefore provide the new interfaces drm_gem_shmem_vmap_local() and > > > drm_gem_shmem_vunmap_local(), which only perform the vmap/vunmap > > > operations. Callers have to hold the reservation lock while the mapping > > > persists. > > > > > > This patch also connects GEM SHMEM helpers to GEM object functions with > > > equivalent functionality. > > > > > > v4: > > > * call dma_buf_{vmap,vunmap}_local() where necessary (Daniel) > > > * move driver changes into separate patches (Daniel) > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > > > --- > > > drivers/gpu/drm/drm_gem_shmem_helper.c | 90 +++++++++++++++++++++++--- > > > include/drm/drm_gem_shmem_helper.h | 2 + > > > 2 files changed, 84 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > > index 9825c378dfa6..298832b2b43b 100644 > > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > > @@ -32,6 +32,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = { > > > .get_sg_table = drm_gem_shmem_get_sg_table, > > > .vmap = drm_gem_shmem_vmap, > > > .vunmap = drm_gem_shmem_vunmap, > > > + .vmap_local = drm_gem_shmem_vmap_local, > > > + .vunmap_local = drm_gem_shmem_vunmap_local, > > > .mmap = drm_gem_shmem_mmap, > > > }; > > > @@ -261,7 +263,8 @@ void drm_gem_shmem_unpin(struct drm_gem_object *obj) > > > } > > > EXPORT_SYMBOL(drm_gem_shmem_unpin); > > > -static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct dma_buf_map *map) > > > +static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct dma_buf_map *map, > > > + bool local) > > > > This is a bit spaghetti and also has the problem that we're not changing > > shmem->vmap_use_count under different locks, depending upon which path > > we're taking. > > > > I think the cleanest would be if we pull the if (import_attach) case out > > of the _locked() version completely, for all cases, and also outside of > > the shmem->vmap_lock. This means no caching of vmaps in the shmem layer > > anymore for imported buffers, but this is no longer a problem: We cache > > them in the exporters instead (I think at least, if not maybe need to fix > > that where it's expensive). > > If we do that, what protects shmem->vaddr from concurrent access near line > 281? would it be kept NULL then? > > Also, we have some stats in debugfs (see drm_gem_shmem_print_info) which > would be incorrect (or misleading at least). We'd need to disable all that for pass-through vmap of imported objects. > Given all that, would it be possible to remove vmap_lock in favor of taking > the resv lock in vmap/vunmap? All possible (and imo long-term desirable), the trouble is in rolling it out. I've looked at rolling out dma_resv as the one and only lock for shmem helpers before, and gave up. Exynos is the worst (but not the only) offender: - it has it's own per-object lock - that per-object lock is taken most often before calling into various vfuncs, which means for a gradual transition the dma_resv lock would nest within that existing per-object lock (until we've completely replaced it) - but exynos also uses dma_resv already as an outermost lock in its command submission path iow as soon as you add dma_resv_lock anywhere in shmem helpers, we've angered lockdep with a deadlock. That means the only path I think is feasible is adding dma_resv lock to all drivers paths first, _outside_ of any existing driver specific per-object locks. Then remove the driver-specific object locks, and only then can we sprinkle dma_resv_assert_locked all over shmem helpers. Ofc any driver without per-driver locks of their own could directly switch over to dma_resv lock, but until we've converted over all the drivers with their own locking shmem helpers would be stuck where they are right now. I gave up :-/ But maybe if you only try to tackle vmap it might be feasible, since a lot fewer callers. Cheers, Daniel > > Best regards > Thomas > > > > > Other option would be to unly pull it out for the _vmap_local case, but > > that's a bit ugly because no longer symmetrical in the various paths. > > > > > { > > > struct drm_gem_object *obj = &shmem->base; > > > int ret = 0; > > > @@ -272,7 +275,10 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct > > > } > > > if (obj->import_attach) { > > > - ret = dma_buf_vmap(obj->import_attach->dmabuf, map); > > > + if (local) > > > + ret = dma_buf_vmap_local(obj->import_attach->dmabuf, map); > > > + else > > > + ret = dma_buf_vmap(obj->import_attach->dmabuf, map); > > > if (!ret) { > > > if (WARN_ON(map->is_iomem)) { > > > ret = -EIO; > > > @@ -313,7 +319,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct > > > return ret; > > > } > > > -/* > > > +/** > > > * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object > > > * @shmem: shmem GEM object > > > * @map: Returns the kernel virtual address of the SHMEM GEM object's backing > > > @@ -339,15 +345,53 @@ int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) > > > ret = mutex_lock_interruptible(&shmem->vmap_lock); > > > if (ret) > > > return ret; > > > - ret = drm_gem_shmem_vmap_locked(shmem, map); > > > + ret = drm_gem_shmem_vmap_locked(shmem, map, false); > > > mutex_unlock(&shmem->vmap_lock); > > > return ret; > > > } > > > EXPORT_SYMBOL(drm_gem_shmem_vmap); > > > +/** > > > + * drm_gem_shmem_vmap_local - Create a virtual mapping for a shmem GEM object > > > + * @shmem: shmem GEM object > > > + * @map: Returns the kernel virtual address of the SHMEM GEM object's backing > > > + * store. > > > + * > > > + * This function makes sure that a contiguous kernel virtual address mapping > > > + * exists for the buffer backing the shmem GEM object. > > > + * > > > + * The function is called with the BO's reservation object locked. Callers must > > > + * hold the lock until after unmapping the buffer. > > > + * > > > + * This function can be used to implement &drm_gem_object_funcs.vmap_local. But > > > + * it can also be called by drivers directly, in which case it will hide the > > > + * differences between dma-buf imported and natively allocated objects. > > > > So for the other callbacks I tried to make sure we have different entry > > points for this, since it's not really the same thing and because of the > > locking mess we have with dma_resv_lock vs various pre-existing local > > locking scheme, it's easy to get a mess. > > > > I think the super clean version here would be to also export just the > > internal stuff for the ->v(un)map_local hooks, but that's maybe a bit too > > much boilerplate for no real gain. > > -Daniel > > > > > + * > > > + * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap_local(). > > > + * > > > + * Returns: > > > + * 0 on success or a negative error code on failure. > > > + */ > > > +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > > > +{ > > > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > > > + int ret; > > > + > > > + dma_resv_assert_held(obj->resv); > > > + > > > + ret = mutex_lock_interruptible(&shmem->vmap_lock); > > > + if (ret) > > > + return ret; > > > + ret = drm_gem_shmem_vmap_locked(shmem, map, true); > > > + mutex_unlock(&shmem->vmap_lock); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL(drm_gem_shmem_vmap_local); > > > + > > > static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > > > - struct dma_buf_map *map) > > > + struct dma_buf_map *map, bool local) > > > { > > > struct drm_gem_object *obj = &shmem->base; > > > @@ -358,7 +402,10 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > > > return; > > > if (obj->import_attach) > > > - dma_buf_vunmap(obj->import_attach->dmabuf, map); > > > + if (local) > > > + dma_buf_vunmap_local(obj->import_attach->dmabuf, map); > > > + else > > > + dma_buf_vunmap(obj->import_attach->dmabuf, map); > > > else > > > vunmap(shmem->vaddr); > > > @@ -366,7 +413,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > > > drm_gem_shmem_put_pages(shmem); > > > } > > > -/* > > > +/** > > > * drm_gem_shmem_vunmap - Unmap a virtual mapping fo a shmem GEM object > > > * @shmem: shmem GEM object > > > * @map: Kernel virtual address where the SHMEM GEM object was mapped > > > @@ -384,11 +431,38 @@ void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map) > > > struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > > > mutex_lock(&shmem->vmap_lock); > > > - drm_gem_shmem_vunmap_locked(shmem, map); > > > + drm_gem_shmem_vunmap_locked(shmem, map, false); > > > mutex_unlock(&shmem->vmap_lock); > > > } > > > EXPORT_SYMBOL(drm_gem_shmem_vunmap); > > > +/** > > > + * drm_gem_shmem_vunmap_local - Unmap a virtual mapping fo a shmem GEM object > > > + * @shmem: shmem GEM object > > > + * @map: Kernel virtual address where the SHMEM GEM object was mapped > > > + * > > > + * This function cleans up a kernel virtual address mapping acquired by > > > + * drm_gem_shmem_vmap_local(). The mapping is only removed when the use count > > > + * drops to zero. > > > + * > > > + * The function is called with the BO's reservation object locked. > > > + * > > > + * This function can be used to implement &drm_gem_object_funcs.vmap_local. > > > + * But it can also be called by drivers directly, in which case it will hide > > > + * the differences between dma-buf imported and natively allocated objects. > > > + */ > > > +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > > > +{ > > > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > > > + > > > + dma_resv_assert_held(obj->resv); > > > + > > > + mutex_lock(&shmem->vmap_lock); > > > + drm_gem_shmem_vunmap_locked(shmem, map, true); > > > + mutex_unlock(&shmem->vmap_lock); > > > +} > > > +EXPORT_SYMBOL(drm_gem_shmem_vunmap_local); > > > + > > > struct drm_gem_shmem_object * > > > drm_gem_shmem_create_with_handle(struct drm_file *file_priv, > > > struct drm_device *dev, size_t size, > > > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > > > index 434328d8a0d9..3f59bdf749aa 100644 > > > --- a/include/drm/drm_gem_shmem_helper.h > > > +++ b/include/drm/drm_gem_shmem_helper.h > > > @@ -114,7 +114,9 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem); > > > int drm_gem_shmem_pin(struct drm_gem_object *obj); > > > void drm_gem_shmem_unpin(struct drm_gem_object *obj); > > > int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map); > > > +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > > > void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map); > > > +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > > > int drm_gem_shmem_madvise(struct drm_gem_object *obj, int madv); > > > -- > > > 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 6 months

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

by 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(a)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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 6 months

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

by 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(a)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

4 years, 6 months

Re: [Linaro-mm-sig] [PATCH v4 06/13] drm/cirrus: Use drm_gem_shmem_vmap_local() in damage handling

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:33AM +0100, Thomas Zimmermann wrote: > Damage handling in cirrus requires a short-term mapping of the source > BO. Use drm_gem_shmem_vmap_local(). > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Hm more possible errors that we don't report to userspace ... Why don't we vmap/vunmap these in prepare/cleanup_fb? Generally we'd want a long-term vmap here to make sure this all works nicely. Since it's nothing new, on this patch: Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > --- > drivers/gpu/drm/tiny/cirrus.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c > index a043e602199e..21cd7056d45f 100644 > --- a/drivers/gpu/drm/tiny/cirrus.c > +++ b/drivers/gpu/drm/tiny/cirrus.c > @@ -315,6 +315,7 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > struct drm_rect *rect) > { > struct cirrus_device *cirrus = to_cirrus(fb->dev); > + struct drm_gem_object *obj = fb->obj[0]; > struct dma_buf_map map; > void *vmap; > int idx, ret; > @@ -323,9 +324,12 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > if (!drm_dev_enter(&cirrus->dev, &idx)) > goto out; > > - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > + ret = dma_resv_lock(obj->resv, NULL); > if (ret) > goto out_dev_exit; > + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > + if (ret) > + goto out_dma_resv_unlock; > vmap = map.vaddr; /* TODO: Use mapping abstraction properly */ > > if (cirrus->cpp == fb->format->cpp[0]) > @@ -345,9 +349,11 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > else > WARN_ON_ONCE("cpp mismatch"); > > - drm_gem_shmem_vunmap(fb->obj[0], &map); > ret = 0; > > + drm_gem_shmem_vunmap_local(obj, &map); > +out_dma_resv_unlock: > + dma_resv_unlock(obj->resv); > out_dev_exit: > drm_dev_exit(idx); > out: > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 6 months

Re: [Linaro-mm-sig] [PATCH v4 07/13] drm/gm12u320: Use drm_gem_shmem_vmap_local() in damage handling

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:34AM +0100, Thomas Zimmermann wrote: > Damage handling in gm12u320 requires a short-term mapping of the source > BO. Use drm_gem_shmem_vmap_local(). > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > --- > drivers/gpu/drm/tiny/gm12u320.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c > index 33f65f4626e5..b0c6e350f2b3 100644 > --- a/drivers/gpu/drm/tiny/gm12u320.c > +++ b/drivers/gpu/drm/tiny/gm12u320.c > @@ -265,11 +265,16 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320) > y1 = gm12u320->fb_update.rect.y1; > y2 = gm12u320->fb_update.rect.y2; > > - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > + ret = dma_resv_lock(fb->obj[0]->resv, NULL); > if (ret) { > - GM12U320_ERR("failed to vmap fb: %d\n", ret); > + GM12U320_ERR("failed to reserve fb: %d\n", ret); > goto put_fb; > } > + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > + if (ret) { > + GM12U320_ERR("failed to vmap fb: %d\n", ret); > + goto unlock_resv; > + } > vaddr = map.vaddr; /* TODO: Use mapping abstraction properly */ > > if (fb->obj[0]->import_attach) { > @@ -321,8 +326,11 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320) > if (ret) > GM12U320_ERR("dma_buf_end_cpu_access err: %d\n", ret); > } > + > +unlock_resv: > + dma_resv_unlock(fb->obj[0]->resv); Unlock before vunmap. -Daniel > vunmap: > - drm_gem_shmem_vunmap(fb->obj[0], &map); > + drm_gem_shmem_vunmap_local(fb->obj[0], &map); > put_fb: > drm_framebuffer_put(fb); > gm12u320->fb_update.fb = NULL; > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 6 months

Re: [Linaro-mm-sig] [PATCH v4 05/13] drm/mgag200: Use drm_gem_shmem_vmap_local() in damage handling

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:32AM +0100, Thomas Zimmermann wrote: > Damage handling in mgag200 requires a short-term mapping of the source > BO. Use drm_gem_shmem_vmap_local(). > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > --- > drivers/gpu/drm/mgag200/mgag200_mode.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > index 1dfc42170059..a33e28d4c5e9 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > @@ -1552,22 +1552,32 @@ mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb, > struct drm_rect *clip) > { > struct drm_device *dev = &mdev->base; > + struct drm_gem_object *obj = fb->obj[0]; > struct dma_buf_map map; > void *vmap; > int ret; > > - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > + ret = dma_resv_lock(obj->resv, NULL); > if (drm_WARN_ON(dev, ret)) > - return; /* BUG: SHMEM BO should always be vmapped */ > + return; > + ret = drm_gem_shmem_vmap_local(obj, &map); > + if (drm_WARN_ON(dev, ret)) > + goto err_dma_resv_unlock; /* BUG: SHMEM BO should always be vmapped */ > vmap = map.vaddr; /* TODO: Use mapping abstraction properly */ > > drm_fb_memcpy_dstclip(mdev->vram, vmap, fb, clip); > > - drm_gem_shmem_vunmap(fb->obj[0], &map); > + drm_gem_shmem_vunmap_local(obj, &map); > + dma_resv_unlock(obj->resv); > > /* Always scanout image at VRAM offset 0 */ > mgag200_set_startadd(mdev, (u32)0); > mgag200_set_offset(mdev, fb); > + > + return; > + > +err_dma_resv_unlock: > + dma_resv_unlock(obj->resv); > } > > static void > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 6 months

Re: [Linaro-mm-sig] [PATCH v4 13/13] drm/vram-helper: Remove unused drm_gem_vram_{vmap, vunmap}()

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:40AM +0100, Thomas Zimmermann wrote: > VRAM-helper BO's cannot be exported, so calls for vmap and vunmap > can only come from the BO's drivers or a kernel client. These are > supposed use vmap_local functionality. ^to > > The vmap and vunmap operations in VRAM helpers are therefore unused > and can be removed. > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 98 --------------------------- > include/drm/drm_gem_vram_helper.h | 2 - > 2 files changed, 100 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index c7fba3a0758e..c7d166cd16df 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -379,72 +379,6 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo) > } > EXPORT_SYMBOL(drm_gem_vram_unpin); > > -/** > - * drm_gem_vram_vmap() - Pins and maps a GEM VRAM object into kernel address > - * space > - * @gbo: The GEM VRAM object to map > - * @map: Returns the kernel virtual address of the VRAM GEM object's backing > - * store. > - * > - * The vmap function pins a GEM VRAM object to its current location, either > - * system or video memory, and maps its buffer into kernel address space. > - * As pinned object cannot be relocated, you should avoid pinning objects > - * permanently. Call drm_gem_vram_vunmap() with the returned address to > - * unmap and unpin the GEM VRAM object. > - * > - * Returns: > - * 0 on success, or a negative error code otherwise. > - */ > -int drm_gem_vram_vmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map) > -{ > - int ret; > - > - ret = ttm_bo_reserve(&gbo->bo, true, false, NULL); > - if (ret) > - return ret; > - > - ret = drm_gem_vram_pin_locked(gbo, 0); > - if (ret) > - goto err_ttm_bo_unreserve; > - ret = drm_gem_vram_vmap_local(gbo, map); > - if (ret) > - goto err_drm_gem_vram_unpin_locked; > - > - ttm_bo_unreserve(&gbo->bo); > - > - return 0; > - > -err_drm_gem_vram_unpin_locked: > - drm_gem_vram_unpin_locked(gbo); > -err_ttm_bo_unreserve: > - ttm_bo_unreserve(&gbo->bo); > - return ret; > -} > -EXPORT_SYMBOL(drm_gem_vram_vmap); > - > -/** > - * drm_gem_vram_vunmap() - Unmaps and unpins a GEM VRAM object > - * @gbo: The GEM VRAM object to unmap > - * @map: Kernel virtual address where the VRAM GEM object was mapped > - * > - * A call to drm_gem_vram_vunmap() unmaps and unpins a GEM VRAM buffer. See > - * the documentation for drm_gem_vram_vmap() for more information. > - */ > -void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map) > -{ > - int ret; > - > - ret = ttm_bo_reserve(&gbo->bo, false, false, NULL); > - if (WARN_ONCE(ret, "ttm_bo_reserve_failed(): ret=%d\n", ret)) > - return; > - > - drm_gem_vram_vunmap_local(gbo, map); > - drm_gem_vram_unpin_locked(gbo); > - > - ttm_bo_unreserve(&gbo->bo); > -} > -EXPORT_SYMBOL(drm_gem_vram_vunmap); > - > /** > * drm_gem_vram_vmap_local() - Maps a GEM VRAM object into kernel address space > * @gbo: The GEM VRAM object to map > @@ -870,36 +804,6 @@ static void drm_gem_vram_object_unpin(struct drm_gem_object *gem) > drm_gem_vram_unpin(gbo); > } > > -/** > - * drm_gem_vram_object_vmap() - > - * Implements &struct drm_gem_object_funcs.vmap > - * @gem: The GEM object to map > - * @map: Returns the kernel virtual address of the VRAM GEM object's backing > - * store. > - * > - * Returns: > - * 0 on success, or a negative error code otherwise. > - */ > -static int drm_gem_vram_object_vmap(struct drm_gem_object *gem, struct dma_buf_map *map) > -{ > - struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); > - > - return drm_gem_vram_vmap(gbo, map); > -} > - > -/** > - * drm_gem_vram_object_vunmap() - > - * Implements &struct drm_gem_object_funcs.vunmap > - * @gem: The GEM object to unmap > - * @map: Kernel virtual address where the VRAM GEM object was mapped > - */ > -static void drm_gem_vram_object_vunmap(struct drm_gem_object *gem, struct dma_buf_map *map) > -{ > - struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); > - > - drm_gem_vram_vunmap(gbo, map); > -} > - > static int drm_gem_vram_object_vmap_local(struct drm_gem_object *gem, struct dma_buf_map *map) > { > struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); > @@ -922,8 +826,6 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = { > .free = drm_gem_vram_object_free, > .pin = drm_gem_vram_object_pin, > .unpin = drm_gem_vram_object_unpin, > - .vmap = drm_gem_vram_object_vmap, > - .vunmap = drm_gem_vram_object_vunmap, > .vmap_local = drm_gem_vram_object_vmap_local, > .vunmap_local = drm_gem_vram_object_vunmap_local, > .mmap = drm_gem_ttm_mmap, > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h > index bd6a60e7c22b..e571867f4069 100644 > --- a/include/drm/drm_gem_vram_helper.h > +++ b/include/drm/drm_gem_vram_helper.h > @@ -97,8 +97,6 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo); > s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo); > int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag); > int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo); > -int drm_gem_vram_vmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map); > -void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map); > int drm_gem_vram_vmap_local(struct drm_gem_vram_object *gbo, struct dma_buf_map *map); > void drm_gem_vram_vunmap_local(struct drm_gem_vram_object *gbo, struct dma_buf_map *map); > > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 6 months

Re: [Linaro-mm-sig] [PATCH v4 04/13] drm/shmem-helper: Provide a vmap function for short-term mappings

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:31AM +0100, Thomas Zimmermann wrote: > Implementations of the vmap/vunmap GEM callbacks may perform pinning > of the BO and may acquire the associated reservation object's lock. > Callers that only require a mapping of the contained memory can thus > interfere with other tasks that require exact pinning, such as scanout. > This is less of an issue with private SHMEM buffers, but may happen > with imported ones. > > Therefore provide the new interfaces drm_gem_shmem_vmap_local() and > drm_gem_shmem_vunmap_local(), which only perform the vmap/vunmap > operations. Callers have to hold the reservation lock while the mapping > persists. > > This patch also connects GEM SHMEM helpers to GEM object functions with > equivalent functionality. > > v4: > * call dma_buf_{vmap,vunmap}_local() where necessary (Daniel) > * move driver changes into separate patches (Daniel) > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 90 +++++++++++++++++++++++--- > include/drm/drm_gem_shmem_helper.h | 2 + > 2 files changed, 84 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 9825c378dfa6..298832b2b43b 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -32,6 +32,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = { > .get_sg_table = drm_gem_shmem_get_sg_table, > .vmap = drm_gem_shmem_vmap, > .vunmap = drm_gem_shmem_vunmap, > + .vmap_local = drm_gem_shmem_vmap_local, > + .vunmap_local = drm_gem_shmem_vunmap_local, > .mmap = drm_gem_shmem_mmap, > }; > > @@ -261,7 +263,8 @@ void drm_gem_shmem_unpin(struct drm_gem_object *obj) > } > EXPORT_SYMBOL(drm_gem_shmem_unpin); > > -static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct dma_buf_map *map) > +static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct dma_buf_map *map, > + bool local) This is a bit spaghetti and also has the problem that we're not changing shmem->vmap_use_count under different locks, depending upon which path we're taking. I think the cleanest would be if we pull the if (import_attach) case out of the _locked() version completely, for all cases, and also outside of the shmem->vmap_lock. This means no caching of vmaps in the shmem layer anymore for imported buffers, but this is no longer a problem: We cache them in the exporters instead (I think at least, if not maybe need to fix that where it's expensive). Other option would be to unly pull it out for the _vmap_local case, but that's a bit ugly because no longer symmetrical in the various paths. > { > struct drm_gem_object *obj = &shmem->base; > int ret = 0; > @@ -272,7 +275,10 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct > } > > if (obj->import_attach) { > - ret = dma_buf_vmap(obj->import_attach->dmabuf, map); > + if (local) > + ret = dma_buf_vmap_local(obj->import_attach->dmabuf, map); > + else > + ret = dma_buf_vmap(obj->import_attach->dmabuf, map); > if (!ret) { > if (WARN_ON(map->is_iomem)) { > ret = -EIO; > @@ -313,7 +319,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct > return ret; > } > > -/* > +/** > * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object > * @shmem: shmem GEM object > * @map: Returns the kernel virtual address of the SHMEM GEM object's backing > @@ -339,15 +345,53 @@ int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) > ret = mutex_lock_interruptible(&shmem->vmap_lock); > if (ret) > return ret; > - ret = drm_gem_shmem_vmap_locked(shmem, map); > + ret = drm_gem_shmem_vmap_locked(shmem, map, false); > mutex_unlock(&shmem->vmap_lock); > > return ret; > } > EXPORT_SYMBOL(drm_gem_shmem_vmap); > > +/** > + * drm_gem_shmem_vmap_local - Create a virtual mapping for a shmem GEM object > + * @shmem: shmem GEM object > + * @map: Returns the kernel virtual address of the SHMEM GEM object's backing > + * store. > + * > + * This function makes sure that a contiguous kernel virtual address mapping > + * exists for the buffer backing the shmem GEM object. > + * > + * The function is called with the BO's reservation object locked. Callers must > + * hold the lock until after unmapping the buffer. > + * > + * This function can be used to implement &drm_gem_object_funcs.vmap_local. But > + * it can also be called by drivers directly, in which case it will hide the > + * differences between dma-buf imported and natively allocated objects. So for the other callbacks I tried to make sure we have different entry points for this, since it's not really the same thing and because of the locking mess we have with dma_resv_lock vs various pre-existing local locking scheme, it's easy to get a mess. I think the super clean version here would be to also export just the internal stuff for the ->v(un)map_local hooks, but that's maybe a bit too much boilerplate for no real gain. -Daniel > + * > + * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap_local(). > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > +{ > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > + int ret; > + > + dma_resv_assert_held(obj->resv); > + > + ret = mutex_lock_interruptible(&shmem->vmap_lock); > + if (ret) > + return ret; > + ret = drm_gem_shmem_vmap_locked(shmem, map, true); > + mutex_unlock(&shmem->vmap_lock); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_gem_shmem_vmap_local); > + > static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > - struct dma_buf_map *map) > + struct dma_buf_map *map, bool local) > { > struct drm_gem_object *obj = &shmem->base; > > @@ -358,7 +402,10 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > return; > > if (obj->import_attach) > - dma_buf_vunmap(obj->import_attach->dmabuf, map); > + if (local) > + dma_buf_vunmap_local(obj->import_attach->dmabuf, map); > + else > + dma_buf_vunmap(obj->import_attach->dmabuf, map); > else > vunmap(shmem->vaddr); > > @@ -366,7 +413,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > drm_gem_shmem_put_pages(shmem); > } > > -/* > +/** > * drm_gem_shmem_vunmap - Unmap a virtual mapping fo a shmem GEM object > * @shmem: shmem GEM object > * @map: Kernel virtual address where the SHMEM GEM object was mapped > @@ -384,11 +431,38 @@ void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map) > struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > > mutex_lock(&shmem->vmap_lock); > - drm_gem_shmem_vunmap_locked(shmem, map); > + drm_gem_shmem_vunmap_locked(shmem, map, false); > mutex_unlock(&shmem->vmap_lock); > } > EXPORT_SYMBOL(drm_gem_shmem_vunmap); > > +/** > + * drm_gem_shmem_vunmap_local - Unmap a virtual mapping fo a shmem GEM object > + * @shmem: shmem GEM object > + * @map: Kernel virtual address where the SHMEM GEM object was mapped > + * > + * This function cleans up a kernel virtual address mapping acquired by > + * drm_gem_shmem_vmap_local(). The mapping is only removed when the use count > + * drops to zero. > + * > + * The function is called with the BO's reservation object locked. > + * > + * This function can be used to implement &drm_gem_object_funcs.vmap_local. > + * But it can also be called by drivers directly, in which case it will hide > + * the differences between dma-buf imported and natively allocated objects. > + */ > +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > +{ > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > + > + dma_resv_assert_held(obj->resv); > + > + mutex_lock(&shmem->vmap_lock); > + drm_gem_shmem_vunmap_locked(shmem, map, true); > + mutex_unlock(&shmem->vmap_lock); > +} > +EXPORT_SYMBOL(drm_gem_shmem_vunmap_local); > + > struct drm_gem_shmem_object * > drm_gem_shmem_create_with_handle(struct drm_file *file_priv, > struct drm_device *dev, size_t size, > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > index 434328d8a0d9..3f59bdf749aa 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -114,7 +114,9 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem); > int drm_gem_shmem_pin(struct drm_gem_object *obj); > void drm_gem_shmem_unpin(struct drm_gem_object *obj); > int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map); > +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map); > +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > > int drm_gem_shmem_madvise(struct drm_gem_object *obj, int madv); > > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 6 months

Re: [Linaro-mm-sig] [PATCH v4 01/13] dma-buf: Add vmap_local and vnumap_local operations

by Ruhl, Michael J

>-----Original Message----- >From: dri-devel <dri-devel-bounces(a)lists.freedesktop.org> On Behalf Of >Thomas Zimmermann >Sent: Friday, January 8, 2021 4:43 AM >To: sumit.semwal(a)linaro.org; christian.koenig(a)amd.com; >airlied(a)redhat.com; daniel(a)ffwll.ch; maarten.lankhorst(a)linux.intel.com; >mripard(a)kernel.org; kraxel(a)redhat.com; hdegoede(a)redhat.com; >sean(a)poorly.run; eric(a)anholt.net; sam(a)ravnborg.org >Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch>; dri-devel(a)lists.freedesktop.org; >virtualization(a)lists.linux-foundation.org; linaro-mm-sig(a)lists.linaro.org; >Thomas Zimmermann <tzimmermann(a)suse.de>; linux- >media(a)vger.kernel.org >Subject: [PATCH v4 01/13] dma-buf: Add vmap_local and vnumap_local >operations > >The existing dma-buf calls dma_buf_vmap() and dma_buf_vunmap() are >allowed to pin the buffer or acquire the buffer's reservation object >lock. > >This is a problem for callers that only require a short-term mapping >of the buffer without the pinning, or callers that have special locking >requirements. These may suffer from unnecessary overhead or interfere >with regular pin operations. > >The new interfaces dma_buf_vmap_local(), dma_buf_vunmapo_local(), and >their rsp callbacks in struct dma_buf_ops provide an alternative without >pinning or reservation locking. Callers are responsible for these >operations. > >v4: > * update documentation (Daniel) > >Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> >Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> >Suggested-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> >--- > drivers/dma-buf/dma-buf.c | 81 >+++++++++++++++++++++++++++++++++++++++ > include/linux/dma-buf.h | 34 ++++++++++++++++ > 2 files changed, 115 insertions(+) > >diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >index b8465243eca2..01f9c74d97fa 100644 >--- a/drivers/dma-buf/dma-buf.c >+++ b/drivers/dma-buf/dma-buf.c >@@ -1295,6 +1295,87 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, >struct dma_buf_map *map) > } > EXPORT_SYMBOL_GPL(dma_buf_vunmap); > >+/** >+ * dma_buf_vmap_local - Create virtual mapping for the buffer object into >kernel >+ * address space. >+ * @dmabuf: [in] buffer to vmap >+ * @map: [out] returns the vmap pointer >+ * >+ * Unlike dma_buf_vmap() this is a short term mapping and will not pin >+ * the buffer. The struct dma_resv for the @dmabuf must be locked until >+ * dma_buf_vunmap_local() is called. >+ * >+ * Returns: >+ * 0 on success, or a negative errno code otherwise. >+ */ >+int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map >*map) >+{ >+ struct dma_buf_map ptr; >+ int ret = 0; >+ >+ dma_buf_map_clear(map); >+ >+ if (WARN_ON(!dmabuf)) >+ return -EINVAL; >+ >+ dma_resv_assert_held(dmabuf->resv); >+ >+ if (!dmabuf->ops->vmap_local) >+ return -EINVAL; You are clearing the map, and then doing the above checks. Is it ok to change the map info and then exit on error? Mike >+ mutex_lock(&dmabuf->lock); >+ if (dmabuf->vmapping_counter) { >+ dmabuf->vmapping_counter++; >+ BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); >+ *map = dmabuf->vmap_ptr; >+ goto out_unlock; >+ } >+ >+ BUG_ON(dma_buf_map_is_set(&dmabuf->vmap_ptr)); >+ >+ ret = dmabuf->ops->vmap_local(dmabuf, &ptr); >+ if (WARN_ON_ONCE(ret)) >+ goto out_unlock; >+ >+ dmabuf->vmap_ptr = ptr; >+ dmabuf->vmapping_counter = 1; >+ >+ *map = dmabuf->vmap_ptr; >+ >+out_unlock: >+ mutex_unlock(&dmabuf->lock); >+ return ret; >+} >+EXPORT_SYMBOL_GPL(dma_buf_vmap_local); >+ >+/** >+ * dma_buf_vunmap_local - Unmap a vmap obtained by >dma_buf_vmap_local. >+ * @dmabuf: [in] buffer to vunmap >+ * @map: [in] vmap pointer to vunmap >+ * >+ * Release a mapping established with dma_buf_vmap_local(). >+ */ >+void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct >dma_buf_map *map) >+{ >+ if (WARN_ON(!dmabuf)) >+ return; >+ >+ dma_resv_assert_held(dmabuf->resv); >+ >+ BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); >+ BUG_ON(dmabuf->vmapping_counter == 0); >+ BUG_ON(!dma_buf_map_is_equal(&dmabuf->vmap_ptr, map)); >+ >+ mutex_lock(&dmabuf->lock); >+ if (--dmabuf->vmapping_counter == 0) { >+ if (dmabuf->ops->vunmap_local) >+ dmabuf->ops->vunmap_local(dmabuf, map); >+ dma_buf_map_clear(&dmabuf->vmap_ptr); >+ } >+ mutex_unlock(&dmabuf->lock); >+} >+EXPORT_SYMBOL_GPL(dma_buf_vunmap_local); >+ > #ifdef CONFIG_DEBUG_FS > static int dma_buf_debug_show(struct seq_file *s, void *unused) > { >diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >index 628681bf6c99..aeed754b5467 100644 >--- a/include/linux/dma-buf.h >+++ b/include/linux/dma-buf.h >@@ -264,6 +264,38 @@ struct dma_buf_ops { > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map >*map); >+ >+ /** >+ * @vmap_local: >+ * >+ * Creates a virtual mapping for the buffer into kernel address space. >+ * >+ * This callback establishes short-term mappings for situations where >+ * callers only use the buffer for a bounded amount of time; such as >+ * updates to the framebuffer or reading back contained information. >+ * In contrast to the regular @vmap callback, vmap_local does never >pin >+ * the buffer to a specific domain or acquire the buffer's reservation >+ * lock. >+ * >+ * This is called with the &dma_buf.resv object locked. Callers must >hold >+ * the lock until after removing the mapping with @vunmap_local. >+ * >+ * This callback is optional. >+ * >+ * Returns: >+ * >+ * 0 on success or a negative error code on failure. >+ */ >+ int (*vmap_local)(struct dma_buf *dmabuf, struct dma_buf_map >*map); >+ >+ /** >+ * @vunmap_local: >+ * >+ * Removes a virtual mapping that was established by @vmap_local. >+ * >+ * This callback is optional. >+ */ >+ void (*vunmap_local)(struct dma_buf *dmabuf, struct dma_buf_map >*map); > }; > > /** >@@ -501,4 +533,6 @@ int dma_buf_mmap(struct dma_buf *, struct >vm_area_struct *, > unsigned long); > int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map >*map); >+int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map >*map); >+void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct >dma_buf_map *map); > #endif /* __DMA_BUF_H__ */ >-- >2.29.2 > >_______________________________________________ >dri-devel mailing list >dri-devel(a)lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel

4 years, 6 months

Re: [Linaro-mm-sig] [PATCH v3 6/8] drm/shmem-helper: Provide a vmap function for short-term mappings

by Daniel Vetter

On Thu, Jan 7, 2021 at 11:28 AM Thomas Zimmermann <tzimmermann(a)suse.de> wrote: > > Hi Daniel > > Am 11.12.20 um 10:50 schrieb Daniel Vetter: > [...] > >> +/** > >> + * drm_gem_shmem_vmap_local - Create a virtual mapping for a shmem GEM object > >> + * @shmem: shmem GEM object > >> + * @map: Returns the kernel virtual address of the SHMEM GEM object's backing > >> + * store. > >> + * > >> + * This function makes sure that a contiguous kernel virtual address mapping > >> + * exists for the buffer backing the shmem GEM object. > >> + * > >> + * The function is called with the BO's reservation object locked. Callers must > >> + * hold the lock until after unmapping the buffer. > >> + * > >> + * This function can be used to implement &drm_gem_object_funcs.vmap_local. But > >> + * it can also be called by drivers directly, in which case it will hide the > >> + * differences between dma-buf imported and natively allocated objects. > >> + * > >> + * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap_local(). > >> + * > >> + * Returns: > >> + * 0 on success or a negative error code on failure. > >> + */ > >> +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > >> +{ > >> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > >> + int ret; > >> + > >> + dma_resv_assert_held(obj->resv); > >> + > >> + ret = mutex_lock_interruptible(&shmem->vmap_lock); > > > > This bites. You need to check for shmem->import_attach and call > > dma_buf_vmap_local directly here before you take any shmem helper locks. > > Real fix would be to replace both vmap_lock and pages_lock with dma_resv > > lock, but that's more work. Same for vunmap_local > > This comment confuses me. AFAICT vmap_lock protects vmap_use_count. Why > does this exact code work in drm_gem_shmem_vmap() but not in _local() ? You'd hold the dma_resv lock both inside and outside of the ->vmap_lock mutex. Which deadlocks. For the _vmap version we more or less mandate that the caller doens't hold the dma_resv lock already, hence why we can get away with that. -Daniel > Best regards > Thomas > > > > > With that fixed on the helper part of this patch: > > > > Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > > > > > >> + if (ret) > >> + return ret; > >> + ret = drm_gem_shmem_vmap_locked(shmem, map); > >> + mutex_unlock(&shmem->vmap_lock); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL(drm_gem_shmem_vmap_local); > >> + > >> static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > >> struct dma_buf_map *map) > >> { > >> @@ -366,7 +406,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > >> drm_gem_shmem_put_pages(shmem); > >> } > >> > >> -/* > >> +/** > >> * drm_gem_shmem_vunmap - Unmap a virtual mapping fo a shmem GEM object > >> * @shmem: shmem GEM object > >> * @map: Kernel virtual address where the SHMEM GEM object was mapped > >> @@ -389,6 +429,33 @@ void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map) > >> } > >> EXPORT_SYMBOL(drm_gem_shmem_vunmap); > >> > >> +/** > >> + * drm_gem_shmem_vunmap_local - Unmap a virtual mapping fo a shmem GEM object > >> + * @shmem: shmem GEM object > >> + * @map: Kernel virtual address where the SHMEM GEM object was mapped > >> + * > >> + * This function cleans up a kernel virtual address mapping acquired by > >> + * drm_gem_shmem_vmap_local(). The mapping is only removed when the use count > >> + * drops to zero. > >> + * > >> + * The function is called with the BO's reservation object locked. > >> + * > >> + * This function can be used to implement &drm_gem_object_funcs.vmap_local. > >> + * But it can also be called by drivers directly, in which case it will hide > >> + * the differences between dma-buf imported and natively allocated objects. > >> + */ > >> +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > >> +{ > >> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > >> + > >> + dma_resv_assert_held(obj->resv); > >> + > >> + mutex_lock(&shmem->vmap_lock); > >> + drm_gem_shmem_vunmap_locked(shmem, map); > >> + mutex_unlock(&shmem->vmap_lock); > >> +} > >> +EXPORT_SYMBOL(drm_gem_shmem_vunmap_local); > >> + > >> struct drm_gem_shmem_object * > >> drm_gem_shmem_create_with_handle(struct drm_file *file_priv, > >> struct drm_device *dev, size_t size, > >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > >> index 1dfc42170059..a33e28d4c5e9 100644 > >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > >> @@ -1552,22 +1552,32 @@ mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb, > >> struct drm_rect *clip) > >> { > >> struct drm_device *dev = &mdev->base; > >> + struct drm_gem_object *obj = fb->obj[0]; > >> struct dma_buf_map map; > >> void *vmap; > >> int ret; > >> > >> - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > >> + ret = dma_resv_lock(obj->resv, NULL); > >> if (drm_WARN_ON(dev, ret)) > >> - return; /* BUG: SHMEM BO should always be vmapped */ > >> + return; > >> + ret = drm_gem_shmem_vmap_local(obj, &map); > >> + if (drm_WARN_ON(dev, ret)) > >> + goto err_dma_resv_unlock; /* BUG: SHMEM BO should always be vmapped */ > >> vmap = map.vaddr; /* TODO: Use mapping abstraction properly */ > >> > >> drm_fb_memcpy_dstclip(mdev->vram, vmap, fb, clip); > >> > >> - drm_gem_shmem_vunmap(fb->obj[0], &map); > >> + drm_gem_shmem_vunmap_local(obj, &map); > >> + dma_resv_unlock(obj->resv); > >> > >> /* Always scanout image at VRAM offset 0 */ > >> mgag200_set_startadd(mdev, (u32)0); > >> mgag200_set_offset(mdev, fb); > >> + > >> + return; > >> + > >> +err_dma_resv_unlock: > >> + dma_resv_unlock(obj->resv); > >> } > >> > >> static void > >> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c > >> index 561c49d8657a..58c694964148 100644 > >> --- a/drivers/gpu/drm/tiny/cirrus.c > >> +++ b/drivers/gpu/drm/tiny/cirrus.c > >> @@ -315,6 +315,7 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > >> struct drm_rect *rect) > >> { > >> struct cirrus_device *cirrus = to_cirrus(fb->dev); > >> + struct drm_gem_object *obj = fb->obj[0]; > >> struct dma_buf_map map; > >> void *vmap; > >> int idx, ret; > >> @@ -323,9 +324,12 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > >> if (!drm_dev_enter(&cirrus->dev, &idx)) > >> goto out; > >> > >> - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > >> + ret = dma_resv_lock(obj->resv, NULL); > >> if (ret) > >> goto out_dev_exit; > >> + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > >> + if (ret) > >> + goto out_dma_resv_unlock; > >> vmap = map.vaddr; /* TODO: Use mapping abstraction properly */ > >> > >> if (cirrus->cpp == fb->format->cpp[0]) > >> @@ -345,9 +349,11 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > >> else > >> WARN_ON_ONCE("cpp mismatch"); > >> > >> - drm_gem_shmem_vunmap(fb->obj[0], &map); > >> ret = 0; > >> > >> + drm_gem_shmem_vunmap_local(obj, &map); > >> +out_dma_resv_unlock: > >> + dma_resv_unlock(obj->resv); > >> out_dev_exit: > >> drm_dev_exit(idx); > >> out: > >> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c > >> index 33f65f4626e5..b0c6e350f2b3 100644 > >> --- a/drivers/gpu/drm/tiny/gm12u320.c > >> +++ b/drivers/gpu/drm/tiny/gm12u320.c > >> @@ -265,11 +265,16 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320) > >> y1 = gm12u320->fb_update.rect.y1; > >> y2 = gm12u320->fb_update.rect.y2; > >> > >> - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > >> + ret = dma_resv_lock(fb->obj[0]->resv, NULL); > >> if (ret) { > >> - GM12U320_ERR("failed to vmap fb: %d\n", ret); > >> + GM12U320_ERR("failed to reserve fb: %d\n", ret); > >> goto put_fb; > >> } > >> + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > >> + if (ret) { > >> + GM12U320_ERR("failed to vmap fb: %d\n", ret); > >> + goto unlock_resv; > >> + } > >> vaddr = map.vaddr; /* TODO: Use mapping abstraction properly */ > >> > >> if (fb->obj[0]->import_attach) { > >> @@ -321,8 +326,11 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320) > >> if (ret) > >> GM12U320_ERR("dma_buf_end_cpu_access err: %d\n", ret); > >> } > >> + > >> +unlock_resv: > >> + dma_resv_unlock(fb->obj[0]->resv); > >> vunmap: > >> - drm_gem_shmem_vunmap(fb->obj[0], &map); > >> + drm_gem_shmem_vunmap_local(fb->obj[0], &map); > >> put_fb: > >> drm_framebuffer_put(fb); > >> gm12u320->fb_update.fb = NULL; > >> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c > >> index 9d34ec9d03f6..46b55b4d03c2 100644 > >> --- a/drivers/gpu/drm/udl/udl_modeset.c > >> +++ b/drivers/gpu/drm/udl/udl_modeset.c > >> @@ -290,14 +290,18 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> else if ((clip.x2 > fb->width) || (clip.y2 > fb->height)) > >> return -EINVAL; > >> > >> + ret = dma_resv_lock(fb->obj[0]->resv, NULL); > >> + if (ret) > >> + return ret; > >> + > >> if (import_attach) { > >> ret = dma_buf_begin_cpu_access(import_attach->dmabuf, > >> DMA_FROM_DEVICE); > >> if (ret) > >> - return ret; > >> + goto out_dma_resv_unlock; > >> } > >> > >> - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > >> + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > >> if (ret) { > >> DRM_ERROR("failed to vmap fb\n"); > >> goto out_dma_buf_end_cpu_access; > >> @@ -307,7 +311,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> urb = udl_get_urb(dev); > >> if (!urb) { > >> ret = -ENOMEM; > >> - goto out_drm_gem_shmem_vunmap; > >> + goto out_drm_gem_shmem_vunmap_local; > >> } > >> cmd = urb->transfer_buffer; > >> > >> @@ -320,7 +324,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> &cmd, byte_offset, dev_byte_offset, > >> byte_width); > >> if (ret) > >> - goto out_drm_gem_shmem_vunmap; > >> + goto out_drm_gem_shmem_vunmap_local; > >> } > >> > >> if (cmd > (char *)urb->transfer_buffer) { > >> @@ -336,8 +340,8 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> > >> ret = 0; > >> > >> -out_drm_gem_shmem_vunmap: > >> - drm_gem_shmem_vunmap(fb->obj[0], &map); > >> +out_drm_gem_shmem_vunmap_local: > >> + drm_gem_shmem_vunmap_local(fb->obj[0], &map); > >> out_dma_buf_end_cpu_access: > >> if (import_attach) { > >> tmp_ret = dma_buf_end_cpu_access(import_attach->dmabuf, > >> @@ -345,6 +349,8 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> if (tmp_ret && !ret) > >> ret = tmp_ret; /* only update ret if not set yet */ > >> } > >> +out_dma_resv_unlock: > >> + dma_resv_unlock(fb->obj[0]->resv); > >> > >> return ret; > >> } > >> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > >> index 434328d8a0d9..3f59bdf749aa 100644 > >> --- a/include/drm/drm_gem_shmem_helper.h > >> +++ b/include/drm/drm_gem_shmem_helper.h > >> @@ -114,7 +114,9 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem); > >> int drm_gem_shmem_pin(struct drm_gem_object *obj); > >> void drm_gem_shmem_unpin(struct drm_gem_object *obj); > >> int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map); > >> +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > >> void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map); > >> +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > >> > >> int drm_gem_shmem_madvise(struct drm_gem_object *obj, int madv); > >> > >> -- > >> 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 6 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig