[PATCH] drm/i915: Do not clobber config status after a forced restore of hw state

[PATCH] drm/i915: Do not clobber config status after a forced restore of hw state

[Chris Wilson]

We call intel_modeset_setup_hw_state() along two paths, driver
load/resume and after a lid event notification. During initialisation of
the driver, it is imperative that we reset the config state. This
correctly sets up the initial connector statuses and prepares the
hardware for a thorough probing. However, during a lid event, we only
want to undo the damage caused by the bios by resetting our last known
mode. In this cirumstance, we do not want to clobber our desired state.

In order to try and keep sanity between the config state and our own
tracking, do the drm_mode_config_reset() first along the load/resume
paths before reading out the hw state and apply any definite known
corrections.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.c      | 1 +
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c2e00ed23195..66191875d632 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -648,6 +648,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		intel_modeset_init_hw(dev);
 
 		drm_modeset_lock_all(dev);
+		drm_mode_config_reset(dev);
 		intel_modeset_setup_hw_state(dev, true);
 		drm_modeset_unlock_all(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 271d9e526a72..d97c90586672 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11117,6 +11117,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			connector->base.dpms = DRM_MODE_DPMS_ON;
 			connector->encoder->connectors_active = true;
 			connector->base.encoder = &connector->encoder->base;
+			connector->base.status = connector_status_connected;
 		} else {
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
 			connector->base.encoder = NULL;
@@ -11203,8 +11204,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	}
 
 	intel_modeset_check_state(dev);
-
-	drm_mode_config_reset(dev);
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
@@ -11213,6 +11212,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
 
 	intel_setup_overlay(dev);
 
+	drm_mode_config_reset(dev);
 	intel_modeset_setup_hw_state(dev, false);
 }
 
-- 
1.8.4.3

Re: [PATCH] drm/i915: Do not clobber config status after a forced restore of hw state

[Daniel Vetter]

On Mon, Nov 18, 2013 at 02:45:09PM +0000, Chris Wilson wrote:
> We call intel_modeset_setup_hw_state() along two paths, driver
> load/resume and after a lid event notification. During initialisation of
> the driver, it is imperative that we reset the config state. This
> correctly sets up the initial connector statuses and prepares the
> hardware for a thorough probing. However, during a lid event, we only
> want to undo the damage caused by the bios by resetting our last known
> mode. In this cirumstance, we do not want to clobber our desired state.
> 
> In order to try and keep sanity between the config state and our own
> tracking, do the drm_mode_config_reset() first along the load/resume
> paths before reading out the hw state and apply any definite known
> corrections.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      | 1 +
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c2e00ed23195..66191875d632 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -648,6 +648,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  		intel_modeset_init_hw(dev);
>  
>  		drm_modeset_lock_all(dev);
> +		drm_mode_config_reset(dev);
>  		intel_modeset_setup_hw_state(dev, true);
>  		drm_modeset_unlock_all(dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 271d9e526a72..d97c90586672 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11117,6 +11117,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			connector->base.dpms = DRM_MODE_DPMS_ON;
>  			connector->encoder->connectors_active = true;
>  			connector->base.encoder = &connector->encoder->base;
> +			connector->base.status = connector_status_connected;

As discussed on irc I don't think we should force the connector state to
anything here: Imo connector->status should reflect what we believe to be
the true output connection state, whereas connector->encoder reflects
whether this connector is wired up to a pipe. And since we no longer
reject modeset on disconnected connectors and never nuked the pipe if the
connector gets disconnected there's no reason for that - such policy is
userspace's job.

Note though that the multi-connector encoders are a bit botched still and
select the connector according to the hotplug state (or some other
semi-random piece of state) instead of obeying userspace's wish. With some
resulting hilarity.

Oh and: This regression has been introduced in

commit 2e9388923e83bc4e2726f170a984621f1d582e77
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 11 20:08:24 2012 +0200

    drm/i915/crt: explicitly set up HOTPLUG_BITS on resume

so needs

Cc: stable@vger.kernel.org (v3.8 and later)
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

Cheers, Daniel

>  		} else {
>  			connector->base.dpms = DRM_MODE_DPMS_OFF;
>  			connector->base.encoder = NULL;
> @@ -11203,8 +11204,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	}
>  
>  	intel_modeset_check_state(dev);
> -
> -	drm_mode_config_reset(dev);
>  }
>  
>  void intel_modeset_gem_init(struct drm_device *dev)
> @@ -11213,6 +11212,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  
>  	intel_setup_overlay(dev);
>  
> +	drm_mode_config_reset(dev);
>  	intel_modeset_setup_hw_state(dev, false);
>  }
>  
> -- 
> 1.8.4.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Do not clobber config status after a forced restore of hw state

