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

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

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@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.
>>> 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
>