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
