timestamp API

[catkira]

A possible way to support timestamps would be to add the following functions to libiio

struct meta_command
{
    uint64 timestamp,
    uint64 num_samples,
    enum type
    {
        send_timed,
        send_untimed  // timestamp is ignored
        receive_timed,
        receive_untimed
    },
    uint64 command_counter
};

set_timestamp_mode(bool mode)
{
    // write mode to mode iio attribute of timestamper core
   // if mode is false, everything should behave like now
}

write_timed_command(meta_command cmd)
{
    // write cmd to command iio channel of timestamper core
}

For getting the timestamp from HDL to user there could be a timestamp added to each block. The disadvantage of this would be, that the blocksize would not be num_samples * sample_size anymore. Also inside the axi_dmac it would be a bit more complicated, because the internal fifo size is calculated like burst_size * num_bursts.
I think the cleanest way would be to add a receive channel to the timestamper module, that gives out a timestamp for every meta command. The difficulty here would be to match the received timestamp to the meta command (probably easy for sync reads and more difficult for async/non-blocking reads). One could add a command_id to the meta_command struct to make this easier. Or add a get_timestamp() function to libiio that matches timestamps to received data blocks. But one would have to make sure, that this matching also works correctly in case of overflows in the HDL. So adding a command_counter to meta_command would probably be the easiest. The command_counter value could then also just be a field of the received_timestamp struct

struct received_timestamp
{
    uint64 timestamp,
    command_counter
}

get_received_timestamp(received_timestamp &timestamp)
{
    // read an item from the rx timestamp channel of the timestamper core
}

[pcercuei]

I disagree with your suggestion, the timestamp value should not be written by the DMA to the samples buffer. Instead, it should be written by the IP to a separate hardware register, that the kernel can read in the IRQ handler, and provide to the userspace through a device or buffer attribute. That would make things way simpler: the application would just need to dequeue a block of samples, then optionally read the attribute.

Same for TX - the userspace would write a device/buffer "timestamp" attribute, which would cause the kernel to write to a register of the IP, that would control how the next buffer is transferred.

Note that I don't really think we need a "timestamp", but a "samples counter". Since we know the sample rate we can retrieve the timestamps anyway, and having a "samples counter" would make it much easier to detect DMA underruns / overruns.

[pcercuei]

There's no need for buffering in the FPGA. The DMA should process the requests sequentially like it already does, and wait until the transfer time is reached if needed. So it does not need to know about any following request before the previous ones are finished.

The number of bytes does not need to be specified, just transfer the whole block of samples. If you want to transfer a non-standard number of samples, then just create a block of that size.

Adding a separate iio channel is a bad idea, there would be no way of matching the data from that channel to the samples of the main buffer. You only need to instruct the DMA to wait for a specified time before transferring a whole buffer, so a hardware register (appearing as a device/buffer attribute in userspace) looks appropriate to me.

Baseband frequency changes don't change the sample rate of your ADC. The sample rate does not change on-the-fly like that. Making the timestamp core work with samples count would work much better and be way more precise.

[pcercuei]

The DMA cores already have hardware queues, and switching between one configured transfer to another is instant and does not cause samples to be missed.

[pcercuei]

I don't really know what the FPGA program does, I just know that we already have a hardware queue for the DMA, and no samples are lost when the DMA switches from streaming one block of samples to another. So we already manage "consecutive burst writes" if you will.

You can manage things with just a single register for meta-data, if this register is read by the IP at the moment it starts streaming from a new block of samples. Then every time the DMA is done streaming from a block of samples, the driver can update the value of the register to the next transfer's setting. And since the timestamper is basically about pausing / unpausing the DMA until a special point in time is reached, the magic register could also be a new field in the DMA hardware descriptor.

As for using time vs. samples count... The example of a random software changing the sample rate isn't really valid IMHO. This software does not use timestamped buffers, and if it did, it would either not change the sample rate (keep the fast rate at all times), or handle it properly (not programming timestamped buffers when a sample rate change is under way). Using a samples counter just makes more sense, as it permits to be extremely precise in the time domain, and detect overruns/underruns.

[pcercuei]

If the samples counter simply represents the number of samples by which the next transfer should be delayed, then "now" is just "counter=0".

Programming transfers at a specific point in time, which you call "timestamping", is one mechanism. The other mechanism is tagging the RX buffers with a timestamp, to be able to know when exactly those RX buffers were received. Again, it would make sense to use a samples count value, because then you can detect gaps between buffers. You can compute the age of the RX buffer by comparing with the "live" samples counter, which would be a register updated by the IP every time a sample is received.

Whatever we choose to do, there will be driver changes needed, so whether or not changes are intrusive isn't really a problem.

You are thinking about having a "timestamper control logic" after the AXI DMA core. What I'm telling you is that the AXI DMA core could gain the "programming transfers" functionality, and the rest of the HDL wouldn't change.

[catkira]

Changes in the hdl code:
FLAG register will get another flag, i.e. BURST.
Another register will be added, i.e. "start_sample" at address 0x0454. It would be nicer if it was at address 0x410 or so, because thats were all the transfer config registers are, but then all the following register addresses would need to change as well. What do You prefer?

and in buffer.c

struct iio_buffer * iio_device_create_buffer(const struct iio_device *dev,
		size_t samples_count, bool cyclic)

could be changed to

struct iio_buffer * iio_device_create_buffer(const struct iio_device *dev,
		size_t samples_count, int flag, unsigned int start_sample)

in iio-private.h iio_buffer gets a new field:

struct iio_buffer {
	const struct iio_device *dev;
	void *buffer, *userdata;
	size_t length, data_length;
	unsgined int start_sample;  // new field

	uint32_t *mask;
	unsigned int dev_sample_size;
	unsigned int sample_size;
	bool dev_is_high_speed;
};

in local.c block get a new fild

struct block {
	uint32_t id,
		 size,
		 bytes_used,
		 type,
		 flags,
                 start_sample,                 // new field
		 offset;
	uint64_t timestamp;
};

and stuff needs to be modified in buffer_impl.h as well.

The callchain when writing a buffer is:

local.c        ioctl_nointr(f, BLOCK_ENQUEUE_IOCTL, last_block)
  -> industrialio-buffer.c        iio_buffer_ioctl()
    -> industrialio-buffer.c         iio_buffer_enqueue_block(buffer, (struct iio_buffer_block __user *)arg)
      -> industrialio-buffer-dma.c         iio_dma_buffer_enqueue_block(buffer, &block)
        -> industrialio-buffer-dma.c         iio_dma_buffer_enqueue(queue, block)
          -> industrialio-buffer-dma.c          iio_dma_buffer_submit_block(queue, block)
            -> industrialio-buffer-dma.c          queue->ops->submit(queue, block)
              -> cf_axi_dds_buffer_stream.c        dds_buffer_submit_block(queue, block)
                -> cf_axi_dds_buffer_stream.c        iio_dmaengine_buffer_submit_block(queue, block, DMA_TO_DEVICE)
                 -> industrialio-buffer-dmaengine.c          desc = dmaengine_prep_slave_single(...)
                   -> dma-axi-dmac.c       axi_dmac_prep_slave_sg(...)
                 -> industrialio-buffer-dmaengine.c          desc->callback_param = block
                 -> industrialio-buffer-dmaengine.c          dmaengine_submit(desc)

desc has type dma_async_tx_descriptor. I guess that cannot contain meta information, so the new meta information would have to go into block.

Edit: After checking again, i see that there is a field dma_descriptor_metadata_ops in dma_async_tx_descriptor, maybe that can be used.

buffer for tx will be prepared with dmaengine_prep_slave_single which will get forwarded to axi_dmac_prep_slave_sg() of the axi-dmac driver, see here.

After preparing the buffer, it will be submitted with dmaengine_submit(), which does

desc->tx_submit(desc)  // defined in include/linux/dmaengine.h

but I dont see yet how this reaches the axi_dmac driver... But I think the desc will somehow end up in axi_dmac_start_transfer(...).
I think the meta information could be added here:

