Re: [PATCH v3 01/13] drm/i915/gen9: csr_init after runtime pm enable

[Imre Deak <imre.deak@intel.com>]

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


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx