Re: [RFC PATCH 5/8] s5p-fimc: Add ISP video capture driver stubs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/12/2013 03:44 PM, Hans Verkuil wrote:
On Mon 11 March 2013 20:44:49 Sylwester Nawrocki wrote:
[...]
+static int isp_video_capture_open(struct file *file)
+{
+	struct fimc_isp *isp = video_drvdata(file);
+	int ret = 0;
+
+	if (mutex_lock_interruptible(&isp->video_lock))
+		return -ERESTARTSYS;
+
+	/* ret = pm_runtime_get_sync(&isp->pdev->dev); */
+	if (ret<  0)
+		goto done;
+
+	ret = v4l2_fh_open(file);
+	if (ret<  0)
+		goto done;
+
+	/* TODO: prepare video pipeline */
+done:
+	mutex_unlock(&isp->video_lock);
+	return ret;
+}
+
+static int isp_video_capture_close(struct file *file)
+{
+	struct fimc_isp *isp = video_drvdata(file);
+	int ret = 0;
+
+	mutex_lock(&isp->video_lock);
+
+	if (isp->out_path == FIMC_IO_DMA) {
+		/* TODO: stop capture, cleanup */
+	}
+
+	/* pm_runtime_put(&isp->pdev->dev); */
+
+	if (isp->ref_count == 0)
+		vb2_queue_release(&isp->capture_vb_queue);
+
+	ret = v4l2_fh_release(file);
+
+	mutex_unlock(&isp->video_lock);
+	return ret;
+}
+
+static unsigned int isp_video_capture_poll(struct file *file,
+				   struct poll_table_struct *wait)
+{
+	struct fimc_isp *isp = video_drvdata(file);
+	int ret;
+
+	mutex_lock(&isp->video_lock);
+	ret = vb2_poll(&isp->capture_vb_queue, file, wait);
+	mutex_unlock(&isp->video_lock);
+	return ret;
+}
+
+static int isp_video_capture_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct fimc_isp *isp = video_drvdata(file);
+	int ret;
+
+	if (mutex_lock_interruptible(&isp->video_lock))
+		return -ERESTARTSYS;
+
+	ret = vb2_mmap(&isp->capture_vb_queue, vma);
+	mutex_unlock(&isp->video_lock);
+
+	return ret;
+}
+
+static const struct v4l2_file_operations isp_video_capture_fops = {
+	.owner		= THIS_MODULE,
+	.open		= isp_video_capture_open,
+	.release	= isp_video_capture_close,
+	.poll		= isp_video_capture_poll,
+	.unlocked_ioctl	= video_ioctl2,
+	.mmap		= isp_video_capture_mmap,

Can't you use the helper functions vb2_fop_open/release/poll/mmap here?

It seems vb2_fop_mmap/poll can be used directly, open(), release() are
a bit more complicated as some media pipeline operations need to
additionally be done within these callbacks. There is no vb2_fop_open(),
and AFAICS v4l2_fh_open() is sufficient and intended as open() helper.
For the next iteration I have used vb2_fop_release(), called indirectly,
as it nicely simplifies things a bit.

BTW, shouldn't vb2_fop_release() also be taking the lock ? Actually it is
more useful for me in current form, but the drivers that directly assign
it to struct v4l2_file_operations::open might be in trouble, unless I'm
missing something.

+};
+
+/*
+ * Video node ioctl operations
+ */
[...]
+static int fimc_isp_capture_streamon(struct file *file, void *priv,
+				     enum v4l2_buf_type type)
+{
+	struct fimc_isp *isp = video_drvdata(file);
+	struct v4l2_subdev *sensor = isp->pipeline.subdevs[IDX_SENSOR];
+	struct fimc_pipeline *p =&isp->pipeline;
+	int ret;
+
+	/* TODO: check if the OTF interface is not running */
+
+	ret = media_entity_pipeline_start(&sensor->entity, p->m_pipeline);
+	if (ret<  0)
+		return ret;
+
+	ret = fimc_isp_pipeline_validate(isp);
+	if (ret) {
+		media_entity_pipeline_stop(&sensor->entity);
+		return ret;
+	}
+
+	return vb2_streamon(&isp->capture_vb_queue, type);
+}
+
+static int fimc_isp_capture_streamoff(struct file *file, void *priv,
+				      enum v4l2_buf_type type)
+{
+	struct fimc_isp *isp = video_drvdata(file);
+	struct v4l2_subdev *sd = isp->pipeline.subdevs[IDX_SENSOR];
+	int ret;
+
+	ret = vb2_streamoff(&isp->capture_vb_queue, type);
+	if (ret == 0)
+		media_entity_pipeline_stop(&sd->entity);
+	return ret;
+}
+
+static int fimc_isp_capture_reqbufs(struct file *file, void *priv,
+				    struct v4l2_requestbuffers *reqbufs)
+{
+	struct fimc_isp *isp = video_drvdata(file);
+	int ret;
+
+	reqbufs->count = max_t(u32, FIMC_IS_REQ_BUFS_MIN, reqbufs->count);
+	ret = vb2_reqbufs(&isp->capture_vb_queue, reqbufs);

You probably want to call vb2_ioctl_reqbufs here since that does additional
ownership checks that vb2_reqbufs doesn't.

Yes, thanks for the suggestion. That was actually helpful, previously
it wasn't immediately clear to me one can still take advantage of those
vb2_ioctl_* helpers, mainly have the ownership handling in the core,
and have some handlers assigned directly to v4l2_ioctl_ops an some called
indirectly from the driver's own callbacks, should they need something
else that is done in the helpers.

I found those helpers really useful, especially in drivers that need to
support several video nodes. Lots of boilerplate can be eliminated. And
also vb2_ops_wait_prepare/finish are a simple and nice improvement.

The same is true for vb2_ioctl_streamon/off, BTW.

Indeed. I have already applied all possible helpers for the next iteration.

I need to yet resolve an issue with locking order, as I previously missed
that media_entity_pipeline_start/stop() also takes the graph mutex.

And currently the driver is supposed to take the graph mutex first and
then the video mutex. Since the link_notify callback of the media device
is called with the graph mutex already held.

The only solution I came up so far is to provide unlocked versions of
media_entity_pipeline_start/stop().

Ideally using video mutex in link_notify() callback should not be needed,
but there are things done there needed for backward video device
compatibility.

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux