Re: [PATCH RFC v3 1/3] V4L: Add driver for S3C244X/S3C64XX SoC series camera interface

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Hans,

On 11/16/2012 02:45 PM, Hans Verkuil wrote:
> Hi Sylwester,
>
> Just one comment, see below...
>
> On Thu November 15 2012 23:05:13 Sylwester Nawrocki wrote:
>> This patch adds V4L2 driver for Samsung S3C244X/S3C64XX SoC series
>> camera interface. The driver exposes a subdev device node for CAMIF
>> pixel resolution and crop control and two video capture nodes - for
>> the "codec" and "preview" data paths. It has been tested on Mini2440
>> (s3c2440) and Mini6410 (s3c6410) board with gstreamer and mplayer.
>>
>> Signed-off-by: Sylwester Nawrocki<sylvester.nawrocki@gmail.com>
>> Signed-off-by: Tomasz Figa<tomasz.figa@gmail.com>
>> ---
...
>> +static int s3c_camif_streamon(struct file *file, void *priv,
>> +			      enum v4l2_buf_type type)
>> +{
>> +	struct camif_vp *vp = video_drvdata(file);
>> +	struct camif_dev *camif = vp->camif;
>> +	struct media_entity *sensor =&camif->sensor.sd->entity;
>> +	int ret;
>> +
>> +	pr_debug("[vp%d]\n", vp->id);
>> +
>> +	if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> +		return -EINVAL;
>> +
>> +	if (vp->owner&&  vp->owner != priv)
>> +		return -EBUSY;
>> +
>> +	if (s3c_vp_active(vp))
>> +		return 0;
>> +
>> +	ret = media_entity_pipeline_start(sensor, camif->m_pipeline);
>> +	if (ret<  0)
>> +		return ret;
>> +
>> +	ret = camif_pipeline_validate(camif);
>> +	if (ret<  0) {
>> +		media_entity_pipeline_stop(sensor);
>> +		return ret;
>> +	}
>> +
>> +	return vb2_streamon(&vp->vb_queue, type);
>> +}
>> +
>> +static int s3c_camif_streamoff(struct file *file, void *priv,
>> +			       enum v4l2_buf_type type)
>> +{
>> +	struct camif_vp *vp = video_drvdata(file);
>> +	struct camif_dev *camif = vp->camif;
>> +	int ret;
>> +
>> +	pr_debug("[vp%d]\n", vp->id);
>> +
>> +	if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> +		return -EINVAL;
>> +
>> +	if (vp->owner&&  vp->owner != priv)
>> +		return -EBUSY;
>> +
>> +	ret = vb2_streamoff(&vp->vb_queue, type);
>> +	if (ret == 0)
>> +		media_entity_pipeline_stop(&camif->sensor.sd->entity);
>> +	return ret;
>> +}
>> +
>> +static int s3c_camif_reqbufs(struct file *file, void *priv,
>> +			     struct v4l2_requestbuffers *rb)
>> +{
>> +	struct camif_vp *vp = video_drvdata(file);
>> +	int ret;
>> +
>> +	pr_debug("[vp%d] rb count: %d, owner: %p, priv: %p\n",
>> +		 vp->id, rb->count, vp->owner, priv);
>> +
>> +	if (vp->owner&&  vp->owner != priv)
>> +		return -EBUSY;
>> +
>> +	if (rb->count)
>> +		rb->count = max_t(u32, CAMIF_REQ_BUFS_MIN, rb->count);
>> +	else
>> +		vp->owner = NULL;
>> +
>> +	ret = vb2_reqbufs(&vp->vb_queue, rb);
>> +	if (!ret) {
>> +		vp->reqbufs_count = rb->count;
>> +		if (vp->owner == NULL&&  rb->count>  0)
>> +			vp->owner = priv;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int s3c_camif_querybuf(struct file *file, void *priv,
>> +			      struct v4l2_buffer *buf)
>> +{
>> +	struct camif_vp *vp = video_drvdata(file);
>> +	return vb2_querybuf(&vp->vb_queue, buf);
>> +}
>> +
>> +static int s3c_camif_qbuf(struct file *file, void *priv,
>> +			  struct v4l2_buffer *buf)
>> +{
>> +	struct camif_vp *vp = video_drvdata(file);
>> +
>> +	pr_debug("[vp%d]\n", vp->id);
>> +
>> +	if (vp->owner&&  vp->owner != priv)
>> +		return -EBUSY;
>> +
>> +	return vb2_qbuf(&vp->vb_queue, buf);
>> +}
>> +
>> +static int s3c_camif_dqbuf(struct file *file, void *priv,
>> +			   struct v4l2_buffer *buf)
>> +{
>> +	struct camif_vp *vp = video_drvdata(file);
>> +
>> +	pr_debug("[vp%d] sequence: %d\n", vp->id, vp->frame_sequence);
>> +
>> +	if (vp->owner&&  vp->owner != priv)
>> +		return -EBUSY;
>> +
>> +	return vb2_dqbuf(&vp->vb_queue, buf, file->f_flags&  O_NONBLOCK);
>> +}
>> +
>> +static int s3c_camif_create_bufs(struct file *file, void *priv,
>> +				 struct v4l2_create_buffers *create)
>> +{
>> +	struct camif_vp *vp = video_drvdata(file);
>> +	int ret;
>> +
>> +	if (vp->owner&&  vp->owner != priv)
>> +		return -EBUSY;
>> +
>> +	create->count = max_t(u32, 1, create->count);
>> +	ret = vb2_create_bufs(&vp->vb_queue, create);
>> +
>> +	if (!ret&&  vp->owner == NULL)
>> +		vp->owner = priv;
>> +
>> +	return ret;
>> +}
>> +
>> +static int s3c_camif_prepare_buf(struct file *file, void *priv,
>> +				 struct v4l2_buffer *b)
>> +{
>> +	struct camif_vp *vp = video_drvdata(file);
>> +	return vb2_prepare_buf(&vp->vb_queue, b);
>> +}
>> +
>
> Are you aware of the vb2 ioctl helper functions I've added? See videobuf2-core.h,
> at the end.

Yes, I just chose not to introduce these helpers now, to make any 
back-porting
of this driver to older kernels easier.

> They can probably replace some of these ioctls. It's something you can do later
> in a separate patch, so this isn't blocking as far as I am concerned. It's just
> a hint.

Thanks, I'll see which ones can be replaced. But I would prefer to make it
a separate patch for subsequent kernel release.

Looking at v4l2_ioctl_create_bufs(), v4l2_ioctl_prepare_buf(), is it he 
right
thing not to allow other process/thread than the current buffer queue owner
to create and prepare buffers ? This would prevent for example, having two
threads where one is currently streaming and the other creates buffers. 
The use
case is maybe not very common but I can't see why we need to disallow that.

--

Regards,
Sylwester