Re: [PATCH] drm/i915: decouple runtime PM enablement from DMC presence

[Imre Deak <imre.deak@intel.com>]

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@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@lists.freedesktop.org; marcheu@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@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@lists.freedesktop.org; marcheu@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@intel.com wrote:
> >> > >> > From: Matt Atwood <matthew.s.atwood@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@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@lists.freedesktop.org
> >> > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> > >>
> >> > > _______________________________________________
> >> > > Intel-gfx mailing list
> >> > > Intel-gfx@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx