[Intel-gfx] [PATCH v2 5/6] drm/i915: Register the backlight device after the modeset init

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Currently we register the backlight device as soon as we register the
> connector. That means we can get backlight requests from userspace
> already before reading out the current modeset hardware state.
>
> That means we don't yet know the current crtc->encoder->connector mapping,
> which causes problems for VLV/CHV which need to know the current pipe in
> order to figure out which BLC registers to poke. Currently we just
> ignore such requests fairly deep in the backlight code which means the
> backlight device brightness property will get out of sync with our
> backlight.level and the actual hardware state.
>
> Fix the problem by delaying the backlight device registration until the
> entire modeset init has been performed. And we also move the
> backlight unregisteration to happen as the first thing during the
> modeset cleanup so that we also won't be bothered with userspace
> backlight requested during teardown.
>
> This is a real world problem on machines using systemd, because systemd,
> for some reason, wants to restore the backlight to the level it used last
> time. And that happens as soon as it sees the backlight device appearing
> in the system. Sometimes the userspace access makes it through before
> the modeset init, sometimes not.
>
> v2: Do not lie to the user in the debug prints (Jani)
>     Include connector name in the prints (Jani)
>     Fix a typo in the commit message (Jani)
>
> Reviewed-by: Jani Nikula <jani.nikula at intel.com>

Yup.

> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  4 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_panel.c   | 33 ++++++++++++++++++++++++++-------
>  3 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9529d0..158d65b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13378,6 +13378,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  		}
>  	}
>  	mutex_unlock(&dev->struct_mutex);
> +
> +	intel_backlight_register(dev);
>  }
>  
>  void intel_connector_unregister(struct intel_connector *intel_connector)
> @@ -13393,6 +13395,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_connector *connector;
>  
> +	intel_backlight_unregister(dev);
> +
>  	/*
>  	 * Interrupts and polling as the first thing to avoid creating havoc.
>  	 * Too much stuff here (turning of rps, connectors, ...) would
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0ff011e..2a1f790 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1113,6 +1113,9 @@ extern struct drm_display_mode *intel_find_panel_downclock(
>  				struct drm_device *dev,
>  				struct drm_display_mode *fixed_mode,
>  				struct drm_connector *connector);
> +void intel_backlight_register(struct drm_device *dev);
> +void intel_backlight_unregister(struct drm_device *dev);
> +
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 69bbfba..708642a 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1041,6 +1041,9 @@ static int intel_backlight_device_register(struct intel_connector *connector)
>  	if (WARN_ON(panel->backlight.device))
>  		return -ENODEV;
>  
> +	if (!panel->backlight.present)
> +		return 0;
> +
>  	WARN_ON(panel->backlight.max == 0);
>  
>  	memset(&props, 0, sizeof(props));
> @@ -1076,6 +1079,10 @@ static int intel_backlight_device_register(struct intel_connector *connector)
>  		panel->backlight.device = NULL;
>  		return -ENODEV;
>  	}
> +
> +	DRM_DEBUG_KMS("Connector %s backlight sysfs interface registered\n",
> +		      connector->base.name);
> +
>  	return 0;
>  }
>  
> @@ -1302,15 +1309,12 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
>  		return ret;
>  	}
>  
> -	intel_backlight_device_register(intel_connector);
> -
>  	panel->backlight.present = true;
>  
> -	DRM_DEBUG_KMS("backlight initialized, %s, brightness %u/%u, "
> -		      "sysfs interface %sregistered\n",
> +	DRM_DEBUG_KMS("Connector %s backlight initialized, %s, brightness %u/%u\n",
> +		      connector->name,
>  		      panel->backlight.enabled ? "enabled" : "disabled",
> -		      panel->backlight.level, panel->backlight.max,
> -		      panel->backlight.device ? "" : "not ");
> +		      panel->backlight.level, panel->backlight.max);
>  
>  	return 0;
>  }
> @@ -1321,7 +1325,6 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
>  	struct intel_panel *panel = &intel_connector->panel;
>  
>  	panel->backlight.present = false;
> -	intel_backlight_device_unregister(intel_connector);
>  }
>  
>  /* Set up chip specific backlight functions */
> @@ -1384,3 +1387,19 @@ void intel_panel_fini(struct intel_panel *panel)
>  		drm_mode_destroy(intel_connector->base.dev,
>  				panel->downclock_mode);
>  }
> +
> +void intel_backlight_register(struct drm_device *dev)
> +{
> +	struct intel_connector *connector;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> +		intel_backlight_device_register(connector);
> +}
> +
> +void intel_backlight_unregister(struct drm_device *dev)
> +{
> +	struct intel_connector *connector;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> +		intel_backlight_device_unregister(connector);
> +}
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center