Hi Ezequiel,
On 05/26/2012 07:50 PM, Hans Verkuil wrote:
>> +/*
>> + * IOCTL vidioc handling
>> + */
>> +static int vidioc_reqbufs(struct file *file, void *priv,
>> + struct v4l2_requestbuffers *p)
>> +{
>> + struct stk1160 *dev = video_drvdata(file);
>> + return vb2_reqbufs(&dev->vb_vidq, p);
>> +}
>> +
>> +static int vidioc_querybuf(struct file *file, void *priv, struct v4l2_buffer *p)
>> +{
>> + struct stk1160 *dev = video_drvdata(file);
>> + return vb2_querybuf(&dev->vb_vidq, p);
>> +}
>> +
>> +static int vidioc_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>> +{
>> + struct stk1160 *dev = video_drvdata(file);
>> + return vb2_qbuf(&dev->vb_vidq, p);
>> +}
>> +
>> +static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>> +{
>> + struct stk1160 *dev = video_drvdata(file);
>> + return vb2_dqbuf(&dev->vb_vidq, p, file->f_flags& O_NONBLOCK);
>> +}
>> +
>> +static int vidioc_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
>> +{
>> + struct stk1160 *dev = video_drvdata(file);
>> + return vb2_streamon(&dev->vb_vidq, i);
>> +}
>> +
>> +static int vidioc_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
>> +{
>> + struct stk1160 *dev = video_drvdata(file);
>> + return vb2_streamoff(&dev->vb_vidq, i);
>> +}
>> +
>> +static int vidioc_querycap(struct file *file,
>> + void *priv, struct v4l2_capability *dev)
>> +{
>> + strcpy(dev->driver, "stk1160");
>> + strcpy(dev->card, "stk1160");
>> + dev->version = STK1160_VERSION_NUM;
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.
>> + dev->capabilities =
>> + V4L2_CAP_VIDEO_CAPTURE |
>> + V4L2_CAP_STREAMING |
>> + V4L2_CAP_READWRITE;
>> + return 0;
>> +}
>> +
>> +static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
>> + struct v4l2_fmtdesc *f)
>> +{
>> + if (f->index != 0)
>> + return -EINVAL;
>> +
>> + strlcpy(f->description, format[f->index].name, sizeof(f->description));
>> + f->pixelformat = format[f->index].fourcc;
>> + return 0;
>> +}
>> +
>> +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
>> + struct v4l2_format *f)
>> +{
>> + struct stk1160 *dev = video_drvdata(file);
>> +
>> + 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.
>> + f->fmt.pix.sizeimage = dev->height * f->fmt.pix.bytesperline;
>> + f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
>> +
>> + return 0;
>> +}
>> +
>> +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
>> + struct v4l2_format *f)
>> +{
>> + struct stk1160 *dev = video_drvdata(file);
>> +
>> + if (f->fmt.pix.pixelformat != format[0].fourcc) {
I would rename format[] to stk1160_formats[], but it might just be me.
>> + stk1160_err("fourcc format 0x%08x invalid\n",
>> + f->fmt.pix.pixelformat);
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * User can't choose size at his own will,
>> + * so we just return him the current size chosen
>> + * at standard selection.
>> + * TODO: Implement frame scaling?
>> + */
>> +
>> + f->fmt.pix.width = dev->width;
>> + f->fmt.pix.height = dev->height;
>> + f->fmt.pix.field = V4L2_FIELD_INTERLACED;
>> + f->fmt.pix.bytesperline = dev->width<< 1;
dev->width * 2;
>> + f->fmt.pix.sizeimage = dev->height * f->fmt.pix.bytesperline;
>> + f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
>> +
>> + return 0;
>> +}
...
>> +/*
>> + * Videobuf2 operations
>> + */
>> +static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *v4l_fmt,
>> + unsigned int *nbuffers, unsigned int *nplanes,
>> + unsigned int sizes[], void *alloc_ctxs[])
>> +{
>> + struct stk1160 *dev = vb2_get_drv_priv(vq);
>> + unsigned long size;
>> +
>> + size = dev->width * dev->height * 2;
>> +
>> + /*
>> + * Here we can change the number of buffers being requested.
>> + * For instance, we could set a minimum and a maximum,
>> + * like this:
>> + */
>> + if (*nbuffers< STK1160_MIN_VIDEO_BUFFERS)
>> + *nbuffers = STK1160_MIN_VIDEO_BUFFERS;
>> + else if (*nbuffers> STK1160_MAX_VIDEO_BUFFERS)
>> + *nbuffers = STK1160_MAX_VIDEO_BUFFERS;
Could be also done as:
*buffers = clamp(*buffers, STK1160_MIN_VIDEO_BUFFERS,
STK1160_MAX_VIDEO_BUFFERS);
>> +
>> + /* This means a packed colorformat */
>> + *nplanes = 1;
>> +
>> + sizes[0] = size;
>> +
>> + /*
>> + * 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.
>> +
>> + stk1160_info("%s: buffer count %d, each %ld bytes\n",
>> + __func__, *nbuffers, size);
>> +
>> + return 0;
>> +}
>> +
>> +static int buffer_init(struct vb2_buffer *vb)
>> +{
>> + return 0;
>> +}
>> +
>> +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().
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.
>> + 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.
>> +
>> +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
--
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
