Re: [PATCH] drm/i915: restrict PP save/restore to platforms with lvds

["Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>]

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

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