From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sunil Kamath Subject: Re: [PATCH v3 01/13] drm/i915/gen9: csr_init after runtime pm enable Date: Wed, 04 Nov 2015 14:57:23 +0530 Message-ID: <5639CF7B.6080605@intel.com> References: <1446069547-24760-1-git-send-email-imre.deak@intel.com> <1446069547-24760-2-git-send-email-imre.deak@intel.com> <5631F284.6030602@intel.com> <1446126908.28303.18.camel@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1617379605==" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id B5F396E2AB for ; Wed, 4 Nov 2015 01:27:25 -0800 (PST) In-Reply-To: <1446126908.28303.18.camel@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: imre.deak@intel.com Cc: Daniel Vetter , "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============1617379605== Content-Type: multipart/alternative; boundary="------------040001000306010306020600" This is a multi-part message in MIME format. --------------040001000306010306020600 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On Thursday 29 October 2015 07:25 PM, Imre Deak wrote: > On to, 2015-10-29 at 15:48 +0530, Sunil Kamath wrote: >> On Thursday 29 October 2015 03:28 AM, Imre Deak wrote: >>> From: Animesh Manna >>> >>> Skl is fully dependent on dmc for going to low power state (dc5/dc6). >>> This requires a trigger from rpm. To ensure the dmc firmware >>> is available for runtime pm support rpm-reference-count is used >>> by not releasing the rpm reference if firmware loading is >>> not completed. >>> >>> So moved the intel_csr_ucode_init call after runtime pm enable. >>> >>> Cc: Daniel Vetter >>> Cc: Damien Lespiau >>> Cc: Imre Deak >>> Cc: Sunil Kamath >>> Signed-off-by: Animesh Manna >>> [imre: moved the call right after power domain init to avoid race wit= h >>> the console modesetting] >>> --- >>> drivers/gpu/drm/i915/i915_dma.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i= 915_dma.c >>> index 2336af9..5d68942 100644 >>> --- a/drivers/gpu/drm/i915/i915_dma.c >>> +++ b/drivers/gpu/drm/i915/i915_dma.c >>> @@ -398,6 +398,8 @@ static int i915_load_modeset_init(struct drm_devi= ce *dev) >>> =20 >>> intel_power_domains_init_hw(dev_priv); >>> =20 >>> + intel_csr_ucode_init(dev); >>> + >> Its really unclear why this call to be done here. >> We need to call just after intel_runtime_pm_enable. >> With this change it will do much in advance. > In general the earlier we start the firmware loading work the better, > since we can start saving power the earliest this way. > >> Can you please add some more details about "how adding change after >> iniet_runtime_pm_enable() will create race condition for console >> modesetting?" > We want to avoid to call into either the power well code or the runtime > s/r handlers. This is why we take a ref on the INIT power domain in > intel_csr_ucode_init() which should prevent both. Now if we do this onl= y > after the point where a power well reference can drop to zero, there is > a chance that we call into the power well code before we took the above > INIT power domain ref. > > If you look at i915_load_modeset_init(), it schedules > intel_fbdev_initial_config(), which may end up doing a modeset for the > console, leading up to intel_modeset_setup_hw_state() and > intel_display_set_init_power(false). Depending on the the active output= s > this may drop the last reference on some domains and call into the powe= r > well code. > > Due to similar reasons we could drop the last runtime PM reference and > call the runtime s/r handlers before the firmware is loaded if we took > the INIT power domain ref only after calling intel_runtime_pm_enable(). > > --Imre > >> - Sunil >> Concern that we had was: In intel_csr_ucode_init() we are calling=20 intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); which intern call=20 intel_runtime_pm_get. That means rpm will not be enabled before this=20 code flow. Now it=E2=80=99s clear that that's not a problem. As we do RPM gets even=20 earlier. It will only drop an RPM reference, that in effect will make it=20 possible to enter RPM. There are places taking an RPM ref already=20 earlier like in intel_power_domains_init_hw() and RPM references are=20 taken even before our driver's probe runtime is called by the pci core.=20 So that's completely normal. Reviewed-by: A.Sunil Kamath =20 >>> ret =3D intel_irq_install(dev_priv); >>> if (ret) >>> goto cleanup_gem_stolen; >>> @@ -942,9 +944,6 @@ int i915_driver_load(struct drm_device *dev, unsi= gned long flags) >>> =20 >>> intel_uncore_init(dev); >>> =20 >>> - /* Load CSR Firmware for SKL */ >>> - intel_csr_ucode_init(dev); >>> - >>> ret =3D i915_gem_gtt_init(dev); >>> if (ret) >>> goto out_freecsr; > --------------040001000306010306020600 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On Thursday 29 October 2015 07:25 PM, Imre Deak wrote:
On to, 2015-10-29 at 15:48 +0530, Sunil Kamath wrote=
:

On Thursday 29 October 2015 03:28 AM, Imre Deak wr=
ote:

From: Animesh Manna <animesh.manna@intel.co=
m>

Skl is fully dependent on dmc for going to low power state (dc5/dc6).
This requires a trigger from rpm. To ensure the dmc firmware
is available for runtime pm support rpm-reference-count is used
by not releasing the rpm reference if firmware loading is
not completed.

So moved the intel_csr_ucode_init call after runtime pm enable.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
[imre: moved the call right after power domain init to avoid race with
  the console modesetting]
---
  drivers/gpu/drm/i915/i915_dma.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_=
dma.c
index 2336af9..5d68942 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -398,6 +398,8 @@ static int i915_load_modeset_init(struct drm_device *=
dev)
 =20
  	intel_power_domains_init_hw(dev_priv);
 =20
+	intel_csr_ucode_init(dev);
+

Its really unclear why this call to be done here.
We need to call just after intel_runtime_pm_enable.
With this change it will do much in advance.

In general the earlier we start the firmware loading work the better,
since we can start saving power the earliest this way.


Can you please add some more details about "how ad=
ding change after=20
iniet_runtime_pm_enable() will create race condition for console=20
modesetting?"

We want to avoid to call into either the power well code or the runtime
s/r handlers. This is why we take a ref on the INIT power domain in
intel_csr_ucode_init() which should prevent both. Now if we do this only
after the point where a power well reference can drop to zero, there is
a chance that we call into the power well code before we took the above
INIT power domain ref.

If you look at i915_load_modeset_init(), it schedules
intel_fbdev_initial_config(), which may end up doing a modeset for the
console, leading up to intel_modeset_setup_hw_state() and
intel_display_set_init_power(false). Depending on the the active outputs
this may drop the last reference on some domains and call into the power
well code.

Due to similar reasons we could drop the last runtime PM reference and
call the runtime s/r handlers before the firmware is loaded if we took
the INIT power domain ref only after calling intel_runtime_pm_enable().

--Imre


- Sunil

Concern that we had was:

In intel_csr_ucode_init() we are calling intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); which intern call intel_runtime_pm_get. That means rpm will not be enabled before this code flow.

Now it=E2=80=99s clear that that's not a problem. As we do RPM gets e= ven earlier. It will only drop an RPM reference, that in effect will make it possible to enter RPM. There are places taking an RPM ref already earlier like in intel_power_domains_init_hw() and RPM references are taken even before our driver's probe runtime is called by the pci core. So that's completely normal.

=C2=A0

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>


        

          
  	ret =3D intel_irq_install(dev_priv);
  	if (ret)
  		goto cleanup_gem_stolen;
@@ -942,9 +944,6 @@ int i915_driver_load(struct drm_device *dev, unsigned=
 long flags)
 =20
  	intel_uncore_init(dev);
 =20
-	/* Load CSR Firmware for SKL */
-	intel_csr_ucode_init(dev);
-
  	ret =3D i915_gem_gtt_init(dev);
  	if (ret)
  		goto out_freecsr;


        

        

      

      



    

    

  


--------------040001000306010306020600--

--===============1617379605==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4
IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0
cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK

--===============1617379605==--