[Chris Wilson]

We call intel_modeset_setup_hw_state() along two paths, driver
load/resume and after a lid event notification. During initialisation of
the driver, it is imperative that we reset the config state. This
correctly sets up the initial connector statuses and prepares the
hardware for a thorough probing. However, during a lid event, we only
want to undo the damage caused by the bios by resetting our last known
mode. In this cirumstance, we do not want to clobber our desired state.

In order to try and keep sanity between the config state and our own
tracking, do the drm_mode_config_reset() first along the load/resume
paths before reading out the hw state and apply any definite known
corrections.

v2: "As discussed on irc I don't think we should force the connector
state to anything here: Imo connector->status should reflect what we
believe to be the true output connection state, whereas connector->encoder
reflects whether this connector is wired up to a pipe. And since we no
longer reject modeset on disconnected connectors and never nuked the pipe
if the connector gets disconnected there's no reason for that - such policy
is userspace's job.

This regression has been introduced in

commit 2e9388923e83bc4e2726f170a984621f1d582e77
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 11 20:08:24 2012 +0200

    drm/i915/crt: explicitly set up HOTPLUG_BITS on resume"
so sayeth Daniel.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: stable@vger.kernel.org (v3.8 and later)
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 1 +
 drivers/gpu/drm/i915/intel_display.c | 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0ec0fb32a103..65b5c83df3bb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -652,6 +652,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		intel_modeset_init_hw(dev);
 
 		drm_modeset_lock_all(dev);
+		drm_mode_config_reset(dev);
 		intel_modeset_setup_hw_state(dev, true);
 		drm_modeset_unlock_all(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a449a877a6ca..fb293f41050d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11226,8 +11226,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	}
 
 	intel_modeset_check_state(dev);
-
-	drm_mode_config_reset(dev);
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
@@ -11236,6 +11234,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
 
 	intel_setup_overlay(dev);
 
+	drm_mode_config_reset(dev);
 	intel_modeset_setup_hw_state(dev, false);
 }
 
-- 
1.8.5

Re: [PATCH] drm/i915: Do not clobber config status after a forced restore of hw state

[Daniel Vetter]

On Mon, Dec 02, 2013 at 05:39:09PM +0000, Chris Wilson wrote:
> We call intel_modeset_setup_hw_state() along two paths, driver
> load/resume and after a lid event notification. During initialisation of
> the driver, it is imperative that we reset the config state. This
> correctly sets up the initial connector statuses and prepares the
> hardware for a thorough probing. However, during a lid event, we only
> want to undo the damage caused by the bios by resetting our last known
> mode. In this cirumstance, we do not want to clobber our desired state.
> 
> In order to try and keep sanity between the config state and our own
> tracking, do the drm_mode_config_reset() first along the load/resume
> paths before reading out the hw state and apply any definite known
> corrections.
> 
> v2: "As discussed on irc I don't think we should force the connector
> state to anything here: Imo connector->status should reflect what we
> believe to be the true output connection state, whereas connector->encoder
> reflects whether this connector is wired up to a pipe. And since we no
> longer reject modeset on disconnected connectors and never nuked the pipe
> if the connector gets disconnected there's no reason for that - such policy
> is userspace's job.
> 
> This regression has been introduced in
> 
> commit 2e9388923e83bc4e2726f170a984621f1d582e77
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Oct 11 20:08:24 2012 +0200
> 
>     drm/i915/crt: explicitly set up HOTPLUG_BITS on resume"
> so sayeth Daniel.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: stable@vger.kernel.org (v3.8 and later)
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Picked up for -fixes, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch