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

[Imre Deak]

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.

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