drm/i915: decouple runtime PM enablement from DMC presence

Commit Message

Matt Atwood June 14, 2017, 5:12 p.m. UTC

From: Matt Atwood <[e-mail]>

Runtime PM is disabled when DMC firmware is not present. Runtime PM is still
enabled even if DMC firmware fails to load. This patch enables runtime PM to
be enabled if DMC firmware is not present.

Signed-off-by: Matt Atwood <[e-mail]>
---
 drivers/gpu/drm/i915/intel_csr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Imre Deak June 14, 2017, 5:33 p.m. UTC | #1

On Wed, Jun 14, 2017 at 10:12:21AM -0700, [e-mail] wrote:
> From: Matt Atwood <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Matt Atwood June 14, 2017, 5:40 p.m. UTC | #2

intel_csr_load_program can fail (if not supported by SoC, or if file is size 0) and theres no conditional that it succeeds before releasing power_put on POWER_DOMAIN_INIT, enabling runtime PM. 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?

Imre Deak June 14, 2017, 5:55 p.m. UTC | #3

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.

> ________________________________________
> From: Deak, Imre
> Sent: Wednesday, June 14, 2017 10:33 AM
> To: Atwood, Matthew S
> Cc: [e-mail]; [e-mail]; 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, [e-mail] wrote:
> > From: Matt Atwood <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Rodrigo Vivi June 14, 2017, 8:02 p.m. UTC | #4

On Wed, Jun 14, 2017 at 10:55 AM, Imre Deak <[e-mail]> 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?

>
>> ________________________________________
>> From: Deak, Imre
>> Sent: Wednesday, June 14, 2017 10:33 AM
>> To: Atwood, Matthew S
>> Cc: [e-mail]; [e-mail]; 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, [e-mail] wrote:
>> > From: Matt Atwood <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > ---
>> >  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
>> > [e-mail]
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Matt Atwood June 14, 2017, 8:07 p.m. UTC | #5

"dmc_payload is checked right before..." You are correct, there is no bug. It was my misunderstanding.

"What if someone doesn't really want to use DMC but still wants D3hot?"  or other great stuff that RPM can do without DMC firmware support?

Imre Deak June 14, 2017, 8:17 p.m. UTC | #6

On Wed, Jun 14, 2017 at 01:02:27PM -0700, Rodrigo Vivi wrote:
> On Wed, Jun 14, 2017 at 10:55 AM, Imre Deak <[e-mail]> 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: [e-mail]; [e-mail]; 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, [e-mail] wrote:
> >> > From: Matt Atwood <[e-mail]>
> >> >
> >> > 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 <[e-mail]>
> >> > ---
> >> >  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
> >> > [e-mail]
> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

Matt Atwood June 15, 2017, 4:16 p.m. UTC | #7

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.

Imre Deak June 15, 2017, 4:30 p.m. UTC | #8

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; [e-mail]; [e-mail]; 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 <[e-mail]> 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: [e-mail]; [e-mail]; 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, [e-mail] wrote:
> > >> > From: Matt Atwood <[e-mail]>
> > >> >
> > >> > 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 <[e-mail]>
> > >> > ---
> > >> >  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
> > >> > [e-mail]
> > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >>
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
>

Rodrigo Vivi June 15, 2017, 5:20 p.m. UTC | #9

On Thu, Jun 15, 2017 at 9:30 AM, Imre Deak <[e-mail]> 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?!


>
> --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; [e-mail]; [e-mail]; 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 <[e-mail]> 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: [e-mail]; [e-mail]; 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, [e-mail] wrote:
>> > >> > From: Matt Atwood <[e-mail]>
>> > >> >
>> > >> > 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 <[e-mail]>
>> > >> > ---
>> > >> >  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
>> > >> > [e-mail]
>> > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> > >>
>> > > _______________________________________________
>> > > Intel-gfx mailing list
>> > > [e-mail]
>> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> >
>> >
>> > --
>> > Rodrigo Vivi
>> > Blog: http://blog.vivi.eng.br
>>

Imre Deak June 15, 2017, 5:38 p.m. UTC | #10

On Thu, Jun 15, 2017 at 10:20:57AM -0700, Rodrigo Vivi wrote:
> On Thu, Jun 15, 2017 at 9:30 AM, Imre Deak <[e-mail]> 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; [e-mail]; [e-mail]; 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 <[e-mail]> 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: [e-mail]; [e-mail]; 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, [e-mail] wrote:
> >> > >> > From: Matt Atwood <[e-mail]>
> >> > >> >
> >> > >> > 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 <[e-mail]>
> >> > >> > ---
> >> > >> >  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
> >> > >> > [e-mail]
> >> > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> > >>
> >> > > _______________________________________________
> >> > > Intel-gfx mailing list
> >> > > [e-mail]
> >> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> >
> >> >
> >> > --
> >> > Rodrigo Vivi
> >> > Blog: http://blog.vivi.eng.br
> >>
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

Jani Nikula June 19, 2017, 6:45 p.m. UTC | #11

On Thu, 15 Jun 2017, Imre Deak <[e-mail]> wrote:
> On Thu, Jun 15, 2017 at 10:20:57AM -0700, Rodrigo Vivi wrote:
>> 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.

Agreed. We already have too many configurations to support, and we
already struggle with our testing coverage as-is. Adding new
configurations to be tested is not to be taken lightly.

BR,
Jani.

Stéphane Marchesin Aug. 18, 2017, 1 a.m. UTC | #12

On Mon, Jun 19, 2017 at 11:45 AM, Jani Nikula
<[e-mail]> wrote:
>
> On Thu, 15 Jun 2017, Imre Deak <[e-mail]> wrote:
> > On Thu, Jun 15, 2017 at 10:20:57AM -0700, Rodrigo Vivi wrote:
> >> 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.
>
> Agreed. We already have too many configurations to support, and we
> already struggle with our testing coverage as-is. Adding new
> configurations to be tested is not to be taken lightly.
>

For context, we are seeing GPU hangs at suspend/resume with DMC
enabled (feel free to email me if you want to be added to the internal
bug) which is the reason for this patch. If those GPU hangs can be
fixed instead, then that would be an even better solution, IMO. Is
there someone on your side who could help with these hangs?

Stéphane

>
> BR,
> Jani.
>
> --
> Jani Nikula, Intel Open Source Technology Center

Jani Nikula Aug. 18, 2017, 9:07 a.m. UTC | #13

On Thu, 17 Aug 2017, Stéphane Marchesin <[e-mail]> wrote:
> On Mon, Jun 19, 2017 at 11:45 AM, Jani Nikula
> <[e-mail]> wrote:
>>
>> On Thu, 15 Jun 2017, Imre Deak <[e-mail]> wrote:
>> > On Thu, Jun 15, 2017 at 10:20:57AM -0700, Rodrigo Vivi wrote:
>> >> 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.
>>
>> Agreed. We already have too many configurations to support, and we
>> already struggle with our testing coverage as-is. Adding new
>> configurations to be tested is not to be taken lightly.
>>
>
> For context, we are seeing GPU hangs at suspend/resume with DMC
> enabled (feel free to email me if you want to be added to the internal
> bug) which is the reason for this patch. If those GPU hangs can be
> fixed instead, then that would be an even better solution, IMO. Is
> there someone on your side who could help with these hangs?

We need to get the GPU hangs fixed, obviously. Is this reproducible with
upstream code? Do we have a bug about this at bugs.freedesktop.org?
Jani, Jari?

BR,
Jani.


>
> Stéphane
>
>>
>> BR,
>> Jani.
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center

Tahvanainen, Jari Aug. 18, 2017, 2:26 p.m. UTC | #14

" Do we have a bug about this at bugs.freedesktop.org?" I did quick query on fdo.bugzilla and did not find any matching item (afaik) from there (with i915_feature = GPU *hang*|*DMC*) so I would claim that bug is not filed. Stephane can correct my wrong sayings here. 

BR, Jari

-----Original Message-----
From: Jani Nikula [mailto:[e-mail]] 

Sent: Friday, August 18, 2017 12:08 PM
To: Stéphane Marchesin <[e-mail]>
Cc: Deak, Imre <[e-mail]>; Rodrigo Vivi <[e-mail]>; [e-mail]; Matt Atwood <[e-mail]>; Saarinen, Jani <[e-mail]>; Tahvanainen, Jari <[e-mail]>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: decouple runtime PM enablement from DMC presence

On Thu, 17 Aug 2017, Stéphane Marchesin <[e-mail]> wrote:
> On Mon, Jun 19, 2017 at 11:45 AM, Jani Nikula 

> <[e-mail]> wrote:

>>

>> On Thu, 15 Jun 2017, Imre Deak <[e-mail]> wrote:

>> > On Thu, Jun 15, 2017 at 10:20:57AM -0700, Rodrigo Vivi wrote:

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

>>

>> Agreed. We already have too many configurations to support, and we 

>> already struggle with our testing coverage as-is. Adding new 

>> configurations to be tested is not to be taken lightly.

>>

>

> For context, we are seeing GPU hangs at suspend/resume with DMC 

> enabled (feel free to email me if you want to be added to the internal

> bug) which is the reason for this patch. If those GPU hangs can be 

> fixed instead, then that would be an even better solution, IMO. Is 

> there someone on your side who could help with these hangs?


We need to get the GPU hangs fixed, obviously. Is this reproducible with upstream code? Do we have a bug about this at bugs.freedesktop.org?
Jani, Jari?

BR,
Jani.


>

> Stéphane

>

>>

>> BR,

>> Jani.

>>

>> --

>> Jani Nikula, Intel Open Source Technology Center


--
Jani Nikula, Intel Open Source Technology Center

Chris Wilson Aug. 18, 2017, 2:35 p.m. UTC | #15

Quoting Tahvanainen, Jari (2017-08-18 15:26:03)
> " Do we have a bug about this at bugs.freedesktop.org?" I did quick query on fdo.bugzilla and did not find any matching item (afaik) from there (with i915_feature = GPU *hang*|*DMC*) so I would claim that bug is not filed. Stephane can correct my wrong sayings here. 

https://bugs.freedesktop.org/show_bug.cgi?id=101888
https://bugs.freedesktop.org/show_bug.cgi?id=100572

That I know of.
-Chris

Stéphane Marchesin Aug. 19, 2017, 1 a.m. UTC | #16

On Fri, Aug 18, 2017 at 2:07 AM, Jani Nikula
<[e-mail]> wrote:
> On Thu, 17 Aug 2017, Stéphane Marchesin <[e-mail]> wrote:
>> On Mon, Jun 19, 2017 at 11:45 AM, Jani Nikula
>> <[e-mail]> wrote:
>>>
>>> On Thu, 15 Jun 2017, Imre Deak <[e-mail]> wrote:
>>> > On Thu, Jun 15, 2017 at 10:20:57AM -0700, Rodrigo Vivi wrote:
>>> >> 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.
>>>
>>> Agreed. We already have too many configurations to support, and we
>>> already struggle with our testing coverage as-is. Adding new
>>> configurations to be tested is not to be taken lightly.
>>>
>>
>> For context, we are seeing GPU hangs at suspend/resume with DMC
>> enabled (feel free to email me if you want to be added to the internal
>> bug) which is the reason for this patch. If those GPU hangs can be
>> fixed instead, then that would be an even better solution, IMO. Is
>> there someone on your side who could help with these hangs?
>
> We need to get the GPU hangs fixed, obviously. Is this reproducible with
> upstream code? Do we have a bug about this at bugs.freedesktop.org?
> Jani, Jari?
>

I am CCing intel people who were looking into this bug, and who
suggested to disable DMC as a solution. You can probably talk to them
directly :)

Stéphane


> BR,
> Jani.
>
>
>>
>> Stéphane
>>
>>>
>>> BR,
>>> Jani.
>>>
>>> --
>>> Jani Nikula, Intel Open Source Technology Center
>
> --
> Jani Nikula, Intel Open Source Technology Center

Patch

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);
 }