Re: [PATCHv2 8/9] hackrf: add support for transmitter

[Antti Palosaari <crope@iki.fi>]

On 07/28/2015 10:06 AM, Hans Verkuil wrote:
> On 07/28/2015 02:50 AM, Antti Palosaari wrote:
>> On 07/27/2015 11:38 PM, Hans Verkuil wrote:
>>> On 07/27/2015 10:21 PM, Antti Palosaari wrote:
>>>> On 07/17/2015 05:43 PM, Hans Verkuil wrote:
>>>>> On 07/16/2015 09:04 AM, Antti Palosaari wrote:
>>>>>> HackRF SDR device has both receiver and transmitter. There is limitation
>>>>>> that receiver and transmitter cannot be used at the same time
>>>>>> (half-duplex operation). That patch implements transmitter support to
>>>>>> existing receiver only driver.
>>>>>>
>>>>>> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>>>>>> Signed-off-by: Antti Palosaari <crope@iki.fi>
>>>>>> ---
>>>>>>     drivers/media/usb/hackrf/hackrf.c | 787 +++++++++++++++++++++++++++-----------
>>>>>>     1 file changed, 572 insertions(+), 215 deletions(-)
>>>>>>
>>>>>
>>>>>
>>>>>> @@ -611,8 +751,15 @@ static int hackrf_queue_setup(struct vb2_queue *vq,
>>>>>>     		unsigned int *nplanes, unsigned int sizes[], void *alloc_ctxs[])
>>>>>>     {
>>>>>>     	struct hackrf_dev *dev = vb2_get_drv_priv(vq);
>>>>>> +	struct usb_interface *intf = dev->intf;
>>>>>> +	int ret;
>>>>>>
>>>>>> -	dev_dbg(dev->dev, "nbuffers=%d\n", *nbuffers);
>>>>>> +	dev_dbg(&intf->dev, "nbuffers=%d\n", *nbuffers);
>>>>>> +
>>>>>> +	if (test_and_set_bit(QUEUE_SETUP, &dev->flags)) {
>>>>>> +		ret = -EBUSY;
>>>>>> +		goto err;
>>>>>> +	}
>>>>>
>>>>> This doesn't work. The bit is only cleared when start_streaming fails or
>>>>> stop_streaming is called. But the application can also call REQBUFS again
>>>>> or just close the file handle, and then QUEUE_SETUP should also be cleared.
>>>>>
>>>>> But why is this here in the first place? It doesn't seem to do anything
>>>>> useful (except mess up the v4l2-compliance tests).
>>>>>
>>>>> I've removed it and it now seems to work OK.
>>>>
>>>> It is there to block simultaneous use of receiver and transmitter.
>>>> Device could operate only single mode at the time - receiving or
>>>> transmitting. Driver shares streaming buffers.
>>>>
>>>> Any idea how I can easily implement correct blocking?
>>>
>>> Since each video_device struct has its own vb2_queue I wouldn't put the check
>>> here. Instead, put the check in the start_streaming callback. And the check
>>> is easy enough: if you want to start capturing, then call
>>> vb2_is_streaming(&tx_vb2_queue). If you want to start output, then call
>>> vb2_is_streaming(&rx_vb2_queue). If the other 'side' is streaming, then
>>> return EBUSY.
>>>
>>> It's perfectly valid to allocate the buffers, but actually streaming is an
>>> exclusive operation.
>>
>> Currently there is two queues, but only single buffer. If I do check on
>> start_streaming() it is too late as buffers are queue during buf_queue()
>> which is called earlier (and now both sides are added to same
>> queued_bufs lists, which messes up).
>
> It's not that you have a single buffer, it is that you have a single buffer list.
> I'd say that you should make two buffer lists and use the appropriate one. They
> really are independent, it's just that VIDIOC_STREAMON can run only one queue
> at a time.

I have done too looong day about that dual buffer list implementation. 
There is some very strange behavior, which makes it almost impossible. 
If I try return receiver buffers with status VB2_BUF_STATE_QUEUED and 
EBUSY from start_streaming() it does not stop, but starts wildly calling 
buf_queue() in a endless loop eating all the CPU and so. For transmitter 
it seems to work as I expected.

You could repeat that same issue using vivid:
# modprobe vivid
# v4l2-ctl -d /dev/video0 -C inject_vidioc_streamon_error
# cat /dev/video0 > /dev/null

and it does not stop with ctrl-C

regards
Antti

-- 
http://palosaari.fi/