Re: [PATCH 3/4] drm/i915: ensure single initialization and cleanup of backlight device

From: Daniel Vetter <daniel@ffwll.ch>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/4] drm/i915: ensure single initialization and cleanup of backlight device
Date: Mon, 15 Apr 2013 09:33:28 +0200	[thread overview]
Message-ID: <20130415073328.GZ27612@phenom.ffwll.local> (raw)
In-Reply-To: <87obdgb8wm.fsf@intel.com>

On Mon, Apr 15, 2013 at 09:33:13AM +0300, Jani Nikula wrote:
> On Fri, 12 Apr 2013, Jani Nikula <jani.nikula@intel.com> wrote:
> > Backlight cleanup in the eDP connector destroy callback caused the
> > backlight device to be removed on some systems that first initialized LVDS
> > and then attempted to initialize eDP. Prevent multiple backlight
> > initializations, and ensure backlight cleanup is only done once by moving
> > it to modeset cleanup.
> >
> > A small wrinkle is the introduced asymmetry in backlight
> > setup/cleanup. This could be solved by adding refcounting, but it seems
> > overkill considering that there should only ever be one backlight device.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=55701
> 
> Via bugzilla,
> 
> Tested-by: Peter Verthez <peter.verthez@skynet.be>
> 
> CC: Stable?

Yeah, agreed. Queued for -next, thanks for the patch.
-Daniel
> 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    3 +++
> >  drivers/gpu/drm/i915/intel_dp.c      |    5 +----
> >  drivers/gpu/drm/i915/intel_lvds.c    |    1 -
> >  drivers/gpu/drm/i915/intel_panel.c   |    7 ++++++-
> >  4 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 457a0a0..712b0af 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9419,6 +9419,9 @@ void intel_modeset_cleanup(struct drm_device *dev)
> >  	/* flush any delayed tasks or pending work */
> >  	flush_scheduled_work();
> >  
> > +	/* destroy backlight, if any, before the connectors */
> > +	intel_panel_destroy_backlight(dev);
> > +
> >  	drm_mode_config_cleanup(dev);
> >  
> >  	intel_cleanup_overlay(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 173add1..8845e82 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2474,17 +2474,14 @@ done:
> >  static void
> >  intel_dp_destroy(struct drm_connector *connector)
> >  {
> > -	struct drm_device *dev = connector->dev;
> >  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> >  	struct intel_connector *intel_connector = to_intel_connector(connector);
> >  
> >  	if (!IS_ERR_OR_NULL(intel_connector->edid))
> >  		kfree(intel_connector->edid);
> >  
> > -	if (is_edp(intel_dp)) {
> > -		intel_panel_destroy_backlight(dev);
> > +	if (is_edp(intel_dp))
> >  		intel_panel_fini(&intel_connector->panel);
> > -	}
> >  
> >  	drm_sysfs_connector_remove(connector);
> >  	drm_connector_cleanup(connector);
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index ca2d903..f36f1ba 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -631,7 +631,6 @@ static void intel_lvds_destroy(struct drm_connector *connector)
> >  	if (!IS_ERR_OR_NULL(lvds_connector->base.edid))
> >  		kfree(lvds_connector->base.edid);
> >  
> > -	intel_panel_destroy_backlight(connector->dev);
> >  	intel_panel_fini(&lvds_connector->base.panel);
> >  
> >  	drm_sysfs_connector_remove(connector);
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 464c3d7..5d3e9d7 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -459,6 +459,9 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
> >  
> >  	intel_panel_init_backlight(dev);
> >  
> > +	if (WARN_ON(dev_priv->backlight.device))
> > +		return -ENODEV;
> > +
> >  	memset(&props, 0, sizeof(props));
> >  	props.type = BACKLIGHT_RAW;
> >  	props.brightness = dev_priv->backlight.level;
> > @@ -488,8 +491,10 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
> >  void intel_panel_destroy_backlight(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	if (dev_priv->backlight.device)
> > +	if (dev_priv->backlight.device) {
> >  		backlight_device_unregister(dev_priv->backlight.device);
> > +		dev_priv->backlight.device = NULL;
> > +	}
> >  }
> >  #else
> >  int intel_panel_setup_backlight(struct drm_connector *connector)
> > -- 
> > 1.7.9.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