linux-kernel - Re: [PATCH v10 07/11] drm/etnaviv: Add support for the dma coherent device

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 21 Jun 2023 19:45:57 +0200
From:   Lucas Stach <l.stach@...gutronix.de>
To:     Sui Jingfeng <suijingfeng@...ngson.cn>,
        Sui Jingfeng <18949883232@....com>,
        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, dri-devel@...ts.freedesktop.org,
        etnaviv@...ts.freedesktop.org,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH v10 07/11] drm/etnaviv: Add support for the dma coherent
 device

Am Donnerstag, dem 22.06.2023 um 01:21 +0800 schrieb Sui Jingfeng:
> Hi,
> 
> On 2023/6/21 23:58, Lucas Stach wrote:
> > Am Mittwoch, dem 21.06.2023 um 23:30 +0800 schrieb Sui Jingfeng:
> > > Hi,
> > > 
> > > On 2023/6/21 18:00, Lucas Stach wrote:
> > > > >    		dma_sync_sgtable_for_cpu(dev->dev, etnaviv_obj->sgt,
> > > > >    					 etnaviv_op_to_dma_dir(op));
> > > > >    		etnaviv_obj->last_cpu_prep_op = op;
> > > > > @@ -408,8 +421,9 @@ int etnaviv_gem_cpu_fini(struct drm_gem_object *obj)
> > > > >    {
> > > > >    	struct drm_device *dev = obj->dev;
> > > > >    	struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
> > > > > +	struct etnaviv_drm_private *priv = dev->dev_private;
> > > > >    
> > > > > -	if (etnaviv_obj->flags & ETNA_BO_CACHED) {
> > > > > +	if (!priv->dma_coherent && etnaviv_obj->flags & ETNA_BO_CACHED) {
> > > > >    		/* fini without a prep is almost certainly a userspace error */
> > > > >    		WARN_ON(etnaviv_obj->last_cpu_prep_op == 0);
> > > > >    		dma_sync_sgtable_for_device(dev->dev, etnaviv_obj->sgt,
> > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> > > > > index 3524b5811682..754126992264 100644
> > > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> > > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> > > > > @@ -112,11 +112,16 @@ 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 = ETNA_BO_WC;
> > > > >    	int ret, npages;
> > > > >    
> > > > > -	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
> > > > > +	if (priv->dma_coherent)
> > > > > +		cache_flags = ETNA_BO_CACHED;
> > > > > +
> > > > Drop this change. Instead etnaviv_gem_new_impl() should do the upgrade
> > > > from WC to CACHED as necessary by adding something like this:
> > > I understand you are a profession person in vivante GPU driver domain.
> > > 
> > > I respect you reviews and instruction.
> > > 
> > > But, I'm really reluctant to agree with this, is there any space to
> > > negotiate?
> > > 
> > > > /*
> > > >    * Upgrade WC to CACHED when the device is hardware coherent and the
> > > >    * platform doesn't allow mixing cached and writecombined mappings to
> > > >    * the same memory area.
> > > >    */
> > > > if ((flags & ETNA_BO_CACHE_MASK) == ETNA_BO_WC &&
> > > >       dev_is_dma_coherent(dev) && !drm_arch_can_wc_memory())
> > > >           flags = (flags & ~ETNA_BO_CACHE_MASK) & ETNA_BO_CACHED;
> > > This is policy, not a mechanism.
> > > 
> > > Using what cache property is a user-space program's choice.
> > > 
> > > While you are override the WC with CACHED mapping. This is not correct
> > > in the concept!
> > > 
> > Please explain why you think that this isn't correct.
> 
> Again,
> 
> this is user-space things!
> 
> this is user-space things!
> 
> this is user-space things!
> 
> I have explained several times.
> 
> made the decision for the user-space program is wrong.
> 
This mode of communication isn't helpful. Please stop it.

As I tried to explain to you multiple times: if userspace can break
coherency by selecting the wrong mapping type then this is something
the kernel should prevent.

> 
> > It allows
> > userspace to use WC mappings that would potentially cause loss of
> > coherency between CPU and GPU, which isn't acceptable.
> 
> Before made the WC works correctly,  you need the developing environment.
> 
> userspace program can tune the BO cache mapping easily.
> 
> Either environment or supply a conf file.
> 
> 
> While with your implement, we don't have the opportunity to do debugging 
> and the development.

You can always add a patch to your local kernel to re-allow WC mappings
while you work on making them behave as expected on your platform. With
the mainline kernel there is no way that the kernel driver will allow
broken coherency.

And as I also mentioned before, there is a clear upgrade path here:
once WC mappings work as expected on your platform we can easily drop
the upgrading from the kernel driver again. The userspace driver can
already be changed to use CACHED BOs where beneficial on your platform
in the meantime.

Regards,
Lucas