| Message ID | 20200620121758.14836-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API | expand |
Hi, On 6/30/20 3:51 PM, Jani Nikula wrote: > On Sat, 20 Jun 2020, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi All, >> >> Here is v3 of my patch series converting the i915 driver's code for >> controlling the panel's backlight with an external PWM controller to >> use the atomic PWM API. See below for the changelog. >> >> Initially the plan was for this series to consist of 2 parts: >> 1. convert the pwm-crc driver to support the atomic PWM API and >> 2. convert the i915 driver's PWM code to use the atomic PWM API. >> >> But during testing I've found a number of bugs in the pwm-lpss and I >> found that the acpi_lpss code needs some special handling because of >> some ugliness found in most Cherry Trail DSDTs. >> >> So now this series has grown somewhat large and consists of 4 parts: >> >> 1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness >> 2. various fixes to the pwm-lpss driver >> 3. convert the pwm-crc driver to support the atomic PWM API and >> 4. convert the i915 driver's PWM code to use the atomic PWM API >> >> So we need to discuss how to merge this (once it passes review). >> Although the inter-dependencies are only runtime I still think we should >> make sure that 1-3 are in the drm-intel-next-queued (dinq) tree before >> merging the i915 changes. Both to make sure that the intel-gfx CI system >> does not become unhappy and for bisecting reasons. >> >> The involved acpi_lpss and pwm drivers do not see a whole lot of churn, so >> it likely is the easiest to just merge everything through dinq. >> >> Rafael and Thierry, can I get your Acked-by for directly merging this into >> dinq? >> >> This series has been tested (and re-tested after adding various bug-fixes) >> extensively. It has been tested on the following devices: >> >> -Asus T100TA BYT + CRC-PMIC PWM >> -Toshiba WT8-A BYT + CRC-PMIC PWM >> -Thundersoft TS178 BYT + CRC-PMIC PWM, inverse PWM >> -Asus T100HA CHT + CRC-PMIC PWM >> -Terra Pad 1061 BYT + LPSS PWM >> -Trekstor Twin 10.1 BYT + LPSS PWM >> -Asus T101HA CHT + CRC-PMIC PWM >> -GPD Pocket CHT + CRC-PMIC PWM > > For the drm/i915 patches 12-15, > > Acked-by: Jani Nikula <jani.nikula@intel.com> > > I eyeballed through them, and didn't spot anything obviously wrong, but > at the same time didn't have the time to do an in-depth review. That > said, I do have a lot of trust in you testing this with all the above > devices. I think that's enough for merging. > > As for that matter, I'm fine with merging this via whichever tree you > find best. Kind of stating the obvious, but please do ensure you have > the proper acks in place for this. Thank you. I plan to push the entire series to dinq once I have a full set of acks for the PWM changes (I already have acks for the rest). Regards, Hans >> Changelog: >> >> Changes in v2: >> - Fix coverletter subject >> - Drop accidentally included debugging patch >> - "[PATCH v3 02/15] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once ( >> - Move #define LPSS_SAVE_CTX_ONCE define to group it with LPSS_SAVE_CTX >> >> Changes in v3: >> - "[PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value" >> - Use base_unit_range - 1 as maximum value for the clamp() >> - "[PATCH v3 05/15] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume" >> - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM" >> patch from previous versions of this patch-set, which really was a hack >> working around the resume issue which this patch fixes properly. >> - PATCH v3 6 - 11 pwm-crc changes: >> - Various small changes resulting from the reviews by Andy and Uwe, >> including some refactoring of the patches to reduce the amount of churn >> in the patch-set >> >> Regards, >> >> Hans >> >
Hi, On 6/22/20 9:55 AM, Uwe Kleine-König wrote: > Hello, > > [adding Shobhit Kumar <shobhit.kumar@intel.com> to Cc who is the author > of this driver according to the comment on the top of the driver] > > On Sat, Jun 20, 2020 at 02:17:52PM +0200, Hans de Goede wrote: >> The pwm-crc code is using 2 different enable bits: >> 1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE) >> 2. bit 0 of the BACKLIGHT_EN register >> >> So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM, >> this commit makes crc_pwm_disable() clear it on disable and makes >> crc_pwm_enable() set it again on re-enable. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v3: >> - Remove paragraph about tri-stating the output from the commit message, >> we don't have a datasheet so this was just an unfounded guess > > I have the impression you spend quite some time with this driver trying > to understand it. Yes, my initial plan for this patch series was to just convert this driver to atomic PWM, but it turned out to need a bit of TLC first. > What I still think is a bit unfortunate is that there > is quite some guesswork involved. Actually for 99% of the changes I'm pretty sure they are correct. This patch is the 1% where I'm not sure, and in this case I'm playing it safe by keeping the code as is. As the commit message tries to explain I strongly suspect that bit 0 of the BACKLIGHT_EN register really drives a separate GPIO pin on the PMIC which is earmarked as backlight-enable pin (many LCD panels have both a pwm input for brightness-level and a separate enable/disable pin). If we can get information that my hunch here is correct then the right thing to do would be to change things so that the PWM driver stops poking bit 0 of the BACKLIGHT_EN register and this gets done by the CRC GPIO driver instead. But the poking of that bit is already happening now and since I'm not 100% sure that my hunch is correct, the safe thing to do is to keep this as is. Note that for the main consumer of the CRC PWM, the i915 driver it does not matter. If we change that bit into a GPIO then the i915 drv will need to be modified to drive the GPIO high / low when enabling / disabling the panel. Just like it already enables/ disables the PWM when enabling / disabling the panel. So the end result will still be bit 0 of the BACKLIGHT_EN register going high/low on LCD panel enable/disable. So even if my hunch is right functionality wise nothing will change. The code doing the poking will be technically more correct, but that is all that we would gain. > I wonder if it would be possible to > get the manual of that PWM. Do I understand correctly that this is IP > from Intel? There are quite some Intel people on Cc; maybe someone can > help/put in a good word/check and ack the changes? IIRC last time I asked no one from the Intel folks on the Cc has access to the Crystal Cove PMIC datasheet. Regards, Hans
Hi, On 6/22/20 9:57 AM, Uwe Kleine-König wrote: > On Sat, Jun 20, 2020 at 02:17:54PM +0200, Hans de Goede wrote: >> Implement the pwm_ops.get_state() method to complete the support for the >> new atomic PWM API. >> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v3: >> - Add Andy's Reviewed-by tag >> - Remove extra whitespace to align some code after assignments (requested by >> Uwe Kleine-König) >> --- >> drivers/pwm/pwm-crc.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c >> index 8a7f4707279c..b311354d40a3 100644 >> --- a/drivers/pwm/pwm-crc.c >> +++ b/drivers/pwm/pwm-crc.c >> @@ -119,8 +119,37 @@ static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, >> return 0; >> } >> >> +static void crc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, >> + struct pwm_state *state) >> +{ >> + struct crystalcove_pwm *crc_pwm = to_crc_pwm(chip); >> + struct device *dev = crc_pwm->chip.dev; >> + unsigned int clk_div, clk_div_reg, duty_cycle_reg; >> + int error; >> + >> + error = regmap_read(crc_pwm->regmap, PWM0_CLK_DIV, &clk_div_reg); >> + if (error) { >> + dev_err(dev, "Error reading PWM0_CLK_DIV %d\n", error); >> + return; >> + } >> + >> + error = regmap_read(crc_pwm->regmap, PWM0_DUTY_CYCLE, &duty_cycle_reg); >> + if (error) { >> + dev_err(dev, "Error reading PWM0_DUTY_CYCLE %d\n", error); >> + return; >> + } >> + >> + clk_div = (clk_div_reg & ~PWM_OUTPUT_ENABLE) + 1; >> + >> + state->period = clk_div * NSEC_PER_USEC * 256 / PWM_BASE_CLK_MHZ; >> + state->duty_cycle = duty_cycle_reg * state->period / PWM_MAX_LEVEL; > > Please round up here. Ok, I can fix that for the next version of this patch-set. Before I post a new version of this patch-set, you have only responded to some of the PWM patches in this set. Do you have any remarks on the other PWM patches ? Regards, Hans
Hi Uwe, On 7/7/20 9:50 AM, Uwe Kleine-König wrote: > 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). I understand, but the main reason to do the conversion to the atomic API, is to be able to skip the step where we force the backlight to 100% brightness (which can look quite ugly during boot). After this patch the intel-panel code initializes its internal backlight state and the brightness reported under /sys/class/backlight with the "brightness" returned from the PWM-drivers' get_state callback. Without getting the PWM patches in first I think that things will mostly work, but we will always report an initial brightness value of 0. Lets say it is actually 50% and the user then presses the increase-brightness hotkey, causing userspace to change it from 0% to 5% so instead of increasing it by 1/20th, it just decreased it a lot. So I do believe it is better to get the whole series in as a whole, since we are at rc4 already (time flies) I guess it might not make it in this cycle, but that is fine. Talking about merging this, is it ok for me to push the entire series upstream through the intel-drm-next-queued branch, once all the PWM patches have your Ack? >> 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@redhat.com> >> --- >> .../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. Ah nice, that is better then doing our own stuff. I will switch to that for v4 of this patch-set. >> + 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? Yes, that was on purpose. But I should probably have added a note about this to the commit message. For v4 of the patchset I will add the following note about this to the commit message for this patch: "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." >> } >> >> 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()? The intel_panel.c code deals with 7 different types of PWM controllers which are built into the GPU + support for external PWM controllers through the kernel's PWM subsystem. The code this patch changes is for the external PWM controller case, intel_panel_compute_brightness() is used for all supported PWM controllers. intel_panel_compute_brightness()'s function is to deal with panels where 100% duty-cycle is backlight fully off instead of fully-on. Normally it is called just before setting the duty-cycle, inverting the value/range before sending it to the hardware, since here we are reading back the current value we call it after reading back the value from the controller as the internally cached value is always in 0==min/off 100==max range, so if the panel inverts the range, we need to invert the read-back value to be in our "normalized" internal range. What we can do here is use pwm_get_relative_duty_cycle as you suggested above. I will change that for v4. > >> + panel->backlight.level = clamp(level, panel->backlight.min, >> + panel->backlight.max); >> + panel->backlight.enabled = true; >> + > > Best regards > Uwe > Regards, Hans
Hi All, Here is v3 of my patch series converting the i915 driver's code for controlling the panel's backlight with an external PWM controller to use the atomic PWM API. See below for the changelog. Initially the plan was for this series to consist of 2 parts: 1. convert the pwm-crc driver to support the atomic PWM API and 2. convert the i915 driver's PWM code to use the atomic PWM API. But during testing I've found a number of bugs in the pwm-lpss and I found that the acpi_lpss code needs some special handling because of some ugliness found in most Cherry Trail DSDTs. So now this series has grown somewhat large and consists of 4 parts: 1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness 2. various fixes to the pwm-lpss driver 3. convert the pwm-crc driver to support the atomic PWM API and 4. convert the i915 driver's PWM code to use the atomic PWM API So we need to discuss how to merge this (once it passes review). Although the inter-dependencies are only runtime I still think we should make sure that 1-3 are in the drm-intel-next-queued (dinq) tree before merging the i915 changes. Both to make sure that the intel-gfx CI system does not become unhappy and for bisecting reasons. The involved acpi_lpss and pwm drivers do not see a whole lot of churn, so it likely is the easiest to just merge everything through dinq. Rafael and Thierry, can I get your Acked-by for directly merging this into dinq? This series has been tested (and re-tested after adding various bug-fixes) extensively. It has been tested on the following devices: -Asus T100TA BYT + CRC-PMIC PWM -Toshiba WT8-A BYT + CRC-PMIC PWM -Thundersoft TS178 BYT + CRC-PMIC PWM, inverse PWM -Asus T100HA CHT + CRC-PMIC PWM -Terra Pad 1061 BYT + LPSS PWM -Trekstor Twin 10.1 BYT + LPSS PWM -Asus T101HA CHT + CRC-PMIC PWM -GPD Pocket CHT + CRC-PMIC PWM Changelog: Changes in v2: - Fix coverletter subject - Drop accidentally included debugging patch - "[PATCH v3 02/15] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once ( - Move #define LPSS_SAVE_CTX_ONCE define to group it with LPSS_SAVE_CTX Changes in v3: - "[PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value" - Use base_unit_range - 1 as maximum value for the clamp() - "[PATCH v3 05/15] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume" - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM" patch from previous versions of this patch-set, which really was a hack working around the resume issue which this patch fixes properly. - PATCH v3 6 - 11 pwm-crc changes: - Various small changes resulting from the reviews by Andy and Uwe, including some refactoring of the patches to reduce the amount of churn in the patch-set Regards, Hans