On Mon, Dec 13, 2021 at 10:04:48AM +0100, Uwe Kleine-König wrote: > 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.) All right your argument makes sense, the primary purpose of this timestamp is to mark the time the event actually occurred so queue latency debugging would certainly be secondary to that end. In that case, providing a more accurate timestamp via a counter_push_event_ts() call would be preferable if the hardware is capable of doing so. > > 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/ | We can additionally expose the local_event and local_now values as Counter extensions if userspace wants to look into that data more directly. If the objective is to reduce latency by moving the timestamp calculation to userspace, we might need to introduce some way to adjust the "timestamp mode" or similar to disable the automatic calculation when the user wants to do calculate it themselves in userspace instead. However, I suspect the effort is not worth it if you're just saving one or two read operations on the stm32mp1; you might as well just calculate the ktime automatically for each event and not worry about exposing the local hardware data to userspace for now until the need arises. William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature
