[Intel-gfx] [PATCH] drm/i915: decouple runtime PM enablement from DMC presence

[Atwood, Matthew S]

On ChromeOS I've tested a couple hundred thousand iterations, during their Power Load test (Google's battery claim test) an avg of 400 mW is saved when RPM is turned on with DMC missing vs both turned off, So i think D3 is definitely relevant even without DMC.
________________________________________
From: Deak, Imre
Sent: Wednesday, June 14, 2017 1:17 PM
To: Rodrigo Vivi
Cc: Atwood, Matthew S; intel-gfx at lists.freedesktop.org; marcheu at google.com; Matt Atwood
Subject: Re: [Intel-gfx] [PATCH] drm/i915: decouple runtime PM enablement from DMC presence

On Wed, Jun 14, 2017 at 01:02:27PM -0700, Rodrigo Vivi wrote:
> On Wed, Jun 14, 2017 at 10:55 AM, Imre Deak <imre.deak at intel.com> wrote:
> > On Wed, Jun 14, 2017 at 08:40:55PM +0300, Atwood, Matthew S wrote:
> >> intel_csr_load_program can fail (if not supported by SoC, or if file
> >> is size 0)
> >
> > Those are really just sanity checks, they can't happen normally. We
> > should actually convert them to be WARNs.
> >
> >> and theres no conditional that it succeeds before releasing
> >> power_put on POWER_DOMAIN_INIT, enabling runtime PM.
> >
> > It can't fail during loading, HAS_CSR() is already checked in
> > intel_csr_ucode_init() and dev_priv->csr.dmc_payload is checked right
> > before calling intel_csr_load_program().
> >
> >> As long as the driver *thinks* it has a valid path to a DMC firmware
> >> this will execute.
> >>
> >> "without DMC loaded we want to keep runtime PM disabled" - Why?
> >
> > There is no reason to support that configuration.
>
> What if someone doesn't really want to use DMC but still wants D3hot?

D3 in itself won't result in power saving you need to enable DC6/9 for
that. I'm not even sure if it's even valid to enter PCI D3 without
enabling these states first. And for that you need the firmware.

>
> >
> >> ________________________________________
> >> From: Deak, Imre
> >> Sent: Wednesday, June 14, 2017 10:33 AM
> >> To: Atwood, Matthew S
> >> Cc: intel-gfx at lists.freedesktop.org; marcheu at google.com; Matt Atwood
> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915: decouple runtime PM enablement from DMC presence
> >>
> >> On Wed, Jun 14, 2017 at 10:12:21AM -0700, matthew.s.atwood at intel.com wrote:
> >> > From: Matt Atwood <matthew.s.atwood at intel.corp-partner.google.com>
> >> >
> >> > Runtime PM is disabled when DMC firmware is not present. Runtime PM is still
> >> > enabled even if DMC firmware fails to load.
> >>
> >> Hm, that would be a bug, but I can't see how it can happen; could you
> >> explain? We get a runtime PM reference in intel_csr_ucode_init() and
> >> only put it if we loaded the firmware successfully.
> >>
> >> > This patch enables runtime PM to be enabled if DMC firmware is not present.
> >>
> >> Without DMC loaded we want to keep runtime PM disabled.
> >>
> >> --Imre
> >>
> >> >
> >> > Signed-off-by: Matt Atwood <matthew.s.atwood at intel.corp-partner.google.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_csr.c | 7 +++----
> >> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> >> > index 965988f..3e4e705 100644
> >> > --- a/drivers/gpu/drm/i915/intel_csr.c
> >> > +++ b/drivers/gpu/drm/i915/intel_csr.c
> >> > @@ -411,8 +411,6 @@ static void csr_load_work_fn(struct work_struct *work)
> >> >       if (dev_priv->csr.dmc_payload) {
> >> >               intel_csr_load_program(dev_priv);
> >> >
> >> > -             intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> >> > -
> >> >               DRM_INFO("Finished loading DMC firmware %s (v%u.%u)\n",
> >> >                        dev_priv->csr.fw_path,
> >> >                        CSR_VERSION_MAJOR(csr->version),
> >> > @@ -420,10 +418,11 @@ static void csr_load_work_fn(struct work_struct *work)
> >> >       } else {
> >> >               dev_notice(dev_priv->drm.dev,
> >> >                          "Failed to load DMC firmware"
> >> > -                        " [" FIRMWARE_URL "],"
> >> > -                        " disabling runtime power management.\n");
> >> > +                        " [" FIRMWARE_URL "]");
> >> >       }
> >> >
> >> > +     intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> >> > +
> >> >       release_firmware(fw);
> >> >  }
> >> >
> >> > --
> >> > 2.7.4
> >> >
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx at lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br