On 7/5/23 21:58, Quan, Evan wrote:
[AMD Official Use Only - General]
Hi Andrew,
I discussed with Mario about your proposal/concerns here.
We believe some changes below might address your concerns.
- place/move the wbrf_supported_producer check inside acpi_amd_wbrf_add_exclusion and acpi_amd_wbrf_add_exclusion
- place the wbrf_supported_consumer check inside acpi_amd_wbrf_retrieve_exclusions
So that the wbrf_supported_producer and wbrf_supported_consumer can be dropped.
We made some prototypes and even performed some tests which showed technically it is absolutely practicable.
However, we found several issues with that.
- The overhead caused by the extra _producer/_consumer check on every calling of wbrf_add/remove/retrieve_ecxclusion.
Especially when you consider there might be multiple producers and consumers in the system at the same time. And some of
them might do in-use band/frequency switching frequently.
One more piece of overhead that is in this same theme that Evan didn't
mention is the case of a system "without" AMD's ACPI WBRF but the kernel
was configured with it enabled. Think like a distro kernel.
Moving it into add/remove exclusion would mean that every single time
frequency changed by a producer the _DSM would attempt to be evaluated
and fail. To avoid that extra call overhead after the first time would
mean needing to keep a variable somewhere, and at that point what did
you save?
- Some extra costs caused by the "know it only at the last minute". For example, to support WBRF, amdgpu driver needs some preparations: install the notification hander,
setup the delay workqueue(to handle possible events flooding) and even notify firmware engine to be ready. However, only on the 1st notification receiving,
it is realized(reported by wbrf_supported_consumer check) the WBRF feature is actually not supported. All those extra costs can be actually avoided if we can know the WBRF is not supported at first.
This could happen to other consumers and producers too.
After a careful consideration, we think the changes do not benefit us much. It does not deserve us to spend extra efforts.
Thus we would like to stick with original implementations. That is to have wbrf_supported_producer and wbrf_supported_consumer interfaces exposed.
Then other drivers/subsystems can do necessary wbrf support check in advance and coordinate their actions accordingly.
Please let us know your thoughts.
BR,
Evan
-----Original Message-----
From: Andrew Lunn <andrew@xxxxxxx>
Sent: Tuesday, July 4, 2023 9:07 PM
To: Quan, Evan <Evan.Quan@xxxxxxx>
Cc: rafael@xxxxxxxxxx; lenb@xxxxxxxxxx; Deucher, Alexander
<Alexander.Deucher@xxxxxxx>; Koenig, Christian
<Christian.Koenig@xxxxxxx>; Pan, Xinhui <Xinhui.Pan@xxxxxxx>;
airlied@xxxxxxxxx; daniel@xxxxxxxx; johannes@xxxxxxxxxxxxxxxx;
davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
pabeni@xxxxxxxxxx; Limonciello, Mario <Mario.Limonciello@xxxxxxx>;
mdaenzer@xxxxxxxxxx; maarten.lankhorst@xxxxxxxxxxxxxxx;
tzimmermann@xxxxxxx; hdegoede@xxxxxxxxxx; jingyuwang_vip@xxxxxxx;
Lazar, Lijo <Lijo.Lazar@xxxxxxx>; jim.cromie@xxxxxxxxx;
bellosilicio@xxxxxxxxx; andrealmeid@xxxxxxxxxx; trix@xxxxxxxxxx;
jsg@xxxxxxxxx; arnd@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
acpi@xxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-
devel@xxxxxxxxxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx;
netdev@xxxxxxxxxxxxxxx
Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF
mitigations
What is the purpose of this stage? Why would it not be supported for
this device?
This is needed for wbrf support via ACPI mechanism. If BIOS(AML code)
does not support the wbrf adding/removing for some device, it should
speak that out so that the device can be aware of that.
How much overhead is this adding? How deep do you need to go to find the
BIOS does not support it? And how often is this called?
Where do we want to add complexity? In the generic API? Or maybe a little
deeper in the ACPI specific code?
Andrew
