Re: [PATCH v3.5 02.5/22] drm/i915: add intel_display_suspend

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 23-05-15 om 01:03 schreef Matt Roper:
> On Thu, May 21, 2015 at 02:33:31PM +0200, Maarten Lankhorst wrote:
>> This is a function used to disable all crtc's. This makes it clearer
>> to distinguish between when mode needs to be preserved and when
>> it can be trashed.
> To clarify, when you talk about mode being preserved or trashed here,
> you're talking about the hardware's idea of the mode, not the driver's
> software state, right?  I.e., because when we shut down a power well the
> registers vanish and whatever was programmed in them is lost?
>
> See my comments farther down.
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> Oops, I was trashing all state during suspend and on gpu reset.
>> I will send an amended intel_crtc_control patch too with the
>> suspend and prepare_reset parts taken out.
>>
>>  drivers/gpu/drm/i915/i915_drv.c      |  4 +---
>>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  3 files changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 5cc57f2ec192..d1a090a9f653 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -600,7 +600,6 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv);
>>  static int i915_drm_suspend(struct drm_device *dev)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct drm_crtc *crtc;
>>  	pci_power_t opregion_target_state;
>>  	int error;
>>  
>> @@ -631,8 +630,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>>  	 * for _thaw. Also, power gate the CRTC power wells.
>>  	 */
>>  	drm_modeset_lock_all(dev);
>> -	for_each_crtc(dev, crtc)
>> -		intel_crtc_control(crtc, false);
>> +	intel_display_suspend(dev);
> I'm not terribly familiar with the power well details, but it looks like
> part of the motivation of commit
>
>         commit b04c5bd6fda54703e56f29569e4bca489d6c5a5c
>         Author: Borun Fu <borun.fu@intel.com>
>         Date:   Sat Jul 12 10:02:27 2014 +0530
>
>             drm/i915: Power gating display wells during i915_pm_suspend
>
> which added intel_crtc_control() was to ensure the power wells were
> gated at this point; by replacing the intel_crtc_control() with
> intel_display_suspend() here, you're removing that power well
> programming...is that intentional (and is it going to cause the display
> to stay in D0 state)?
>
> If it is intentional, the comment above this block is out of date now.
> Since this patch (and the following one) seem to change the semantics of
> when we're touching power wells at various points in the code, maybe you
> can elaborate a little bit on that in the commit message of one or both
> commits.
>
You're right, I'm not touching power wells here. For the hang case that doesn't matter but for pm_suspend it probably does.

The followup patch that converts intel_display_suspend to atomic modeset should restore the old behavior.
I'll respin with some changes that I'll undo when converting intel_display_suspend to atomic modeset.

One thing that also seems to unintentionally change behavior is intel_display_set_init_power being unset by modeset_update_crtc_power_domains.
I'll fix that in the followup patch that converts this function to atomic.

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