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 <[email protected]> > Signed-off-by: Hans de Goede <[email protected]> > --- > 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/ |
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
