Message-ID: <20200502181929.2409dcde@archlinux>
Date:   Sat, 2 May 2020 18:19:29 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Sa, Nuno" <Nuno.Sa@...log.com>
Cc:     "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lars@...afoo.de" <lars@...afoo.de>
Subject: Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
 basis

On Mon, 27 Apr 2020 12:09:18 +0000
"Sa, Nuno" <Nuno.Sa@...log.com> wrote:

> > From: linux-iio-owner@...r.kernel.org <linux-iio-owner@...r.kernel.org>
> > On Behalf Of Jonathan Cameron
> > Sent: Sonntag, 26. April 2020 12:51
> > To: Sa, Nuno <Nuno.Sa@...log.com>
> > Cc: Ardelean, Alexandru <alexandru.Ardelean@...log.com>; linux-
> > iio@...r.kernel.org; linux-kernel@...r.kernel.org; lars@...afoo.de
> > Subject: Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> > basis
> > 
> > On Fri, 24 Apr 2020 07:51:05 +0000
> > "Sa, Nuno" <Nuno.Sa@...log.com> wrote:
> >   
> > > > From: linux-iio-owner@...r.kernel.org <linux-iio-  
> > owner@...r.kernel.org>  
> > > > On Behalf Of Alexandru Ardelean
> > > > Sent: Freitag, 24. April 2020 07:18
> > > > To: linux-iio@...r.kernel.org; linux-kernel@...r.kernel.org
> > > > Cc: jic23@...nel.org; lars@...afoo.de; Ardelean, Alexandru
> > > > <alexandru.Ardelean@...log.com>
> > > > Subject: [RFC PATCH 4/4] iio: Track enabled channels on a per channel  
> > basis  
> > > >
> > > > From: Lars-Peter Clausen <lars@...afoo.de>
> > > >
> > > > Now that we support multiple channels with the same scan index we can  
> > no  
> > > > longer use the scan mask to track which channels have been enabled.
> > > > Otherwise it is not possible to enable channels with the same scan index
> > > > independently.
> > > >
> > > > Introduce a new channel mask which is used instead of the scan mask to
> > > > track which channels are enabled. Whenever the channel mask is  
> > changed a  
> > > > new scan mask is computed based on it.
> > > >
> > > > Signed-off-by: Lars-Peter Clausen <lars@...afoo.de>
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> > > > ---
> > > >  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++----------
> > > >  drivers/iio/inkern.c              | 19 +++++++++-
> > > >  include/linux/iio/buffer_impl.h   |  3 ++
> > > >  include/linux/iio/consumer.h      |  2 +
> > > >  4 files changed, 64 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-  
> > buffer.c  
> > > > index c06691281287..1821a3e32fb3 100644
> > > > --- a/drivers/iio/industrialio-buffer.c
> > > > +++ b/drivers/iio/industrialio-buffer.c
> > > > @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct iio_buffer
> > > > *buffer,
> > > >  	if (buffer->scan_mask == NULL)
> > > >  		return -ENOMEM;
> > > >
> > > > +	buffer->channel_mask = bitmap_zalloc(indio_dev->num_channels,
> > > > +					     GFP_KERNEL);
> > > > +	if (buffer->channel_mask == NULL) {
> > > > +		bitmap_free(buffer->scan_mask);
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > > >
> > > >  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > > >  {
> > > > +	bitmap_free(buffer->channel_mask);
> > > >  	bitmap_free(buffer->scan_mask);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > > > @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device  
> > *dev,  
> > > >
> > > >  	/* Ensure ret is 0 or 1. */
> > > >  	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> > > > -		       indio_dev->buffer->scan_mask);
> > > > +		       indio_dev->buffer->channel_mask);
> > > >
> > > >  	return sprintf(buf, "%d\n", ret);
> > > >  }
> > > > @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct  
> > iio_dev  
> > > > *indio_dev,
> > > >   * buffers might request, hence this code only verifies that the
> > > >   * individual buffers request is plausible.
> > > >   */
> > > > -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> > > > -		      struct iio_buffer *buffer, int bit)
> > > > +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> > > > +				struct iio_buffer *buffer, int bit)
> > > >  {
> > > >  	const unsigned long *mask;
> > > >  	unsigned long *trialmask;
> > > > +	unsigned int ch;
> > > >
> > > >  	trialmask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
> > > >  	if (trialmask == NULL)
> > > > @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev
> > > > *indio_dev,
> > > >  		WARN(1, "Trying to set scanmask prior to registering
> > > > buffer\n");
> > > >  		goto err_invalid_mask;
> > > >  	}
> > > > -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev-  
> > > > >masklength);  
> > > > -	set_bit(bit, trialmask);
> > > > +
> > > > +	set_bit(bit, buffer->channel_mask);
> > > > +
> > > > +	for_each_set_bit(ch, buffer->channel_mask, indio_dev-  
> > > > >num_channels)  
> > > > +		set_bit(indio_dev->channels[ch].scan_index, trialmask);  
> > >
> > > So, here if the channels all have the same scan_index, we will end up with a  
> > scan_mask which is  
> > > different that channel_mask, right? I saw that in our internal driver's we  
> > then just access the  
> > > channel_mask field directly to know what pieces/channels do we need to  
> > enable prior to  
> > > buffering, which implies including buffer_impl.h.  
> > Given that we handle the demux only at the level of scan elements that
> > won't work in general
> > (even if it wasn't a horrible layering issue).  
> 
> Yes, and the driver just adds 16 channels and points all of them to scan_index 0. It then
> sets real_bits and the shift so that userspace can get the right channel bit. So, in the end
> we have just one buffer/scan element with 16bits. My problem here is more architectural...
> We should not directly include "buffer_impl.h" in drivers...
> 
> > >
> > > So, for me it would make sense to compute scan_mask so that it will be the  
> > same as channel_mask  
> > > (hmm but that would be a problem when computing the buffer size...) and  
> > drivers can correctly use  
> > > ` validate_scan_mask ()` cb. Alternatively, we need to expose  
> > channel_mask either on a new cb or  
> > > change the ` validate_scan_mask ()` footprint.  
> > 
> > Excellent points. We need to address support for:
> > 
> > 1) available_scan_mask - if we have complicated rules on mixtures of
> > channels inside
> >    a given buffer element.  
> 
> Maybe one solution to expose channel mask is to check if channel_mask != scan_mask
> before calling the ` validate_scan_mask()`. If it is, we pass channel_mask to the callback.
> Driver's should then know what to do with it...

