[PATCH 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function

[jimqu]

在 2018/7/13 17:27, Lukas Wunner 写道:
> On Fri, Jul 13, 2018 at 04:06:02PM +0800, Jim Qu wrote:
>> On modern laptop, there are more and more platforms
>> have two GPUs, and each of them maybe have audio codec
>> for HDMP/DP output. For some dGPU which is no output,
>> audio codec usually is disabled.
>>
>> In currect HDA audio driver, it will set all codec as
>> VGA_SWITCHEROO_DIS, the audio which is binded to UMA
>> will be suspended if user use debugfs to contorl power
>>
>> In HDA driver side, it is difficult to know which GPU
>> the audio has binded to. So set the bound gpu pci dev
>> to vgaswitchroo, the correct audio id will be set in
>> vgaswitchreoo enable function.
>>
>> Signed-off-by: Jim Qu <Jim.Qu at amd.com>
> The approach you've taken in this patch won't work if the HDA controller
> is bound to a driver after both GPUs have already been bound to a driver
> and the handler has already been registered.
>
> That's because vga_switcheroo_enable() is run when two GPUs have registered
> as clients and the handler has also registered.  If the HDA controller is
> probed afterwards, its ID will be stuck at VGA_SWITCHEROO_UNKNOWN_ID.

In genernic speaking, there are two cases , a. audio client is the third 
client. b. audio client is not the third client.

if audio is third client. vga_switcheroo_ready() is true, 
vga_switcheroo_enable() can be called in audio client register fuction.
if audio is not the third client, vga_switcheroo_enable() will be called 
in the second GFX client register.
In vga_switcheroo_enable() , the first list loop will set two GPU 
clients' id, and the second list loop will select audio client, set id 
by its bound gpu pci dev.

> Before resubmitting this patch, please run it through scripts/checkpatch.pl,
> there are multiple trivial issues here.
>
> Also, please use scripts/get_maintainers.pl on all files you're touching
> and cc the patch to the listed maintainers and reviewers.
>
OK, will do it in next v2.
>> @@ -161,9 +163,8 @@ struct vgasr_priv {
>>   };
>>   
>>   #define ID_BIT_AUDIO		0x100
>> -#define client_is_audio(c)	((c)->id & ID_BIT_AUDIO)
>> -#define client_is_vga(c)	((c)->id == VGA_SWITCHEROO_UNKNOWN_ID || \
>> -				 !client_is_audio(c))
>> +#define client_is_audio(c)		((c)->id & ID_BIT_AUDIO)
>> +#define client_is_vga(c)		(!client_is_audio(c))
>>   #define client_id(c)		((c)->id & ~ID_BIT_AUDIO)
> There's a spurious whitespace change in the client_is_audio() line,
> please get rid of it.
>

Will update it in v2.

>> --- a/include/linux/vga_switcheroo.h
>> +++ b/include/linux/vga_switcheroo.h
>> @@ -84,8 +84,8 @@ enum vga_switcheroo_state {
>>    * Client identifier. Audio clients use the same identifier & 0x100.
>>    */
>>   enum vga_switcheroo_client_id {
>> -	VGA_SWITCHEROO_UNKNOWN_ID = -1,
>> -	VGA_SWITCHEROO_IGD,
>> +	VGA_SWITCHEROO_UNKNOWN_ID = 0x1000,
>> +	VGA_SWITCHEROO_IGD = 0,
> Hm, why is this change necessary?
Before , we suppose  VGA_SWITCHEROO_UNKNOWN_ID can be only used for gpu 
client, and now we also use it on the audio client. if we set 
VGA_SWITCHEROO_UNKNOWN_ID to gpu clients, client_is_audio(client) will 
be true, since -1 in memory is presented as 0xffffffffffffffff(if it is 
64bit asic).

thanks
JimQu
>
> Thanks,
>
> Lukas