Re: [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 7/11/20 8:32 AM, Uwe Kleine-König wrote:
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@xxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
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?

Yes I'm aware of the change in rounding and I do not believe that it is
relevant. One of the advantages of switching to the helpers is not having
to worry about the rounding and letting the helpers figure that out :)

  }
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.

Idem.


  }
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?!

It shouldn't happen this code uses only 2 PWM controller drivers,
pwm-crc and pwm-lpss and the get_state of neither ever sets
period tto 0. This check is just here for extra safety, since getting it
wrong would lead to a divide by 0. Which I see has been fixed by the
helper now (which does its own period==0 check). So I guess I can
(and I will) just drop this extra check for the next version.

+		/* 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



Regards,

Hans

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux