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

[Sunil Kamath <sunil.kamath@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4061 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 5618 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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