That's liable to be flakey as there is no requirement for the scan_mask to
be ordered or indeed not have holes.

> 
> > 2) channel enabling though I'm sort of inclined to say that if you are using this
> > approach
> >    you only get information on channels that make up a scan mask element.
> > Tough luck you
> >    may end up enabling more than you'd like.  
> 
> Not sure if I'm fully understanding this point. I believe with this approach channel
> enablement works as before since the core is kind of mapping channel_mask to
> scan_mask. So if we have 16 channels using only 1 scan_element we can still
> enable/disable all 16 channels.

Its more subtle than that.  Because of the mux, a number of different channels can
be enabled by different consumers, but all that is exposed to the driver is
the resulting fused scan_mask across all consumers.  It has no idea what channels
have been enabled if they lie within a scan_mask element.

Hence, whilst there can be individual channel enable and disable attributes
they driver only seems enable and disable of scan mask elements. That means
it needs to turn on ALL of the channels within one scan mask element.
To do anything more complex requires us to carry all the following to the demux
calculator

1) scan_mask
2) channel_mask
3) mapping from channel mask to scan mask

It could be done, but it's potentially nasty.  Even then we don't want to
get into breaking out particular elements within a scan mask element so we'd
end up providing all enabled channels (within each scan mask element)
to the all consumers who are after any of them.

We'd also have to expose the fused channel mask as well as scan mask
to the driver which is not exactly elegant.

That's why I'd suggest initial work uses scan mask as the fundamental
unit of enable / disable, not the channel mask.

Nothing stops then improving that later to deal with the channel mask
fusion needed to work out the enables, but it's not something I'd do
for step 1.

> 
> In the end, if we have a traditional driver with one channel per scan_index, channel_mask
> should be equal to scan_mask. As we start to have more than one channel pointing to the
> same scan_index, these masks will be different.
> 
> > It might be possible to make switch to using a channel mask but given the
> > channel index is
> > implicit that is going to be at least a little bit nasty.
> > 
> > How much does it hurt to not have the ability to separately control channels
> > within
> > a given buffer element?   Userspace can enable / disable them but reality is
> > you'll  
> 
> As long as we are "ok" with the extra amount of allocated memory, I think it would work.
> Though drivers will have to replicate the same data trough all the enabled scan elements...

Hmm. I think we are talking about different things.  Let me give an example.

8 channels in scan mask element 0 size 8 bits, 8 channels in scan mask element 1

Enable a channel in scan mask element 0 on consumer 0, and a
different one on consumer 1.  If they were in different scan mask elements
we'd deliver the first element only to consumer 0 and the second element
only to consumer 1 (that's what the demux does for us)

Here, in what I would suggest for the initial implementation, channel mask
is not exposed at all to buffer setup op (update_scan_mask) - so we
don't know which channels in that scan mask element are needed.
Only answer, turn all 8 on.

In this case we would deliver one 8 bit buffer element to each of the consumers
but it would include the values for all 8 channels (but none from the 8 channels
in our second scan mask element).

This keeps the channel mask logic (for now) separate from the demux
and the buffered capture setup logic, but at the cost of sampling channels
no one cares about.  Note we often do that anyway as a lot of hardware does
not have per channel enables, or is more efficient if we grab all the channels
in a single transaction.

Jonathan

> 
> - Nuno Sá
> 
> > get data for all the channels in a buffer element if any of them are enabled.
> > 
> > Given the demux will copy the whole element anyway (don't want to waste
> > time doing
> > masking inside an element), userspace might get these extra channels
> > anyway if another
> > consumer has enabled them.
> > 
> > Jonathan
> > 
> >   
> > >
> > > - Nuno Sá  
> 

Powered by blists - more mailing lists