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@xxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
