Date:   Wed, 5 Jul 2023 22:09:00 -0500
From:   Mario Limonciello <mario.limonciello@....com>
To:     "Quan, Evan" <Evan.Quan@....com>, Andrew Lunn <andrew@...n.ch>
Cc:     "rafael@...nel.org" <rafael@...nel.org>,
        "lenb@...nel.org" <lenb@...nel.org>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Koenig, Christian" <Christian.Koenig@....com>,
        "Pan, Xinhui" <Xinhui.Pan@....com>,
        "airlied@...il.com" <airlied@...il.com>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        "johannes@...solutions.net" <johannes@...solutions.net>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "mdaenzer@...hat.com" <mdaenzer@...hat.com>,
        "maarten.lankhorst@...ux.intel.com" 
        <maarten.lankhorst@...ux.intel.com>,
        "tzimmermann@...e.de" <tzimmermann@...e.de>,
        "hdegoede@...hat.com" <hdegoede@...hat.com>,
        "jingyuwang_vip@....com" <jingyuwang_vip@....com>,
        "Lazar, Lijo" <Lijo.Lazar@....com>,
        "jim.cromie@...il.com" <jim.cromie@...il.com>,
        "bellosilicio@...il.com" <bellosilicio@...il.com>,
        "andrealmeid@...lia.com" <andrealmeid@...lia.com>,
        "trix@...hat.com" <trix@...hat.com>,
        "jsg@....id.au" <jsg@....id.au>, "arnd@...db.de" <arnd@...db.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF
 mitigations

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@...n.ch>
>> Sent: Tuesday, July 4, 2023 9:07 PM
>> To: Quan, Evan <Evan.Quan@....com>
>> Cc: rafael@...nel.org; lenb@...nel.org; Deucher, Alexander
>> <Alexander.Deucher@....com>; Koenig, Christian
>> <Christian.Koenig@....com>; Pan, Xinhui <Xinhui.Pan@....com>;
>> airlied@...il.com; daniel@...ll.ch; johannes@...solutions.net;
>> davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
>> pabeni@...hat.com; Limonciello, Mario <Mario.Limonciello@....com>;
>> mdaenzer@...hat.com; maarten.lankhorst@...ux.intel.com;
>> tzimmermann@...e.de; hdegoede@...hat.com; jingyuwang_vip@....com;
>> Lazar, Lijo <Lijo.Lazar@....com>; jim.cromie@...il.com;
>> bellosilicio@...il.com; andrealmeid@...lia.com; trix@...hat.com;
>> jsg@....id.au; arnd@...db.de; linux-kernel@...r.kernel.org; linux-
>> acpi@...r.kernel.org; amd-gfx@...ts.freedesktop.org; dri-
>> devel@...ts.freedesktop.org; linux-wireless@...r.kernel.org;
>> netdev@...r.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
> 

Powered by blists - more mailing lists