| Message ID | 1446069547-24760-2-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 29 October 2015 03:28 AM, Imre Deak wrote: > From: Animesh Manna <animesh.manna@intel.com> > > 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) > > intel_power_domains_init_hw(dev_priv); > > + 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. Can you please add some more details about "how adding change after iniet_runtime_pm_enable() will create race condition for console modesetting?" - Sunil > ret = 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) > > intel_uncore_init(dev); > > - /* Load CSR Firmware for SKL */ > - intel_csr_ucode_init(dev); > - > ret = i915_gem_gtt_init(dev); > if (ret) > goto out_freecsr;
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 <animesh.manna@intel.com> > > > > 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) > > > > intel_power_domains_init_hw(dev_priv); > > > > + 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 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 > > > > ret = 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) > > > > intel_uncore_init(dev); > > > > - /* Load CSR Firmware for SKL */ > > - intel_csr_ucode_init(dev); > > - > > ret = i915_gem_gtt_init(dev); > > if (ret) > > goto out_freecsr; >
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 <animesh.manna@intel.com> >>> >>> 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) >>> >>> intel_power_domains_init_hw(dev_priv); >>> >>> + 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 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’s clear that that's not a problem. As we do RPM gets even 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. Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com> <mailto:sunil.kamath@intel.com> >>> ret = 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) >>> >>> intel_uncore_init(dev); >>> >>> - /* Load CSR Firmware for SKL */ >>> - intel_csr_ucode_init(dev); >>> - >>> ret = i915_gem_gtt_init(dev); >>> if (ret) >>> goto out_freecsr; >
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) intel_power_domains_init_hw(dev_priv); + intel_csr_ucode_init(dev); + ret = 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) intel_uncore_init(dev); - /* Load CSR Firmware for SKL */ - intel_csr_ucode_init(dev); - ret = i915_gem_gtt_init(dev); if (ret) goto out_freecsr;