Hi Robin,

On Tue, 18 May 2021 at 00:35, Robin Murphy <robin.murphy@arm.com> wrote:

> On 2021-05-17 10:27, Joerg Roedel wrote:
> > On Fri, Apr 30, 2021 at 08:20:10PM +0800, Kevin Tang wrote:
> >> Cc  Robin & Joerg
> >
> > This is just some GPU internal MMU being used here, it seems. It doesn't
> > use the IOMMU core code, so no Ack needed from the IOMMU side.
>
> Except the actual MMU being used is drivers/iommu/sprd_iommu.c - this is


Yes, it is using drivers/iommu/sprd_iommu.c.


>
> just the display driver poking directly at the interrupt registers of
> its associated IOMMU instance.


Actually the display driver is poking its own interrupt registers in which
some interrupts are caused by using IOMMU, others are purely its own ones:
+/* Interrupt control & status bits */
+#define BIT_DPU_INT_DONE               BIT(0)
+#define BIT_DPU_INT_TE                 BIT(1)
+#define BIT_DPU_INT_ERR                        BIT(2)
+#define BIT_DPU_INT_UPDATE_DONE                BIT(4)
+#define BIT_DPU_INT_VSYNC              BIT(5)
+#define BIT_DPU_INT_MMU_VAOR_RD                BIT(16)
+#define BIT_DPU_INT_MMU_VAOR_WR                BIT(17)
+#define BIT_DPU_INT_MMU_INV_RD         BIT(18)
+#define BIT_DPU_INT_MMU_INV_WR         BIT(19)

From what I see in the product code, along with the information my
colleagues told me, these _INT_MMU_ interrupts only need to be dealt with
by client devices(i.e. display). IOMMU doesn't even have the INT_STS
register for some early products which we're trying to support in the
mainstream kernel.


> I still think this is wrong, and that it
> should be treated as a shared interrupt, with the IOMMU driver handling
> its own registers and reporting to the client through the standard
> report_iommu_fault() API, especially since there are apparently more
> blocks using these IOMMU instances than just the display.
>

For the next generation IOMMU, we will handle interrupts in IOMMU drivers
like you say here.
But like I explained above, we have to leave interrupt handling in the
client device driver since the IOMMU we 're using in this display device
doesn't have an INT_STS register in the IOMMU register range.

Thanks for the review,
Chunyan


> Robin.
>