| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <712eea49-37f3-4d15-9e04-676937766b65@linux.dev>
Date: Sat, 26 Oct 2024 01:42:06 +0800
From: Sui Jingfeng <sui.jingfeng@...ux.dev>
To: Lucas Stach <l.stach@...gutronix.de>,
Russell King <linux+etnaviv@...linux.org.uk>,
Christian Gmeiner <christian.gmeiner@...il.com>
Cc: David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
etnaviv@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] drm/etnaviv: Track GPU VA size separately
Hi,
On 2024/10/7 18:12, Lucas Stach wrote:
> Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
>> Etnaviv assumes that GPU page size is 4KiB, yet on some systems, the CPU
>> page size is 16KiB. The size of etnaviv buffer objects will be aligned
>> to CPU page size on kernel side, however, userspace still assumes the
>> page size is 4KiB and doing allocation with 4KiB page as unit. This
>> results in userspace allocated GPU virtual address range collision and
>> therefore unable to be inserted to the specified hole exactly.
>>
>> The root cause is that kernel side BO takes up bigger address space than
>> userspace assumes when the size of it is not CPU page size aligned. To
>> Preserve GPU VA continuous as much as possible, track the size that
>> userspace/GPU think of it is.
>>
>> Yes, we still need to overallocate to suit the CPU, but there is no need
>> to waste GPU VA space anymore.
>>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@...ux.dev>
>> ---
>> drivers/gpu/drm/etnaviv/etnaviv_gem.c | 8 +++++---
>> drivers/gpu/drm/etnaviv/etnaviv_gem.h | 1 +
>> drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 8 ++++----
>> 3 files changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> index 5c0c9d4e3be1..943fc20093e6 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> @@ -543,7 +543,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>> .vm_ops = &vm_ops,
>> };
>>
>> -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>> +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>> const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>> {
>> struct etnaviv_gem_object *etnaviv_obj;
>> @@ -570,6 +570,7 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>> if (!etnaviv_obj)
>> return -ENOMEM;
>>
>> + etnaviv_obj->user_size = size;
>> etnaviv_obj->flags = flags;
>> etnaviv_obj->ops = ops;
>>
>> @@ -588,11 +589,12 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>> {
>> struct etnaviv_drm_private *priv = dev->dev_private;
>> struct drm_gem_object *obj = NULL;
>> + unsigned int user_size = size;
> This still needs to be be aligned to 4K. Userspace may request
> unaligned buffer sizes and we don't want to risk any confusion about
> which part is visible to the GPU, so better make sure this size is
> aligned to the GPU page size.
OK,aligned to the GPU page size is reasonable. Since the buffer is very high likely be used by GPU.
> Also, that more personal preference, but I would call this gpu_size or
> something like that, to avoid any confusion with the user_size in
> etnaviv_cmdbuf, where user_size doesn't denote the GPU visible size.
Yeah, theuser_size denote the length of command buffer, it's usually just need to
aligned to 8 bytes. And generally, the size command buffer won't larger
than 4KiB (a GPU PAGE).
I'm imagine that just 'size' with some extra comment, as it's possible
that a buffer is only get used by CPU for specific purpose.
Best Regards,
Sui
> Regards,
> Lucas
>
>> int ret;
>>
>> size = PAGE_ALIGN(size);
>>
>> - ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
>> + ret = etnaviv_gem_new_impl(dev, user_size, flags, &etnaviv_gem_shmem_ops, &obj);
>> if (ret)
>> goto fail;
>>
>> @@ -627,7 +629,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>> struct drm_gem_object *obj;
>> int ret;
>>
>> - ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
>> + ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
>> if (ret)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> index a42d260cac2c..c6e27b9abb0c 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> @@ -36,6 +36,7 @@ struct etnaviv_gem_object {
>> const struct etnaviv_gem_ops *ops;
>> struct mutex lock;
>>
>> + u32 user_size;
>> u32 flags;
>>
>> struct list_head gem_node;
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> index 1661d589bf3e..6fbc62772d85 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> @@ -281,6 +281,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>> {
>> struct sg_table *sgt = etnaviv_obj->sgt;
>> struct drm_mm_node *node;
>> + unsigned int user_size;
>> int ret;
>>
>> lockdep_assert_held(&etnaviv_obj->lock);
>> @@ -303,13 +304,12 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>> }
>>
>> node = &mapping->vram_node;
>> + user_size = etnaviv_obj->user_size;
>>
>> if (va)
>> - ret = etnaviv_iommu_insert_exact(context, node,
>> - etnaviv_obj->base.size, va);
>> + ret = etnaviv_iommu_insert_exact(context, node, user_size, va);
>> else
>> - ret = etnaviv_iommu_find_iova(context, node,
>> - etnaviv_obj->base.size);
>> + ret = etnaviv_iommu_find_iova(context, node, user_size);
>> if (ret < 0)
>> goto unlock;
>>
--
Best regards,
Sui
Powered by blists - more mailing lists