Message-ID: <c0a21067-f636-ce88-6edc-06176e973459@xs4all.nl>
Date:   Tue, 17 Jul 2018 14:56:09 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
Cc:     Michael Krufky <mkrufky@...uxtv.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Andy Walls <awalls@...metrocast.net>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-media@...r.kernel.org
Subject: Re: [PATCH v7 6/6] [media] cxusb: add analog mode support for Medion
 MD95700

On 10/07/18 23:48, Maciej S. Szmigiero wrote:
> Hi Hans,
> 
> On 04.07.2018 11:33, Hans Verkuil wrote:
>> Hi Maciej,
>>
>> On 02/07/18 23:23, Maciej S. Szmigiero wrote:
> (..)
>>> +static int cxusb_medion_v_queue_setup(struct vb2_queue *q,
>>> +				      unsigned int *num_buffers,
>>> +				      unsigned int *num_planes,
>>> +				      unsigned int sizes[],
>>> +				      struct device *alloc_devs[])
>>> +{
>>> +	struct dvb_usb_device *dvbdev = vb2_get_drv_priv(q);
>>> +	struct cxusb_medion_dev *cxdev = dvbdev->priv;
>>> +	unsigned int size = cxdev->raw_mode ?
>>> +		CXUSB_VIDEO_MAX_FRAME_SIZE :
>>> +		cxdev->width * cxdev->height * 2;
>>> +
>>> +	if (*num_planes > 0) {
>>> +		if (*num_planes != 1)
>>> +			return -EINVAL;
>>> +
>>> +		if (sizes[0] < size)
>>> +			return -EINVAL;
>>> +	} else {
>>> +		*num_planes = 1;
>>> +		sizes[0] = size;
>>> +	}
>>> +
>>> +	if (q->num_buffers + *num_buffers < 6)
>>> +		*num_buffers = 6 - q->num_buffers;
>>
>> Huh? These two lines should be removed. I'm not sure what the purpose is.
> 
> This is to request that the vb2 framework keeps at least 6 buffers in the
> queue.
> 
> There is a similar code in usb/usbtv/usbtv-video.c:
>> if (vq->num_buffers + *nbuffers < 2)
>>                 *nbuffers = 2 - vq->num_buffers;
> 
> And in usb/hackrf/hackrf.c and usb/airspy/airspy.c:
>> /* Need at least 8 buffers */
>> if (vq->num_buffers + *nbuffers < 8)
>> 	*nbuffers = 8 - vq->num_buffers;
> 
> In addition to this, many drivers (like stk1160, s2255, pwc, msi2500,
> go7007, ...) always enforce some minimum *num_buffers count, regardless
> of the number of buffers currently in the queue.

Yeah, that's old code. These days you set the min_buffers_needed field of
struct vb2_queue and vb2 will take care of this itself.

Please set that field instead.

> 
>>> +static void cxusb_auxbuf_init(struct cxusb_medion_auxbuf *auxbuf,
>>> +			      u8 *buf, unsigned int len)
>>> +{
>>> +	auxbuf->buf = buf;
>>> +	auxbuf->len = len;
>>> +	auxbuf->paylen = 0;
>>> +}
>>> +
>>> +static void cxusb_auxbuf_head_trim(struct dvb_usb_device *dvbdev,
>>> +				   struct cxusb_medion_auxbuf *auxbuf,
>>> +				   unsigned int pos)
>>> +{
>>> +	if (pos == 0)
>>> +		return;
>>> +
>>> +	if (WARN_ON(pos > auxbuf->paylen))
>>> +		return;
>>> +
>>> +	cxusb_vprintk(dvbdev, AUXB,
>>> +		      "trimming auxbuf len by %u to %u\n",
>>> +		      pos, auxbuf->paylen - pos);
>>> +
>>> +	memmove(auxbuf->buf, auxbuf->buf + pos, auxbuf->paylen - pos);
>>> +	auxbuf->paylen -= pos;
>>> +}
>>> +
>>> +static unsigned int cxusb_auxbuf_paylen(struct cxusb_medion_auxbuf *auxbuf)
>>> +{
>>> +	return auxbuf->paylen;
>>> +}
>>> +
>>> +static bool cxusb_auxbuf_make_space(struct dvb_usb_device *dvbdev,
>>> +				    struct cxusb_medion_auxbuf *auxbuf,
>>> +				    unsigned int howmuch)
>>> +{
>>> +	unsigned int freespace;
>>> +
>>> +	if (WARN_ON(howmuch >= auxbuf->len))
>>> +		howmuch = auxbuf->len - 1;
>>> +
>>> +	freespace = auxbuf->len - cxusb_auxbuf_paylen(auxbuf);
>>> +
>>> +	cxusb_vprintk(dvbdev, AUXB, "freespace is %u\n", freespace);
>>> +
>>> +	if (freespace >= howmuch)
>>> +		return true;
>>> +
>>> +	howmuch -= freespace;
>>> +
>>> +	cxusb_vprintk(dvbdev, AUXB, "will overwrite %u bytes of buffer\n",
>>> +		      howmuch);
>>> +
>>> +	cxusb_auxbuf_head_trim(dvbdev, auxbuf, howmuch);
>>> +
>>> +	return false;
>>> +}
>>> +
>>> +/* returns false if some data was overwritten */
>>> +static bool cxusb_auxbuf_append_urb(struct dvb_usb_device *dvbdev,
>>> +				    struct cxusb_medion_auxbuf *auxbuf,
>>> +				    struct urb *urb)
>>> +{
>>> +	unsigned long len = 0;
>>> +	int i;
>>> +	bool ret;
>>> +
>>> +	for (i = 0; i < urb->number_of_packets; i++)
>>> +		len += urb->iso_frame_desc[i].actual_length;
>>> +
>>> +	ret = cxusb_auxbuf_make_space(dvbdev, auxbuf, len);
>>> +
>>> +	for (i = 0; i < urb->number_of_packets; i++) {
>>> +		unsigned int to_copy;
>>> +
>>> +		to_copy = urb->iso_frame_desc[i].actual_length;
>>> +
>>> +		memcpy(auxbuf->buf + auxbuf->paylen, urb->transfer_buffer +
>>> +		       urb->iso_frame_desc[i].offset, to_copy);
>>> +
>>> +		auxbuf->paylen += to_copy;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static bool cxusb_auxbuf_copy(struct cxusb_medion_auxbuf *auxbuf,
>>> +			      unsigned int pos, unsigned char *dest,
>>> +			      unsigned int len)
>>> +{
>>> +	if (pos + len > auxbuf->paylen)
>>> +		return false;
>>> +
>>> +	memcpy(dest, auxbuf->buf + pos, len);
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +static unsigned int cxusb_auxbuf_advance(struct cxusb_medion_auxbuf *auxbuf,
>>> +					 unsigned int pos,
>>> +					 unsigned int increment)
>>> +{
>>> +	return pos + increment;
>>> +}
>>> +
>>> +static unsigned int cxusb_auxbuf_begin(struct cxusb_medion_auxbuf *auxbuf)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static bool cxusb_auxbuf_isend(struct cxusb_medion_auxbuf *auxbuf,
>>> +			       unsigned int pos)
>>> +{
>>> +	return pos >= auxbuf->paylen;
>>> +}
>>
>> These three functions seem pointless to me.
> 
> I will remove cxusb_auxbuf_begin().
> 
> cxusb_auxbuf_advance() is called 6 times in the code while
> cxusb_auxbuf_isend() keeps the buffer implementation opaque for users so
> I think they make the code a bit nicer.

Why would you want to keep these one-liners opaque? It doesn't help me
understand the code. Please don't do this.

> 
>>> +static void cxusb_medion_v_process_urb_raw_mode(struct cxusb_medion_dev *cxdev,
>>> +						struct urb *urb)
>>> +{
>>> +	struct dvb_usb_device *dvbdev = cxdev->dvbdev;
>>> +	u8 *buf;
>>> +	struct cxusb_medion_vbuffer *vbuf;
>>> +	int i;
>>> +	unsigned long len = 0;
>>> +
>>> +	if (list_empty(&cxdev->buflist)) {
>>> +		dev_warn(&dvbdev->udev->dev, "no free buffers\n");
>>> +		return;
>>> +	}
>>> +
>>> +	vbuf = list_first_entry(&cxdev->buflist, struct cxusb_medion_vbuffer,
>>> +				list);
>>> +	list_del(&vbuf->list);
>>> +
>>> +	vbuf->vb2.timestamp = ktime_get_ns();
>>> +
>>> +	buf = vb2_plane_vaddr(&vbuf->vb2, 0);
>>> +
>>> +	for (i = 0; i < urb->number_of_packets; i++) {
>>> +		memcpy(buf, urb->transfer_buffer +
>>> +		       urb->iso_frame_desc[i].offset,
>>> +		       urb->iso_frame_desc[i].actual_length);
>>> +
>>> +		buf += urb->iso_frame_desc[i].actual_length;
>>> +		len += urb->iso_frame_desc[i].actual_length;
>>> +	}
>>> +
>>> +	vb2_set_plane_payload(&vbuf->vb2, 0, len);
>>> +
>>> +	vb2_buffer_done(&vbuf->vb2, VB2_BUF_STATE_DONE);
>>
>> The frame sequence counter in vb2_v4l2_buffer does not seem to be incremented.
>>
>> Did you test this driver with v4l2-compliance? It will detect such errors.
> 
> Hmm, I did test this driver with v4l2-compliance last year and don't
> remember seeing any error about the frame sequence counter.
> 
> When I'll be doing a respin I will check this again and see whether
> v4l2-compliance reports anything (it seems it should).
> 
>>> +static int cxusb_medion_v_start_streaming(struct vb2_queue *q,
>>> +					  unsigned int count)
>>> +{
>>> +	struct dvb_usb_device *dvbdev = vb2_get_drv_priv(q);
>>> +	struct cxusb_medion_dev *cxdev = dvbdev->priv;
>>> +	u8 streamon_params[2] = { 0x03, 0x00 };
>>> +	int npackets, i;
>>> +	int ret;
>>> +
>>> +	cxusb_vprintk(dvbdev, OPS, "should start streaming\n");
>>> +
>>> +	/* already streaming */
>>> +	if (cxdev->streaming)
>>> +		return 0;
>>
>> Unnecessary check, this can't happen.
>>
>>> +
>>> +	if (cxusb_medion_stream_busy(cxdev)) {
>>> +		ret = -EBUSY;
>>> +		goto ret_retbufs;
>>> +	}
>>> +
>>> +	ret = v4l2_subdev_call(cxdev->cx25840, video, s_stream, 1);
>>> +	if (ret != 0) {
>>> +		dev_err(&dvbdev->udev->dev,
>>> +			"unable to start stream (%d)\n", ret);
>>> +		goto ret_retbufs;
>>> +	}
>>> +
>>> +	ret = cxusb_ctrl_msg(dvbdev, CMD_STREAMING_ON, streamon_params, 2,
>>> +			     NULL, 0);
>>> +	if (ret != 0) {
>>> +		dev_err(&dvbdev->udev->dev,
>>> +			"unable to start streaming (%d)\n", ret);
>>> +		goto ret_unstream_cx;
>>> +	}
>>> +
>>> +	if (cxdev->raw_mode)
>>> +		npackets = CXUSB_VIDEO_MAX_FRAME_PKTS;
>>> +	else {
>>> +		u8 *buf;
>>> +		unsigned int urblen, auxbuflen;
>>> +
>>> +		/* has to be less than full frame size */
>>> +		urblen = (cxdev->width * 2 + 4 + 4) * cxdev->height;
>>> +		npackets = urblen / CXUSB_VIDEO_PKT_SIZE;
>>> +		urblen = npackets * CXUSB_VIDEO_PKT_SIZE;
>>> +
>>> +		auxbuflen = (cxdev->width * 2 + 4 + 4) *
>>> +			(cxdev->height + 50 /* VBI lines */) + urblen;
>>> +
>>> +		buf = vmalloc(auxbuflen);
>>> +		if (buf == NULL) {
>>> +			ret = -ENOMEM;
>>> +			goto ret_unstream_md;
>>> +		}
>>> +
>>> +		cxusb_auxbuf_init(&cxdev->auxbuf, buf, auxbuflen);
>>> +	}
>>> +
>>> +	for (i = 0; i < CXUSB_VIDEO_URBS; i++) {
>>> +		int framen;
>>> +		u8 *streambuf;
>>> +		struct urb *surb;
>>> +
>>> +		streambuf = kmalloc(npackets * CXUSB_VIDEO_PKT_SIZE,
>>> +				    GFP_KERNEL);
>>> +		if (streambuf == NULL) {
>>> +			if (i == 0) {
>>> +				ret = -ENOMEM;
>>> +				goto ret_freeab;
>>> +			} else
>>> +				break;
>>> +		}
>>> +
>>> +		surb = usb_alloc_urb(npackets, GFP_KERNEL);
>>> +		if (surb == NULL) {
>>> +			kfree(streambuf);
>>> +			ret = -ENOMEM;
>>> +			goto ret_freeu;
>>> +		}
>>> +
>>> +		cxdev->streamurbs[i] = surb;
>>> +		surb->dev = dvbdev->udev;
>>> +		surb->context = dvbdev;
>>> +		surb->pipe = usb_rcvisocpipe(dvbdev->udev, 2);
>>> +
>>> +		surb->interval = 1;
>>> +		surb->transfer_flags = URB_ISO_ASAP;
>>> +
>>> +		surb->transfer_buffer = streambuf;
>>> +
>>> +		surb->complete = cxusb_medion_v_complete;
>>> +		surb->number_of_packets = npackets;
>>> +		surb->transfer_buffer_length = npackets * CXUSB_VIDEO_PKT_SIZE;
>>> +
>>> +		for (framen = 0; framen < npackets; framen++) {
>>> +			surb->iso_frame_desc[framen].offset =
>>> +				CXUSB_VIDEO_PKT_SIZE * framen;
>>> +
>>> +			surb->iso_frame_desc[framen].length =
>>> +				CXUSB_VIDEO_PKT_SIZE;
>>> +		}
>>> +	}
>>> +
>>> +	cxdev->urbcomplete = 0;
>>> +	cxdev->nexturb = 0;
>>> +	cxdev->vbuf = NULL;
>>> +	cxdev->bt656.mode = NEW_FRAME;
>>> +	cxdev->bt656.buf = NULL;
>>> +
>>> +	for (i = 0; i < CXUSB_VIDEO_URBS; i++)
>>> +		if (cxdev->streamurbs[i] != NULL) {
>>> +			ret = usb_submit_urb(cxdev->streamurbs[i],
>>> +					GFP_KERNEL);
>>> +			if (ret != 0)
>>> +				dev_err(&dvbdev->udev->dev,
>>> +					"URB %d submission failed (%d)\n", i,
>>> +					ret);
>>> +		}
>>> +
>>> +	cxdev->streaming = true;
>>
>> No need to keep track of the streaming state. You can use vb2_start_streaming_called()
>> instead since vb2 already keeps track of this.
> 
> The driver still needs some flag to tell its workqueue task to not
> process and resubmit URBs when the stream is being stopped.
> 
> Otherwise, as the device lock gets released to flush the workqueue task
> there will be a time window where this task could still resubmit URBs
> (not knowing that we are stopping the stream).
> And we need to have the URBs killed before we flush the workqueue since
> their completion routine can schedule the workqueue task.
> 
> There seems to be no vb2 specific flag for this:
> q->start_streaming_called is zeroed only after stop_streaming callback
> has already returned.
> And we need to have URBs killed and the workqueue task flushed before
> we return from stop_streaming callback so we can clean up their
> resources in this callback.

Ah, it's used in cxusb_medion_v_complete_work. I think it would be better
to set a 'stop_streaming' flag that cxusb_medion_v_complete_work() looks
at, rather than this 'streaming' flag.

There should not be any URBs in flight before start_streaming is called, and
there should not be any URBs in flight when stop_streaming exists.

cxusb_medion_stream_busy() is very dubious as well. It should never return
true when you call it in start_streaming, and in the other cases it is called
it adds nothing to vb2_is_busy().

> 
>>> +
>>> +	return 0;
>>> +
>>> +ret_freeu:
>>> +	for (i = 0; i < CXUSB_VIDEO_URBS; i++)
>>> +		if (cxdev->streamurbs[i] != NULL) {
>>> +			kfree(cxdev->streamurbs[i]->transfer_buffer);
>>> +			usb_free_urb(cxdev->streamurbs[i]);
>>> +			cxdev->streamurbs[i] = NULL;
>>> +		}
>>> +
>>> +ret_freeab:
>>> +	if (!cxdev->raw_mode)
>>> +		vfree(cxdev->auxbuf.buf);
>>> +
>>> +ret_unstream_md:
>>> +	cxusb_ctrl_msg(dvbdev, CMD_STREAMING_OFF, NULL, 0, NULL, 0);
>>> +
>>> +ret_unstream_cx:
>>> +	v4l2_subdev_call(cxdev->cx25840, video, s_stream, 0);
>>> +
>>> +ret_retbufs:
>>> +	cxusb_medion_return_buffers(cxdev, true);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void cxusb_medion_v_stop_streaming(struct vb2_queue *q)
>>> +{
>>> +	struct dvb_usb_device *dvbdev = vb2_get_drv_priv(q);
>>> +	struct cxusb_medion_dev *cxdev = dvbdev->priv;
>>> +	int i, ret;
>>> +
>>> +	cxusb_vprintk(dvbdev, OPS, "should stop streaming\n");
>>> +
>>> +	if (!cxdev->streaming)
>>> +		return;
>>> +
>>> +	cxdev->streaming = false;
>>> +
>>> +	cxusb_ctrl_msg(dvbdev, CMD_STREAMING_OFF, NULL, 0, NULL, 0);
>>> +
>>> +	ret = v4l2_subdev_call(cxdev->cx25840, video, s_stream, 0);
>>> +	if (ret != 0)
>>> +		dev_err(&dvbdev->udev->dev, "unable to stop stream (%d)\n",
>>> +			ret);
>>> +
>>> +	/* let URB completion run */
>>> +	mutex_unlock(cxdev->videodev->lock);
>>> +
>>> +	for (i = 0; i < CXUSB_VIDEO_URBS; i++)
>>> +		if (cxdev->streamurbs[i] != NULL)
>>> +			usb_kill_urb(cxdev->streamurbs[i]);
>>> +
>>> +	flush_work(&cxdev->urbwork);
>>> +
>>> +	mutex_lock(cxdev->videodev->lock);
>>> +
>>> +	/* free transfer buffer and URB */
>>> +	if (!cxdev->raw_mode)
>>> +		vfree(cxdev->auxbuf.buf);
>>> +
>>> +	for (i = 0; i < CXUSB_VIDEO_URBS; i++)
>>> +		if (cxdev->streamurbs[i] != NULL) {
>>> +			kfree(cxdev->streamurbs[i]->transfer_buffer);
>>> +			usb_free_urb(cxdev->streamurbs[i]);
>>> +			cxdev->streamurbs[i] = NULL;
>>> +		}
>>> +
>>> +	cxusb_medion_return_buffers(cxdev, false);
>>> +}
>>> +
>>> +static void cxusub_medion_v_buf_queue(struct vb2_buffer *vb)
>>> +{
>>> +	struct cxusb_medion_vbuffer *vbuf =
>>> +		container_of(vb, struct cxusb_medion_vbuffer, vb2);
>>> +	struct dvb_usb_device *dvbdev = vb2_get_drv_priv(vb->vb2_queue);
>>> +	struct cxusb_medion_dev *cxdev = dvbdev->priv;
>>> +
>>> +	/* cxusb_vprintk(dvbdev, OPS, "mmmm.. fresh buffer...\n"); */
>>> +
>>> +	list_add_tail(&vbuf->list, &cxdev->buflist);
>>
>> I would expect a spinlock here to protect the list.
> 
> All buffer list accesses are done either while holding main 
> lock or in vb2 callbacks (which accoring to
> https://lwn.net/Articles/447435/ can assume that the lock has been
> taken).

Ah, nothing runs in interrupt context in this driver. Got it.

>  
>>> +static int cxusb_medion_g_fmt_vid_cap(struct file *file, void *fh,
>>> +				      struct v4l2_format *f)
>>> +{
>>> +	struct dvb_usb_device *dvbdev = video_drvdata(file);
>>> +	struct cxusb_medion_dev *cxdev = dvbdev->priv;
>>> +
>>> +	f->fmt.pix.width = cxdev->width;
>>> +	f->fmt.pix.height = cxdev->height;
>>> +	f->fmt.pix.pixelformat = V4L2_PIX_FMT_UYVY;
>>> +	f->fmt.pix.field = V4L2_FIELD_SEQ_TB;
>>> +	f->fmt.pix.bytesperline = cxdev->raw_mode ? 0 : cxdev->width * 2;
>>
>> Dunno what raw_mode is, but it looks suspicous.
> 
> It is simply an ability to capture a raw BT.656 stream as received by
> the driver from the device.
> This functionality is very useful for debugging.

Please drop raw_mode support. Feel free to keep it as a separate patch for
your own debugging, but this isn't the way it should be done if you want
to get this mainlined. Since it is only used for debugging, I don't think
it is worth the effort.

>> I stopped reviewing halfway because I was wondering whether this is the right approach.
>>
>> Most of the analog support is not actually tied to the medion but is fairly generic.
>> This is how other drivers do that as well: the implementation is generic and the board
>> specific bits are implemented via card structures.
>>
>> They also make use of v4l2_device_call_all() instead of calling each subdev directly,
>> this means that the board-specific code loads the correct subdevs, but the generic
>> code doesn't need to know (in general) what those are, it just calls all of them.
> 
> There are only three subdevices here: a RF tuner, TDA9887 demod and
> cx25840 chip.
> Most of the subdev calls are for c25840 only, some (like g_tuner) should
> be done in a proper order so returned details aren't overwritten by less
> specific data being returned from the next subdevice.
> The RF tuner and the demod are target of just a few of the subdev calls.
> 
> v4l2_device_call_all() ignores any errors, which makes for example trying

If you need to intercept errors, then you can use v4l2_device_call_until_err().

> a video format hard (and which format will be accepted by the cx25840
> depends on the currently set broadcast standard and parameters of the
> last signal that was received), while V4L2 docs say that
> VIDIOC_{S,TRY}_FMT ioctls "should not return an error code unless the
> type field is invalid", that is, they should not return an error for
> invalid or unsupported image widths or heights.
> They should instead return something sensible for these image parameters
> which requires a feedback from the cx25840 subdev which one it will
> finally accept.
> 
> With respect to making the code more generic: considering this is a
> 13-year old hardware I think it is fairly unlikely that there are going
> to be devices with similar internal design released in the future.
> And in the unlikely case that this indeed happens then the code can
> always be refactored into some more generic framework.

That's a fair point.

I would like to see some comments that explain that if we ever need to
support other analog hardware, then this code should be made generic.

Regards,

	Hans

> 
> This driver has been alredy nearly merged back in 2012 (even without last
> year cleanups), just I wasn't able to find time to rebase it then:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg50449.html
> so it isn't something completely new.
> 
>> Regards,
>>
>> 	Hans
>>
> 
> Thanks and best regards,
> Maciej
> 

Powered by blists - more mailing lists