Re: [PATCH v5 08/10] drm/gem: Store client buffer mappings as struct dma_buf_map
On Thu, Oct 22, 2020 at 11:18 AM Thomas Zimmermann wrote:
>
> Hi
>
> On 22.10.20 10:49, Daniel Vetter wrote:
> > On Tue, Oct 20, 2020 at 02:20:44PM +0200, Thomas Zimmermann wrote:
> >> Kernel DRM clients now store their framebuffer address in an instance
> >> of struct dma_buf_map. Depending on the buffer's location, the address
> >> refers to system or I/O memory.
> >>
> >> Callers of drm_client_buffer_vmap() receive a copy of the value in
> >> the call's supplied arguments. It can be accessed and modified with
> >> dma_buf_map interfaces.
> >>
> >> Signed-off-by: Thomas Zimmermann
> >> Reviewed-by: Daniel Vetter
> >> Tested-by: Sam Ravnborg
> >> ---
> >> drivers/gpu/drm/drm_client.c| 34 +++--
> >> drivers/gpu/drm/drm_fb_helper.c | 23 +-
> >> include/drm/drm_client.h| 7 ---
> >> 3 files changed, 38 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> >> index ac0082bed966..fe573acf1067 100644
> >> --- a/drivers/gpu/drm/drm_client.c
> >> +++ b/drivers/gpu/drm/drm_client.c
> >> @@ -235,7 +235,7 @@ 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);
> >> +drm_gem_vunmap(buffer->gem, >map);
> >>
> >> if (buffer->gem)
> >> drm_gem_object_put(buffer->gem);
> >> @@ -291,25 +291,31 @@ drm_client_buffer_create(struct drm_client_dev
> >> *client, u32 width, u32 height, u
> >> /**
> >> * drm_client_buffer_vmap - 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 mapping's address.
> >> + * 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() should be followed by a call to
> >> * drm_client_buffer_vunmap(); 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:
> >> - * The mapped memory's address
> >> + * 0 on success, or a negative errno code otherwise.
> >> */
> >> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer)
> >> +int
> >> +drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct
> >> dma_buf_map *map_copy)
> >> {
> >> -struct dma_buf_map map;
> >> +struct dma_buf_map *map = >map;
> >> int ret;
> >>
> >> -if (buffer->vaddr)
> >> -return buffer->vaddr;
> >> +if (dma_buf_map_is_set(map))
> >> +goto out;
> >>
> >> /*
> >> * FIXME: The dependency on GEM here isn't required, we could
> >> @@ -319,13 +325,14 @@ void *drm_client_buffer_vmap(struct
> >> drm_client_buffer *buffer)
> >> * fd_install step out of the driver backend hooks, to make that
> >> * final step optional for internal users.
> >> */
> >> -ret = drm_gem_vmap(buffer->gem, );
> >> +ret = drm_gem_vmap(buffer->gem, map);
> >> if (ret)
> >> -return ERR_PTR(ret);
> >> +return ret;
> >>
> >> -buffer->vaddr = map.vaddr;
> >> +out:
> >> +*map_copy = *map;
> >>
> >> -return map.vaddr;
> >> +return 0;
> >> }
> >> EXPORT_SYMBOL(drm_client_buffer_vmap);
> >>
> >> @@ -339,10 +346,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
> >> */
> >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
> >> {
> >> -struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(buffer->vaddr);
> >> +struct dma_buf_map *map = >map;
> >>
> >> -drm_gem_vunmap(buffer->gem, );
> >> -buffer->vaddr = NULL;
> >> +drm_gem_vunmap(buffer->gem, map);
> >> }
> >> EXPORT_SYMBOL(drm_client_buffer_vunmap);
> >>
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> >> b/drivers/gpu/drm/drm_fb_helper.c
> >> index c2f72bb6afb1..6212cd7cde1d 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -378,7 +378,7 @@ static void drm_fb_helper_dirty_blit_real(struct
> >> drm_fb_helper *fb_helper,
> >> unsigned int cpp = fb->format->cpp[0];
> >> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> >> void *src = fb_helper->fbdev->screen_buffer + offset;
> >> -void *dst = fb_helper->buffer->vaddr + offset;
> >> +void *dst = fb_helper->buffer->map.vaddr + offset;
> >> size_t len = (clip->x2 - clip->x1) * cpp;
> >> unsigned int y;
> >>
> >> @@ -400,7 +400,8 @@ static void drm_fb_helper_dirty_work(struct
> >>
Re: [PATCH v5 08/10] drm/gem: Store client buffer mappings as struct dma_buf_map
Hi
On 22.10.20 10:49, Daniel Vetter wrote:
> On Tue, Oct 20, 2020 at 02:20:44PM +0200, Thomas Zimmermann wrote:
>> Kernel DRM clients now store their framebuffer address in an instance
>> of struct dma_buf_map. Depending on the buffer's location, the address
>> refers to system or I/O memory.
>>
>> Callers of drm_client_buffer_vmap() receive a copy of the value in
>> the call's supplied arguments. It can be accessed and modified with
>> dma_buf_map interfaces.
>>
>> Signed-off-by: Thomas Zimmermann
>> Reviewed-by: Daniel Vetter
>> Tested-by: Sam Ravnborg
>> ---
>> drivers/gpu/drm/drm_client.c| 34 +++--
>> drivers/gpu/drm/drm_fb_helper.c | 23 +-
>> include/drm/drm_client.h| 7 ---
>> 3 files changed, 38 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index ac0082bed966..fe573acf1067 100644
>> --- a/drivers/gpu/drm/drm_client.c
>> +++ b/drivers/gpu/drm/drm_client.c
>> @@ -235,7 +235,7 @@ 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);
>> +drm_gem_vunmap(buffer->gem, >map);
>>
>> if (buffer->gem)
>> drm_gem_object_put(buffer->gem);
>> @@ -291,25 +291,31 @@ drm_client_buffer_create(struct drm_client_dev
>> *client, u32 width, u32 height, u
>> /**
>> * drm_client_buffer_vmap - 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 mapping's address.
>> + * 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() should be followed by a call to
>> * drm_client_buffer_vunmap(); 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:
>> - * The mapped memory's address
>> + * 0 on success, or a negative errno code otherwise.
>> */
>> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer)
>> +int
>> +drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map
>> *map_copy)
>> {
>> -struct dma_buf_map map;
>> +struct dma_buf_map *map = >map;
>> int ret;
>>
>> -if (buffer->vaddr)
>> -return buffer->vaddr;
>> +if (dma_buf_map_is_set(map))
>> +goto out;
>>
>> /*
>> * FIXME: The dependency on GEM here isn't required, we could
>> @@ -319,13 +325,14 @@ void *drm_client_buffer_vmap(struct drm_client_buffer
>> *buffer)
>> * fd_install step out of the driver backend hooks, to make that
>> * final step optional for internal users.
>> */
>> -ret = drm_gem_vmap(buffer->gem, );
>> +ret = drm_gem_vmap(buffer->gem, map);
>> if (ret)
>> -return ERR_PTR(ret);
>> +return ret;
>>
>> -buffer->vaddr = map.vaddr;
>> +out:
>> +*map_copy = *map;
>>
>> -return map.vaddr;
>> +return 0;
>> }
>> EXPORT_SYMBOL(drm_client_buffer_vmap);
>>
>> @@ -339,10 +346,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
>> */
>> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
>> {
>> -struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(buffer->vaddr);
>> +struct dma_buf_map *map = >map;
>>
>> -drm_gem_vunmap(buffer->gem, );
>> -buffer->vaddr = NULL;
>> +drm_gem_vunmap(buffer->gem, map);
>> }
>> EXPORT_SYMBOL(drm_client_buffer_vunmap);
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index c2f72bb6afb1..6212cd7cde1d 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -378,7 +378,7 @@ static void drm_fb_helper_dirty_blit_real(struct
>> drm_fb_helper *fb_helper,
>> unsigned int cpp = fb->format->cpp[0];
>> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
>> void *src = fb_helper->fbdev->screen_buffer + offset;
>> -void *dst = fb_helper->buffer->vaddr + offset;
>> +void *dst = fb_helper->buffer->map.vaddr + offset;
>> size_t len = (clip->x2 - clip->x1) * cpp;
>> unsigned int y;
>>
>> @@ -400,7 +400,8 @@ static void drm_fb_helper_dirty_work(struct work_struct
>> *work)
>> struct drm_clip_rect *clip = >dirty_clip;
>> struct drm_clip_rect clip_copy;
>> unsigned long flags;
>> -void *vaddr;
>> +struct dma_buf_map map;
>> +int ret;
>>
>> spin_lock_irqsave(>dirty_lock, flags);
>> clip_copy = *clip;
>> @@ -413,8
Re: [PATCH v5 08/10] drm/gem: Store client buffer mappings as struct dma_buf_map
On Tue, Oct 20, 2020 at 02:20:44PM +0200, Thomas Zimmermann wrote:
> Kernel DRM clients now store their framebuffer address in an instance
> of struct dma_buf_map. Depending on the buffer's location, the address
> refers to system or I/O memory.
>
> Callers of drm_client_buffer_vmap() receive a copy of the value in
> the call's supplied arguments. It can be accessed and modified with
> dma_buf_map interfaces.
>
> Signed-off-by: Thomas Zimmermann
> Reviewed-by: Daniel Vetter
> Tested-by: Sam Ravnborg
> ---
> drivers/gpu/drm/drm_client.c| 34 +++--
> drivers/gpu/drm/drm_fb_helper.c | 23 +-
> include/drm/drm_client.h| 7 ---
> 3 files changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index ac0082bed966..fe573acf1067 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -235,7 +235,7 @@ 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);
> + drm_gem_vunmap(buffer->gem, >map);
>
> if (buffer->gem)
> drm_gem_object_put(buffer->gem);
> @@ -291,25 +291,31 @@ drm_client_buffer_create(struct drm_client_dev *client,
> u32 width, u32 height, u
> /**
> * drm_client_buffer_vmap - 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 mapping's address.
> + * 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() should be followed by a call to
> * drm_client_buffer_vunmap(); 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:
> - * The mapped memory's address
> + * 0 on success, or a negative errno code otherwise.
> */
> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer)
> +int
> +drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map
> *map_copy)
> {
> - struct dma_buf_map map;
> + struct dma_buf_map *map = >map;
> int ret;
>
> - if (buffer->vaddr)
> - return buffer->vaddr;
> + if (dma_buf_map_is_set(map))
> + goto out;
>
> /*
>* FIXME: The dependency on GEM here isn't required, we could
> @@ -319,13 +325,14 @@ void *drm_client_buffer_vmap(struct drm_client_buffer
> *buffer)
>* fd_install step out of the driver backend hooks, to make that
>* final step optional for internal users.
>*/
> - ret = drm_gem_vmap(buffer->gem, );
> + ret = drm_gem_vmap(buffer->gem, map);
> if (ret)
> - return ERR_PTR(ret);
> + return ret;
>
> - buffer->vaddr = map.vaddr;
> +out:
> + *map_copy = *map;
>
> - return map.vaddr;
> + return 0;
> }
> EXPORT_SYMBOL(drm_client_buffer_vmap);
>
> @@ -339,10 +346,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
> */
> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
> {
> - struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(buffer->vaddr);
> + struct dma_buf_map *map = >map;
>
> - drm_gem_vunmap(buffer->gem, );
> - buffer->vaddr = NULL;
> + drm_gem_vunmap(buffer->gem, map);
> }
> EXPORT_SYMBOL(drm_client_buffer_vunmap);
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index c2f72bb6afb1..6212cd7cde1d 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -378,7 +378,7 @@ static void drm_fb_helper_dirty_blit_real(struct
> drm_fb_helper *fb_helper,
> unsigned int cpp = fb->format->cpp[0];
> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> void *src = fb_helper->fbdev->screen_buffer + offset;
> - void *dst = fb_helper->buffer->vaddr + offset;
> + void *dst = fb_helper->buffer->map.vaddr + offset;
> size_t len = (clip->x2 - clip->x1) * cpp;
> unsigned int y;
>
> @@ -400,7 +400,8 @@ static void drm_fb_helper_dirty_work(struct work_struct
> *work)
> struct drm_clip_rect *clip = >dirty_clip;
> struct drm_clip_rect clip_copy;
> unsigned long flags;
> - void *vaddr;
> + struct dma_buf_map map;
> + int ret;
>
> spin_lock_irqsave(>dirty_lock, flags);
> clip_copy = *clip;
> @@ -413,8 +414,8 @@ static void drm_fb_helper_dirty_work(struct work_struct
> *work)
>
> /* Generic fbdev uses a shadow
