Re: [PATCH 5/6] drm/i915: Register the backlight device after the modeset init

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently we register the backlight device as soon as we register the
> connector. That means we can get backlight requestes 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 requestes 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.

Makes sense.

Please update the debug print at the end of intel_panel_setup_backlight
accordingly, and add a debug print about sysfs registration at the end
of intel_backlight_device_register. With that done, this is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

While at it, how about adding drm_get_connector_name(connector) in the
debug prints?

BR,
Jani.


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@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   | 22 +++++++++++++++++++---
>  3 files changed, 26 insertions(+), 3 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 f6e8d23..2bc3309 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));
> @@ -1302,8 +1305,6 @@ 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, "
> @@ -1321,7 +1322,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 +1384,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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx