Re: [PATCH v7 7/7] drm/etnaviv: add support for the dma coherent device

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jun 07, 2023 at 02:43:27AM +0800, Sui Jingfeng wrote:
> On 2023/6/7 00:56, Bjorn Helgaas wrote:
> > On Sat, Jun 03, 2023 at 06:59:43PM +0800, Sui Jingfeng wrote:
> > > From: Sui Jingfeng <suijingfeng@loongson.cn>
> > > 
> > > Loongson CPUs maintain cache coherency by hardware, which means that the
> > > data in the CPU cache is identical to the data in main system memory. As
> > > for the peripheral device, most of Loongson chips chose to define the
> > > peripherals as DMA coherent by default, device drivers do not need to
> > > maintain the coherency between a processor and an I/O device manually.
> > ...

> > I guess the only way to discover this coherency attribute is via the
> > DT "vivante,gc" property?  Seems a little weird but I'm really not a
> > DT person.
> 
> I'm not sure it is *only*, but it is very convenient to achieve such a thing
> with DT.

I don't know if this is a property of the PCI device, or a property of
the system as a whole.  I asked because PCI devices should be
self-describing (the Device/Vendor ID should be enough to identify the
correct driver, and the driver should know how to learn anything else
it needs to know about the device from PCI config space) and should
not require extra DT properties. 

But if this is a CPU or system property, you probably have to use a
firmware interface like DT or ACPI.

> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > > @@ -8,6 +8,7 @@
> > >   #include <linux/delay.h>
> > >   #include <linux/dma-fence.h>
> > >   #include <linux/dma-mapping.h>
> > > +#include <linux/dma-map-ops.h>
> >
> > It looks like this #include might not be needed?
> 
> No, the dev_is_dma_coherent() function is declared and defined in
> dma-map-ops.h.  if remove it, gcc will complain:
> 
> drivers/gpu/drm/etnaviv/etnaviv_drv.c: In function
> ‘etnaviv_is_dma_coherent’:
> drivers/gpu/drm/etnaviv/etnaviv_drv.c:56:14: error: implicit declaration of
> function ‘dev_is_dma_coherent’; did you mean ‘etnaviv_is_dma_coherent’?
> [-Werror=implicit-function-declaration]
>    56 |   coherent = dev_is_dma_coherent(dev);
>       |              ^~~~~~~~~~~~~~~~~~~

Of course, but that warning is for etnaviv_drv.c, not for
etnaviv_gpu.c.  So etnaviv_drv.c needs to include dma-map-ops.h, but I
don't think etnaviv_gpu.c does.  I removed this #include from
etnaviv_gpu.c and it still built without errors.

> > You're only adding a
> > new reference to priv->dma_coherent, which looks like it was added to
> > etnaviv_drv.h.