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

[Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>]

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@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.

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.


> @@ -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.


> --- 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?


Thanks,

Lukas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx