Re: [PATCH v3 8/8] drm/fb-helper: Move BO locking from DRM client to fbdev damage worker

[Daniel Vetter <daniel@ffwll.ch>]

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@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@ffwll.ch>
-Daniel

> 
> Best regards
> Thomas
> 
> > 
> > With that: Reviewed-by: Daniel Vetter <daniel.vetter@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