diff mbox

[DMC_REDESIGN_V2,03/14] drm/i915/bxt: release rpm reference if csr firmware failed to load.

Message ID 1440588497-13954-4-git-send-email-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Manna, Animesh Aug. 26, 2015, 11:28 a.m. UTC 
Note that for bxt without dmc, display engine can go to lowest
possible state (dc9), so releasing the rpm reference.

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>
---
 drivers/gpu/drm/i915/intel_csr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Imre Deak Sept. 30, 2015, 2:24 p.m. UTC | #1 
On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote:
> Note that for bxt without dmc, display engine can go to lowest
> possible state (dc9), so releasing the rpm reference.
> 
> 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>
> ---
>  drivers/gpu/drm/i915/intel_csr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 75a775b..be388da 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>  
>  	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
>  out:
> -	if (fw_loaded)
> +	if (fw_loaded || IS_BROXTON(dev))
>  		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);

I don't think this is needed. We disable all display side runtime power
management if the firmware is not available. So no need to special case
this either imo.

>  	else
>  		intel_csr_load_status_set(dev_priv, FW_FAILED);
Manna, Animesh Oct. 14, 2015, 6:29 a.m. UTC | #2 
On 9/30/2015 7:54 PM, Imre Deak wrote:
> On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote:
>> Note that for bxt without dmc, display engine can go to lowest
>> possible state (dc9), so releasing the rpm reference.
>>
>> 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>
>> ---
>>   drivers/gpu/drm/i915/intel_csr.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> index 75a775b..be388da 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>   
>>   	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
>>   out:
>> -	if (fw_loaded)
>> +	if (fw_loaded || IS_BROXTON(dev))
>>   		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> I don't think this is needed. We disable all display side runtime power
> management if the firmware is not available. So no need to special case
> this either imo.
In display off case for broxton platform we can directly goto dc9 state 
for which dmc is not needed.
So, don't want to block runtime power management from display side.

-Animesh
>
>>   	else
>>   		intel_csr_load_status_set(dev_priv, FW_FAILED);
>
Imre Deak Oct. 14, 2015, 8:27 a.m. UTC | #3 
On Wed, 2015-10-14 at 11:59 +0530, Animesh Manna wrote:
> 
> On 9/30/2015 7:54 PM, Imre Deak wrote:
> > On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote:
> >> Note that for bxt without dmc, display engine can go to lowest
> >> possible state (dc9), so releasing the rpm reference.
> >>
> >> 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>
> >> ---
> >>   drivers/gpu/drm/i915/intel_csr.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> >> index 75a775b..be388da 100644
> >> --- a/drivers/gpu/drm/i915/intel_csr.c
> >> +++ b/drivers/gpu/drm/i915/intel_csr.c
> >> @@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>   
> >>   	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
> >>   out:
> >> -	if (fw_loaded)
> >> +	if (fw_loaded || IS_BROXTON(dev))
> >>   		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > I don't think this is needed. We disable all display side runtime power
> > management if the firmware is not available. So no need to special case
> > this either imo.
> In display off case for broxton platform we can directly goto dc9 state 
> for which dmc is not needed.
> So, don't want to block runtime power management from display side.

We could also runtime suspend on Skylake without the DMC firmware. We
made the decision that we won't allow that, for no other reason than to
simplify the code. Doing that we don't need to check for the firmware
loaded status for instance to decide whether to enable DC5/6 or not
(which can be racy). So why would we now add the complexity for one
platform when we decidedly removed it from another?

--Imre

> 
> -Animesh
> >
> >>   	else
> >>   		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >
>
Jani Nikula Oct. 14, 2015, 8:39 a.m. UTC | #4 
On Wed, 14 Oct 2015, Animesh Manna <animesh.manna@intel.com> wrote:
> On 9/30/2015 7:54 PM, Imre Deak wrote:
>> On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote:
>>> Note that for bxt without dmc, display engine can go to lowest
>>> possible state (dc9), so releasing the rpm reference.
>>>
>>> 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>
>>> ---
>>>   drivers/gpu/drm/i915/intel_csr.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>>> index 75a775b..be388da 100644
>>> --- a/drivers/gpu/drm/i915/intel_csr.c
>>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>>> @@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>>   
>>>   	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
>>>   out:
>>> -	if (fw_loaded)
>>> +	if (fw_loaded || IS_BROXTON(dev))
>>>   		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
>> I don't think this is needed. We disable all display side runtime power
>> management if the firmware is not available. So no need to special case
>> this either imo.
> In display off case for broxton platform we can directly goto dc9 state 
> for which dmc is not needed.
> So, don't want to block runtime power management from display side.

Your patch doesn't set load status to failed on bxt. There's another
version from Sagar that does [1].

BR,
Jani.


[1] http://mid.gmane.org/1444754985-15734-1-git-send-email-sagar.a.kamble@intel.com


>
> -Animesh
>>
>>>   	else
>>>   		intel_csr_load_status_set(dev_priv, FW_FAILED);
>>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 75a775b..be388da 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -392,7 +392,7 @@  static void finish_csr_load(const struct firmware *fw, void *context)
 
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
 out:
-	if (fw_loaded)
+	if (fw_loaded || IS_BROXTON(dev))
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 	else
 		intel_csr_load_status_set(dev_priv, FW_FAILED);