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

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

[matthew.s.atwood]

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. This patch enables runtime PM to
be enabled if DMC firmware is not present.

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

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

[Imre Deak]

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

✗ Fi.CI.BAT: failure for drm/i915: decouple runtime PM enablement from DMC presence

[Patchwork]

== Series Details ==

Series: drm/i915: decouple runtime PM enablement from DMC presence
URL   : https://patchwork.freedesktop.org/series/25787/
State : failure

== Summary ==

Series 25787v1 drm/i915: decouple runtime PM enablement from DMC presence
https://patchwork.freedesktop.org/api/1.0/series/25787/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> FAIL       (fi-bdw-5557u)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#100461

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#100461 https://bugs.freedesktop.org/show_bug.cgi?id=100461

fi-bdw-5557u     total:278  pass:266  dwarn:0   dfail:0   fail:1   skip:11  time:442s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:439s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:579s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:511s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:484s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:483s
fi-glk-2a        total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:585s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:428s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:413s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:415s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:494s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:469s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:471s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:569s
fi-kbl-r         total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time:576s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:459s
fi-skl-6700hq    total:278  pass:227  dwarn:2   dfail:0   fail:27  skip:22  time:403s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:464s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:475s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:436s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:401s

66555696fc0f33fc19eca5068783b95ecc8a91d5 drm-tip: 2017y-06m-14d-09h-56m-45s UTC integration manifest
3d9846d drm/i915: decouple runtime PM enablement from DMC presence

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4951/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Atwood, Matthew S]

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

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

[Imre Deak]

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

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

[Rodrigo Vivi]

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?

>
>> ________________________________________
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Atwood, Matthew S]

"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?
________________________________________
From: Rodrigo Vivi [rodrigo.vivi@gmail.com]
Sent: Wednesday, June 14, 2017 1:02 PM
To: Deak, Imre
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 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?

>
>> ________________________________________
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

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


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

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

[Imre Deak]

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

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

[Jani Nikula]

On Thu, 15 Jun 2017, Imre Deak <imre.deak@intel.com> 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.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Stéphane Marchesin]

On Mon, Jun 19, 2017 at 11:45 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
>
> On Thu, 15 Jun 2017, Imre Deak <imre.deak@intel.com> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Thu, 17 Aug 2017, Stéphane Marchesin <marcheu@chromium.org> wrote:
> On Mon, Jun 19, 2017 at 11:45 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>>
>> On Thu, 15 Jun 2017, Imre Deak <imre.deak@intel.com> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tahvanainen, Jari]

" 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:jani.nikula@linux.intel.com] 
Sent: Friday, August 18, 2017 12:08 PM
To: Stéphane Marchesin <marcheu@chromium.org>
Cc: Deak, Imre <imre.deak@intel.com>; Rodrigo Vivi <rodrigo.vivi@gmail.com>; intel-gfx@lists.freedesktop.org; Matt Atwood <matthew.s.atwood@intel.corp-partner.google.com>; Saarinen, Jani <jani.saarinen@intel.com>; Tahvanainen, Jari <jari.tahvanainen@intel.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: decouple runtime PM enablement from DMC presence

On Thu, 17 Aug 2017, Stéphane Marchesin <marcheu@chromium.org> wrote:
> On Mon, Jun 19, 2017 at 11:45 AM, Jani Nikula 
> <jani.nikula@linux.intel.com> wrote:
>>
>> On Thu, 15 Jun 2017, Imre Deak <imre.deak@intel.com> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Stéphane Marchesin]

On Fri, Aug 18, 2017 at 2:07 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Thu, 17 Aug 2017, Stéphane Marchesin <marcheu@chromium.org> wrote:
>> On Mon, Jun 19, 2017 at 11:45 AM, Jani Nikula
>> <jani.nikula@linux.intel.com> wrote:
>>>
>>> On Thu, 15 Jun 2017, Imre Deak <imre.deak@intel.com> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx