Date:   Sat, 11 Feb 2023 10:54:47 +1100
From:   Orlando Chamberlain <orlandoch.dev@gmail.com>
To:     Alex Deucher <alexdeucher@gmail.com>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        platform-driver-x86@vger.kernel.org, amd-gfx@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        Lijo Lazar <lijo.lazar@amd.com>,
        Rander Wang <rander.wang@intel.com>,
        YiPeng Chai <YiPeng.Chai@amd.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        David Airlie <airlied@gmail.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Evan Quan <evan.quan@amd.com>,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        Yong Zhi <yong.zhi@intel.com>,
        Aun-Ali Zaidi <admin@kodeit.net>,
        Bokun Zhang <Bokun.Zhang@amd.com>,
        Mark Gross <markgross@kernel.org>,
        Kerem Karabay <kekrby@gmail.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Jack Xiao <Jack.Xiao@amd.com>,
        Kai Vehmanen <kai.vehmanen@linux.intel.com>,
        Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>,
        Takashi Iwai <tiwai@suse.com>,
        Aditya Garg <gargaditya08@live.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Amadeusz =?UTF-8?B?U8WCYXdpxYRza2k=?= 
        <amadeuszx.slawinski@linux.intel.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        Christian =?UTF-8?B?S8O2bmln?= <christian.koenig@amd.com>,
        Hawking Zhang <Hawking.Zhang@amd.com>
Subject: Re: [RFC PATCH 9/9] drm/amdgpu: register a vga_switcheroo client
 for all GPUs that are not thunderbolt attached
Message-ID: <20230211105447.0d8dda6b@redecorated-mbp>
In-Reply-To: <CADnq5_PXa=d8ie8jj7sGxvxfn+ZNwed-eSyNg89PfWCvpo8LCw@mail.gmail.com>
References: <20230210044826.9834-1-orlandoch.dev@gmail.com>
        <20230210044826.9834-10-orlandoch.dev@gmail.com>
        <CADnq5_PVsrk4+K45kVgU=r98B9GgC2CxbC8AvGUXRtv2sZAHPg@mail.gmail.com>
        <b3a2d525-b30c-164c-705c-97fcb0016540@redhat.com>
        <CADnq5_PXa=d8ie8jj7sGxvxfn+ZNwed-eSyNg89PfWCvpo8LCw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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