On Sun March 17 2013 22:03:38 Sylwester Nawrocki wrote:
> 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.
That's correct. Sorry for the misinformation about the non-existant
vb2_fop_open.
> 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.
I don't see where a lock would be needed. I don't see any concurrency here.
Nobody else can mess with the queue as long as they are not the owner.
>
> >> +};
> >> +
> >> +/*
> >> + * 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.
Glad to hear!
I still think there are some more improvements that could be made with some
helper functions and perhaps some more fields: most (all?) drivers need a
list for their active buffers, so I really think adding a struct list to
vb2_buffer and a list for active buffers + a spinlock to vb2_queue would
help.
Helper functions could then be added to handle that queue and also to set
the timestamp and sequence fields. I would need to think more about the
details, but after using vb2 for a while I see a lot of shared code between
the various drivers.
Regards,
Hans
> > 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
