From: Hans de Goede <hdegoede@redhat.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
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 15:51:57 +0200 [thread overview]
Message-ID: <0fdcf465-3981-390f-7af4-235c7a48e438@redhat.com> (raw)
In-Reply-To: <20200711063223.czly2ftjraomuxz6@pengutronix.de>
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@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?
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
next prev parent reply other threads:[~2020-07-11 13:52 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-08 21:14 [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
2020-07-08 21:14 ` [PATCH v4 01/16] ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase Hans de Goede
2020-07-08 21:14 ` [PATCH v4 02/16] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (at activation) Hans de Goede
2020-07-08 21:14 ` [PATCH v4 03/16] pwm: lpss: Fix off by one error in base_unit math in pwm_lpss_prepare() Hans de Goede
2020-07-08 21:14 ` [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value Hans de Goede
2020-07-09 12:53 ` Andy Shevchenko
2020-07-09 13:23 ` Hans de Goede
2020-07-09 14:21 ` Andy Shevchenko
2020-07-09 14:33 ` Hans de Goede
2020-07-09 14:51 ` Andy Shevchenko
2020-07-08 21:14 ` [PATCH v4 05/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume Hans de Goede
2020-07-09 13:36 ` Andy Shevchenko
2020-07-09 13:48 ` Hans de Goede
2020-07-08 21:14 ` [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0 Hans de Goede
2020-07-09 14:50 ` Andy Shevchenko
2020-07-09 15:47 ` Hans de Goede
2020-07-11 6:11 ` Uwe Kleine-König
2020-07-11 13:58 ` Hans de Goede
2020-07-08 21:14 ` [PATCH v4 07/16] pwm: crc: Fix period / duty_cycle times being off by a factor of 256 Hans de Goede
2020-07-08 21:14 ` [PATCH v4 08/16] pwm: crc: Fix off-by-one error in the clock-divider calculations Hans de Goede
2020-07-08 21:14 ` [PATCH v4 09/16] pwm: crc: Fix period changes not having any effect Hans de Goede
2020-07-08 21:14 ` [PATCH v4 10/16] pwm: crc: Enable/disable PWM output on enable/disable Hans de Goede
2020-07-08 21:14 ` [PATCH v4 11/16] pwm: crc: Implement apply() method to support the new atomic PWM API Hans de Goede
2020-07-08 21:14 ` [PATCH v4 12/16] pwm: crc: Implement get_state() method Hans de Goede
2020-07-08 21:14 ` [PATCH v4 13/16] drm/i915: panel: Add get_vbt_pwm_freq() helper Hans de Goede
2020-07-08 21:14 ` [PATCH v4 14/16] drm/i915: panel: Honor the VBT PWM frequency for devs with an external PWM controller Hans de Goede
2020-07-08 21:14 ` [PATCH v4 15/16] drm/i915: panel: Honor the VBT PWM min setting " Hans de Goede
2020-07-08 21:14 ` [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API " Hans de Goede
2020-07-11 6:32 ` Uwe Kleine-König
2020-07-11 13:51 ` Hans de Goede [this message]
2020-07-09 14:14 ` [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Sam Ravnborg
2020-07-09 14:40 ` Hans de Goede
2020-07-09 15:23 ` Sam Ravnborg
2020-07-11 6:19 ` Uwe Kleine-König
2020-07-11 13:46 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0fdcf465-3981-390f-7af4-235c7a48e438@redhat.com \
--to=hdegoede@redhat.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
--cc=jani.nikula@linux.intel.com \
--cc=joonas.lahtinen@linux.intel.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=mika.westerberg@linux.intel.com \
--cc=rjw@rjwysocki.net \
--cc=rodrigo.vivi@intel.com \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).