thr3ads.net - Virtualization - [PATCH v3 8/8] drm/fb-helper: Move BO locking from DRM client to fbdev damage worker [Dec 2020]

If this information is useful, please help other people find it:
Share via:

Daniel Vetter

2020-Dec-11 10:01 UTC

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

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.

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

Thomas Zimmermann

2020-Dec-11 10:16 UTC

head link

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

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().

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20201211/d6beadf4/attachment.sig>

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

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

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