| 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: <d8d87496-e695-7d48-4433-0be18ec41caf@loongson.cn>
Date: Thu, 1 Jun 2023 18:13:18 +0800
From: Sui Jingfeng <suijingfeng@...ngson.cn>
To: Lucas Stach <l.stach@...gutronix.de>,
Russell King <linux+etnaviv@...linux.org.uk>,
Christian Gmeiner <christian.gmeiner@...il.com>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>
Cc: linux-kernel@...r.kernel.org, etnaviv@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, loongson-kernel@...ts.loongnix.cn
Subject: Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached
coherent bo
Hi,
On 2023/6/1 01:53, Lucas Stach wrote:
> Am Donnerstag, dem 01.06.2023 um 01:29 +0800 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/6/1 00:33, Lucas Stach wrote:
>>> Hi Sui Jingfeng,
>>>
>>> Am Mittwoch, dem 31.05.2023 um 00:06 +0800 schrieb Sui Jingfeng:
>>>> cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
>>>> snoop the CPU's cache. write-combine caching property is not suitiable for
>>>> us.
>>>>
>>> As previously mentioned in the Mesa MR, I don't think this is the right
>>> approach.
>>>
>>> ETNA_BO_CACHED already looks coherent to userspace, as all accesses are
>>> bracketed via the ETNAVIV_GEM_CPU_PREP and ETNAVIV_GEM_CPU_FINI ioctls,
>>> which will do the necessary cache maintenance on platforms where device
>>> coherence isn't enforced by the hardware, so there is no need for a
>>> separate ETNA_BO_CACHED_COHERENT.
>> As far as I can see, ETNA_BO_CACHED_COHERENT could probably help to
>> bypass the overhead of
>>
>> dma_sync_sgtable_for_cpu() and dma_sync_sgtable_for_device() brings to us.
>>
>>
>> I have tested long time ago, there no need call this function on our
>> platform.
>>
>> The glmark2 works as before if I comment out thoes two function.
>>
>> Are you serious, sir?
>>
> The dma_sync* functions are more or less no-ops when the device is
> marked as being coherent. ce
> instance, you might need to propagate the coherent property from the
> GPU core device to the virtual DRM device, along the lines of how we
> propagate other DMA properties from the GPU device to the DRM device in
> etnaviv_pdev_probe.
>
> Other than that things should just work with minimal overhead.
>>> Instead we just need a new ETNAVIV_PARAM to inform userspace about
>>> hardware cache coherence being available for a specific GPU core,
>> Ok, let me think about for a while how to implement this.
>>
> Simple: add new ETNAVIV_PARAM_GPU_COHERENT to
> include/uapi/drm/etnaviv_drm.h, return the result from
> dev_is_dma_coherent in etnaviv_gpu_get_param().
Okay, agree
>> But How about we merge this first, I create another patch to improve it
>>
>> with a roughly working base first? I'm just asking if the answer is No :-)
>>
> The answer is a firm no.
>
> This impacts UAPI, so there is no chance to ever get rid of any wrong
> decisions here, as any added UAPI needs to be supported indefinitely.
> I'm not signing up for maintaining something I believe is implemented
> upside down.
>
> Please don't take this the wrong way: I'm pretty excited to see etnaviv
> used on more architectures and outside of the proven platform device
> paths, so I'm happy to assist in working out the design and help you
> get things merged in both kernel and Mesa. But I think we are still
> quite a few steps away from having things worked out enough to even
> think about merging those patchsets.
>
> Also please allow me to comment on the other patches of the series, so
> I can get a better understanding of your platform/integration, before
> sending another revision of those patches.
I go to sleep yesterday.
Okay, this sound fine.
> Regards,
> Lucas
>
>>> in
>>> which case the userspace driver should switch to preferring
>>> ETNA_BO_CACHED over ETNA_BO_WC.
>> Yeah, ETNA_BO_CACHED is enough.
>>
>> ETNA_BO_CACHED_COHERENT is actually a special case of ETNA_BO_CACHED.
>>
>>> Regards,
>>> Lucas
>>>
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@...ngson.cn>
>>>> ---
>>>> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 2 +-
>>>> drivers/gpu/drm/etnaviv/etnaviv_gem.c | 22 +++++++++++++++++++--
>>>> drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 9 ++++++++-
>>>> include/uapi/drm/etnaviv_drm.h | 11 ++++++-----
>>>> 4 files changed, 35 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> index 052f745cecc0..2816c654c023 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> @@ -274,7 +274,7 @@ static int etnaviv_ioctl_gem_new(struct drm_device *dev, void *data,
>>>> struct drm_etnaviv_gem_new *args = data;
>>>>
>>>> if (args->flags & ~(ETNA_BO_CACHED | ETNA_BO_WC | ETNA_BO_UNCACHED |
>>>> - ETNA_BO_FORCE_MMU))
>>>> + ETNA_BO_CACHED_COHERENT | ETNA_BO_FORCE_MMU))
>>>> return -EINVAL;
>>>>
>>>> return etnaviv_gem_new_handle(dev, file, args->size,
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> index b5f73502e3dd..d8b559bd33d3 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> @@ -343,6 +343,7 @@ void *etnaviv_gem_vmap(struct drm_gem_object *obj)
>>>> static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>>>> {
>>>> struct page **pages;
>>>> + pgprot_t prot;
>>>>
>>>> lockdep_assert_held(&obj->lock);
>>>>
>>>> @@ -350,8 +351,20 @@ static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>>>> if (IS_ERR(pages))
>>>> return NULL;
>>>>
>>>> - return vmap(pages, obj->base.size >> PAGE_SHIFT,
>>>> - VM_MAP, pgprot_writecombine(PAGE_KERNEL));
>>>> + switch (obj->flags) {
>>>> + case ETNA_BO_CACHED_COHERENT:
>>>> + case ETNA_BO_CACHED:
>>>> + prot = PAGE_KERNEL;
>>>> + break;
>>>> + case ETNA_BO_UNCACHED:
>>>> + prot = pgprot_noncached(PAGE_KERNEL);
>>>> + break;
>>>> + case ETNA_BO_WC:
>>>> + default:
>>>> + prot = pgprot_writecombine(PAGE_KERNEL);
>>>> + }
>>>> +
>>>> + return vmap(pages, obj->base.size >> PAGE_SHIFT, VM_MAP, prot);
>>>> }
>>>>
>>>> static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
>>>> @@ -545,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>>>> 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_drm_private *priv = dev->dev_private;
>>>> struct etnaviv_gem_object *etnaviv_obj;
>>>> unsigned sz = sizeof(*etnaviv_obj);
>>>> bool valid = true;
>>>> @@ -555,6 +569,10 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>>>> case ETNA_BO_CACHED:
>>>> case ETNA_BO_WC:
>>>> break;
>>>> + case ETNA_BO_CACHED_COHERENT:
>>>> + if (priv->has_cached_coherent)
>>>> + break;
>>>> + fallthrough;
>>>> default:
>>>> valid = false;
>>>> }
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>>>> index 3524b5811682..671d91d8f1c6 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>>>> @@ -112,11 +112,18 @@ static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
>>>> struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>>>> struct dma_buf_attachment *attach, struct sg_table *sgt)
>>>> {
>>>> + struct etnaviv_drm_private *priv = dev->dev_private;
>>>> struct etnaviv_gem_object *etnaviv_obj;
>>>> size_t size = PAGE_ALIGN(attach->dmabuf->size);
>>>> + u32 cache_flags;
>>>> int ret, npages;
>>>>
>>>> - ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
>>>> + if (priv->has_cached_coherent)
>>>> + cache_flags = ETNA_BO_CACHED_COHERENT;
>>>> + else
>>>> + cache_flags = ETNA_BO_WC;
>>>> +
>>>> + ret = etnaviv_gem_new_private(dev, size, cache_flags,
>>>> &etnaviv_gem_prime_ops, &etnaviv_obj);
>>>> if (ret < 0)
>>>> return ERR_PTR(ret);
>>>> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
>>>> index af024d90453d..474b0db286de 100644
>>>> --- a/include/uapi/drm/etnaviv_drm.h
>>>> +++ b/include/uapi/drm/etnaviv_drm.h
>>>> @@ -90,13 +90,14 @@ struct drm_etnaviv_param {
>>>> * GEM buffers:
>>>> */
>>>>
>>>> -#define ETNA_BO_CACHE_MASK 0x000f0000
>>>> +#define ETNA_BO_CACHE_MASK 0x000f0000
>>>> /* cache modes */
>>>> -#define ETNA_BO_CACHED 0x00010000
>>>> -#define ETNA_BO_WC 0x00020000
>>>> -#define ETNA_BO_UNCACHED 0x00040000
>>>> +#define ETNA_BO_CACHED 0x00010000
>>>> +#define ETNA_BO_WC 0x00020000
>>>> +#define ETNA_BO_UNCACHED 0x00040000
>>>> +#define ETNA_BO_CACHED_COHERENT 0x00080000
>>>> /* map flags */
>>>> -#define ETNA_BO_FORCE_MMU 0x00100000
>>>> +#define ETNA_BO_FORCE_MMU 0x00100000
>>>>
>>>> struct drm_etnaviv_gem_new {
>>>> __u64 size; /* in */
--
Jingfeng
Powered by blists - more mailing lists