Re: [PATCH 11/16] media: rockchip: rga: add iommu restore function

[Sven Püschel]

On 10/7/25 20:30, Nicolas Dufresne wrote:
> Hi,
>
> Le mardi 07 octobre 2025 à 10:32 +0200, Sven Püschel a écrit :
> > Add an iommu restore function in preparation for the rga3 addition.
> > This is necessary for a soft reset, as the rga3 will also reset
> > it's iommu paging table to 0 and disable paging.
> >
> > The empty domain attach/detach to restore the iommu is copied
> > from the rkvdec driver.
> We did receive negative feedback after the fact on this one. We will likely
> upset further the iommu subsystem maintainers with that. Have you considered
> adding a restore function in the rkiommu driver, similar to TI mmu and Benjamin
> VSI MMU proposal ?
>
> I have no precise objection, I know it works, but adding a restore function
> seems also pretty straight forward.

I haven't considered adding an restore function. I've implemented this 
to handle potential command stream failures like scaling beyond the 
supported 8x factor. I'll probably drop this for now to keep it simple 
and instead correctly announce the constraints to avoid creating invalid 
commands in the first place.

> > Signed-off-by: Sven Püschel <s.pueschel@xxxxxxxxxxxxxx>
> > ---
> >   drivers/media/platform/rockchip/rga/rga.c | 24 ++++++++++++++++++++++++
> >   drivers/media/platform/rockchip/rga/rga.h |  7 +++++++
> >   2 files changed, 31 insertions(+)
> >
> > diff --git a/drivers/media/platform/rockchip/rga/rga.c
> > b/drivers/media/platform/rockchip/rga/rga.c
> > index
> > cd4da01645611e5fb51ed94e09b5f1463dad72c5..0a725841b0cfa41bbc5b861b8f5ceac2452f
> > c2b5 100644
> > --- a/drivers/media/platform/rockchip/rga/rga.c
> > +++ b/drivers/media/platform/rockchip/rga/rga.c
> > @@ -9,6 +9,7 @@
> >   #include <linux/delay.h>
> >   #include <linux/fs.h>
> >   #include <linux/interrupt.h>
> > +#include <linux/iommu.h>
> >   #include <linux/module.h>
> >   #include <linux/of.h>
> >   #include <linux/pm_runtime.h>
> > @@ -560,6 +561,19 @@ static const struct video_device rga_videodev = {
> >   	.device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING,
> >   };
> >   
> > +void rga_iommu_restore(struct rockchip_rga *rga)
> > +{
> > +	if (rga->empty_domain) {
> > +		/*
> > +		 * To rewrite mapping into the attached IOMMU core, attach a
> > new empty domain that
> > +		 * will program an empty table, then detach it to restore the
> > default domain and
> > +		 * all cached mappings.
> > +		 */
> > +		iommu_attach_device(rga->empty_domain, rga->dev);
> > +		iommu_detach_device(rga->empty_domain, rga->dev);
> > +	}
> > +}
> > +
> >   static int rga_parse_dt(struct rockchip_rga *rga)
> >   {
> >   	struct reset_control *core_rst, *axi_rst, *ahb_rst;
> > @@ -657,6 +671,13 @@ static int rga_probe(struct platform_device *pdev)
> >   		goto err_put_clk;
> >   	}
> >   
> > +	if (iommu_get_domain_for_dev(rga->dev)) {
> > +		rga->empty_domain = iommu_paging_domain_alloc(rga->dev);
> > +
> > +		if (!rga->empty_domain)
> Its an error pointer, see:
>
> https://gitlab.freedesktop.org/linux-media/media-committers/-/commit/6347dc7fb967521a77f9ff0774d25ef0cca4c6cd
>
> > +			dev_warn(rga->dev, "cannot alloc new empty
> > domain\n");
> > +	}
> > +
> >   	ret = v4l2_device_register(&pdev->dev, &rga->v4l2_dev);
> >   	if (ret)
> >   		goto err_put_clk;
> > @@ -741,6 +762,9 @@ static void rga_remove(struct platform_device *pdev)
> >   	v4l2_device_unregister(&rga->v4l2_dev);
> >   
> >   	pm_runtime_disable(rga->dev);
> > +
> > +	if (rga->empty_domain)
> > +		iommu_domain_free(rga->empty_domain);
> >   }
> >   
> >   static int __maybe_unused rga_runtime_suspend(struct device *dev)
> > diff --git a/drivers/media/platform/rockchip/rga/rga.h
> > b/drivers/media/platform/rockchip/rga/rga.h
> > index
> > fc4805ba4e8ef7fb311f780a198ba6ba4d3aff17..e19c4c82aca5ae2056f52d525138093fbbb8
> > 1af8 100644
> > --- a/drivers/media/platform/rockchip/rga/rga.h
> > +++ b/drivers/media/platform/rockchip/rga/rga.h
> > @@ -75,6 +75,7 @@ struct rockchip_rga {
> >   	void __iomem *regs;
> >   	struct clk_bulk_data clks[3];
> >   	struct rockchip_rga_version version;
> > +	struct iommu_domain *empty_domain;
> >   
> >   	/* vfd lock */
> >   	struct mutex mutex;
> > @@ -114,6 +115,12 @@ static inline struct rga_vb_buffer *vb_to_rga(struct
> > vb2_v4l2_buffer *vb)
> >   
> >   struct rga_frame *rga_get_frame(struct rga_ctx *ctx, enum v4l2_buf_type
> > type);
> >   
> > +/*
> > + * This should be called in an interrupt handler to make sure no memory
> > + * is mapped through the IOMMU while the empty domain is attached.
> > + */
> > +void rga_iommu_restore(struct rockchip_rga *rga);
> > +
> >   /* RGA Buffers Manage */
> >   extern const struct vb2_ops rga_qops;
> >