[PATCH v3 8/8] drm/fb-helper: Move BO locking from DRM client to fbdev damage worker
Thomas Zimmermann
tzimmermann at suse.de
Fri Dec 11 10:16:25 UTC 2020
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: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201211/d6beadf4/attachment-0001.sig>
More information about the dri-devel
mailing list