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

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

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@collabora.com>
>> ---
>> 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@collabora.com/
>
> 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.

Thanks for your review.
Benjamin

>
> Will