Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7329 bytes --]

Hello David,

On Tue, Dec 07, 2021 at 08:16:02AM +0100, David Jander wrote:
> On Mon, 6 Dec 2021 13:24:18 -0600
> David Lechner <david@lechnology.com> wrote:
> 
> > On 11/24/21 7:58 PM, William Breathitt Gray wrote:
> > > On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:  
> > >> Hi William,
> > >>
> > >> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:  
> > >>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:  
> > >>>> Add counter_push_event() to notify user space about new pulses
> > >>>>
> > >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > >>>> ---
> > >>>>   drivers/counter/interrupt-cnt.c | 2 ++
> > >>>>   1 file changed, 2 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > >>>> index 8514a87fcbee..b237137b552b 100644
> > >>>> --- a/drivers/counter/interrupt-cnt.c
> > >>>> +++ b/drivers/counter/interrupt-cnt.c
> > >>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > >>>>   
> > >>>>   	atomic_inc(&priv->count);
> > >>>>   
> > >>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > >>>> +
> > >>>>   	return IRQ_HANDLED;
> > >>>>   }
> > >>>>   
> > >>>> -- 
> > >>>> 2.30.2  
> > >>>
> > >>> Hi Oleksij,
> > >>>
> > >>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > >>> an interrupt is handled, which I suspect is not what you want to happen.
> > >>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > >>> so you'll need to check for a count value overflow before pushing the
> > >>> event.
> > >>>
> > >>> It would be good idea to implement a ceiling extension as well (you can
> > >>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > >>> particular point where the value overflows.  
> > >>
> > >> Thank you!
> > >>
> > >> What would be the best and resource effective strategy for periodically
> > >> getting frequency of interrupts/pulses? This is actual information which is
> > >> needed for my use case.
> > >>
> > >> So far, I was pushing every event to the user space, which is working
> > >> but probably not the most resource effective method of doing it.
> > >>
> > >> Regards,
> > >> Oleskij
> > >> -- 
> > >> Pengutronix e.K.                           |                             |
> > >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |  
> > > 
> > > We could introduce a new Counter change-of-state event type which would
> > > trigger whenever the count value changes, but I agree with you that this
> > > is likely not the best way for us to derive the frequency of the
> > > interrupts due to the indirection of handling and parsing the event
> > > data.
> > > 
> > > Instead, perhaps introducing a "frequency" or "period" Count extension
> > > would make more sense here. This extension could report the value delta
> > > between counts, or alternatively the time delta from which you can
> > > derive frequency. Regarding implementation, you can store the previous
> > > value in a variable, updating it whenever an interrupt occurs, and
> > > compute the particular delta every time a read is requested by the user.
> > > 
> > > David Lechner is implementing something similar for the TI eQEP driver
> > > to expose speed, so I'm CCing them here in case this is of interest to
> > > them.
> > >   
> > 
> > Based on my experience, I would recommend that counter drivers be as
> > "thin" as possible. They shouldn't try to provide any information that
> > the hardware itself doesn't provide. In other words, the kernel should
> > provide userspace the information needed to calculate the speed/rate
> > but not try to do the actual calculation in the kernel. Inevitably
> > there are nuances for specific use cases that can't all possibly be
> > handled by such an implementation.
> 
> I completely agree with this. While interrupts aren't really meant for
> measuring frequency, and this being somewhat of a mis-use of something, it is
> still possible to do and very useful in many cases. That said, while the
> counter framework is AFAIK the best fit for this, the main use-case for this
> driver is measuring wheel speed (and similar "speeds"). For this, the minimum
> amount of information the driver needs to provide user-space with to do
> reliable calculations, is high-resolution time-stamps of GPIO events. A simple
> counter is not suited, because there can be glitches that need to be detected.
> If user-space gets a buffer full of consecutive time-stamps (don't need to be
> all of them, just a sample of n consecutive timestamps), as well as total
> count, all needed calculations, glitch filtering, low-pass filtering, etc...
> can be done in user-space just fine.
> 
> > I've tried using gpio interrupts to try to calculate speed/rate in
> > the kernel before and it simply doesn't work reliably. Interrupts
> > get missed and the calculation will be off.
> 
> Exactly. Been there, done that.
> For reliable speed calculations of a mechanical system, the properties of the
> mechanical system need to be known, like physical limits of accelerations,
> maximum (or minimum) speed, etc. The minimum set of input data needed by a
> user-space application to do these calculations is total pulse count in
> addition to a buffer of timestamps of n consecutive input events (raising or
> falling edges on GPIO). So IMHO this is what the driver should provide, and
> in the most resource-efficient way possible. This particular driver will be
> used 3 times on the same SoC, with each up to 10-15k pulses per second. That
> is a lot of interrupts for an embedded system, so they better consume as
> little resources as possible. Filling a ring buffer with timestamps should be
> possible, as long as no locking is involved. Locks in IRQ context must be
> avoided at all costs, specially in this case.
> 
> > For really slow counts (i.e. 1 count/second), I can see a use for
> > generating an event on each count though. For high rates, I would
> > just read the count every 100ms in usespace and divide the change in
> > the number of counts by the time period to get the rate.
> 
> For slow counts, I agree, but for high rates, I don't (see above). There can
> be glitches and false events that can (and must) be effectively filtered out.
> For that user-space needs to know the time of each event during the
> measurement period.

No sure I understood the problem here. If you keep the driver as is and
in userspace just read out the counter value twice and measure the time
between the reads[1], you can calculate the average frequency of the
event in userspace.

Isn't that good enough?

Best regards
Uwe

[1] maybe support this timing by providing a timestamp with the read
    value to reduce timing jitter.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]