diff mbox series

[RFC,9/9] drm/amdgpu: register a vga_switcheroo client for all GPUs that are not thunderbolt attached

Message ID 20230210044826.9834-10-orlandoch.dev@gmail.com
State New
Headers
Series apple-gmux: support MMIO gmux type on T2 Macs |

Commit Message

Orlando Chamberlain Feb. 10, 2023, 4:48 a.m. UTC 
  From: Kerem Karabay <kekrby@gmail.com>

Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
vga_switcheroo") made amdgpu only register a vga_switcheroo client for
GPU's with PX, however AMD GPUs in dual gpu Apple Macbooks do need to
register, but don't have PX. Instead of AMD's PX, they use apple-gmux.

Revert to the old logic of registering for all non-thunderbolt gpus,
like radeon and nouveau.

Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and vga_switcheroo")
Signed-off-by: Kerem Karabay <kekrby@gmail.com>
[Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit description]
Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Alex Deucher Feb. 10, 2023, 3:53 p.m. UTC | #1 
On Fri, Feb 10, 2023 at 3:04 AM Orlando Chamberlain
<orlandoch.dev@gmail.com> wrote:
>
> From: Kerem Karabay <kekrby@gmail.com>
>
> Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
> vga_switcheroo") made amdgpu only register a vga_switcheroo client for
> GPU's with PX, however AMD GPUs in dual gpu Apple Macbooks do need to
> register, but don't have PX. Instead of AMD's PX, they use apple-gmux.

Is there a way to detect apple-gmux instead?  Otherwise, we register
vga_switcheroo on any system with multiple GPUs which is not what we
want.

Alex

