Re: [PATCH v11 3/7] iommu: Add verisilicon IOMMU driver

[Benjamin Gaignard]

Le 19/01/2026 à 13:32, Will Deacon a écrit :
> On Wed, Jan 14, 2026 at 02:10:48PM +0100, Benjamin Gaignard wrote:
> > Le 14/01/2026 à 13:59, Will Deacon a écrit :
> > > On Tue, Jan 13, 2026 at 05:25:38PM +0100, Benjamin Gaignard wrote:
> > > > Le 13/01/2026 à 17:10, Will Deacon a écrit :
> > > > > Hi Benjamin,
> > > > >
> > > > > Thanks for posting a v11.
> > > > >
> > > > > On Wed, Jan 07, 2026 at 11:09:53AM +0100, Benjamin Gaignard wrote:
> > > > > > The Verisilicon IOMMU hardware block can be found in combination
> > > > > > with Verisilicon hardware video codecs (encoders or decoders) on
> > > > > > different SoCs.
> > > > > > Enable it will allow us to use non contiguous memory allocators
> > > > > > for Verisilicon video codecs.
> > > > > > If both decoder and this iommu driver are compiled has modules
> > > > > > there is undefined symboles issues so this iommu driver could
> > > > > > only be compiled has built-in.
> > > > > >
> > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
> > > > > > ---
> > > > > > changes in version 11:
> > > > > > - Fix dependency issue when decoder driver is build as module.
> > > > > >
> > > > > >     drivers/iommu/Kconfig     |  11 +
> > > > > >     drivers/iommu/Makefile    |   1 +
> > > > > >     drivers/iommu/vsi-iommu.c | 808 ++++++++++++++++++++++++++++++++++++++
> > > > > >     include/linux/vsi-iommu.h |  21 +
> > > > > >     4 files changed, 841 insertions(+)
> > > > > >     create mode 100644 drivers/iommu/vsi-iommu.c
> > > > > >     create mode 100644 include/linux/vsi-iommu.h
> > > > > Based on your reply to v9:
> > > > >
> > > > > https://lore.kernel.org/all/0eff8b1a-c45f-47b1-a871-59f4a0101f0f@xxxxxxxxxxxxx/
> > > > >
> > > > > I took another look at this to see whether it had changed significantly
> > > > > from v6 when compared to the rockchip driver. Sadly, they still look
> > > > > very similar to me and I continue to suspect that the hardware is a
> > > > > derivative. I really don't understand why having a shared implementation
> > > > > of the default domain ops is difficult or controversial. Have you tried
> > > > > to write it?
> > > > >
> > > > > However, given that nobody from the Rockchip side has contributed to the
> > > > > discussion and you claim that this is a distinct piece of IP, I don't
> > > > > want to block the merging of the driver by leaving the conversation
> > > > > hanging.
> > > > >
> > > > > There is still one thing I don't understand (which, amusingly, the
> > > > > rockchip driver doesn't seem to suffer from):
> > > > >
> > > > > > +static void vsi_iommu_flush_tlb_all(struct iommu_domain *domain)
> > > > > > +{
> > > > > > +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
> > > > > > +	struct list_head *pos;
> > > > > > +	unsigned long flags;
> > > > > > +
> > > > > > +	spin_lock_irqsave(&vsi_domain->lock, flags);
> > > > > > +
> > > > > > +	list_for_each(pos, &vsi_domain->iommus) {
> > > > > > +		struct vsi_iommu *iommu;
> > > > > > +		int ret;
> > > > > > +
> > > > > > +		iommu = list_entry(pos, struct vsi_iommu, node);
> > > > > > +		ret = pm_runtime_resume_and_get(iommu->dev);
> > > > > > +		if (ret < 0)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		spin_lock(&iommu->lock);
> > > > > > +
> > > > > > +		writel(VSI_MMU_BIT_FLUSH, iommu->regs + VSI_MMU_FLUSH_BASE);
> > > > > > +		writel(0, iommu->regs + VSI_MMU_FLUSH_BASE);
> > > > > > +
> > > > > > +		spin_unlock(&iommu->lock);
> > > > > > +		pm_runtime_put_autosuspend(iommu->dev);
> > > > > > +	}
> > > > > > +
> > > > > > +	spin_unlock_irqrestore(&vsi_domain->lock, flags);
> > > > > > +}
> > > > > [...]
> > > > >
> > > > > > +static const struct iommu_ops vsi_iommu_ops = {
> > > > > > +	.identity_domain = &vsi_identity_domain,
> > > > > > +	.release_domain = &vsi_identity_domain,
> > > > > > +	.domain_alloc_paging = vsi_iommu_domain_alloc_paging,
> > > > > > +	.of_xlate = vsi_iommu_of_xlate,
> > > > > > +	.probe_device = vsi_iommu_probe_device,
> > > > > > +	.release_device = vsi_iommu_release_device,
> > > > > > +	.device_group = generic_single_device_group,
> > > > > > +	.owner = THIS_MODULE,
> > > > > > +	.default_domain_ops = &(const struct iommu_domain_ops) {
> > > > > > +		.attach_dev		= vsi_iommu_attach_device,
> > > > > > +		.map_pages		= vsi_iommu_map,
> > > > > > +		.unmap_pages		= vsi_iommu_unmap,
> > > > > > +		.flush_iotlb_all	= vsi_iommu_flush_tlb_all,
> > > > > This has no callers and so your unmap routine appears to be broken.
> > > > It is a leftover of previous attempt to allow video decoder to clean/flush
> > > > the iommu by using a function from the API.
> > > > Now it is using vsi_iommu_restore_ctx().
> > > > I while remove it in version 12.
> > > Don't you still need some invalidation on the unmap path?
> > In vsi_iommu_unmap_iova() page is invalided by calling vsi_mk_pte_invalid().
> But that just writes an invalid descriptor and doesn't appear to invalidate
> the TLB at all.
>
> > That clear BIT(0) so the hardware knows the page is invalid.
> > Do I have miss something here ?
> Yes, the TLB structure needs to be invalidated so that the page-table
> walker sees the new value that you have written in memory.
>
> The rockchip driver gets this correct...

Rockchip hardware have a ZAP_ONE_LINE register which didn't exist on Verisilicon
hardware.
I have tried to use VSI_MMU_BIT_FLUSH on VSI driver after unmapping iova
but it doesn't work.
So far calling dma_sync_single_for_device() seems to be enough to make iommu
and video decoder work together.

Regards,
Benjamin

> Will