On 25-02-27 16:51:15, Uwe Kleine-König wrote:
> Hello Abel,
>
> On Thu, Feb 27, 2025 at 03:07:21PM +0200, Abel Vesa wrote:
> > On 25-02-26 17:34:50, Uwe Kleine-König wrote:
> > > On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> > > > The current implementation assumes that the PWM provider will be able to
> > > > meet the requested period, but that is not always the case. Some PWM
> > > > providers have limited HW configuration capabilities and can only
> > > > provide a period that is somewhat close to the requested one. This
> > > > simply means that the duty cycle requested might either be above the
> > > > PWM's maximum value or the 100% duty cycle is never reached.
> > >
> > > If you request a state with 100% relative duty cycle you should get 100%
> > > unless the hardware cannot do that. Which PWM hardware are you using?
> > > Which requests are you actually doing that don't match your expectation?
> >
> > The PWM hardware is Qualcomm PMK8550 PMIC. The way the duty cycle is
> > controlled is described in the following comment found in lpg_calc_freq
> > of the leds-qcom-lpg driver:
> >
> > /*
> > * The PWM period is determined by:
> > *
> > * resolution * pre_div * 2^M
> > * period = --------------------------
> > * refclk
> > *
> > * Resolution = 2^9 bits for PWM or
> > * 2^{8, 9, 10, 11, 12, 13, 14, 15} bits for high resolution PWM
> > * pre_div = {1, 3, 5, 6} and
> > * M = [0..7].
> > *
> > * This allows for periods between 27uS and 384s for PWM channels and periods between
> > * 3uS and 24576s for high resolution PWMs.
> > * The PWM framework wants a period of equal or lower length than requested,
> > * reject anything below minimum period.
> > */
> >
> > So if we request a period of 5MHz, that will not ever be reached no matter what config
> > is used. Instead, the 4.26 MHz is selected as closest possible.
>
> The trace in the other mail thread suggest that you asked for a period
> of 5 ms, not 5 MHz. And that results in a period of 4.26 ms.
OK. So unit is ms. Got it.
>
> > Now, the pwm_bl is not aware of this limitation and will request duty cycle values that
> > go above 4.26MHz.
>
> It requests .period = 5 ms + .duty_cycle = 5 ms. This is fine, and
> according to the trace this results in both values becoming 4.26 ms in
> real life. Seems fine to me.
Right, but as I keep trying to explain is that, the consumer keeps
asking for duty cycles that go over the 4.26ms, which is the period that
the provider decided it can do instead of 5ms.
>
> > > > This could be easily fixed if the pwm_apply*() API family would allow
> > > > overriding the period within the PWM state that's used for providing the
> > > > duty cycle. But that is currently not the case.
> > >
> > > I don't understand what you mean here.
> >
> > What I was trying to say is that the PWM generic framework currently doesn't
> > allow overriding the PWM state's period with one provided by the consumer,
> > when calling pwm_apply_might_sleep().
>
> Either I still don't understand what you want, or that is impossible or
> useless. If you target .period = 5 ms and the hardware can only do 4.26
> ms, why would you want to override period to 5 ms?
Meaning the consumer should become aware of the period the provider can
do before asking for a duty cycle.
If you look at the other mail thread, the trace there shows the
following sequence for every new backlight update request:
1. pwm_apply with consumer's period (5ms)
2. pwm_get reads the provider's period (4.25ms)
- which is what the provider is able to do instead of 5ms
3. pwm_apply (due to debug) which uses the state from 2.
4. pwm_get reads back exactly as 2.
So we can ignore 3 and 4 for now as they are there due to debug,
but the step 1 still requests a value over the 4.26ms (5ms),
which in the provider will translate to a pwm value higher than allowed
by the selected configuration.
>
> > Also, the pwm_get_state_hw() doesn't cache the state either.
>
> *shrug*.
>
> > This results in always having to call pwm_get_state_hw() before calling
> > pwm_apply_might_sleep().
>
> I cannot follow this conclusion. At least one of us two didn't
> understand some detail yet.
>
> > On top of that, pwm_get_state_hw() doesn't default to the cached value if the
> > provider doesn't implement the ->get_state() op.
>
> If it did that, the consumer wouldn't know if the request was
> implemented exactly or if there is no way to read back the actual
> configuration.
>
> > Please correct me if I'm wrong about these.
> >
> > >
> > > > So easiest fix here is to read back the period from the PWM provider via
> > > > the provider's ->get_state() op, if implemented, which should provide the
> > > > best matched period. Do this on probe after the first ->pwm_apply() op has
> > > > been done, which will allow the provider to determine the best match
> > > > period based on available configuration knobs. From there on, the
> > > > backlight will use the best matched period, since the driver's internal
> > > > PWM state is now synced up with the one from provider.
> > > > [...]
> > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > index 237d3d3f3bb1a6d713c5f6ec3198af772bf1268c..71a3e9cd8844095e85c01b194d7466978f1ca78e 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -525,6 +525,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > goto err_alloc;
> > > > }
> > > >
> > > > + /*
> > > > + * The actual period might differ from the requested one due to HW
> > > > + * limitations, so sync up the period with one determined by the
> > > > + * provider driver.
> > > > + */
> > > > + ret = pwm_get_state_hw(pb->pwm, &pb->pwm->state);
> > >
> > > As a consumer you're not supposed to write to &pb->pwm->state. That's a
> > > layer violation. Please call pwm_get_state_hw() with a struct pwm_state
> > > that you own and save the relevant parts in your driver data.
> >
> > Yep, that is indeed wrong. Maybe making the pwm opaque might be a good idea as well.
> >
> > [1] Calling pwm_get_state_hw() would be wrong if the provider doesn't implement the ->get_state(),
> > as I mentioned above.
> >
> > But are you suggesting we replace all calls to pwm_get_state() with
> > pwm_get_state_hw() in pwm_bl?
>
> No, I still didn't understand the problem you want to fix here. So I'm
> not suggesting anything yet.
>
> Best regards
> Uwe
