diff mbox series

[v6,16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller

Message ID 20200824120126.7116-17-hdegoede@redhat.com
State Superseded
Headers show
Series acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API | expand

Commit Message

Hans de Goede Aug. 24, 2020, 12:01 p.m. UTC
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 v6:
- Drop the pwm_get_period() check in pwm_setup(), it is now longer needed
  now that we use pwm_get_relative_duty_cycle()

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    | 70 ++++++++-----------
 2 files changed, 33 insertions(+), 40 deletions(-)

Comments

kernel test robot Aug. 24, 2020, 6:52 p.m. UTC | #1
Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on pwm/for-next]
[also build test ERROR on pm/linux-next v5.9-rc2 next-20200824]
[cannot apply to drm-intel/for-linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/acpi-pwm-i915-Convert-pwm-crc-and-i915-driver-s-PWM-code-to-use-the-atomic-PWM-API/20200824-200516
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: i386-randconfig-r021-20200824 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: drivers/gpu/drm/i915/display/intel_panel.o: in function `pwm_setup_backlight':
>> drivers/gpu/drm/i915/display/intel_panel.c:1932: undefined reference to `__udivdi3'

# https://github.com/0day-ci/linux/commit/72f1809d336477ff767fc9d5b65ac79dd31b6dca
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Hans-de-Goede/acpi-pwm-i915-Convert-pwm-crc-and-i915-driver-s-PWM-code-to-use-the-atomic-PWM-API/20200824-200516
git checkout 72f1809d336477ff767fc9d5b65ac79dd31b6dca
vim +1932 drivers/gpu/drm/i915/display/intel_panel.c

  1892	
  1893	static int pwm_setup_backlight(struct intel_connector *connector,
  1894				       enum pipe pipe)
  1895	{
  1896		struct drm_device *dev = connector->base.dev;
  1897		struct drm_i915_private *dev_priv = to_i915(dev);
  1898		struct intel_panel *panel = &connector->panel;
  1899		const char *desc;
  1900		u32 level;
  1901	
  1902		/* Get the right PWM chip for DSI backlight according to VBT */
  1903		if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) {
  1904			panel->backlight.pwm = pwm_get(dev->dev, "pwm_pmic_backlight");
  1905			desc = "PMIC";
  1906		} else {
  1907			panel->backlight.pwm = pwm_get(dev->dev, "pwm_soc_backlight");
  1908			desc = "SoC";
  1909		}
  1910	
  1911		if (IS_ERR(panel->backlight.pwm)) {
  1912			drm_err(&dev_priv->drm, "Failed to get the %s PWM chip\n",
  1913				desc);
  1914			panel->backlight.pwm = NULL;
  1915			return -ENODEV;
  1916		}
  1917	
  1918		panel->backlight.max = 100; /* 100% */
  1919		panel->backlight.min = get_backlight_min_vbt(connector);
  1920	
  1921		if (pwm_is_enabled(panel->backlight.pwm)) {
  1922			/* PWM is already enabled, use existing settings */
  1923			pwm_get_state(panel->backlight.pwm, &panel->backlight.pwm_state);
  1924	
  1925			level = pwm_get_relative_duty_cycle(&panel->backlight.pwm_state,
  1926							    100);
  1927			level = intel_panel_compute_brightness(connector, level);
  1928			panel->backlight.level = clamp(level, panel->backlight.min,
  1929						       panel->backlight.max);
  1930			panel->backlight.enabled = true;
  1931	
> 1932			drm_dbg_kms(&dev_priv->drm, "PWM already enabled at freq %lld, VBT freq %d, level %d\n",
  1933				    NSEC_PER_SEC / panel->backlight.pwm_state.period,
  1934				    get_vbt_pwm_freq(dev_priv), level);
  1935		} else {
  1936			/* Set period from VBT frequency, leave other settings at 0. */
  1937			panel->backlight.pwm_state.period =
  1938				NSEC_PER_SEC / get_vbt_pwm_freq(dev_priv);
  1939		}
  1940	
  1941		drm_info(&dev_priv->drm, "Using %s PWM for LCD backlight control\n",
  1942			 desc);
  1943		return 0;
  1944	}
  1945	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 7171e7c8d928..e3ebe7c313ba 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 6d27630dd1e4..f2b725f85f99 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);
 }
 
 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);
 }
 
 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,35 +1915,28 @@  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 is already enabled, use existing settings */
+		pwm_get_state(panel->backlight.pwm, &panel->backlight.pwm_state);
 
-	level = DIV_ROUND_UP_ULL(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;
+		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 %lld, VBT freq %d, level %d\n",
+			    NSEC_PER_SEC / panel->backlight.pwm_state.period,
+			    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);
+	}
 
 	drm_info(&dev_priv->drm, "Using %s PWM for LCD backlight control\n",
 		 desc);