Hello Hans,
On Sat, Jun 20, 2020 at 02:17:58PM +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.
Note that there is no hard dependency, the atomic API should work just
fine even with a non-converted driver. (Of course a converted driver
behaves better, but the i915 using the atomic API with a non-converted
driver is just as good as the legacy API with the same driver.)
So regarding your plan to get this series in soon: There is no hard need
to halt the efforts for the drm part if the pwm patches take a bit
longer (or vice versa).
> 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.
>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> .../drm/i915/display/intel_display_types.h | 3 +-
> drivers/gpu/drm/i915/display/intel_panel.c | 73 +++++++++----------
> 2 files changed, 37 insertions(+), 39 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..a0f76343f381 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -592,10 +592,11 @@ 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;
> + int duty_ns, period_ns;
>
> duty_ns = pwm_get_duty_cycle(panel->backlight.pwm);
> - return DIV_ROUND_UP(duty_ns * 100, panel->backlight.pwm_period_ns);
> + period_ns = pwm_get_period(panel->backlight.pwm);
The transformation is correct, but using
pwm_get_state(pwm, &state);
duty_ns = state.duty_cycle;
period_ns = state.period;
is a bit more effective as it calls pwm_get_state() only once.
There is a function pwm_get_relative_duty_cycle which is similar (with
scale = 100) just used different rounding.
> + return DIV_ROUND_UP(duty_ns * 100, period_ns);
> }
>
> static void lpt_set_backlight(const struct drm_connector_state *conn_state, u32 level)
> @@ -669,10 +670,10 @@ 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);
> + panel->backlight.pwm_state.duty_cycle =
> + DIV_ROUND_UP(level * panel->backlight.pwm_state.period, 100);
> + pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> }
>
> static void
> @@ -841,10 +842,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);
Did you drop intel_panel_actually_set_backlight and the sleep on purpose?
> }
>
> void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_state)
> [...]
> @@ -1916,36 +1919,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)) {
> + /* PWM is already enabled, use existing settings */
> + pwm_get_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> +
> + level = DIV_ROUND_UP(panel->backlight.pwm_state.duty_cycle *
> + 100, panel->backlight.pwm_state.period);
> + level = intel_panel_compute_brightness(connector, level);
In pwm_enable_backlight() the order of these two steps is reversed
compared to here. Maybe this calculation can be moved into
intel_panel_compute_brightness()?
> + panel->backlight.level = clamp(level, panel->backlight.min,
> + panel->backlight.max);
> + panel->backlight.enabled = true;
> +
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
