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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx; marcheu@xxxxxxxxxx; Matt Atwood
> >> Subject: Re: [PATCH] drm/i915: decouple runtime PM enablement from DMC presence
> >>
> >> On Wed, Jun 14, 2017 at 10:12:21AM -0700, matthew.s.atwood@xxxxxxxxx wrote:
> >> > From: Matt Atwood <matthew.s.atwood@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> >> >
> >> > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> >> > ---
> >> > 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@xxxxxxxxxxxxxxxxxxxxx
> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
