| Message ID | 555DD09B.7050407@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Matt > drm_modeset_unlock_all(dev); > > intel_dp_mst_suspend(dev); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cae36ec1c2ef..8d50c4ca561f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3204,9 +3204,6 @@ void intel_crtc_reset(struct intel_crtc *crtc) > > void intel_prepare_reset(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > - struct intel_crtc *crtc; > - > /* no reset support for gen2 */ > if (IS_GEN2(dev)) > return; > @@ -3221,13 +3218,7 @@ void intel_prepare_reset(struct drm_device *dev) > * Disabling the crtcs gracefully seems nicer. Also the > * g33 docs say we should at least disable all the planes. > */ > - for_each_intel_crtc(dev, crtc) { > - if (!crtc->active) > - continue; > - > - intel_crtc_disable_planes(&crtc->base); > - dev_priv->display.crtc_disable(&crtc->base); > - } > + intel_display_suspend(dev); > } > > void intel_finish_reset(struct drm_device *dev) > @@ -6018,6 +6009,24 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > mutex_unlock(&dev->struct_mutex); > } > > +/* > + * turn all crtc's off, but do not adjust state > + * This has to be paired with a call to intel_modeset_setup_hw_state. > + */ > +void intel_display_suspend(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_crtc *crtc; > + > + for_each_crtc(dev, crtc) { > + if (!to_intel_crtc(crtc)->active) > + continue; > + > + intel_crtc_disable_planes(crtc); > + dev_priv->display.crtc_disable(crtc); > + } > +} > + > /* Master function to enable/disable CRTC and corresponding power wells */ > void intel_crtc_control(struct drm_crtc *crtc, bool enable) > { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 70d0cc8fe70e..c24d670529e3 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -992,6 +992,7 @@ int intel_pch_rawclk(struct drm_device *dev); > void intel_mark_busy(struct drm_device *dev); > void intel_mark_idle(struct drm_device *dev); > void intel_crtc_restore_mode(struct drm_crtc *crtc); > +void intel_display_suspend(struct drm_device *dev); > void intel_crtc_control(struct drm_crtc *crtc, bool enable); > void intel_crtc_reset(struct intel_crtc *crtc); > void intel_crtc_update_dpms(struct drm_crtc *crtc); > -- > 2.1.0 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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
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); drm_modeset_unlock_all(dev); intel_dp_mst_suspend(dev); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cae36ec1c2ef..8d50c4ca561f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3204,9 +3204,6 @@ void intel_crtc_reset(struct intel_crtc *crtc) void intel_prepare_reset(struct drm_device *dev) { - struct drm_i915_private *dev_priv = to_i915(dev); - struct intel_crtc *crtc; - /* no reset support for gen2 */ if (IS_GEN2(dev)) return; @@ -3221,13 +3218,7 @@ void intel_prepare_reset(struct drm_device *dev) * Disabling the crtcs gracefully seems nicer. Also the * g33 docs say we should at least disable all the planes. */ - for_each_intel_crtc(dev, crtc) { - if (!crtc->active) - continue; - - intel_crtc_disable_planes(&crtc->base); - dev_priv->display.crtc_disable(&crtc->base); - } + intel_display_suspend(dev); } void intel_finish_reset(struct drm_device *dev) @@ -6018,6 +6009,24 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) mutex_unlock(&dev->struct_mutex); } +/* + * turn all crtc's off, but do not adjust state + * This has to be paired with a call to intel_modeset_setup_hw_state. + */ +void intel_display_suspend(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = to_i915(dev); + struct drm_crtc *crtc; + + for_each_crtc(dev, crtc) { + if (!to_intel_crtc(crtc)->active) + continue; + + intel_crtc_disable_planes(crtc); + dev_priv->display.crtc_disable(crtc); + } +} + /* Master function to enable/disable CRTC and corresponding power wells */ void intel_crtc_control(struct drm_crtc *crtc, bool enable) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 70d0cc8fe70e..c24d670529e3 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -992,6 +992,7 @@ int intel_pch_rawclk(struct drm_device *dev); void intel_mark_busy(struct drm_device *dev); void intel_mark_idle(struct drm_device *dev); void intel_crtc_restore_mode(struct drm_crtc *crtc); +void intel_display_suspend(struct drm_device *dev); void intel_crtc_control(struct drm_crtc *crtc, bool enable); void intel_crtc_reset(struct intel_crtc *crtc); void intel_crtc_update_dpms(struct drm_crtc *crtc);
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. 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(-)