Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations

From: Mario Limonciello <mario.limonciello@amd.com>
To: "Quan, Evan" <Evan.Quan@amd.com>, Andrew Lunn <andrew@lunn.ch>
Cc: "rafael@kernel.org" <rafael@kernel.org>,
	"lenb@kernel.org" <lenb@kernel.org>,
	"Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"Koenig, Christian" <Christian.Koenig@amd.com>,
	"Pan, Xinhui" <Xinhui.Pan@amd.com>,
	"airlied@gmail.com" <airlied@gmail.com>,
	"daniel@ffwll.ch" <daniel@ffwll.ch>,
	"johannes@sipsolutions.net" <johannes@sipsolutions.net>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"edumazet@google.com" <edumazet@google.com>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"pabeni@redhat.com" <pabeni@redhat.com>,
	"mdaenzer@redhat.com" <mdaenzer@redhat.com>,
	"maarten.lankhorst@linux.intel.com" 
	<maarten.lankhorst@linux.intel.com>,
	"tzimmermann@suse.de" <tzimmermann@suse.de>,
	"hdegoede@redhat.com" <hdegoede@redhat.com>,
	"jingyuwang_vip@163.com" <jingyuwang_vip@163.com>,
	"Lazar, Lijo" <Lijo.Lazar@amd.com>,
	"jim.cromie@gmail.com" <jim.cromie@gmail.com>,
	"bellosilicio@gmail.com" <bellosilicio@gmail.com>,
	"andrealmeid@igalia.com" <andrealmeid@igalia.com>,
	"trix@redhat.com" <trix@redhat.com>,
	"jsg@jsg.id.au" <jsg@jsg.id.au>, "arnd@arndb.de" <arnd@arndb.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
Date: Wed, 5 Jul 2023 22:09:00 -0500	[thread overview]
Message-ID: <f2612b12-5c9d-dcba-2221-220261ac7b44@amd.com> (raw)
In-Reply-To: <DM6PR12MB26198720EBBAAB8C989F8D4BE42CA@DM6PR12MB2619.namprd12.prod.outlook.com>

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@lunn.ch>
>> Sent: Tuesday, July 4, 2023 9:07 PM
>> To: Quan, Evan <Evan.Quan@amd.com>
>> Cc: rafael@kernel.org; lenb@kernel.org; Deucher, Alexander
>> <Alexander.Deucher@amd.com>; Koenig, Christian
>> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>;
>> airlied@gmail.com; daniel@ffwll.ch; johannes@sipsolutions.net;
>> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
>> pabeni@redhat.com; Limonciello, Mario <Mario.Limonciello@amd.com>;
>> mdaenzer@redhat.com; maarten.lankhorst@linux.intel.com;
>> tzimmermann@suse.de; hdegoede@redhat.com; jingyuwang_vip@163.com;
>> Lazar, Lijo <Lijo.Lazar@amd.com>; jim.cromie@gmail.com;
>> bellosilicio@gmail.com; andrealmeid@igalia.com; trix@redhat.com;
>> jsg@jsg.id.au; arnd@arndb.de; linux-kernel@vger.kernel.org; linux-
>> acpi@vger.kernel.org; amd-gfx@lists.freedesktop.org; dri-
>> devel@lists.freedesktop.org; linux-wireless@vger.kernel.org;
>> netdev@vger.kernel.org
>> 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
> 