Re: [PATCH v2 22/24] drm/i915: Backlight update

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

On Sun, 23 Aug 2020, Sam Ravnborg <sam@ravnborg.org> wrote:
> Update backlight implementation to utilize newly added backlight
> functionality.
>
> - Use macros for initialization
> - Replace direct access to backlight_properties with get and set
>   operations
> - Moved enable/disable after registering backlight device
>
> One side-effect of these changes is that the confusing power states
> are now replaced by backligt_{enable,disable}.

Please split this up to individual changes, from non-functional to
functional. The existing code may be confusing, but it also means the
changes aren't easy to review when conflated into one, and should there
be a regression, the problem would be difficult to pinpoint.

I've commented on some of the changes inline, and some of them are
clearly unacceptable.


BR,
Jani.

>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Wambui Karuga <wambui.karugax@gmail.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/i915/display/intel_panel.c | 88 +++++++++++-----------
>  1 file changed, 44 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index 3c5056dbf607..9c6643b41b90 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -716,11 +716,15 @@ void intel_panel_set_backlight_acpi(const struct drm_connector_state *conn_state
>  	hw_level = clamp_user_to_hw(connector, user_level, user_max);
>  	panel->backlight.level = hw_level;
>  
> -	if (panel->backlight.device)
> -		panel->backlight.device->props.brightness =
> -			scale_hw_to_user(connector,
> -					 panel->backlight.level,
> -					 panel->backlight.device->props.max_brightness);
> +	if (panel->backlight.device) {
> +		int brightness;
> +		int max = backlight_get_max_brightness(panel->backlight.device);
> +
> +		brightness = scale_hw_to_user(connector,
> +					      panel->backlight.level,
> +					      max);
> +		backlight_set_brightness(panel->backlight.device, brightness);
> +	}

Seems okay.

>  
>  	if (panel->backlight.enabled)
>  		intel_panel_actually_set_backlight(conn_state, hw_level);
> @@ -871,8 +875,7 @@ void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_st
>  
>  	mutex_lock(&dev_priv->backlight_lock);
>  
> -	if (panel->backlight.device)
> -		panel->backlight.device->props.power = FB_BLANK_POWERDOWN;
> +	backlight_disable(panel->backlight.device);

This is a fairly big functional change, without rationale, as this calls
back to intel_backlight_device_update_status() and does a lot of stuff
that supposedly wasn't needed before. Including grabbing a bunch of
mutexes on the way.

>  	panel->backlight.enabled = false;
>  	panel->backlight.disable(old_conn_state);
>  
> @@ -1192,17 +1195,20 @@ static void __intel_panel_enable_backlight(const struct intel_crtc_state *crtc_s
>  
>  	if (panel->backlight.level <= panel->backlight.min) {
>  		panel->backlight.level = panel->backlight.max;
> -		if (panel->backlight.device)
> -			panel->backlight.device->props.brightness =
> -				scale_hw_to_user(connector,
> -						 panel->backlight.level,
> -						 panel->backlight.device->props.max_brightness);
> +		if (panel->backlight.device) {
> +			int brightness;
> +			int max = backlight_get_max_brightness(panel->backlight.device);
> +
> +			brightness = scale_hw_to_user(connector,
> +						      panel->backlight.level,
> +						      max);
> +			backlight_set_brightness(panel->backlight.device, brightness);
> +		}

Seems okay.

>  	}
>  
>  	panel->backlight.enable(crtc_state, conn_state);
>  	panel->backlight.enabled = true;
> -	if (panel->backlight.device)
> -		panel->backlight.device->props.power = FB_BLANK_UNBLANK;
> +	backlight_enable(panel->backlight.device);

Again, fairly big functional change.

>  }
>  
>  void intel_panel_enable_backlight(const struct intel_crtc_state *crtc_state,
> @@ -1288,10 +1294,11 @@ static int intel_backlight_device_update_status(struct backlight_device *bd)
>  
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	DRM_DEBUG_KMS("updating intel_backlight, brightness=%d/%d\n",
> -		      bd->props.brightness, bd->props.max_brightness);
> -	intel_panel_set_backlight(connector->base.state, bd->props.brightness,
> -				  bd->props.max_brightness);
> -
> +		      backlight_get_brightness(bd),
> +		      backlight_get_max_brightness(bd));
> +	intel_panel_set_backlight(connector->base.state,
> +				  backlight_get_brightness(bd),
> +				  backlight_get_max_brightness(bd));
>  	/*
>  	 * Allow flipping bl_power as a sub-state of enabled. Sadly the
>  	 * backlight class device does not make it easy to to differentiate
> @@ -1299,13 +1306,10 @@ static int intel_backlight_device_update_status(struct backlight_device *bd)
>  	 * callback needs to take this into account.
>  	 */
>  	if (panel->backlight.enabled) {
> -		if (panel->backlight.power) {
> -			bool enable = bd->props.power == FB_BLANK_UNBLANK &&
> -				bd->props.brightness != 0;
> -			panel->backlight.power(connector, enable);
> -		}
> +		if (panel->backlight.power)
> +			panel->backlight.power(connector, !backlight_is_blank(bd));
>  	} else {
> -		bd->props.power = FB_BLANK_POWERDOWN;
> +		backlight_disable(bd);

Err what? Does this not deadlock?

>  	}
>  
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> @@ -1322,12 +1326,12 @@ static int intel_backlight_device_get_brightness(struct backlight_device *bd)
>  
>  	with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref) {
>  		u32 hw_level;
> +		int max = backlight_get_max_brightness(bd);
>  
>  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  
>  		hw_level = intel_panel_get_backlight(connector);
> -		ret = scale_hw_to_user(connector,
> -				       hw_level, bd->props.max_brightness);
> +		ret = scale_hw_to_user(connector, hw_level, max);

Seems okay.

>  
>  		drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  	}
> @@ -1344,7 +1348,12 @@ int intel_backlight_device_register(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
> -	struct backlight_properties props;
> +	/*
> +	 * Note: Everything should work even if the backlight device max
> +	 * presented to the userspace is arbitrarily chosen.
> +	 */
> +	DECLARE_BACKLIGHT_INIT_RAW(props, 0, panel->backlight.max);
> +	int brightness;
>  
>  	if (WARN_ON(panel->backlight.device))
>  		return -ENODEV;
> @@ -1354,23 +1363,6 @@ int intel_backlight_device_register(struct intel_connector *connector)
>  
>  	WARN_ON(panel->backlight.max == 0);
>  
> -	memset(&props, 0, sizeof(props));
> -	props.type = BACKLIGHT_RAW;
> -
> -	/*
> -	 * Note: Everything should work even if the backlight device max
> -	 * presented to the userspace is arbitrarily chosen.
> -	 */
> -	props.max_brightness = panel->backlight.max;
> -	props.brightness = scale_hw_to_user(connector,
> -					    panel->backlight.level,
> -					    props.max_brightness);
> -
> -	if (panel->backlight.enabled)
> -		props.power = FB_BLANK_UNBLANK;
> -	else
> -		props.power = FB_BLANK_POWERDOWN;
> -
>  	/*
>  	 * Note: using the same name independent of the connector prevents
>  	 * registration of multiple backlight devices in the driver.
> @@ -1388,6 +1380,14 @@ int intel_backlight_device_register(struct intel_connector *connector)
>  		return -ENODEV;
>  	}
>  
> +	brightness = scale_hw_to_user(connector, panel->backlight.level, panel->backlight.max);
> +	backlight_set_brightness(panel->backlight.device, brightness);
> +
> +	if (panel->backlight.enabled)
> +		backlight_enable(panel->backlight.device);
> +	else
> +		backlight_disable(panel->backlight.device);
> +

So we've just taken care to read the status from the hardware, and set
up the software state accordingly. Why would it be necessary to write
the status back to the hardware again? Please don't.

>  	drm_dbg_kms(&i915->drm,
>  		    "Connector %s backlight sysfs interface registered\n",
>  		    connector->base.name);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel