Re: [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Hans de Goede <hdegoede@redhat.com>
Cc: "Thierry Reding" <thierry.reding@gmail.com>,
	"Jani Nikula" <jani.nikula@linux.intel.com>,
	"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	"Len Brown" <lenb@kernel.org>,
	linux-pwm@vger.kernel.org,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Mika Westerberg" <mika.westerberg@linux.intel.com>,
	linux-acpi@vger.kernel.org, "Jani Nikula" <jani.nikula@intel.com>
Subject: Re: [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller
Date: Sat, 11 Jul 2020 08:32:23 +0200	[thread overview]
Message-ID: <20200711063223.czly2ftjraomuxz6@pengutronix.de> (raw)
In-Reply-To: <20200708211432.28612-17-hdegoede@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 8465 bytes --]

On Wed, Jul 08, 2020 at 11:14:32PM +0200, Hans de Goede wrote:
> Now that the PWM drivers which we use have been converted to the atomic
> PWM API, we can move the i915 panel code over to using the atomic PWM API.
> 
> The removes a long standing FIXME and this removes a flicker where
> the backlight brightness would jump to 100% when i915 loads even if
> using the fastset path.
> 
> Note that this commit also simplifies pwm_disable_backlight(), by dropping
> the intel_panel_actually_set_backlight(..., 0) call. This call sets the
> PWM to 0% duty-cycle. I believe that this call was only present as a
> workaround for a bug in the pwm-crc.c driver where it failed to clear the
> PWM_OUTPUT_ENABLE bit. This is fixed by an earlier patch in this series.
> 
> After the dropping of this workaround, the usleep call, which seems
> unnecessary to begin with, has no useful effect anymore, so drop that too.
> 
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> - Add a note to the commit message about the dropping of the
>   intel_panel_actually_set_backlight() and usleep() calls from
>   pwm_disable_backlight()
> - Use the pwm_set/get_relative_duty_cycle() helpers instead of using DIY code
>   for this
> ---
>  .../drm/i915/display/intel_display_types.h    |  3 +-
>  drivers/gpu/drm/i915/display/intel_panel.c    | 71 +++++++++----------
>  2 files changed, 34 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index de32f9efb120..4bd9981e70a1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -28,6 +28,7 @@
>  
>  #include <linux/async.h>
>  #include <linux/i2c.h>
> +#include <linux/pwm.h>
>  #include <linux/sched/clock.h>
>  
>  #include <drm/drm_atomic.h>
> @@ -223,7 +224,7 @@ struct intel_panel {
>  		bool util_pin_active_low;	/* bxt+ */
>  		u8 controller;		/* bxt+ only */
>  		struct pwm_device *pwm;
> -		int pwm_period_ns;
> +		struct pwm_state pwm_state;
>  
>  		/* DPCD backlight */
>  		u8 pwmgen_bit_count;
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index cb28b9908ca4..3d97267c8238 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -592,10 +592,10 @@ static u32 bxt_get_backlight(struct intel_connector *connector)
>  static u32 pwm_get_backlight(struct intel_connector *connector)
>  {
>  	struct intel_panel *panel = &connector->panel;
> -	int duty_ns;
> +	struct pwm_state state;
>  
> -	duty_ns = pwm_get_duty_cycle(panel->backlight.pwm);
> -	return DIV_ROUND_UP(duty_ns * 100, panel->backlight.pwm_period_ns);
> +	pwm_get_state(panel->backlight.pwm, &state);
> +	return pwm_get_relative_duty_cycle(&state, 100);

Here you introduce a slight difference: pwm_get_relative_duty_cycle uses
round-closest while you replace a round-up. Is this relevant?

>  }
>  
>  static void lpt_set_backlight(const struct drm_connector_state *conn_state, u32 level)
> @@ -669,10 +669,9 @@ static void bxt_set_backlight(const struct drm_connector_state *conn_state, u32
>  static void pwm_set_backlight(const struct drm_connector_state *conn_state, u32 level)
>  {
>  	struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
> -	int duty_ns = DIV_ROUND_UP(level * panel->backlight.pwm_period_ns, 100);
>  
> -	pwm_config(panel->backlight.pwm, duty_ns,
> -		   panel->backlight.pwm_period_ns);
> +	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
> +	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);

Similar here: The function used to use round-up but
pwm_set_relative_duty_cycle() used round-closest.

>  }
>  
>  static void
> @@ -841,10 +840,8 @@ static void pwm_disable_backlight(const struct drm_connector_state *old_conn_sta
>  	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
>  	struct intel_panel *panel = &connector->panel;
>  
> -	/* Disable the backlight */
> -	intel_panel_actually_set_backlight(old_conn_state, 0);
> -	usleep_range(2000, 3000);
> -	pwm_disable(panel->backlight.pwm);
> +	panel->backlight.pwm_state.enabled = false;
> +	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
>  }
>  
>  void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_state)
> @@ -1176,9 +1173,12 @@ static void pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct intel_panel *panel = &connector->panel;
> +	int level = panel->backlight.level;
>  
> -	pwm_enable(panel->backlight.pwm);
> -	intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> +	level = intel_panel_compute_brightness(connector, level);
> +	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
> +	panel->backlight.pwm_state.enabled = true;
> +	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
>  }
>  
>  static void __intel_panel_enable_backlight(const struct intel_crtc_state *crtc_state,
> @@ -1897,8 +1897,7 @@ static int pwm_setup_backlight(struct intel_connector *connector,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_panel *panel = &connector->panel;
>  	const char *desc;
> -	u32 level, ns;
> -	int retval;
> +	u32 level;
>  
>  	/* Get the right PWM chip for DSI backlight according to VBT */
>  	if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) {
> @@ -1916,36 +1915,30 @@ static int pwm_setup_backlight(struct intel_connector *connector,
>  		return -ENODEV;
>  	}
>  
> -	panel->backlight.pwm_period_ns = NSEC_PER_SEC /
> -					 get_vbt_pwm_freq(dev_priv);
> -
> -	/*
> -	 * FIXME: pwm_apply_args() should be removed when switching to
> -	 * the atomic PWM API.
> -	 */
> -	pwm_apply_args(panel->backlight.pwm);
> -
>  	panel->backlight.max = 100; /* 100% */
>  	panel->backlight.min = get_backlight_min_vbt(connector);
> -	level = intel_panel_compute_brightness(connector, 100);
> -	ns = DIV_ROUND_UP(level * panel->backlight.pwm_period_ns, 100);
>  
> -	retval = pwm_config(panel->backlight.pwm, ns,
> -			    panel->backlight.pwm_period_ns);
> -	if (retval < 0) {
> -		drm_err(&dev_priv->drm, "Failed to configure the pwm chip\n");
> -		pwm_put(panel->backlight.pwm);
> -		panel->backlight.pwm = NULL;
> -		return retval;
> +	if (pwm_is_enabled(panel->backlight.pwm) &&
> +	    pwm_get_period(panel->backlight.pwm)) {

What would pwm_is_enabled(panel->backlight.pwm) == true &&
pwm_get_period(panel->backlight.pwm) == 0 mean? I hope this doesn't
happen?!

> +		/* PWM is already enabled, use existing settings */
> +		pwm_get_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> +
> +		level = pwm_get_relative_duty_cycle(&panel->backlight.pwm_state,
> +						    100);
> +		level = intel_panel_compute_brightness(connector, level);
> +		panel->backlight.level = clamp(level, panel->backlight.min,
> +					       panel->backlight.max);
> +		panel->backlight.enabled = true;
> +
> +		drm_dbg_kms(&dev_priv->drm, "PWM already enabled at freq %ld, VBT freq %d, level %d\n",
> +			    NSEC_PER_SEC / panel->backlight.pwm_state.period,

.period becomes a u64 soon, so be prepared to fixup here.

> +			    get_vbt_pwm_freq(dev_priv), level);
> +	} else {
> +		/* Set period from VBT frequency, leave other settings at 0. */
> +		panel->backlight.pwm_state.period =
> +			NSEC_PER_SEC / get_vbt_pwm_freq(dev_priv);
>  	}
>  
> -	level = DIV_ROUND_UP(pwm_get_duty_cycle(panel->backlight.pwm) * 100,
> -			     panel->backlight.pwm_period_ns);
> -	level = intel_panel_compute_brightness(connector, level);
> -	panel->backlight.level = clamp(level, panel->backlight.min,
> -				       panel->backlight.max);
> -	panel->backlight.enabled = panel->backlight.level != 0;
> -
>  	drm_info(&dev_priv->drm, "Using %s PWM for LCD backlight control\n",
>  		 desc);
>  	return 0;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]