RE: [PATCH v4 1/6] iio: Add output buffer support

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Monday, August 30, 2021 5:43 PM
> To: Nuno Sá <noname.nuno@xxxxxxxxx>
> Cc: Chindris, Mihail <Mihail.Chindris@xxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; lars@xxxxxxxxxx;
> Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>; Sa, Nuno
> <Nuno.Sa@xxxxxxxxxx>; Bogdan, Dragos
> <Dragos.Bogdan@xxxxxxxxxx>; alexandru.ardelean@xxxxxxxxxx
> Subject: Re: [PATCH v4 1/6] iio: Add output buffer support
> 
> On Mon, 23 Aug 2021 15:50:14 +0200
> Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> 
> > On Fri, 2021-08-20 at 16:59 +0000, Mihail Chindris wrote:
> > > From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > >
> > > Currently IIO only supports buffer mode for capture devices like
> > > ADCs. Add
> > > support for buffered mode for output devices like DACs.
> > >
> > > The output buffer implementation is analogous to the input buffer
> > > implementation. Instead of using read() to get data from the
> buffer
> > > write()
> > > is used to copy data into the buffer.
> > >
> > > poll() with POLLOUT will wakeup if there is space available for more
> > > or
> > > equal to the configured watermark of samples.
> > >
> > > Drivers can remove data from a buffer using
> > > iio_buffer_remove_sample(), the
> > > function can e.g. called from a trigger handler to write the data to
> > > hardware.
> > >
> > > A buffer can only be either a output buffer or an input, but not
> > > both. So,
> > > for a device that has an ADC and DAC path, this will mean 2 IIO
> > > buffers
> > > (one for each direction).
> > >
> > > The direction of the buffer is decided by the new direction field of
> > > the
> > > iio_buffer struct and should be set after allocating and before
> > > registering
> > > it.
> > >
> > > Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > > Signed-off-by: Alexandru Ardelean
> <alexandru.ardelean@xxxxxxxxxx>
> > > Signed-off-by: Mihail Chindris <mihail.chindris@xxxxxxxxxx>
> > > ---
> > >  drivers/iio/iio_core.h            |   4 +
> > >  drivers/iio/industrialio-buffer.c | 133
> > > +++++++++++++++++++++++++++++-
> > >  drivers/iio/industrialio-core.c   |   1 +
> > >  include/linux/iio/buffer.h        |   7 ++
> > >  include/linux/iio/buffer_impl.h   |  11 +++
> > >  5 files changed, 154 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > index 8f4a9b264962..61e318431de9 100644
> > > --- a/drivers/iio/iio_core.h
> > > +++ b/drivers/iio/iio_core.h
> > > @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file
> > > *filp,
> > >  				 struct poll_table_struct *wait);
> > >  ssize_t iio_buffer_read_wrapper(struct file *filp, char __user
> *buf,
> > >  				size_t n, loff_t *f_ps);
> > > +ssize_t iio_buffer_write_wrapper(struct file *filp, const char
> > > __user *buf,
> > > +				 size_t n, loff_t *f_ps);
> > >
> > >  int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > >  void iio_buffers_free_sysfs_and_mask(struct iio_dev
> *indio_dev);
> > >
> > >  #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> > >  #define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> > > +#define iio_buffer_write_outer_addr
> (&iio_buffer_write_wrapper)
> > >
> > >  void iio_disable_all_buffers(struct iio_dev *indio_dev);
> > >  void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> > > @@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev
> > > *indio_dev);
> > >
> > >  #define iio_buffer_poll_addr NULL
> > >  #define iio_buffer_read_outer_addr NULL
> > > +#define iio_buffer_write_outer_addr NULL
> > >
> > >  static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > >  {
> > > diff --git a/drivers/iio/industrialio-buffer.c
> > > b/drivers/iio/industrialio-buffer.c
> > > index a95cc2da56be..73d4451a0572 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file
> > > *filp, char __user *buf,
> > >  	return ret;
> > >  }
> > >
> > > +static size_t iio_buffer_space_available(struct iio_buffer *buf)
> > > +{
> > > +	if (buf->access->space_available)
> > > +		return buf->access->space_available(buf);
> > > +
> > > +	return SIZE_MAX;
> > > +}
> > > +
> > > +static ssize_t iio_buffer_write(struct file *filp, const char __user
> > > *buf,
> > > +				size_t n, loff_t *f_ps)
> > > +{
> > > +	struct iio_dev_buffer_pair *ib = filp->private_data;
> > > +	struct iio_buffer *rb = ib->buffer;
> > > +	struct iio_dev *indio_dev = ib->indio_dev;
> > > +	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > > +	size_t datum_size;
> > > +	size_t to_wait;
> > > +	int ret;
> > > +
> >
> > Even though I do not agree that this is suficient, we should have the
> > same check as we have for input buffer:
> >
> >
> > if (!indio_dev->info)
> > 	return -ENODEV;
> >
> > > +	if (!rb || !rb->access->write)
> > > +		return -EINVAL;
> > > +
> > > +	datum_size = rb->bytes_per_datum;
> > > +
> > > +	/*
> > > +	 * If datum_size is 0 there will never be anything to read from
> > > the
> > > +	 * buffer, so signal end of file now.
> > > +	 */
> > > +	if (!datum_size)
> > > +		return 0;
> > > +
> > > +	if (filp->f_flags & O_NONBLOCK)
> > > +		to_wait = 0;
> > > +	else
> > > +		to_wait = min_t(size_t, n / datum_size, rb-
> >watermark);
> > > +
> >
> > I had a bit of a crazy thought... Typically, output devices do not
> > really have a typical trigger as we are used to have in input devices.
> > Hence, typically we just use a hrtimer (maybe a pwm-trigger can also
> be
> > added) trigger where we kind of poll the buffer for new data to send
> to
> > the device.
> 
> htrimer-trigger effectively gives you the hrtimer option but lets you
> swap
> it for other types. Maybe updating your DAC in sync with an ADC
> dataready to
> change some input to a network you are measuring?
> 
> An external pwm type trigger (effectively a hwtrigger) would indeed
> be
> useful for much the same reason those get used for ADCs on SoCs.  For
> ADCs
> that are doing self clocking you can think of that as an internal pwm
> trigger
> that we can't see at all.
> 
> > So, I was wondering if we could just optionally bypass the
> > kbuf path in output devices (via some optional buffer option)? At this
> > point, we pretty much already know that we have data to consume
> :).
> > This would be kind of a synchronous interface. One issue I see with
> > this is that we cannot really have a deterministic (or close) sampling
> > frequency as we have for example with a pwm based trigger.
> 
> If you are running without a buffer, the overhead of sysfs is unlikely to
> be a particularly big problem, so do it that way if you want synchronous
> control.  The purpose of the buffer is to smooth out those interactions.

What I meant was kind of just bypassing the buffer implementation
(in typical cases, kfifo) not the complete buffer infrastructure (as passing
a buffer with all scan bytes is better that writing each element 
separately in sysfs)...  So, we
would still have the write file ops but we would have a mechanism to
synchronously pass control the driver interested in this data. My point
was that at this level, we already now we have data for 'someone' to
consume. Taking a second look I guess we could even keep the FIFO
(even though, at first glance, it would not make much sense).  Or maybe,
we could have a special trigger as you said where we would just do
' iio_trigger_poll()' from the write() fops... 

> I guess it 'might' make sense to have a special trigger that is a similar
> to the poll trigger in that it will push data to the device as quickly
> as possible as long as there is some there...  That would look like a
> synchronous interface.
> 

Anyways, as I said, this was just me throwing some stuff into the air. Mos likely,
this does not bring that much added value when compared with the possible
issues and subtleties I'm not seeing now.. Probably not doable and definitely
not a priority for now.

- Nuno Sá