drm/i915: restrict PP save/restore to platforms with lvds

Submitted by Daniel Vetter on Feb. 8, 2016, 9:55 a.m.

Details

Message ID 1454925346-22479-1-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show
Series "drm/i915: restrict PP save/restore to platforms with lvds" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Daniel Vetter Feb. 8, 2016, 9:55 a.m.
eDP already restores PP state completely on it's own, we only need
this code for LVDS. Since it's more work to move this into the lvds
encoder properly just limit it to affected pch chips for now
(ibx&cpt/ppt).

Cc: Jani Nikula <jani.nikula@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_suspend.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index a2aa09ce3202..7f6b050266a7 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -43,8 +43,8 @@  static void i915_save_display(struct drm_device *dev)
 	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
 		dev_priv->regfile.saveLVDS = I915_READ(LVDS);
 
-	/* Panel power sequencer */
-	if (HAS_PCH_SPLIT(dev)) {
+	/* Panel power sequencer, only needed for LVDS */
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
 		dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
 		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
 		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
@@ -78,8 +78,8 @@  static void i915_restore_display(struct drm_device *dev)
 	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
 		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
 
-	/* Panel power sequencer */
-	if (HAS_PCH_SPLIT(dev)) {
+	/* Panel power sequencer, only needed for LVDS */
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
 		I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
 		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
 		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);

Comments

On 2/8/2016 3:25 PM, Daniel Vetter wrote:
> eDP already restores PP state completely on it's own, we only need
> this code for LVDS. Since it's more work to move this into the lvds
> encoder properly just limit it to affected pch chips for now
> (ibx&cpt/ppt).
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_suspend.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index a2aa09ce3202..7f6b050266a7 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -43,8 +43,8 @@ static void i915_save_display(struct drm_device *dev)
>   	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
>   		dev_priv->regfile.saveLVDS = I915_READ(LVDS);
>   
> -	/* Panel power sequencer */
> -	if (HAS_PCH_SPLIT(dev)) {
> +	/* Panel power sequencer, only needed for LVDS */
> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>   		dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
>   		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
>   		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
won't this code execute for eDP too ? Also it seems incorrect we are 
saving and restoring
PP_CONTROL register too when we should have followed proper sequence 
delays for
each bit in PP_CONTROL.  This will end up bring up the panel before 
modeset in
best case or damage the panel in the worst case.

Sivakumar

> @@ -78,8 +78,8 @@ static void i915_restore_display(struct drm_device *dev)
>   	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
>   		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
>   
> -	/* Panel power sequencer */
> -	if (HAS_PCH_SPLIT(dev)) {
> +	/* Panel power sequencer, only needed for LVDS */
> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>   		I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
>   		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>   		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
On Tue, 09 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com> wrote:
> On 2/8/2016 3:25 PM, Daniel Vetter wrote:
>> eDP already restores PP state completely on it's own, we only need
>> this code for LVDS. Since it's more work to move this into the lvds
>> encoder properly just limit it to affected pch chips for now
>> (ibx&cpt/ppt).
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_suspend.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
>> index a2aa09ce3202..7f6b050266a7 100644
>> --- a/drivers/gpu/drm/i915/i915_suspend.c
>> +++ b/drivers/gpu/drm/i915/i915_suspend.c
>> @@ -43,8 +43,8 @@ static void i915_save_display(struct drm_device *dev)
>>   	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
>>   		dev_priv->regfile.saveLVDS = I915_READ(LVDS);
>>   
>> -	/* Panel power sequencer */
>> -	if (HAS_PCH_SPLIT(dev)) {
>> +	/* Panel power sequencer, only needed for LVDS */
>> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>>   		dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
>>   		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
>>   		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
> won't this code execute for eDP too ? Also it seems incorrect we are
> saving and restoring PP_CONTROL register too when we should have
> followed proper sequence delays for each bit in PP_CONTROL.  This will
> end up bring up the panel before modeset in best case or damage the
> panel in the worst case.

Exactly the reasons why we are trying to limit this code to fewer
platforms! This is very fragile stuff though, I fear we might regress
something (even if that something is currently working by coincidence)
if we limit this further, in one go. I'd like to do this little by
little, and see what breaks, if anything.

With this patch, we'd already limit all of this hackery to cougarpoint
and older, instead of all PCH split up to and including Skylake. If
anyone wants to dig into LVDS power sequencing after that, I'm not
opposed...

BR,
Jani.


>
> Sivakumar
>
>> @@ -78,8 +78,8 @@ static void i915_restore_display(struct drm_device *dev)
>>   	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
>>   		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
>>   
>> -	/* Panel power sequencer */
>> -	if (HAS_PCH_SPLIT(dev)) {
>> +	/* Panel power sequencer, only needed for LVDS */
>> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>>   		I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
>>   		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>>   		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
>
On 2/9/2016 11:54 AM, Jani Nikula wrote:
> On Tue, 09 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com> wrote:
>> On 2/8/2016 3:25 PM, Daniel Vetter wrote:
>>> eDP already restores PP state completely on it's own, we only need
>>> this code for LVDS. Since it's more work to move this into the lvds
>>> encoder properly just limit it to affected pch chips for now
>>> (ibx&cpt/ppt).
>>>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_suspend.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
>>> index a2aa09ce3202..7f6b050266a7 100644
>>> --- a/drivers/gpu/drm/i915/i915_suspend.c
>>> +++ b/drivers/gpu/drm/i915/i915_suspend.c
>>> @@ -43,8 +43,8 @@ static void i915_save_display(struct drm_device *dev)
>>>    	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
>>>    		dev_priv->regfile.saveLVDS = I915_READ(LVDS);
>>>    
>>> -	/* Panel power sequencer */
>>> -	if (HAS_PCH_SPLIT(dev)) {
>>> +	/* Panel power sequencer, only needed for LVDS */
>>> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>>>    		dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
>>>    		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
>>>    		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
>> won't this code execute for eDP too ? Also it seems incorrect we are
>> saving and restoring PP_CONTROL register too when we should have
>> followed proper sequence delays for each bit in PP_CONTROL.  This will
>> end up bring up the panel before modeset in best case or damage the
>> panel in the worst case.
> Exactly the reasons why we are trying to limit this code to fewer
> platforms! This is very fragile stuff though, I fear we might regress
> something (even if that something is currently working by coincidence)
> if we limit this further, in one go. I'd like to do this little by
> little, and see what breaks, if anything.
>
> With this patch, we'd already limit all of this hackery to cougarpoint
> and older, instead of all PCH split up to and including Skylake. If
> anyone wants to dig into LVDS power sequencing after that, I'm not
> opposed...
>
> BR,
> Jani.
missed the part where this patch is reducing the platforms :).
can we reduce this to systems with LVDS alone ? i am sure eDP will 
blankout if
pps is not followed but have observed some LVDS panels working even without
pps delays.

Sivakumar
>
>> Sivakumar
>>
>>> @@ -78,8 +78,8 @@ static void i915_restore_display(struct drm_device *dev)
>>>    	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
>>>    		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
>>>    
>>> -	/* Panel power sequencer */
>>> -	if (HAS_PCH_SPLIT(dev)) {
>>> +	/* Panel power sequencer, only needed for LVDS */
>>> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>>>    		I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
>>>    		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>>>    		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);