Re: [PATCH 7/7] media: rkvdec: Add multicore IOMMU support

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 12646 bytes --]

Le jeudi 09 avril 2026 à 10:49 -0400, Detlev Casanova a écrit :
> Hi Nicolas,
> 
> On 4/9/26 10:19, Nicolas Dufresne wrote:
> > Le jeudi 09 avril 2026 à 09:50 -0400, Detlev Casanova a écrit :
> > > As each core has its own IOMMU core, buffers must be mapped in each
> > > core's IOMMU so that any run() call can use any core without having to
> > > remap everything.
> > > 
> > > To do that, we use rockchip iommu domain's iommu devices list.
> > > With that, one IOMMU domain can be mapped on multiple devices, meaning
> > > that each call to iommu_map() will flush the new mapping on all devices
> > > in the list.
> > > 
> > > The IOMMU domain that will have all devices in its list is the first
> > > core's default domain.
> > > 
> > > Another domain cannot be used because VB2 allocates buffers through the
> > > DMA engine, which uses iommu_get_dma_domain() to find the domain to map
> > > buffers through.
> > > 
> > > The IOMMU restore function can still work as before, but needs to be more
> > > explicit in what domain to attach the device to.
> > > That is because detaching the empty domain will reattach the core's default
> > > domain, which is wrong (except for the first "main" core).
> > > 
> > > The RCB temporary buffers are allocated in a dedicated SRAM, each
> > > core has its own SRAM, so the mapping for each core's SRAM is added in the
> > > global domain.
> > > 
> > > Everything else is mapped through the first core's default domain, making
> > > the driver write the mappings on both IOMMU cores.
> > Just raising an issue with the patch ordering here. I'm worried in a git bisect,
> > the driver will be broken until we apply this last patch. Can we make sure that
> > the driver bisects ? (or tell me if I'm wrong)
> No, you're right. That's also why I mentioned it in the cover letter. 
> This commit is also not too big, so I'll merge the last 3 commits and 
> adapt the commit message to keep the information from the 3 (the commit 
> messages are why I wanted to keep them separate, but there is no good 
> way to keep them that way)


Unless we can implement everything, but only turn the multicore switch on in a
final patch ? Basically removing the code that enable other cores at the end.
But I don't want more code just to justify this, the break nice is nice for
review fwiw, but if squashing is the way, let it be.

Nicolas

> 
> Detlev.
> > 
> > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > > ---
> > >   .../media/platform/rockchip/rkvdec/rkvdec-rcb.c    | 21 ++++++-------
> > >   .../media/platform/rockchip/rkvdec/rkvdec-rcb.h    |  6 ++--
> > >   drivers/media/platform/rockchip/rkvdec/rkvdec.c    | 35 +++++++++++++++++-----
> > >   drivers/media/platform/rockchip/rkvdec/rkvdec.h    |  2 +-
> > >   4 files changed, 44 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec-rcb.c b/drivers/media/platform/rockchip/rkvdec/rkvdec-rcb.c
> > > index 190fb7438e8c..977e37cf209b 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec-rcb.c
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec-rcb.c
> > > @@ -57,7 +57,7 @@ bool rkvdec_rcb_buf_validate_size(struct rkvdec_ctx *ctx)
> > >   	return ret;
> > >   }
> > >   
> > > -void rkvdec_free_rcb(struct rkvdec_core *core)
> > > +void rkvdec_free_rcb(struct rkvdec_dev *rkvdec, struct rkvdec_core *core)
> > >   {
> > >   	struct rkvdec_rcb_config *cfg = core->rcb_config;
> > >   	unsigned long virt_addr;
> > > @@ -76,12 +76,12 @@ void rkvdec_free_rcb(struct rkvdec_core *core)
> > >   		case RKVDEC_ALLOC_SRAM:
> > >   			virt_addr = (unsigned long)cfg->rcb_bufs[i].cpu;
> > >   
> > > -			if (core->iommu_domain)
> > > -				iommu_unmap(core->iommu_domain, virt_addr, rcb_size);
> > > +			if (rkvdec->iommu_global_domain)
> > > +				iommu_unmap(rkvdec->iommu_global_domain, virt_addr, rcb_size);
> > >   			gen_pool_free(core->sram_pool, virt_addr, rcb_size);
> > >   			break;
> > >   		case RKVDEC_ALLOC_DMA:
> > > -			dma_free_coherent(core->dev,
> > > +			dma_free_coherent(rkvdec->main_core->dev,
> > >   					  rcb_size,
> > >   					  cfg->rcb_bufs[i].cpu,
> > >   					  cfg->rcb_bufs[i].dma);
> > > @@ -97,7 +97,8 @@ void rkvdec_free_rcb(struct rkvdec_core *core)
> > >   	core->rcb_config = NULL;
> > >   }
> > >   
> > > -int rkvdec_allocate_rcb(struct rkvdec_core *core, u32 width, u32 height,
> > > +int rkvdec_allocate_rcb(struct rkvdec_dev *rkvdec, struct rkvdec_core *core,
> > > +			u32 width, u32 height,
> > >   			const struct rcb_size_info *size_info,
> > >   			size_t rcb_count)
> > >   {
> > > @@ -132,7 +133,7 @@ int rkvdec_allocate_rcb(struct rkvdec_core *core, u32 width, u32 height,
> > >   
> > >   		/* Try allocating an SRAM buffer */
> > >   		if (core->sram_pool) {
> > > -			if (core->iommu_domain)
> > > +			if (rkvdec->iommu_global_domain)
> > >   				rcb_size = ALIGN(rcb_size, SZ_4K);
> > >   
> > >   			cpu = gen_pool_dma_zalloc_align(core->sram_pool,
> > > @@ -142,11 +143,11 @@ int rkvdec_allocate_rcb(struct rkvdec_core *core, u32 width, u32 height,
> > >   		}
> > >   
> > >   		/* If an IOMMU is used, map the SRAM address through it */
> > > -		if (cpu && core->iommu_domain) {
> > > +		if (cpu && rkvdec->iommu_global_domain) {
> > >   			unsigned long virt_addr = (unsigned long)cpu;
> > >   			phys_addr_t phys_addr = dma;
> > >   
> > > -			ret = iommu_map(core->iommu_domain, virt_addr, phys_addr,
> > > +			ret = iommu_map(rkvdec->iommu_global_domain, virt_addr, phys_addr,
> > >   					rcb_size, IOMMU_READ | IOMMU_WRITE, 0);
> > >   			if (ret) {
> > >   				gen_pool_free(core->sram_pool,
> > > @@ -166,7 +167,7 @@ int rkvdec_allocate_rcb(struct rkvdec_core *core, u32 width, u32 height,
> > >   ram_fallback:
> > >   		/* Fallback to RAM */
> > >   		if (!cpu) {
> > > -			cpu = dma_alloc_coherent(core->dev,
> > > +			cpu = dma_alloc_coherent(rkvdec->main_core->dev,
> > >   						 rcb_size,
> > >   						 &dma,
> > >   						 GFP_KERNEL);
> > > @@ -189,7 +190,7 @@ int rkvdec_allocate_rcb(struct rkvdec_core *core, u32 width, u32 height,
> > >   	return 0;
> > >   
> > >   err_alloc:
> > > -	rkvdec_free_rcb(core);
> > > +	rkvdec_free_rcb(rkvdec, core);
> > >   
> > >   	return ret;
> > >   }
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec-rcb.h b/drivers/media/platform/rockchip/rkvdec/rkvdec-rcb.h
> > > index a12af9b7dc2b..d1149afe7fda 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec-rcb.h
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec-rcb.h
> > > @@ -8,6 +8,7 @@
> > >   
> > >   #include <linux/types.h>
> > >   
> > > +struct rkvdec_dev;
> > >   struct rkvdec_ctx;
> > >   struct rkvdec_core;
> > >   
> > > @@ -21,11 +22,12 @@ struct rcb_size_info {
> > >   	enum rcb_axis axis;
> > >   };
> > >   
> > > -int rkvdec_allocate_rcb(struct rkvdec_core *core, u32 width, u32 height,
> > > +int rkvdec_allocate_rcb(struct rkvdec_dev *rkvdec, struct rkvdec_core *core,
> > > +			u32 width, u32 height,
> > >   			const struct rcb_size_info *size_info,
> > >   			size_t rcb_count);
> > >   dma_addr_t rkvdec_rcb_buf_dma_addr(struct rkvdec_ctx *ctx, int id);
> > >   size_t rkvdec_rcb_buf_size(struct rkvdec_ctx *ctx, int id);
> > >   int rkvdec_rcb_buf_count(struct rkvdec_ctx *ctx);
> > >   bool rkvdec_rcb_buf_validate_size(struct rkvdec_ctx *ctx);
> > > -void rkvdec_free_rcb(struct rkvdec_core *core);
> > > +void rkvdec_free_rcb(struct rkvdec_dev *rkvdec, struct rkvdec_core *core);
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > index c2818f1575ef..2930e9b64906 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > @@ -1204,9 +1204,9 @@ static void rkvdec_device_run(void *priv)
> > >   	}
> > >   
> > >   	if (!rkvdec_rcb_buf_validate_size(ctx)) {
> > > -		rkvdec_free_rcb(ctx->core);
> > > +		rkvdec_free_rcb(ctx->dev, ctx->core);
> > >   
> > > -		ret = rkvdec_allocate_rcb(ctx->core,
> > > +		ret = rkvdec_allocate_rcb(ctx->dev, ctx->core,
> > >   					  ctx->decoded_fmt.fmt.pix_mp.width,
> > >   					  ctx->decoded_fmt.fmt.pix_mp.height,
> > >   					  ctx->dev->variant->rcb_sizes,
> > > @@ -1486,6 +1486,7 @@ static void rkvdec_v4l2_cleanup(struct rkvdec_dev *rkvdec)
> > >   
> > >   static void rkvdec_iommu_restore(struct rkvdec_core *core)
> > >   {
> > > +	int ret;
> > >   	if (core->empty_domain) {
> > >   		/*
> > >   		 * To rewrite mapping into the attached IOMMU core, attach a new empty domain that
> > > @@ -1494,8 +1495,14 @@ static void rkvdec_iommu_restore(struct rkvdec_core *core)
> > >   		 * This is safely done in this interrupt handler to make sure no memory get mapped
> > >   		 * through the IOMMU while the empty domain is attached.
> > >   		 */
> > > -		iommu_attach_device(core->empty_domain, core->dev);
> > > +		iommu_detach_device(core->curr_ctx->dev->iommu_global_domain, core->dev);
> > > +		ret = iommu_attach_device(core->empty_domain, core->dev);
> > > +		if (ret)
> > > +			dev_warn(core->dev, "Cannot attach empty domain: %d\n", ret);
> > >   		iommu_detach_device(core->empty_domain, core->dev);
> > > +		ret = iommu_attach_device(core->curr_ctx->dev->iommu_global_domain, core->dev);
> > > +		if (ret)
> > > +			dev_warn(core->dev, "Cannot attach global domain: %d\n", ret);
> > >   	}
> > >   }
> > >   
> > > @@ -1858,6 +1865,8 @@ static int rkvdec_probe(struct platform_device *pdev)
> > >   
> > >   	core = &rkvdec->cores[rkvdec->core_count++];
> > >   
> > > +	core->id = rkvdec->core_count - 1;
> > > +
> > >   	platform_set_drvdata(pdev, rkvdec);
> > >   	core->dev = &pdev->dev;
> > >   	INIT_DELAYED_WORK(&core->watchdog_work, rkvdec_watchdog_func);
> > > @@ -1883,12 +1892,24 @@ static int rkvdec_probe(struct platform_device *pdev)
> > >   			return PTR_ERR(core->link);
> > >   	}
> > >   
> > > -	core->iommu_domain = iommu_get_domain_for_dev(&pdev->dev);
> > > -	if (core->iommu_domain) {
> > > +	if (iommu_get_domain_for_dev(&pdev->dev)) {
> > >   		core->empty_domain = iommu_paging_domain_alloc(core->dev);
> > >   
> > > -		if (!core->empty_domain)
> > > +		if (IS_ERR(core->empty_domain))
> > >   			dev_warn(core->dev, "cannot alloc new empty domain\n");
> > > +
> > > +		if (!rkvdec->iommu_global_domain) {
> > > +			rkvdec->iommu_global_domain = iommu_get_domain_for_dev(core->dev);
> > > +
> > > +			if (IS_ERR(rkvdec->iommu_global_domain)) {
> > > +				rkvdec->iommu_global_domain = NULL;
> > > +				dev_warn_once(core->dev, "cannot alloc new global domain\n");
> > > +			}
> > > +		}
> > > +
> > > +		ret = iommu_attach_device(rkvdec->iommu_global_domain, core->dev);
> > > +		if (ret)
> > > +			dev_warn(core->dev, "cannot attach global domain to core %d\n", core->id);
> > >   	}
> > >   
> > >   	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> > > @@ -1961,7 +1982,7 @@ static void rkvdec_remove(struct platform_device *pdev)
> > >   		if (rkvdec->cores[i].empty_domain)
> > >   			iommu_domain_free(rkvdec->cores[i].empty_domain);
> > >   
> > > -		rkvdec_free_rcb(&rkvdec->cores[i]);
> > > +		rkvdec_free_rcb(rkvdec, &rkvdec->cores[i]);
> > >   	}
> > >   }
> > >   
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.h b/drivers/media/platform/rockchip/rkvdec/rkvdec.h
> > > index 4f042a367dc0..ccd766b220c7 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.h
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.h
> > > @@ -135,7 +135,6 @@ struct rkvdec_core {
> > >   	void __iomem *link;
> > >   	struct delayed_work watchdog_work;
> > >   	struct gen_pool *sram_pool;
> > > -	struct iommu_domain *iommu_domain;
> > >   	struct iommu_domain *empty_domain;
> > >   	struct rkvdec_rcb_config *rcb_config;
> > >   	struct rkvdec_ctx *curr_ctx;
> > > @@ -155,6 +154,7 @@ struct rkvdec_dev {
> > >   	unsigned int available_core_count;
> > >   	spinlock_t cores_lock; /* serializes core list access */
> > >   	struct rkvdec_core *main_core;
> > > +	struct iommu_domain *iommu_global_domain;
> > >   };
> > >   
> > >   struct rkvdec_ctx {

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]