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

[Matt Roper <matthew.d.roper@intel.com>]

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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx