[PATCH v1 1/1] drm/i915: Perform modeset based on DPMS state during resume

[PATCH v1 1/1] drm/i915: Perform modeset based on DPMS state during resume

[sagar.a.kamble]

From: Akash Goel <akash.goel@intel.com>

During resume, modeset was being performed independent of DPMS state which
increased resume time as well as it kept display wells ON. With this change
this modeset will be skipped.

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 71be3c9..919e552 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -671,6 +671,31 @@ int i915_suspend_legacy(struct drm_device *dev, pm_message_t state)
 	return i915_drm_suspend_late(dev);
 }
 
+static bool display_is_on(struct drm_device *dev)
+{
+	struct drm_connector *connector;
+	bool display_is_on = false;
+
+	drm_modeset_lock_all(dev);
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		if (!connector->encoder || !connector->encoder->crtc)
+			continue;
+		/*
+		 * If Display wasn't turned off, before going to suspend then
+		 * it should be re-enabled now, as we don't expect the DPMS ON
+		 * call to come in that case
+		 */
+		if (connector->dpms != DRM_MODE_DPMS_OFF) {
+			DRM_DEBUG_KMS("Display was on before suspend\n");
+			display_is_on = true;
+			break;
+		}
+	}
+	drm_modeset_unlock_all(dev);
+
+	return display_is_on;
+}
+
 static int i915_drm_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -707,9 +732,11 @@ static int i915_drm_resume(struct drm_device *dev)
 		spin_unlock_irq(&dev_priv->irq_lock);
 
 		intel_dp_mst_resume(dev);
-		drm_modeset_lock_all(dev);
-		intel_modeset_setup_hw_state(dev, true);
-		drm_modeset_unlock_all(dev);
+		if (display_is_on(dev)) {
+			drm_modeset_lock_all(dev);
+			intel_modeset_setup_hw_state(dev, true);
+			drm_modeset_unlock_all(dev);
+		}
 
 		/*
 		 * ... but also need to make sure that hotplug processing
-- 
1.8.5

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

Re: [PATCH v1 1/1] drm/i915: Perform modeset based on DPMS state during resume

[Chris Wilson]

On Mon, Dec 01, 2014 at 12:29:34PM +0530, sagar.a.kamble@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> During resume, modeset was being performed independent of DPMS state which
> increased resume time as well as it kept display wells ON. With this change
> this modeset will be skipped.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 71be3c9..919e552 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -671,6 +671,31 @@ int i915_suspend_legacy(struct drm_device *dev, pm_message_t state)
>  	return i915_drm_suspend_late(dev);
>  }
>  
> +static bool display_is_on(struct drm_device *dev)
> +{
> +	struct drm_connector *connector;
> +	bool display_is_on = false;
> +
> +	drm_modeset_lock_all(dev);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		if (!connector->encoder || !connector->encoder->crtc)
> +			continue;
> +		/*
> +		 * If Display wasn't turned off, before going to suspend then
> +		 * it should be re-enabled now, as we don't expect the DPMS ON
> +		 * call to come in that case
> +		 */
> +		if (connector->dpms != DRM_MODE_DPMS_OFF) {
> +			DRM_DEBUG_KMS("Display was on before suspend\n");
> +			display_is_on = true;
> +			break;
> +		}
> +	}
> +	drm_modeset_unlock_all(dev);
> +
> +	return display_is_on;
> +}
> +
>  static int i915_drm_resume(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -707,9 +732,11 @@ static int i915_drm_resume(struct drm_device *dev)
>  		spin_unlock_irq(&dev_priv->irq_lock);
>  
>  		intel_dp_mst_resume(dev);
> -		drm_modeset_lock_all(dev);
> -		intel_modeset_setup_hw_state(dev, true);

I thought it was meant to be setup_hw_state(force=false) here and then
we use the hotplug event to reconfigure?

You cannot simply skip setup_hw_state() as we need to detect and adjust
for whatever changes happened to the hardware whilst we slept.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1 1/1] drm/i915: Perform modeset based on DPMS state during resume

[Akash Goel]

On Mon, 2014-12-01 at 12:29 +0530, sagar.a.kamble@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> During resume, modeset was being performed independent of DPMS state which
> increased resume time as well as it kept display wells ON. With this change
> this modeset will be skipped.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 71be3c9..919e552 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -671,6 +671,31 @@ int i915_suspend_legacy(struct drm_device *dev, pm_message_t state)
>  	return i915_drm_suspend_late(dev);
>  }
>  
> +static bool display_is_on(struct drm_device *dev)
> +{
> +	struct drm_connector *connector;
> +	bool display_is_on = false;
> +
> +	drm_modeset_lock_all(dev);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		if (!connector->encoder || !connector->encoder->crtc)
> +			continue;
> +		/*
> +		 * If Display wasn't turned off, before going to suspend then
> +		 * it should be re-enabled now, as we don't expect the DPMS ON
> +		 * call to come in that case
> +		 */
> +		if (connector->dpms != DRM_MODE_DPMS_OFF) {
> +			DRM_DEBUG_KMS("Display was on before suspend\n");
> +			display_is_on = true;
> +			break;
> +		}
> +	}
> +	drm_modeset_unlock_all(dev);
> +
> +	return display_is_on;
> +}
> +
>  static int i915_drm_resume(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -707,9 +732,11 @@ static int i915_drm_resume(struct drm_device *dev)
>  		spin_unlock_irq(&dev_priv->irq_lock);
>  
>  		intel_dp_mst_resume(dev);
> -		drm_modeset_lock_all(dev);
> -		intel_modeset_setup_hw_state(dev, true);
> -		drm_modeset_unlock_all(dev);
> +		if (display_is_on(dev)) {
> +			drm_modeset_lock_all(dev);
> +			intel_modeset_setup_hw_state(dev, true);
> +			drm_modeset_unlock_all(dev);
> +		}

Need an additional change here, call to
'intel_display_set_init_power(dev_priv, false);' 
is also required at the end of i915_drm_resume function.
This will put the Display wells back into D3 state before
the runtime suspend is triggered, which could happen immediately, 
as call to modeset was skipped.

Best Regards
Akash

>  		/*
>  		 * ... but also need to make sure that hotplug processing


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

Re: [PATCH v1 1/1] drm/i915: Perform modeset based on DPMS state during resume

[Daniel Vetter]

On Mon, Dec 01, 2014 at 12:29:34PM +0530, sagar.a.kamble@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> During resume, modeset was being performed independent of DPMS state which
> increased resume time as well as it kept display wells ON. With this change
> this modeset will be skipped.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 71be3c9..919e552 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -671,6 +671,31 @@ int i915_suspend_legacy(struct drm_device *dev, pm_message_t state)
>  	return i915_drm_suspend_late(dev);
>  }
>  
> +static bool display_is_on(struct drm_device *dev)
> +{
> +	struct drm_connector *connector;
> +	bool display_is_on = false;
> +
> +	drm_modeset_lock_all(dev);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		if (!connector->encoder || !connector->encoder->crtc)
> +			continue;
> +		/*
> +		 * If Display wasn't turned off, before going to suspend then
> +		 * it should be re-enabled now, as we don't expect the DPMS ON
> +		 * call to come in that case
> +		 */
> +		if (connector->dpms != DRM_MODE_DPMS_OFF) {
> +			DRM_DEBUG_KMS("Display was on before suspend\n");
> +			display_is_on = true;
> +			break;
> +		}
> +	}
> +	drm_modeset_unlock_all(dev);
> +
> +	return display_is_on;
> +}

Imo this is at the wrong level - we should recompute what
intel_crtc->active should be like in intel_crtc_update_dpms. Maybe we need
to recompute intel_encoder->connectors_active, but I think that should
work out without any other changes.

And that logic should be in the setup_hw_state restore logic.
-Daniel

> +
>  static int i915_drm_resume(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -707,9 +732,11 @@ static int i915_drm_resume(struct drm_device *dev)
>  		spin_unlock_irq(&dev_priv->irq_lock);
>  
>  		intel_dp_mst_resume(dev);
> -		drm_modeset_lock_all(dev);
> -		intel_modeset_setup_hw_state(dev, true);
> -		drm_modeset_unlock_all(dev);
> +		if (display_is_on(dev)) {
> +			drm_modeset_lock_all(dev);
> +			intel_modeset_setup_hw_state(dev, true);
> +			drm_modeset_unlock_all(dev);
> +		}
>  
>  		/*
>  		 * ... but also need to make sure that hotplug processing
> -- 
> 1.8.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx