linux-kernel - Re: [PATCH v7 4/6] media: verisilicon: AV1: Restore IOMMU context before decoding a frame

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <01c327e8353bb5b986ef6fb1e7311437659aea4a.camel@collabora.com>
Date: Mon, 25 Aug 2025 13:50:16 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Jason Gunthorpe <jgg@...pe.ca>, Benjamin Gaignard
	 <benjamin.gaignard@...labora.com>
Cc: joro@...tes.org, will@...nel.org, robin.murphy@....com, robh@...nel.org,
 	krzk+dt@...nel.org, conor+dt@...nel.org, heiko@...ech.de,
 p.zabel@...gutronix.de, 	mchehab@...nel.org, iommu@...ts.linux.dev,
 devicetree@...r.kernel.org, 	linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, 	linux-rockchip@...ts.infradead.org,
 kernel@...labora.com, 	linux-media@...r.kernel.org
Subject: Re: [PATCH v7 4/6] media: verisilicon: AV1: Restore IOMMU context
 before decoding a frame

Hi Benjmain, Jason,

Le lundi 25 août 2025 à 14:05 -0300, Jason Gunthorpe a écrit :
> On Mon, Aug 25, 2025 at 05:34:43PM +0200, Benjamin Gaignard wrote:
> > diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h
> > index 81328c63b796..a28a181013b9 100644
> > --- a/drivers/media/platform/verisilicon/hantro.h
> > +++ b/drivers/media/platform/verisilicon/hantro.h
> > @@ -12,6 +12,9 @@
> >  #ifndef HANTRO_H_
> >  #define HANTRO_H_
> >  
> > +#include <linux/dma-map-ops.h>
> > +#include <linux/iommu.h>
> > +#include <linux/iommu-dma.h>
> 
> This is an internal header it should not be included in drivers.
> 
> > +static void rockchip_vpu981_av1_restore_iommu(struct hantro_ctx *ctx)
> > +{
> > +	if (ctx->iommu_domain) {
> > +		iommu_attach_device(ctx->iommu_domain, ctx->dev->v4l2_dev.dev);
> > +		iommu_detach_device(ctx->iommu_domain, ctx->dev->v4l2_dev.dev);
> > +	}
> > +}
> 
> What is this supposed to do? Put it back to the default domain? Who
> changed it away from the default domain?

If you rename the iommu_domain into empty_domain it adds a bit of sense. When
you attach another domain (this one is empty, but it does not matter) and detach
it, the default domain get restored. That effectively forces the reprogramming
(in the VSI case flushing) of the iommu configuration.

> 
> Did some other driver change the attached domain (if so that's wild
> and wrong)? The commit message hints at that but it should be
> explained alot more.
> 
> This just seems wrong and goofy. Driver shouldn't be changing their
> iommu domains if they are using the default domain at all. We now have
> APIs to allow you to allocate wide chunks of IOVA space and manage
> them directly. Maybe these 'multiple stream's should be doing that
> instead of mucking with iommu domains?

This is seeking inspiration of what we do in rkvdec [0], the iommu-dma.h should
not be needed.

Jason, the point is that the iommu and the VPU are not separate devices, which
comes with side effects. On RKVDec side, the iommu configuration get resets
whenever a decoding error leads to a VPU "self reset". I can't remember who from
the iommu subsystem suggested that, but the empty domain method was agreed to be
the least invasive way to workaround that issue. I believe Detlev tried multiple
time to add APIs for that before the discussion lead to this path.

Benjamin, please improve the naming, comment and description, I agree with Jason
its not completely clear. I'm also surprised that you do that every frame, seems
excessive.

regards,
Nicolas

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ff8c5622f9f7c644e995d013af320b59e4d61b93

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)