Hello William, On Mon, Dec 13, 2021 at 04:40:55PM +0900, William Breathitt Gray wrote: > On Tue, Dec 07, 2021 at 07:10:45PM +0100, Uwe Kleine-König wrote: > > Some hardware units capture a timestamp for the counted event. To > > increase precision add a variant of counter_push_event() that allows > > passing this timestamp. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > > --- > > Hello, > > > > the difficulty is that the captured timer value is in a unit different > > from the one provided by ktime_get_ns(). So maybe some helper functions > > will be needed to convert the local timer value to a ktime timestamp? > > > > So usage would be something like: > > > > ktime_now = ktime_get_ns(); > > local_now = readl(CNT); > > local_event = readl(...); > > > > ktime_event = ktime_now - (local_now - local_event) * somefactor >> someshift; > > > > counter_push_event_ts(count, event, channel, ktime_event); > > > > This improves the precision because irq latency doesn't influence > > the resulting timestamp. The precision then only depends on the timer > > resolution and the delay between ktime_get_ns() and readl(CNT); > > > > I don't have a driver (yet) that makes use of this, the hardware where > > this will matter will be stm32mp1. > > Precision logging of counter events was one of the main motivations for > the creation of the Counter character device interface, so if we can > reduce the jitter of the timestamp by utilizing hardware-provided > values, I'm all for it. That being said, we should take care in deciding > how to expose this data in the Counter interfaces because not all > devices support such functionality and yet users should expect a > standard data format to code against. > > Considering this, I think it better to keep the struct counter_event > timestamp the way it is right now as ktime_get_ns() so that users have a > consistent way to retrieve the timestamp of when the Counter event was > pushed to the queue. Isn't it totally irrelevant to userspace when the event was pushed to the queue? I claim userspace is only ever interested when the event happend and the queue is only an implementation detail of the counter framework. The documentation for the timestamp member of struct counter_event is best estimate of time of event occurrence, in nanoseconds . That looks sane, and following that, the best estimate is the hw timestamp?! (Well, if you experience high latencies, the timestamp of the queuing might be interesting for debugging, but in this case I'd prefer tracing support over exposing implementation details in the API.) > In order to support the hardware-provided > timestamp, how about exposing local_event and local_now as Counter > extensions? You can set a watches for the local timestamps to log them > into the queue with each event, then perform the ktime_event calculation > in userspace using the struct counter_event timestamp value. An upside of your suggestion is that calculating the ktime for the event isn't done unless userspace needs it. Still I'd prefer to use .timestamp for an as-accurate-as-possible information for the event. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
