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

[Daniel Vetter <daniel@ffwll.ch>]

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