Hi Sylwester,
On Sat, May 26, 2012 at 4:31 PM, Sylwester Nawrocki <snjw23@xxxxxxxxx> wrote:
>
> You can drop this line, it's overwritten with KERNEL_VERSION in v4l2-ioctl.c.
> Also I could imagine there might be better names, than "dev", for capabilities.
>
Yes, indeed. Starting with "cap".
>>> + dev->capabilities =
>>> + V4L2_CAP_VIDEO_CAPTURE |
>>> + V4L2_CAP_STREAMING |
>>> + V4L2_CAP_READWRITE;
>>> + return 0;
>>> + f->fmt.pix.width = dev->width;
>>> + f->fmt.pix.height = dev->height;
>>> + f->fmt.pix.field = V4L2_FIELD_INTERLACED;
>>> + f->fmt.pix.pixelformat = dev->fmt->fourcc;
>>> + f->fmt.pix.bytesperline = dev->width<< 1;
> ^^^^^^^^^^^^^^^^
> Probably better to just write: dev->width * 2.
I saw like that in a number of places and I tought it was "faster".
Guess, I was being truly naive.
>
> Could be also done as:
>
> *buffers = clamp(*buffers, STK1160_MIN_VIDEO_BUFFERS,
> STK1160_MAX_VIDEO_BUFFERS);
Done.
>>> + /*
>>> + * videobuf2-vmalloc allocator is context-less so no need to set
>>> + * alloc_ctxs array.
>>> + */
>>> +
>>> + if (v4l_fmt) {
>>> + stk1160_info("selected format %d (%d)\n",
>>> + v4l_fmt->fmt.pix.pixelformat,
>>> + dev->fmt->fourcc);
>>> + }
>
> This log is not exactly right. You just ignore v4l_fmt, so it is not selected
> anywhere. The *v4l_fmt argument is here for ioctls like VIDIOC_CREATE_BUFS,
> and in case you wanted to support this ioctl you would need to do something
> like:
> pix = &v4l_fmt->fmt.pix;
> sizes[0] = pix->width * pix->height * 2;
>
> Of course with any required sanity checks.
>
> But this driver does not implement vidioc_create_bufs/vidioc_prepare_buf ioctl
> callbacks are not, so the code is generally OK.
You're right, that was just legacy code from some early stages.
>>> +static int buffer_prepare(struct vb2_buffer *vb)
>>> +{
>>> + struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
>>> + struct stk1160_buffer *buf =
>>> + container_of(vb, struct stk1160_buffer, vb);
>>> +
>>> + /* If the device is disconnected, reject the buffer */
>>> + if (!dev->udev)
>>> + return -ENODEV;
>>> +
>>> + buf->mem = vb2_plane_vaddr(vb, 0);
>>> + buf->length = vb2_plane_size(vb, 0);
>
> Where do you check if the buffer you get from vb2 has correct parameters
> for your hardware (with current settings) to start writing data to it ?
>
> It seems that this driver supports just one pixel format and resolution,
> but still would be good to do such checks in buf_prepare().
You mean I should check buf->length?
>
> And initialization of buf->mem, buf->length is better done from
> buffer_queue() op. This way there would be no need to take dev->buf_lock
> spinlock also in buf_prepare() to protect the driver's buffer (queue),
> which isn't done but it should in buffer_prepare() btw.
Yes, I missed this spot.
>
>>> + buf->bytesused = 0;
>>> + buf->pos = 0;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int buffer_finish(struct vb2_buffer *vb)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static void buffer_cleanup(struct vb2_buffer *vb)
>>> +{
>>> +}
>
> buf_init, buf_finish, buf_cleanup are optional callbacks, so if you
> don't use them they could be removed altogether.
>
Done.
>>> +
>>> +static void buffer_queue(struct vb2_buffer *vb)
>>> +{
>>> + unsigned long flags = 0;
>>> + struct stk1160 *dev = vb2_get_drv_priv(vb->vb2_queue);
>>> + struct stk1160_buffer *buf =
>>> + container_of(vb, struct stk1160_buffer, vb);
>>> +
>>> + spin_lock_irqsave(&dev->buf_lock, flags);
>>> + if (!dev->udev) {
>>> + /*
>>> + * If the device is disconnected return the buffer to userspace
>>> + * directly. The next QBUF call will fail with -ENODEV.
>>> + */
>>> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
>>> + } else {
>>> + list_add_tail(&buf->list,&dev->avail_bufs);
>>> + }
>>> + spin_unlock_irqrestore(&dev->buf_lock, flags);
>>> +}
>
> --
> Regards,
> Sylwester
Thanks,
Ezequiel.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
