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

[Mario Limonciello <superm1@kernel.org>]

On 9/28/26 12:27, Jason Gunthorpe wrote:
> 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.

I believe you're right.  This was an artifact from the tracking I had to 
do when it was a two driver solution.  I'll rework 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..

OK.

> 
> 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)
> 
Yeah; it's only for the privileged integrated GPU to access system memory.

I'll leave a comment around it in a v2.