>
> Revert to the old logic of registering for all non-thunderbolt gpus,
> like radeon and nouveau.
>
> Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and vga_switcheroo")
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> [Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit description]
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 2f28a8c02f64..0bb553a61552 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3919,12 +3919,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>                 vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
>
> -       if (amdgpu_device_supports_px(ddev)) {
> -               px = true;
> -               vga_switcheroo_register_client(adev->pdev,
> -                                              &amdgpu_switcheroo_ops, px);
> +       px = amdgpu_device_supports_px(ddev);
> +
> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> +               vga_switcheroo_register_client(adev->pdev, &amdgpu_switcheroo_ops, px);
> +
> +       if (px)
>                 vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
> -       }
>
>         if (adev->gmc.xgmi.pending_reset)
>                 queue_delayed_work(system_wq, &mgpu_info.delayed_reset_work,
> @@ -4048,10 +4049,13 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
>
>         kfree(adev->bios);
>         adev->bios = NULL;
> -       if (amdgpu_device_supports_px(adev_to_drm(adev))) {
> +
> +       if (!pci_is_thunderbolt_attached(adev->pdev))
>                 vga_switcheroo_unregister_client(adev->pdev);
> +
> +       if (amdgpu_device_supports_px(adev_to_drm(adev)))
>                 vga_switcheroo_fini_domain_pm_ops(adev->dev);
> -       }
> +
>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>                 vga_client_unregister(adev->pdev);
>
> --
> 2.39.1
>
Hans de Goede Feb. 10, 2023, 4:07 p.m. UTC | #2 
Hi,

On 2/10/23 16:53, Alex Deucher wrote:
> On Fri, Feb 10, 2023 at 3:04 AM Orlando Chamberlain
> <orlandoch.dev@gmail.com> wrote:
>>
>> From: Kerem Karabay <kekrby@gmail.com>
>>
>> Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
>> vga_switcheroo") made amdgpu only register a vga_switcheroo client for
>> GPU's with PX, however AMD GPUs in dual gpu Apple Macbooks do need to
>> register, but don't have PX. Instead of AMD's PX, they use apple-gmux.
> 
> Is there a way to detect apple-gmux instead?  Otherwise, we register
> vga_switcheroo on any system with multiple GPUs which is not what we
> want.

Yes since 6.1.y (either stable series or just take 6.2.0) the apple-gmux
detect code has been factored out into a stand-alone
apple_gmux_detect() helper inside:

include/linux/apple-gmux.h

For usage outside of the actual apple-gmux driver you can simply
pass NULL for both arguments.

This was necessary to reliably check if the apple-gmux should be
used for backlight control.

Note there also is the older apple_gmux_present() helper, which is
already used in some drm code. That function is not reliable though
it detects if the ACPI tables contain an ACPI device describing
the presence of a gmux, but it turns out even Apple has buggy ACPI
tables and the mere presence of that ACPI device is not a reliable
indicator the gmux is actually there.

I have not changed over any of the existing apple_gmux_present()
users for fear of unwanted side effects...

Regards,

Hans




>> Revert to the old logic of registering for all non-thunderbolt gpus,
>> like radeon and nouveau.
>>
>> Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and vga_switcheroo")
>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
>> [Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit description]
>> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 2f28a8c02f64..0bb553a61552 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3919,12 +3919,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>>                 vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
>>
>> -       if (amdgpu_device_supports_px(ddev)) {
>> -               px = true;
>> -               vga_switcheroo_register_client(adev->pdev,
>> -                                              &amdgpu_switcheroo_ops, px);
>> +       px = amdgpu_device_supports_px(ddev);
>> +
>> +       if (!pci_is_thunderbolt_attached(adev->pdev))
>> +               vga_switcheroo_register_client(adev->pdev, &amdgpu_switcheroo_ops, px);
>> +
>> +       if (px)
>>                 vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
>> -       }
>>
>>         if (adev->gmc.xgmi.pending_reset)
>>                 queue_delayed_work(system_wq, &mgpu_info.delayed_reset_work,
>> @@ -4048,10 +4049,13 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
>>
>>         kfree(adev->bios);
>>         adev->bios = NULL;
>> -       if (amdgpu_device_supports_px(adev_to_drm(adev))) {
>> +
>> +       if (!pci_is_thunderbolt_attached(adev->pdev))
>>                 vga_switcheroo_unregister_client(adev->pdev);
>> +
>> +       if (amdgpu_device_supports_px(adev_to_drm(adev)))
>>                 vga_switcheroo_fini_domain_pm_ops(adev->dev);
>> -       }
>> +
>>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>>                 vga_client_unregister(adev->pdev);
>>
>> --
>> 2.39.1
>>
>
Alex Deucher Feb. 10, 2023, 4:37 p.m. UTC | #3 
On Fri, Feb 10, 2023 at 11:07 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 2/10/23 16:53, Alex Deucher wrote:
> > On Fri, Feb 10, 2023 at 3:04 AM Orlando Chamberlain
> > <orlandoch.dev@gmail.com> wrote:
> >>
> >> From: Kerem Karabay <kekrby@gmail.com>
> >>
> >> Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
> >> vga_switcheroo") made amdgpu only register a vga_switcheroo client for
> >> GPU's with PX, however AMD GPUs in dual gpu Apple Macbooks do need to
> >> register, but don't have PX. Instead of AMD's PX, they use apple-gmux.
> >
> > Is there a way to detect apple-gmux instead?  Otherwise, we register
> > vga_switcheroo on any system with multiple GPUs which is not what we
> > want.
>
> Yes since 6.1.y (either stable series or just take 6.2.0) the apple-gmux
> detect code has been factored out into a stand-alone
> apple_gmux_detect() helper inside:
>
> include/linux/apple-gmux.h
>
> For usage outside of the actual apple-gmux driver you can simply
> pass NULL for both arguments.
>
> This was necessary to reliably check if the apple-gmux should be
> used for backlight control.
>
> Note there also is the older apple_gmux_present() helper, which is
> already used in some drm code. That function is not reliable though
> it detects if the ACPI tables contain an ACPI device describing
> the presence of a gmux, but it turns out even Apple has buggy ACPI
> tables and the mere presence of that ACPI device is not a reliable
> indicator the gmux is actually there.
>
> I have not changed over any of the existing apple_gmux_present()
> users for fear of unwanted side effects...

Looks like we could maybe use the PWRD ACPI check like patch 8 does as well.

Alex

>
> Regards,
>
> Hans
>
>
>
>
> >> Revert to the old logic of registering for all non-thunderbolt gpus,
> >> like radeon and nouveau.
> >>
> >> Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and vga_switcheroo")
> >> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> >> [Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit description]
> >> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +++++++++++-------
> >>  1 file changed, 11 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index 2f28a8c02f64..0bb553a61552 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -3919,12 +3919,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> >>                 vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
> >>
> >> -       if (amdgpu_device_supports_px(ddev)) {
> >> -               px = true;
> >> -               vga_switcheroo_register_client(adev->pdev,
> >> -                                              &amdgpu_switcheroo_ops, px);
> >> +       px = amdgpu_device_supports_px(ddev);
> >> +
> >> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> >> +               vga_switcheroo_register_client(adev->pdev, &amdgpu_switcheroo_ops, px);
> >> +
> >> +       if (px)
> >>                 vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
> >> -       }
> >>
> >>         if (adev->gmc.xgmi.pending_reset)
> >>                 queue_delayed_work(system_wq, &mgpu_info.delayed_reset_work,
> >> @@ -4048,10 +4049,13 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
> >>
> >>         kfree(adev->bios);
> >>         adev->bios = NULL;
> >> -       if (amdgpu_device_supports_px(adev_to_drm(adev))) {
> >> +
> >> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> >>                 vga_switcheroo_unregister_client(adev->pdev);
> >> +
> >> +       if (amdgpu_device_supports_px(adev_to_drm(adev)))
> >>                 vga_switcheroo_fini_domain_pm_ops(adev->dev);
> >> -       }
> >> +
> >>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> >>                 vga_client_unregister(adev->pdev);
> >>
> >> --
> >> 2.39.1
> >>
> >
>
Orlando Chamberlain Feb. 10, 2023, 11:54 p.m. UTC | #4 
On Fri, 10 Feb 2023 11:37:08 -0500
Alex Deucher <alexdeucher@gmail.com> wrote:

> On Fri, Feb 10, 2023 at 11:07 AM Hans de Goede <hdegoede@redhat.com>
> wrote:
> >
> > Hi,
> >
> > On 2/10/23 16:53, Alex Deucher wrote:  
> > > On Fri, Feb 10, 2023 at 3:04 AM Orlando Chamberlain
> > > <orlandoch.dev@gmail.com> wrote:  
> > >>
> > >> From: Kerem Karabay <kekrby@gmail.com>
> > >>
> > >> Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
> > >> vga_switcheroo") made amdgpu only register a vga_switcheroo
> > >> client for GPU's with PX, however AMD GPUs in dual gpu Apple
> > >> Macbooks do need to register, but don't have PX. Instead of
> > >> AMD's PX, they use apple-gmux.  
> > >
> > > Is there a way to detect apple-gmux instead?  Otherwise, we
> > > register vga_switcheroo on any system with multiple GPUs which is
> > > not what we want.  
> >
> > Yes since 6.1.y (either stable series or just take 6.2.0) the
> > apple-gmux detect code has been factored out into a stand-alone
> > apple_gmux_detect() helper inside:
> >
> > include/linux/apple-gmux.h
> >
> > For usage outside of the actual apple-gmux driver you can simply
> > pass NULL for both arguments.
> >
> > This was necessary to reliably check if the apple-gmux should be
> > used for backlight control.
> >
> > Note there also is the older apple_gmux_present() helper, which is
> > already used in some drm code. That function is not reliable though
> > it detects if the ACPI tables contain an ACPI device describing
> > the presence of a gmux, but it turns out even Apple has buggy ACPI
> > tables and the mere presence of that ACPI device is not a reliable
> > indicator the gmux is actually there.
> >
> > I have not changed over any of the existing apple_gmux_present()
> > users for fear of unwanted side effects...  
> 
> Looks like we could maybe use the PWRD ACPI check like patch 8 does
> as well.

I wasn't using apple_gmux_detect as I mistakenly thought
pnp_get_resource would fail if apple-gmux had bound to the resource but
it looks like I was wrong about that so we can use that to determine if
the system has a gmux. I think I'll do that in v2.

As far as I know there's only one internal (non
thunderbolt) amd gpu inside all Macbooks with gmux so we probably
wouldn't need to check for PWRD to ensure it's  the right gpu.

With PWRD, I don't know if its present on all Dual GPU Macbooks, I've
only found the acpi tables for Macbookpro14,x to Macbookpro16,x, so I
don't know if it will work on older Macs (I'm also not sure if those
macs are using radeon or amdgpu).

> Alex
> 
> >
> > Regards,
> >
> > Hans
> >
> >
> >
> >  
> > >> Revert to the old logic of registering for all non-thunderbolt
> > >> gpus, like radeon and nouveau.
> > >>
> > >> Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
> > >> vga_switcheroo") Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> > >> [Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit
> > >> description] Signed-off-by: Orlando Chamberlain
> > >> <orlandoch.dev@gmail.com> ---
> > >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18
> > >> +++++++++++------- 1 file changed, 11 insertions(+), 7
> > >> deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index
> > >> 2f28a8c02f64..0bb553a61552 100644 ---
> > >> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++
> > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -3919,12
> > >> +3919,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> > >> if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> > >> vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
> > >>
> > >> -       if (amdgpu_device_supports_px(ddev)) {
> > >> -               px = true;
> > >> -               vga_switcheroo_register_client(adev->pdev,
> > >> -
> > >> &amdgpu_switcheroo_ops, px);
> > >> +       px = amdgpu_device_supports_px(ddev);
> > >> +
> > >> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> > >> +               vga_switcheroo_register_client(adev->pdev,
> > >> &amdgpu_switcheroo_ops, px); +
> > >> +       if (px)
> > >>                 vga_switcheroo_init_domain_pm_ops(adev->dev,
> > >> &adev->vga_pm_domain);
> > >> -       }
> > >>
> > >>         if (adev->gmc.xgmi.pending_reset)
> > >>                 queue_delayed_work(system_wq,
> > >> &mgpu_info.delayed_reset_work, @@ -4048,10 +4049,13 @@ void
> > >> amdgpu_device_fini_sw(struct amdgpu_device *adev)
> > >>
> > >>         kfree(adev->bios);
> > >>         adev->bios = NULL;
> > >> -       if (amdgpu_device_supports_px(adev_to_drm(adev))) {
> > >> +
> > >> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> > >>                 vga_switcheroo_unregister_client(adev->pdev);
> > >> +
> > >> +       if (amdgpu_device_supports_px(adev_to_drm(adev)))
> > >>                 vga_switcheroo_fini_domain_pm_ops(adev->dev);
> > >> -       }
> > >> +
> > >>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> > >>                 vga_client_unregister(adev->pdev);
> > >>
> > >> --
> > >> 2.39.1
> > >>  
> > >  
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 2f28a8c02f64..0bb553a61552 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3919,12 +3919,13 @@  int amdgpu_device_init(struct amdgpu_device *adev,
 	if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
 		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
 
-	if (amdgpu_device_supports_px(ddev)) {
-		px = true;
-		vga_switcheroo_register_client(adev->pdev,
-					       &amdgpu_switcheroo_ops, px);
+	px = amdgpu_device_supports_px(ddev);
+
+	if (!pci_is_thunderbolt_attached(adev->pdev))
+		vga_switcheroo_register_client(adev->pdev, &amdgpu_switcheroo_ops, px);
+
+	if (px)
 		vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
-	}
 
 	if (adev->gmc.xgmi.pending_reset)
 		queue_delayed_work(system_wq, &mgpu_info.delayed_reset_work,
@@ -4048,10 +4049,13 @@  void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 
 	kfree(adev->bios);
 	adev->bios = NULL;
-	if (amdgpu_device_supports_px(adev_to_drm(adev))) {
+
+	if (!pci_is_thunderbolt_attached(adev->pdev))
 		vga_switcheroo_unregister_client(adev->pdev);
+
+	if (amdgpu_device_supports_px(adev_to_drm(adev)))
 		vga_switcheroo_fini_domain_pm_ops(adev->dev);
-	}
+
 	if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
 		vga_client_unregister(adev->pdev);