Hi
Am 03.07.19 um 16:07 schrieb Noralf Trønnes:
>
>
> Den 03.07.2019 10.32, skrev Thomas Zimmermann:
>> DRM clients, such as the fbdev emulation, have their buffer objects
>> mapped by default. Mapping a buffer implicitly prevents its relocation.
>> Hence, the buffer may permanently consume video memory while it's
>> allocated. This is a problem for drivers of low-memory devices, such as
>> ast, mgag200 or older framebuffer hardware, which will then not have
>> enough memory to display other content (e.g., X11).
>>
>> This patch introduces drm_client_buffer_vmap() and _vunmap(). Internal
>> DRM clients can use these functions to unmap and remap buffer objects
>> as needed.
>>
>> There's no reference counting for vmap operations. Callers are expected
>> to either keep buffers mapped (as it is now), or call vmap and vunmap
>> in pairs around code that accesses the mapped memory.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>> ---
>> drivers/gpu/drm/drm_client.c | 71 +++++++++++++++++++++++++++++++-----
>> include/drm/drm_client.h | 3 ++
>> 2 files changed, 64 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index 410572f14257..d04660c4470a 100644
>> --- a/drivers/gpu/drm/drm_client.c
>> +++ b/drivers/gpu/drm/drm_client.c
>> @@ -235,7 +235,8 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
>> {
>> struct drm_device *dev = buffer->client->dev;
>>
>> - drm_gem_vunmap(buffer->gem, buffer->vaddr);
>> + if (buffer->vaddr)
>
> No need for this, drm_gem_vunmap() has a NULL check.
>
>> + drm_gem_vunmap(buffer->gem, buffer->vaddr);
>>
>> if (buffer->gem)
>> drm_gem_object_put_unlocked(buffer->gem);
>> @@ -281,6 +282,43 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
>>
>> buffer->gem = obj;
>>
>> + vaddr = drm_client_buffer_vmap(buffer);
>
> I think we should change this and _not_ vmap on buffer creation.
> Eventually we'll get bootsplash and console clients and they will also
> have to deal with these low memory devices. AFAICS the only client that
> will need a virtual address at all times is the fbdev client when it
> doesn't use a shadow buffer.
Exactly my thoughts. I didn't want to overload the patch set with
changing clients to not map the buffer by default. But since you mention
it...
Best regards
Thomas
>
>> + if (IS_ERR(vaddr)) {
>> + ret = PTR_ERR(vaddr);
>> + goto err_delete;
>> + }
>> +
>> + return buffer;
>> +
>> +err_delete:
>> + drm_client_buffer_delete(buffer);
>> +
>> + return ERR_PTR(ret);
>> +}
>> +
>> +/**
>> + * drm_client_buffer_vmap - Map DRM client buffer into address space
>> + * @buffer: DRM client buffer
>> + *
>> + * This function maps a client buffer into kernel address space. If the
>> + * buffer is already mapped, it returns the mapping's address.
>> + *
>> + * Client buffer mappings are not ref'counted. Each call to
>> + * drm_client_buffer_vmap() should be followed by a call to
>> + * drm_client_buffer_vunmap(); or the client buffer should be mapped
>> + * throughout its lifetime. The latter is the default.
>> + *
>> + * Returns:
>> + * The mapped memory's address
>> + */
>> +void *
>> +drm_client_buffer_vmap(struct drm_client_buffer *buffer)
>> +{
>> + void *vaddr;
>> +
>> + if (buffer->vaddr)
>> + return buffer->vaddr;
>> +
>> /*
>> * FIXME: The dependency on GEM here isn't required, we could
>> * convert the driver handle to a dma-buf instead and use the
>> @@ -289,21 +327,34 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
>> * fd_install step out of the driver backend hooks, to make that
>> * final step optional for internal users.
>> */
>> - vaddr = drm_gem_vmap(obj);
>> - if (IS_ERR(vaddr)) {
>> - ret = PTR_ERR(vaddr);
>> - goto err_delete;
>> - }
>> + vaddr = drm_gem_vmap(buffer->gem);
>> + if (IS_ERR(vaddr))
>> + return vaddr;
>>
>> buffer->vaddr = vaddr;
>>
>> - return buffer;
>> + return vaddr;
>> +}
>> +EXPORT_SYMBOL(drm_client_buffer_vmap);
>>
>> -err_delete:
>> - drm_client_buffer_delete(buffer);
>> +/**
>> + * drm_client_buffer_vunmap - Unmap DRM client buffer
>> + * @buffer: DRM client buffer
>> + *
>> + * This function removes a client buffer's memory mmapping. This
>> + * function is only required by clients that manage their buffers
>> + * by themselves. By default, DRM client buffers are mapped throughout
>> + * their entire lifetime.
>> + */
>> +void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
>> +{
>> + if (!buffer->vaddr)
>
> No need for a NULL check here either.
>
> Noralf.
>
>> + return;
>>
>> - return ERR_PTR(ret);
>> + drm_gem_vunmap(buffer->gem, buffer->vaddr);
>> + buffer->vaddr = NULL;
>> }
>> +EXPORT_SYMBOL(drm_client_buffer_vunmap);
>>
>> static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
>> {
>> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
>> index 72d51d1e9dd9..e1db1d9da0bf 100644
>> --- a/include/drm/drm_client.h
>> +++ b/include/drm/drm_client.h
>> @@ -149,6 +149,9 @@ struct drm_client_buffer {
>> 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);
>> +void *
>> +drm_client_buffer_vmap(struct drm_client_buffer *buffer);
>> +void drm_client_buffer_vunmap(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);
>>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
