[Intel-gfx] [PATCH] drm/i915: restrict PP save/restore to platforms with lvds

[Jani Nikula]

On Tue, 09 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasimani at 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 at intel.com>
>> Acked-by: Jani Nikula <jani.nikula at intel.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter at 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);
>

-- 
Jani Nikula, Intel Open Source Technology Center