[Intel-gfx] [PATCH v2] drm/i915: fix dp/sdvo i2c cleanup

[Daniel Vetter]

On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote:
> Atm we try to remove the connector's i2c sysfs entry too late in the
> encoder's destroy callback. By that time the kobject used as the parent
> for all connector sysfs entries is already removed when we do an early
> removal of all connector sysfs entries in intel_modeset_cleanup(). Fix
> this by adding an early_destroy encoder callback, where we remove the
> encoder's i2c adapter.
> 
> v2:
> - add missing static to function, use existing sdvo cast helper,
>   s/intel_sdvo/sdvo/ (Chris)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> 
> Signed-off-by: Imre Deak <imre.deak at intel.com>

This looks fishy ... sysfs should all be reference-counted, so I'm
confused what's going on here. Also I think this smells a bit like it's
going overall in the wrong direction - essentially with sysfs we can't
really force-remove stuff but have to wait until the refcount drops to 0.
Or at least that's how I think it works, I'd need to blow through a pile
of time to figure this all out.

Someone enlighten me please ;-)
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c      | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_sdvo.c    |  9 ++++++++-
>  4 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5e94901..5d7f1fa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11451,6 +11451,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  	struct drm_connector *connector;
> +	struct drm_encoder *encoder;
>  
>  	/*
>  	 * Interrupts and polling as the first thing to avoid creating havoc.
> @@ -11488,6 +11489,13 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	/* flush any delayed tasks or pending work */
>  	flush_scheduled_work();
>  
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> +		struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
> +
> +		if (intel_encoder->early_destroy)
> +			intel_encoder->early_destroy(intel_encoder);
> +	}
> +
>  	/* destroy the backlight and sysfs files before encoders/connectors */
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>  		intel_panel_destroy_backlight(connector);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e37c7a0..33d6943 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3358,7 +3358,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  
> -	i2c_del_adapter(&intel_dp->adapter);
>  	drm_encoder_cleanup(encoder);
>  	if (is_edp(intel_dp)) {
>  		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> @@ -3369,6 +3368,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	kfree(intel_dig_port);
>  }
>  
> +
> +static void intel_dp_encoder_early_destroy(struct intel_encoder *intel_encoder)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> +
> +	i2c_del_adapter(&intel_dp->adapter);
> +}
> +
>  static const struct drm_connector_funcs intel_dp_connector_funcs = {
>  	.dpms = intel_connector_dpms,
>  	.detect = intel_dp_detect,
> @@ -3880,6 +3887,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  		intel_encoder->pre_enable = g4x_pre_enable_dp;
>  		intel_encoder->enable = g4x_enable_dp;
>  	}
> +	intel_encoder->early_destroy = intel_dp_encoder_early_destroy;
>  
>  	intel_dig_port->port = port;
>  	intel_dig_port->dp.output_reg = output_reg;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7b3c209..216f490 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -149,6 +149,7 @@ struct intel_encoder {
>  	 * be set correctly before calling this function. */
>  	void (*get_config)(struct intel_encoder *,
>  			   struct intel_crtc_config *pipe_config);
> +	void (*early_destroy)(struct intel_encoder *);
>  	int crtc_mask;
>  	enum hpd_pin hpd_pin;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 95bdfb3..8ecc702 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2238,7 +2238,6 @@ static void intel_sdvo_enc_destroy(struct drm_encoder *encoder)
>  		drm_mode_destroy(encoder->dev,
>  				 intel_sdvo->sdvo_lvds_fixed_mode);
>  
> -	i2c_del_adapter(&intel_sdvo->ddc);
>  	intel_encoder_destroy(encoder);
>  }
>  
> @@ -2912,6 +2911,13 @@ intel_sdvo_init_ddc_proxy(struct intel_sdvo *sdvo,
>  	return i2c_add_adapter(&sdvo->ddc) == 0;
>  }
>  
> +static void intel_sdvo_early_destroy(struct intel_encoder *intel_encoder)
> +{
> +	struct intel_sdvo *sdvo = to_sdvo(intel_encoder);
> +
> +	i2c_del_adapter(&sdvo->ddc);
> +}
> +
>  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2951,6 +2957,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  	intel_encoder->enable = intel_enable_sdvo;
>  	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
>  	intel_encoder->get_config = intel_sdvo_get_config;
> +	intel_encoder->early_destroy = intel_sdvo_early_destroy;
>  
>  	/* In default case sdvo lvds is false */
>  	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at 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