[PATCH v3 8/8] drm/fb-helper: Move BO locking from DRM client to fbdev damage worker
Daniel Vetter
daniel at ffwll.ch
Fri Dec 11 14:00:19 UTC 2020
On Fri, Dec 11, 2020 at 11:16:25AM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 11.12.20 um 11:01 schrieb Daniel Vetter:
> > On Wed, Dec 09, 2020 at 03:25:27PM +0100, Thomas Zimmermann wrote:
> > > Fbdev emulation has to lock the BO into place while flushing the shadow
> > > buffer into the BO's memory. Remove any interference with pinning by
> > > using vmap_local functionality (instead of full vmap). This requires
> > > BO reservation locking in fbdev's damage worker.
> > >
> > > The new DRM client functions for locking and vmap_local functionality
> > > are added for consistency with the existing style.
> > >
> > > Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> >
> > The old vmap/vunmap in the client library aren't used anymore, please
> > delete. That will also make it clearer in the diff what's going on and
> > that it makes sense to have the client and fb-helper part in one patch.
>
> They are still around for perma-mapped BOs where HW supports it (really only
> CMA-based drivers). See drm_fb_helper_generic_probe() and
> drm_fbdev_cleanup().
Ah right I didn't grep this carefully enough. I guess in that case
splitting this into the drm_client patch and fb-helper patch would be good
I think.
Also some kerneldoc commment addition for normal vmap that vmap_local is
preferred for short-term mappings, and for vmap_local that _vmap() gives
you the long term mapping. Just for more links and stuff in docs.
With that: Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
-Daniel
>
> Best regards
> Thomas
>
> >
> > With that: Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> >
> > > ---
> > > drivers/gpu/drm/drm_client.c | 91 +++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/drm_fb_helper.c | 41 +++++++--------
> > > include/drm/drm_client.h | 4 ++
> > > 3 files changed, 116 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> > > index ce45e380f4a2..795f5cb052ba 100644
> > > --- a/drivers/gpu/drm/drm_client.c
> > > +++ b/drivers/gpu/drm/drm_client.c
> > > @@ -288,6 +288,37 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
> > > return ERR_PTR(ret);
> > > }
> > > +/**
> > > + * drm_client_buffer_lock - Locks the DRM client buffer
> > > + * @buffer: DRM client buffer
> > > + *
> > > + * This function locks the client buffer by acquiring the buffer
> > > + * object's reservation lock.
> > > + *
> > > + * Unlock the buffer with drm_client_buffer_unlock().
> > > + *
> > > + * Returns:
> > > + * 0 on success, or a negative errno code otherwise.
> > > + */
> > > +int
> > > +drm_client_buffer_lock(struct drm_client_buffer *buffer)
> > > +{
> > > + return dma_resv_lock(buffer->gem->resv, NULL);
> > > +}
> > > +EXPORT_SYMBOL(drm_client_buffer_lock);
> > > +
> > > +/**
> > > + * drm_client_buffer_unlock - Unlock DRM client buffer
> > > + * @buffer: DRM client buffer
> > > + *
> > > + * Unlocks a client buffer. See drm_client_buffer_lock().
> > > + */
> > > +void drm_client_buffer_unlock(struct drm_client_buffer *buffer)
> > > +{
> > > + dma_resv_unlock(buffer->gem->resv);
> > > +}
> > > +EXPORT_SYMBOL(drm_client_buffer_unlock);
> > > +
> > > /**
> > > * drm_client_buffer_vmap - Map DRM client buffer into address space
> > > * @buffer: DRM client buffer
> > > @@ -348,6 +379,66 @@ void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
> > > }
> > > EXPORT_SYMBOL(drm_client_buffer_vunmap);
> > > +/**
> > > + * drm_client_buffer_vmap_local - Map DRM client buffer into address space
> > > + * @buffer: DRM client buffer
> > > + * @map_copy: Returns the mapped memory's address
> > > + *
> > > + * This function maps a client buffer into kernel address space. If the
> > > + * buffer is already mapped, it returns the existing mapping's address.
> > > + *
> > > + * Client buffer mappings are not ref'counted. Each call to
> > > + * drm_client_buffer_vmap_local() should be followed by a call to
> > > + * drm_client_buffer_vunmap_local(); or the client buffer should be mapped
> > > + * throughout its lifetime.
> > > + *
> > > + * The returned address is a copy of the internal value. In contrast to
> > > + * other vmap interfaces, you don't need it for the client's vunmap
> > > + * function. So you can modify it at will during blit and draw operations.
> > > + *
> > > + * Returns:
> > > + * 0 on success, or a negative errno code otherwise.
> > > + */
> > > +int
> > > +drm_client_buffer_vmap_local(struct drm_client_buffer *buffer, struct dma_buf_map *map_copy)
> > > +{
> > > + struct dma_buf_map *map = &buffer->map;
> > > + int ret;
> > > +
> > > + /*
> > > + * FIXME: The dependency on GEM here isn't required, we could
> > > + * convert the driver handle to a dma-buf instead and use the
> > > + * backend-agnostic dma-buf vmap_local support instead. This would
> > > + * require that the handle2fd prime ioctl is reworked to pull the
> > > + * fd_install step out of the driver backend hooks, to make that
> > > + * final step optional for internal users.
> > > + */
> > > + ret = drm_gem_vmap_local(buffer->gem, map);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + *map_copy = *map;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_client_buffer_vmap_local);
> > > +
> > > +/**
> > > + * drm_client_buffer_vunmap_local - Unmap DRM client buffer
> > > + * @buffer: DRM client buffer
> > > + *
> > > + * This function removes a client buffer's memory mapping. Calling this
> > > + * function is only required by clients that manage their buffer mappings
> > > + * by themselves.
> > > + */
> > > +void drm_client_buffer_vunmap_local(struct drm_client_buffer *buffer)
> > > +{
> > > + struct dma_buf_map *map = &buffer->map;
> > > +
> > > + drm_gem_vunmap_local(buffer->gem, map);
> > > +}
> > > +EXPORT_SYMBOL(drm_client_buffer_vunmap_local);
> > > +
> > > static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
> > > {
> > > int ret;
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index e82db0f4e771..a56a7d9f7e35 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -399,28 +399,34 @@ static int drm_fb_helper_damage_blit(struct drm_fb_helper *fb_helper,
> > > int ret;
> > > /*
> > > - * We have to pin the client buffer to its current location while
> > > - * flushing the shadow buffer. In the general case, concurrent
> > > - * modesetting operations could try to move the buffer and would
> > > - * fail. The modeset has to be serialized by acquiring the reservation
> > > - * object of the underlying BO here.
> > > - *
> > > * For fbdev emulation, we only have to protect against fbdev modeset
> > > * operations. Nothing else will involve the client buffer's BO. So it
> > > * is sufficient to acquire struct drm_fb_helper.lock here.
> > > */
> > > mutex_lock(&fb_helper->lock);
> > > - ret = drm_client_buffer_vmap(buffer, &map);
> > > + /*
> > > + * We have to keep the client buffer at its current location while
> > > + * flushing the shadow buffer. Concurrent operations could otherwise
> > > + * try to move the buffer. Therefore acquiring the reservation
> > > + * object of the underlying BO here.
> > > + */
> > > + ret = drm_client_buffer_lock(buffer);
> > > + if (ret)
> > > + goto out_mutex_unlock;
> > > +
> > > + ret = drm_client_buffer_vmap_local(buffer, &map);
> > > if (ret)
> > > - goto out;
> > > + goto out_drm_client_buffer_unlock;
> > > dst = map;
> > > drm_fb_helper_damage_blit_real(fb_helper, clip, &dst);
> > > - drm_client_buffer_vunmap(buffer);
> > > + drm_client_buffer_vunmap_local(buffer);
> > > -out:
> > > +out_drm_client_buffer_unlock:
> > > + drm_client_buffer_unlock(buffer);
> > > +out_mutex_unlock:
> > > mutex_unlock(&fb_helper->lock);
> > > return ret;
> > > @@ -946,15 +952,11 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
> > > drm_modeset_lock_all(fb_helper->dev);
> > > drm_client_for_each_modeset(modeset, &fb_helper->client) {
> > > crtc = modeset->crtc;
> > > - if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > + if (!crtc->funcs->gamma_set || !crtc->gamma_size)
> > > + return -EINVAL;
> > > - if (cmap->start + cmap->len > crtc->gamma_size) {
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > + if (cmap->start + cmap->len > crtc->gamma_size)
> > > + return -EINVAL;
> > > r = crtc->gamma_store;
> > > g = r + crtc->gamma_size;
> > > @@ -967,9 +969,8 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
> > > ret = crtc->funcs->gamma_set(crtc, r, g, b,
> > > crtc->gamma_size, NULL);
> > > if (ret)
> > > - goto out;
> > > + return ret;
> > > }
> > > -out:
> > > drm_modeset_unlock_all(fb_helper->dev);
> > > return ret;
> > > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> > > index f07f2fb02e75..df61e339a11c 100644
> > > --- a/include/drm/drm_client.h
> > > +++ b/include/drm/drm_client.h
> > > @@ -156,8 +156,12 @@ struct drm_client_buffer *
> > > drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format);
> > > void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
> > > int drm_client_framebuffer_flush(struct drm_client_buffer *buffer, struct drm_rect *rect);
> > > +int drm_client_buffer_lock(struct drm_client_buffer *buffer);
> > > +void drm_client_buffer_unlock(struct drm_client_buffer *buffer);
> > > int drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map *map);
> > > void drm_client_buffer_vunmap(struct drm_client_buffer *buffer);
> > > +int drm_client_buffer_vmap_local(struct drm_client_buffer *buffer, struct dma_buf_map *map);
> > > +void drm_client_buffer_vunmap_local(struct drm_client_buffer *buffer);
> > > int drm_client_modeset_create(struct drm_client_dev *client);
> > > void drm_client_modeset_free(struct drm_client_dev *client);
> > > --
> > > 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
More information about the dri-devel
mailing list