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

Thu Jun 15 17:38:58 UTC 2017

On Thu, Jun 15, 2017 at 10:20:57AM -0700, Rodrigo Vivi wrote:
> On Thu, Jun 15, 2017 at 9:30 AM, Imre Deak <imre.deak at intel.com> wrote:
> > Yes, probably because i915 runtime PM as whole is disabled without DMC,
> > so we won't turn off display side power wells either; but D3 still won't
> > make a difference.
> >
> > The problem with not having DMC is that with that you'll prevent system
> > level power saving that is deeper package C states.
> 
> But if there are still power savings proved even without reaching the
> deeper package C states
> I believe it is worth allowing RPM to work without requiring DMC, no?!

Not if there is no good reason for not using DMC. It was decided early
on that we won't support that configuration since if you care about the
power saving provided by partially disabling things you probably also
care about the bigger power saving provided by DMC. So that is the
current state of the driver, enabling runtime PM without having DMC
loaded is not supported. Proper support for that would need to be added
after a justification why not to use the firmware.

> >
> > --Imre
> >
> > On Thu, Jun 15, 2017 at 07:16:25PM +0300, Atwood, Matthew S wrote:
> >> 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
> >>
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br