by Hans Verkuil

[permalink] [raw]

Subject: Re: [PATCH v7 3/6] cx25840: add pin to pad mapping and output format configuration

On 10/07/18 23:47, Maciej S. Szmigiero wrote:
> Hi Hans,
>
> On 04.07.2018 11:05, Hans Verkuil wrote:
>> On 02/07/18 23:23, Maciej S. Szmigiero wrote:
> (..)
>>> @@ -316,6 +319,260 @@ static int cx23885_s_io_pin_config(struct v4l2_subdev *sd, size_t n,
>>> return 0;
>>> }
>>>
>>> +static u8 cx25840_function_to_pad(struct i2c_client *client, u8 function)
>>> +{
>>> + switch (function) {
>>> + case CX25840_PAD_ACTIVE:
>>> + return 1;
>>> +
>>> + case CX25840_PAD_VACTIVE:
>>> + return 2;
>>> +
>>> + case CX25840_PAD_CBFLAG:
>>> + return 3;
>>> +
>>> + case CX25840_PAD_VID_DATA_EXT0:
>>> + return 4;
>>> +
>>> + case CX25840_PAD_VID_DATA_EXT1:
>>> + return 5;
>>> +
>>> + case CX25840_PAD_GPO0:
>>> + return 6;
>>> +
>>> + case CX25840_PAD_GPO1:
>>> + return 7;
>>> +
>>> + case CX25840_PAD_GPO2:
>>> + return 8;
>>> +
>>> + case CX25840_PAD_GPO3:
>>> + return 9;
>>> +
>>> + case CX25840_PAD_IRQ_N:
>>> + return 10;
>>> +
>>> + case CX25840_PAD_AC_SYNC:
>>> + return 11;
>>> +
>>> + case CX25840_PAD_AC_SDOUT:
>>> + return 12;
>>> +
>>> + case CX25840_PAD_PLL_CLK:
>>> + return 13;
>>> +
>>> + case CX25840_PAD_VRESET:
>>> + return 14;
>>> +
>>> + default:
>>> + if (function != CX25840_PAD_DEFAULT)
>>> + v4l_err(client,
>>> + "invalid function %u, assuming default\n",
>>> + (unsigned int)function);
>>> + return 0;
>>> + }
>>
>> Unless I am mistaken this function boils down to:
>>
>> static u8 cx25840_function_to_pad(struct i2c_client *client, u8 function)
>> {
>> return function > CX25840_PAD_VRESET ? 0 : function;
>> }
>
> Yes, you are right these functions are equivalent (sans a warning when
> a caller passes an invalid function).
>
> However, these values (CX25840_PAD_*) were meant to be driver-internal.
> If we use them also as register value constants (which is what
> cx25840_function_to_pad() is supposed to return) then we'll need to add
> a comment to their enum cx25840_io_pad so nobody shuffles them or changes
> their values by mistake.

Right. Just add a comment and keep it simple.

>
>>> @@ -1647,6 +1924,119 @@ static void log_audio_status(struct i2c_client *client)
>>> }
>>> }
>>>
>>> +#define CX25840_VCONFIG_OPTION(state, cfg_in, opt_msk) \
>>> + do { \
>>> + if ((cfg_in) & (opt_msk)) { \
>>> + (state)->vid_config &= ~(opt_msk); \
>>> + (state)->vid_config |= (cfg_in) & (opt_msk); \
>>> + } \
>>> + } while (0)
>>> +
>>> +#define CX25840_VCONFIG_SET_BIT(state, opt_msk, voc, idx, bit, oneval) \
>>> + do { \
>>> + if ((state)->vid_config & (opt_msk)) { \
>>> + if (((state)->vid_config & (opt_msk)) == \
>>> + (oneval)) \
>>> + (voc)[idx] |= BIT(bit); \
>>> + else \
>>> + (voc)[idx] &= ~BIT(bit); \
>>> + } \
>>> + } while (0)
>>> +
>>> +int cx25840_vconfig(struct i2c_client *client, u32 cfg_in)
>>> +{
>>> + struct cx25840_state *state = to_state(i2c_get_clientdata(client));
>>> + u8 voutctrl[3];
>>> + unsigned int i;
>>> +
>>> + /* apply incoming options to the current state */
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_FMT_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_RES_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_VBIRAW_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_ANCDATA_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_TASKBIT_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_ACTIVE_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_VALID_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_HRESETW_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_CLKGATE_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_DCMODE_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_IDID0S_MASK);
>>> + CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_VIPCLAMP_MASK);
>>
>> This appears to be a very complex way of saying:
>>
>> state->vid_config = cfg_in;
>
> This is supposed to change in vid_config only these options that are set
> in the incoming cfg_in, leaving the rest as-is.
>
> If the code simply assigns cfg_in to vid_config it will also clear all the
> existing options.

Ah yes, but see my reply below at the end.