struct axi_dmac_desc {
	struct virt_dma_desc vdesc;
	bool cyclic;   // probably need to make this an int or enum flag
        unsigned int start_sample;    // new attribute
	bool have_partial_xfer;

	unsigned int num_submitted;
	unsigned int num_completed;
	unsigned int num_sgs;
	struct axi_dmac_sg sg[];
};

[pcercuei]

I didn't know about metadata support in the dmaengine framework, that's great!

So the axi-dmac driver could be improved to support meta-data, as well as the DMA API core, to add support for sample counters.
The various chip drivers (or common code to all devices using axi-dmac) can then register themselves as meta-data client or provider, and have the sample counters exported to userspace.

Finally, libiio (in the local backend) would use this information when dequeueing buffers to know when samples have been lost, and set this attribute if the enqueued buffer has to be transmitted at a precise time.

[pcercuei]

I don't think the API of libiio will be a problem. You could have something like:

int iio_block_set_enqueue_delay(block, nb_samples);

uint64_t iio_block_get_dequeue_timestamp(block);

[pcercuei]

@catkira these are two different kernel interfaces, yes. The DMABUF one is the one I am trying to get upstream. It is not yet in ADI's kernel.

[pcercuei]

I'd think we could have that in the DMABUF API only.

And now that I think of it… Why do we need anything in the IP core to support programmed transfers?

If you want a transfer to happen in 2ms, then the application (or libiio) can compute the number of dummy samples needed to fit 2ms, and enqueue that number of dummy samples.

[mhennerich]

Well, in a lot of cases you want to synchronize your timestamping to an 1PPS or some other event.
Also you can't assume that your streaming can keep up with the baseband rate.
So in a TDD application you would prepare a slot, queue it and have some HW IP release it at a certain time.

[pcercuei]

@mhennerich but enqueueing an empty buffer is a "free" operation, since we can fill it once and then enqueue it endlessly. IIOD could have a command "enqueue X dummy samples", so that you wouldn't transfer any data.

Unless you mean that the DMA itself cannot keep up with the baseband rate.

@catkira the mmap interface is never going to get upstream, that's what the dmabuf interface is being designed for. Once it is upstream, I believe ADI's kernel will support both, but the mmap interface will be marked as deprecated.

[pcercuei]

@catkira we do have ADCs and DACs that can deliver GSPS, so if the HPC interface is limited to 300-400 MB/s, then we really do need to handle this in the IP core.

[catkira]

This is how iio_block looks currently

struct iio_block {
	struct iio_buffer *buffer;
	struct iio_block_pdata *pdata;
	size_t size;
	void *data;

	struct iio_task_token *token, *old_token;
	size_t bytes_used;
	bool cyclic;
};

I would make it

struct iio_block {
	struct iio_buffer *buffer;
	struct iio_block_pdata *pdata;
	size_t size;
	void *data;

	struct iio_task_token *token, *old_token;
	size_t bytes_used;
	bool cyclic;
        long long time;
        long flags;
};

The signature of iio_buffer_create_blockwould become

struct iio_block *
iio_buffer_create_block(struct iio_buffer *buf, size_t size, long long time, long flags){...}

Or alternative there could be functions like setBlockTime(struct iio_block *, long long time) , setBlockFlags(struct iio_block *, long flags) and corresponding get-functions.

For tx long long time would hold the requested start time in ticks (sample count). For rx it would hold the receive time.
long flags would hold control flags, ie for tx SEND_NOW, SEND_SCHEDULED for rx they could be RECEIVE_NOW, RECEIVE_SCHEDULED and for the response block TIME_PAST, ON_TIME, OVERFLOW.

There would also need to be a possibility to get the timer (counter) value without sending or receiving anything. Because applications need that for the first timed command. Soapy has an abstraction for that which look like this

long long getHardwareTime(const std::string &what = "") const;
void setHardwareTime(const long long timeNs, const std::string &what = "");

It could be done using the existing iio-api by just sending or receiving a block with bytes_used=0.

@pcercuei do You endorse this proposal? Do You prefer having set/get-functions or changing the signature of iio_buffer_create_block?