Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Sui Jingfeng <suijingfeng@xxxxxxxxxxx>]

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?

> 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.

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 :-)

>   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@xxxxxxxxxxx>
> > ---
> >   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