Re: [PATCH] iommu/amd: Make PerfOpt compulsory for APUs in identity

[Jason Gunthorpe <jgg@ziepe.ca>]

On Sun, Sep 27, 2026 at 11:50:50PM -0500, Mario Limonciello (AMD) wrote:
> PerfOpt is only a feature usable by integrated GPUs and only in identity
> mode.  Instead of leaving a policy knob in amdgpu, just turn it on when
> an integrated GPU in an APU is in identity. Re-use the heuristic in
> amd_iommu_def_domain_type() to make this decision.
> 
> This drops quite a bit of compatibility glue.  There was a refcounting
> system, exported symbols, and device attach/detach logic.  By just setting
> it immediately it's a lot more straightforward.
> 
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   1 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  50 -----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  12 --
>  drivers/iommu/amd/amd_iommu.h              |   2 +-
>  drivers/iommu/amd/amd_iommu_types.h        |   3 -
>  drivers/iommu/amd/init.c                   |   3 +-
>  drivers/iommu/amd/iommu.c                  | 234 ++++-----------------
>  include/linux/amd-iommu.h                  |  11 -
>  8 files changed, 48 insertions(+), 268 deletions(-)

Diffing across the originals to net them out it is much smaller:

 5 files changed, 122 insertions(+), 1 deletion(-)

And I think this is much better , but I have a few questions

Why is this setting and clearing perf_opt in dev_data?

I expect probe to make a determination if this device has the special
path and if so then there should be a permanent flag in the dev_data.

Based on that flag amd_iommu_def_domain_type() can return identity to
override things

When the driver does an identity attachment it would enable the
perfopt and write out the right DTE for it.

Whenever the driver removes that identity it would disable the
perf_opt. These points are all marked out in the attach function flow
you don't need another variable to keep track, or the funny logic to
block things. All you want is an attached identity domain that is
"optimized".

Release goes to blocked which should already disable it, so no need to
disable it again in amd_iommu_release_device()

The repeated pattern is a bit much:
+       if (dev_data->perfopt) {
+               if (WARN_ON(amd_iommu_perfopt_clear(iommu)))
+                       dev_err(dev, "IOMMU%d: failed to clear PerfOpt on release\n",
+                               iommu->index);

Clear should probably just do the warn on and not return any error
code. It is never OK to allow this to fail..

I'm also scratching my head a bit why the global register needs to be
set/unset like this? Does that global bit completely bypass the iommu
for a single special device? With no way to discover from FW which BDF
is the special device? If this is the right guess please document this
in a comment around __perfopt_write (and again that's awful, ACPI
should have a pointer to the special device so the OS can understand
this)

Jason