>
>>
>>> +
>>> + for (i = 0; i < 3; i++)
>>> + voutctrl[i] = cx25840_read(client, 0x404 + i);
>>> +
>>> + /* apply state to hardware regs */
>>> + if (state->vid_config & CX25840_VCONFIG_FMT_MASK)
>>> + voutctrl[0] &= ~3;
>>> + switch (state->vid_config & CX25840_VCONFIG_FMT_MASK) {
>>> + case CX25840_VCONFIG_FMT_BT656:
>>> + voutctrl[0] |= 1;
>>> + break;
>>> +
>>> + case CX25840_VCONFIG_FMT_VIP11:
>>> + voutctrl[0] |= 2;
>>> + break;
>>> +
>>> + case CX25840_VCONFIG_FMT_VIP2:
>>> + voutctrl[0] |= 3;
>>> + break;
>>> +
>>> + case CX25840_VCONFIG_FMT_BT601:
>>> + /* zero */
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_RES_MASK, voutctrl,
>>> + 0, 2, CX25840_VCONFIG_RES_10BIT);
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_VBIRAW_MASK, voutctrl,
>>> + 0, 3, CX25840_VCONFIG_VBIRAW_ENABLED);
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_ANCDATA_MASK, voutctrl,
>>> + 0, 4, CX25840_VCONFIG_ANCDATA_ENABLED);
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_TASKBIT_MASK, voutctrl,
>>> + 0, 5, CX25840_VCONFIG_TASKBIT_ONE);
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_ACTIVE_MASK, voutctrl,
>>> + 1, 2, CX25840_VCONFIG_ACTIVE_HORIZONTAL);
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_VALID_MASK, voutctrl,
>>> + 1, 3, CX25840_VCONFIG_VALID_ANDACTIVE);
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_HRESETW_MASK, voutctrl,
>>> + 1, 4, CX25840_VCONFIG_HRESETW_PIXCLK);
>>> +
>>> + if (state->vid_config & CX25840_VCONFIG_CLKGATE_MASK)
>>> + voutctrl[1] &= ~(3 << 6);
>>> + switch (state->vid_config & CX25840_VCONFIG_CLKGATE_MASK) {
>>> + case CX25840_VCONFIG_CLKGATE_VALID:
>>> + voutctrl[1] |= 2;
>>> + break;
>>> +
>>> + case CX25840_VCONFIG_CLKGATE_VALIDACTIVE:
>>> + voutctrl[1] |= 3;
>>> + break;
>>> +
>>> + case CX25840_VCONFIG_CLKGATE_NONE:
>>> + /* zero */
>>> + default:
>>> + break;
>>> + }
>>> +
>>> +
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_DCMODE_MASK, voutctrl,
>>> + 2, 0, CX25840_VCONFIG_DCMODE_BYTES);
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_IDID0S_MASK, voutctrl,
>>> + 2, 1, CX25840_VCONFIG_IDID0S_LINECNT);
>>> + CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_VIPCLAMP_MASK, voutctrl,
>>> + 2, 4, CX25840_VCONFIG_VIPCLAMP_ENABLED);
>>> +
>>> + for (i = 0; i < 3; i++)
>>> + cx25840_write(client, 0x404 + i, voutctrl[i]);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +#undef CX25840_VCONFIG_SET_BIT
>>> +#undef CX25840_VCONFIG_OPTION
>>
>> Why #undef? You would normally never do that.
>
> The idea here is to catch (unintended) other users of these macros, other
> than cx25840_vconfig() and to not pollute the macro namespace.
> But these #undefs can be removed if they are against the coding style.

Please remove. You do not normally use #undef unless you are redefining a
macro.

>
>>
>>> +
>>> /* ----------------------------------------------------------------------- */
>>>
>>> /* This load_fw operation must be called to load the driver's firmware.
>>> @@ -1836,6 +2226,9 @@ static int cx25840_s_video_routing(struct v4l2_subdev *sd,
>>> if (is_cx23888(state))
>>> cx23888_std_setup(client);
>>>
>>> + if (is_cx2584x(state) && state->generic_mode)
>>> + cx25840_vconfig(client, config);
>>> +
>>
>> You do the vconfig configuration when the video routing changes. But isn't this
>> configuration a one-time thing? E.g. something you do only when initializing the
>> board?
>>
>> At least in the cxusb code the cfg_in value is constant and not dependent on what
>> input is chosen.
>>
>> If this is true, then you should add the core init op instead. And as a bonus you
>> can turn on generic_mode if the init op is called instead of having to add it
>> to the platform data.
>>
>
> The problem here is that the Medion MD95700 has two modes:
> digital (DVB-T) and analog.
> When it is operating in the digital mode the device analog components
> (including the cx25840 chip) have their power cut by the hardware.
>
> This means that the cx25840 has to be fully reinitialized (firmware
> loaded, format set, etc.) every time the user opens the Medion's v4l2
> video or radio device node if the device has previously operated in the
> DVB-T mode.
> Mode switching is supported transparently by the driver without needing
> to reload the module or reconnect the device.

So? You call the core init op to initialize vconfig (just pass in all the
bits to simplify things), and whenever s_video_routing it called the driver
will just call cx25840_vconfig() with the vconfig value set by the core init op.

As far as I can see all you want to do is to specify a specific vconfig value
that should be applied whenever s_video_routing is called.

So in cxusb_medion_analog_init() you call the core init op, then the
video s_routing op to switch to COMPOSITE1.

I like that much better since vconfig doesn't change when you switch inputs.
It's only needed when you initialize the analog part.

Regards,

Hans

>
>>
>> Regards,
>>
>> Hans
>>
>
> Thanks and best regards,
> Maciej
>

2018-07-17 12:57:41

by Hans Verkuil

[permalink] [raw]

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/[email protected]/msg50449.html
> so it isn't something completely new.
>
>> Regards,
>>
>> Hans
>>
>
> Thanks and best regards,
> Maciej
>