Hello Neil, On Wed, Mar 05, 2025 at 03:42:56PM +0100, neil.armstrong@xxxxxxxxxx wrote: > On 05/03/2025 15:32, Abel Vesa wrote: > > On 25-03-04 16:38:57, Uwe Kleine-König wrote: > > > On Tue, Mar 04, 2025 at 11:21:33AM +0200, Abel Vesa wrote: > > > > On 25-03-04 07:24:32, Uwe Kleine-König wrote: > > > > > I guess you spend some time understanding the workings of the driver and > > > > > you also have an example request that results in a hardware > > > > > configuration you don't like. Please share the latter to a) support your > > > > > case and b) make it easier for your reviewers to judge if your change is > > > > > indeed an improvement. > > > > > > > > Sure, will bring up the 5ms period scenario again. > > > > > > > > When the consumer requests a period of 5ms, the closest the HW can do in > > > > this case is actually 4.26ms. Since the PWM API will continue to ask for > > > > duty cycle values based on the 5ms period, for any duty cycle value > > > > between 4.26ms and 5ms, the resulting PWM value will be above 255, which > > > > has been selected as best resolution for the 4.26ms best matched period. > > > > > > > > For example, when 5ms duty cycle value is requested, it will result in a > > > > PWM value of 300, which overflows the 255 selected resolution. > > > > > > this is the bug you have to fix then. The PWM value (that defines the > > > duty cycle) has to be calculated based on .period = 4.26 ms and capped > > > at 255. So assuming that 0 yields a duty cycle of 0 ms and 255 yields > > > 4.26 ms, a request for .duty_cycle = 4; + .period = 5 should result in an > > > actual .duty_cycle = 239 / 255 * 4.26 ms = 3.992705882352941 ms; > > > + .period = 4.26 ms. > > > > OK then. The patchset that fixes this according to your suggestion is > > already on the list (re-spun): > > > > https://lore.kernel.org/all/20250305-leds-qcom-lpg-fix-max-pwm-on-hi-res-v4-0-bfe124a53a9f@xxxxxxxxxx/ Yeah, I thought so. It's in my review queue. > > > > > > So change the way the PWM value is determined as a ratio between the > > > > > > requested period and duty cycle, mapped on the resolution interval. > > > > > > > > > > Is the intention here that (for the picked period) a duty_cycle is > > > > > selected that approximates the requested relative duty_cycle (i.e. > > > > > .duty_cycle / .period)? > > > > > > > > Yes, that exactly what the intention is. > > > > > > > > > If it's that: Nack. This might be the right thing for your use case, but > > > > > it's wrong for others, it complicates the driver because you have spend > > > > > more effort in the calculation and (from my POV even worse) the driver's > > > > > behaviour deviates from the usual one for pwm drivers. I admit there are > > > > > some other lowlevel pwm drivers that are not aligned to the procedure I > > > > > described that should be used to determine the register settings for a > > > > > given request. But I target a common behaviour of all pwm drivers > > > > > because that is the only way the pwm API functions can make a promise to > > > > > its consumers about the resulting behaviour. Reaching this is difficult, > > > > > because some consumers might depend on the "broken" behaviour of a given > > > > > lowlevel driver (and also because analysing a driver to check and fix > > > > > its behaviour is an effort). But "fixing" a driver to deviate from the > > > > > declared right behaviour is wrong and includes all downsides that make > > > > > me hesitate to align the old drivers to the common policy. > > > > > > > > OK, fair enough. But I still don't get what you expect from the provider > > > > that can't give the exact requested period. Do you expect the consumer > > > > to request a period, then provider compute a best matched one, which in > > > > our case is pretty far, and then still give exact duty cycle values ? > > > > > > > > Like: request 5ms period, get 4.26ms instead, then request 4ms duty > > > > cycle and get exact 4ms duty cycle when measured, instead of a > > > > proportional value to the best matched period? > > > > > > Yes. > > > > If so, then what happens when consumer asks for 5ms duty cycle? > > > > Everything above the 4.26ms will just represent 100% duty cycle. > > > > > > Yes. > > > > I still think this is wrong. Well, if you asked for .period = 5ms and .duty_cycle = 5ms you even asked for a 100% relative duty cycle. So while I agree that you don't usually get exactly what you requested, this is a bad example to rant about. > I also think this is very wrong, duty_cycle is a factor of the period, > so if the HW gives a lower period, the term Pulse Width Modulation implies > the ratio between the "duty_cycle" and the period is important, > not the exact duration of the components of the modulation. In Linux .duty_cycle was expressed in ns and not relative to period. Apart from that being a historic choice that is hard to change, IMHO this is a sane choice because in the kernel we have to stick to integer math and then if you want to express a relative duty_cycle you probably have to pick a divisor D such that .rel_duty_cycle = n represents a relative duty_cycle of n / D. What to pick for D? 100 to get percent as unit? Something bigger to increase precision? A power of two to match usual hardwares but make it less intuitive for humans? Also note that for some hardwares the "natural" divisor is 255. > So is this a defect of the PWM API ? why would the API insist on > having an exact duty_cycle and a random period ? The way to determine the actual hardware dependent settings for a requested pair of duty_cycle and period is IMHO straight forward, so duty_cycle selection isn't more exact than the one for period and period isn't more random than the one for duty_cycle. It can also happen the other way round that your request results in an near exact match for period and a big deviation for duty_cycle. So judging the defects of the PWM API from just one example is short-sighted. But still I hear you and the rules were defined as they are as a trade-off between consumer needs, needed complexity in lowlevel drivers and what most drivers already did at that time. Regarding consumer needs: I agree that most consumers care about the relative duty_cycle. But there are exceptions. I remember a motor where the absolute length of the duty_cycle defines the rotation speed and the period was more or less irrelevant. While here the point mostly goes to "keep relative duty_cycle", it's still a "you cannot please everyone" situation. Regarding complexity: A simple PWM typically has a fixed clock input and there is a register to define the period as a number of clock cycles. Then there are essentially two subtypes: a) the duty_cycle register uses the same time base as the period register; and b) the duty_cycle register unit is relative to the period length. Let's assume a clock input rate of 32768 Hz, so one cycle tick is q := 1 / (32768 Hz) ≅ 0.030517578125 ms. So the typical PWM of type a) has a 16 bit register for period and another one for duty_cycle. I think it's quite obvious that the chosen policy is very simple to implement for such a device, so I won't go into that further. Considering the "keep relative duty_cycle + round down" policy instead and a request .period = 5ms and .duty_cycle = 3ms. The best match for .period is 163q ≅ 4.974365234375 ms. Then to calculate the duty_cycle you have do determine: 163q * 3 / 5 = 97q ≅ 2.960205078125 ms giving an actual relative duty_cycle of 0.5950920245398773. This is quite a good fit. Using the "use the absolute values" policy we end up with duty_cycle = 98q ≅ 2.99072265625 (and the same period) which gives a relative duty_cycle of 0.6012269938650306. The result is somewhat similar (with 0.6012269938650306 being a slightly better result as 0.5950920245398773?), but the calculation needed for the "use the absolute values" is a bit simpler. In summary we can say that it's quite natural to round down both values in the "use the absolute values" case independent of your preferred policy, while rounding down in combination with the "keep relative duty_cycle" policy is tends to be worse because the duty_cycle register value is rounding down the result of a calculation that has a rounded value as input, so precision suffers. If your reflex now is to not always round down but sometimes(?) round up, please consider maintenance effort: This must be reviewed and it must be explained to driver authors. So that's not a good idea. Now let's consider a PWM of type b) with the same input clock freq. To be able to define the duty cycle in time units relative to the period, the period can be a multiple of 256q and the .duty_cycle register is an 8 bit one. 256q is already above 5 ms, so to get a fairer comparison let's assume a request of .period = 1280 ms and .duty_cycle = 768 ms (which is the above request just scaled by 256). So period ends up being 163 * 256q = 1273.4375 ms. With the "use the absolute values" policy we end up with .duty_cycle = 163 * 154q ≅ 766.05224609375 ms giving an relative duty_cycle of 0.6015625 and with the "keep relative duty_cycle" policy you end up with .duty_cycle = 163 * 153q ≅ 761.077880859375 ms and a relative duty_cycle of 0.59765625. In summary for b) there is again not much difference in the resulting configurations and complexity is again similar with a slight advantage for "keep relative duty_cycle". Without having done a complete survey back when I decided about the policy to pick my impression was that PWMs of type a) were more common. Also in my impression back then the difference in complexity between the two policies to chose among is smaller for type b) than for type a) which gives another slight advantage to "use the absolute values". Also looking at the drivers back then, "use the absolute values" policy was the more common one. Additionally I didn't like the fact that for the "keep relative duty_cycle" policy you have to base the calculation of duty_cycle on rounded values. So overall this made me pick the "use the absolute values" policy. And please believe me when I say this wasn't a whim of the moment decision but I invested quite some thought. The example you gave is somewhat a corner case because the requested and the actual period quite a lot---as you noticed yourself---and can be worked around by picking a better value for .period as I wrote in my previous mail. And no matter which policy you pick, depending on the use case for your consumer you will be able to find such degenerated examples. Having said all that (and hoping that this made it better understandable why we're where we are), there is an effort to improve here and to give consumers a better control over what they get. (But for the needs of a backlight this is probably overkill, so I refer again to the suggestion to pick a period that better matches the hardware.) Best regards Uwe
Attachment:
signature.asc
Description: PGP signature
