Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190827134756.10807-2-kai.heng.feng@canonical.com>
 <20190828180128.1732-1-kai.heng.feng@canonical.com> <20190905213509.GI103977@google.com>
In-Reply-To: <20190905213509.GI103977@google.com>
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Tue, 17 Sep 2019 11:36:51 +0200
Message-ID: <CAAd53p5LUat5MVkjbkk+st0TJA96y36keD898ZOepEdsnaiihw@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] ALSA: hda: Allow HDA to be runtime suspended when
 dGPU is not bound
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     tiwai@suse.com, Linux PCI <linux-pci@vger.kernel.org>,
        alsa-devel@alsa-project.org, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Bjorn,

On Thu, Sep 5, 2019 at 11:35 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Aug 29, 2019 at 02:01:28AM +0800, Kai-Heng Feng wrote:
> > It's a common practice to let dGPU unbound and use PCI platform power
> > management to disable its power through _OFF method of power resource,
> > which is listed by _PR3.
> > When the dGPU comes with an HDA function, the HDA won't be suspended if
> > the dGPU is unbound, so the power resource can't be turned off.
>
> I'm not involved in applying this patch, but from the peanut gallery:
>
>   - The above looks like it might be two paragraphs missing a blank
>     line between?  Or maybe it's one paragraph that needs to be
>     rewrapped?

I think it can be both. I'll rephrase it.

>
>   - I can't parse the first sentence.  I guess "dGPU" and "HDA" are
>     hardware devices, but I don't know what "unbound" means.  Is that
>     something to do with a driver being bound to the dGPU?  Or is it
>     some connection between the dGPU and the HDA?

Yes, "unbound" in this context means the dGPU isn't bound to a driver.

>
>   - "PCI platform power management" is still confusing -- I think we
>     either have "PCI power management" that uses the PCI PM Capability
>     (i.e., writing PCI_PM_CTRL as in pci_raw_set_power_state()) OR we
>     have "platform power management" that uses some other method,
>     typically ACPI.  Since you refer to _OFF and _PR3, I guess you're
>     referring to platform power management here.

Yes, I'll make it clearer in next version. It's indeed referring to
platform power management.

>
>   - "It's common practice to let dGPU unbound" -- does that refer to
>     some programming technique common in drivers, or some user-level
>     thing like unloading a driver, or ...?  My guess is it probably
>     means "unbind the driver from the dGPU" but I still don't know
>     what makes it common practice.

Yes it means keep driver for dGPU unloaded. It's a common practice
since the proprietary nvidia.ko doesn't support runtime power
management, so when users are using integrated GPU, unload the dGPU
driver can make PCI core use platform power management to disable the
power source to save power.

>
> This probably all makes perfect sense to the graphics cognoscenti, but
> for the rest of us there are a lot of dots here that are not
> connected.

Will send a v2 with clearer description.
Kai-Heng

>
> > Commit 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for
> > discrete GPU") only allows HDA to be runtime-suspended once GPU is
> > bound, to keep APU's HDA working.
> >
> > However, HDA on dGPU isn't that useful if dGPU is unbound. So let's
> > relax the runtime suspend requirement for dGPU's HDA function, to save
> > lots of power.
> >
> > BugLink: https://bugs.launchpad.net/bugs/1840835
> > Fixes: b516ea586d71 ("PCI: Enable NVIDIA HDA controllers”)
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > v2:
> > - Change wording.
> > - Rebase to Tiwai's branch.
> >
> >  sound/pci/hda/hda_intel.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 91e71be42fa4..c3654d22795a 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -1284,7 +1284,11 @@ static void init_vga_switcheroo(struct azx *chip)
> >               dev_info(chip->card->dev,
> >                        "Handle vga_switcheroo audio client\n");
> >               hda->use_vga_switcheroo = 1;
> > -             chip->bus.keep_power = 1; /* cleared in either gpu_bound op or codec probe */
> > +
> > +             /* cleared in either gpu_bound op or codec probe, or when its
> > +              * root port has _PR3 (i.e. dGPU).
> > +              */
> > +             chip->bus.keep_power = !pci_pr3_present(p);
> >               chip->driver_caps |= AZX_DCAPS_PM_RUNTIME;
> >               pci_dev_put(p);
> >       }
> > --
> > 2.17.1
> >