| Message ID | 20170616073915.5027-8-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Fri, 16 Jun 2017 16:39:10 +0900 Gustavo Padovan <gustavo@padovan.org> escreveu: > From: Gustavo Padovan <gustavo.padovan@collabora.com> > > Implement the needed pieces to let userspace subscribe for > V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the > DQEVENT ioctl. > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com> > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 6 +++++- > drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++ > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 5aed7bd..f55b5da 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -3435,8 +3435,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status); > int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh, > const struct v4l2_event_subscription *sub) > { > - if (sub->type == V4L2_EVENT_CTRL) > + switch (sub->type) { > + case V4L2_EVENT_CTRL: > return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops); > + case V4L2_EVENT_BUF_QUEUED: > + return v4l2_event_subscribe(fh, sub, 0, NULL); > + } > return -EINVAL; > } > EXPORT_SYMBOL(v4l2_ctrl_subscribe_event); > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 29aa9d4..00d9c35 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -25,6 +25,7 @@ > #include <linux/kthread.h> > > #include <media/videobuf2-core.h> > +#include <media/v4l2-event.h> > #include <media/v4l2-mc.h> > > #include <trace/events/vb2.h> > @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb) > return ret; > } > > +static void vb2_buffer_queued_event(struct vb2_buffer *vb) > +{ > + struct video_device *vdev = to_video_device(vb->vb2_queue->dev); > + struct v4l2_event event; > + > + memset(&event, 0, sizeof(event)); > + event.type = V4L2_EVENT_BUF_QUEUED; > + event.u.buf_queued.index = vb->index; > + > + v4l2_event_queue(vdev, &event); > +} > + It doesn't sound right to add a V4L2 event to VB2 core. The hole point of splitting the core from V4L2 specific stuff is to allow VB2 to be used by non-V4L2 APIs[1]. Please move this to videobuf2-v4l2. [1] The split happened as part of a patchset meant to make the DVB core to use VB2 and provide DMA APIs to it. Unfortunately, the developer that worked on this project moved to some other project. The final patch was not applied yet. I have it on my patchwork queue. I intend to test and apply it sometime this year. > /** > * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing > */ > @@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb) > trace_vb2_buf_queue(q, vb); > > call_void_vb_qop(vb, buf_queue, vb); > + > + vb2_buffer_queued_event(vb); > } > > static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
Hi Mauro, 2017-06-30 Mauro Carvalho Chehab <mchehab@osg.samsung.com>: > Em Fri, 16 Jun 2017 16:39:10 +0900 > Gustavo Padovan <gustavo@padovan.org> escreveu: > > > From: Gustavo Padovan <gustavo.padovan@collabora.com> > > > > Implement the needed pieces to let userspace subscribe for > > V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the > > DQEVENT ioctl. > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com> > > --- > > drivers/media/v4l2-core/v4l2-ctrls.c | 6 +++++- > > drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++ > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > > index 5aed7bd..f55b5da 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > @@ -3435,8 +3435,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status); > > int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh, > > const struct v4l2_event_subscription *sub) > > { > > - if (sub->type == V4L2_EVENT_CTRL) > > + switch (sub->type) { > > + case V4L2_EVENT_CTRL: > > return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops); > > + case V4L2_EVENT_BUF_QUEUED: > > + return v4l2_event_subscribe(fh, sub, 0, NULL); > > + } > > return -EINVAL; > > } > > EXPORT_SYMBOL(v4l2_ctrl_subscribe_event); > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > > index 29aa9d4..00d9c35 100644 > > --- a/drivers/media/v4l2-core/videobuf2-core.c > > +++ b/drivers/media/v4l2-core/videobuf2-core.c > > @@ -25,6 +25,7 @@ > > #include <linux/kthread.h> > > > > #include <media/videobuf2-core.h> > > +#include <media/v4l2-event.h> > > #include <media/v4l2-mc.h> > > > > #include <trace/events/vb2.h> > > @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb) > > return ret; > > } > > > > +static void vb2_buffer_queued_event(struct vb2_buffer *vb) > > +{ > > + struct video_device *vdev = to_video_device(vb->vb2_queue->dev); > > + struct v4l2_event event; > > + > > + memset(&event, 0, sizeof(event)); > > + event.type = V4L2_EVENT_BUF_QUEUED; > > + event.u.buf_queued.index = vb->index; > > + > > + v4l2_event_queue(vdev, &event); > > +} > > + > > It doesn't sound right to add a V4L2 event to VB2 core. The hole point > of splitting the core from V4L2 specific stuff is to allow VB2 to be > used by non-V4L2 APIs[1]. Please move this to videobuf2-v4l2. Yes, that makes sense. My lack of understanding of v4l2 core didn't me allow realize that. I'll move it to videobuf2-v4l2. Gustavo
On 06/16/17 09:39, Gustavo Padovan wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.com> > > Implement the needed pieces to let userspace subscribe for > V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the > DQEVENT ioctl. > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com> > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 6 +++++- > drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++ > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 5aed7bd..f55b5da 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -3435,8 +3435,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status); > int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh, > const struct v4l2_event_subscription *sub) > { > - if (sub->type == V4L2_EVENT_CTRL) > + switch (sub->type) { > + case V4L2_EVENT_CTRL: > return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops); > + case V4L2_EVENT_BUF_QUEUED: > + return v4l2_event_subscribe(fh, sub, 0, NULL); This is dangerous. The '0' argument will only allocate room for a single BUF_QUEUED event. So if two such events are triggered without the application reading the first event, then the first event will be lost. I recommend VIDEO_MAX_FRAME instead. I.e. have room for up to the maximum number of buffers. > + } > return -EINVAL; > } > EXPORT_SYMBOL(v4l2_ctrl_subscribe_event); > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 29aa9d4..00d9c35 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -25,6 +25,7 @@ > #include <linux/kthread.h> > > #include <media/videobuf2-core.h> > +#include <media/v4l2-event.h> > #include <media/v4l2-mc.h> > > #include <trace/events/vb2.h> > @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb) > return ret; > } > > +static void vb2_buffer_queued_event(struct vb2_buffer *vb) > +{ > + struct video_device *vdev = to_video_device(vb->vb2_queue->dev); > + struct v4l2_event event; > + > + memset(&event, 0, sizeof(event)); > + event.type = V4L2_EVENT_BUF_QUEUED; > + event.u.buf_queued.index = vb->index; > + > + v4l2_event_queue(vdev, &event); > +} > + > /** > * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing > */ > @@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb) > trace_vb2_buf_queue(q, vb); > > call_void_vb_qop(vb, buf_queue, vb); > + > + vb2_buffer_queued_event(vb); > } > > static int __buf_prepare(struct vb2_buffer *vb, const void *pb) > Regards, Hans
On 06/30/17 14:04, Mauro Carvalho Chehab wrote: > Em Fri, 16 Jun 2017 16:39:10 +0900 > Gustavo Padovan <gustavo@padovan.org> escreveu: > >> From: Gustavo Padovan <gustavo.padovan@collabora.com> >> >> Implement the needed pieces to let userspace subscribe for >> V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the >> DQEVENT ioctl. >> >> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com> >> --- >> drivers/media/v4l2-core/v4l2-ctrls.c | 6 +++++- >> drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++ >> 2 files changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c >> index 5aed7bd..f55b5da 100644 >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >> @@ -3435,8 +3435,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status); >> int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh, >> const struct v4l2_event_subscription *sub) >> { >> - if (sub->type == V4L2_EVENT_CTRL) >> + switch (sub->type) { >> + case V4L2_EVENT_CTRL: >> return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops); >> + case V4L2_EVENT_BUF_QUEUED: >> + return v4l2_event_subscribe(fh, sub, 0, NULL); >> + } >> return -EINVAL; >> } >> EXPORT_SYMBOL(v4l2_ctrl_subscribe_event); >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c >> index 29aa9d4..00d9c35 100644 >> --- a/drivers/media/v4l2-core/videobuf2-core.c >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >> @@ -25,6 +25,7 @@ >> #include <linux/kthread.h> >> >> #include <media/videobuf2-core.h> >> +#include <media/v4l2-event.h> >> #include <media/v4l2-mc.h> >> >> #include <trace/events/vb2.h> >> @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb) >> return ret; >> } >> >> +static void vb2_buffer_queued_event(struct vb2_buffer *vb) >> +{ >> + struct video_device *vdev = to_video_device(vb->vb2_queue->dev); >> + struct v4l2_event event; >> + >> + memset(&event, 0, sizeof(event)); >> + event.type = V4L2_EVENT_BUF_QUEUED; >> + event.u.buf_queued.index = vb->index; >> + >> + v4l2_event_queue(vdev, &event); >> +} >> + > > It doesn't sound right to add a V4L2 event to VB2 core. The hole point > of splitting the core from V4L2 specific stuff is to allow VB2 to be > used by non-V4L2 APIs[1]. Please move this to videobuf2-v4l2. Good point. So this should be a callback to the higher level. One thing I was wondering about: v4l2_event_queue sends the event to all open filehandles of the video node that subscribed to this event. Is that what we want? Or should we use v4l2_event_queue_fh to only send it to the vb2 queue owner? I don't know what is best. I think it is OK to send it to anyone that is interested. If nothing else it will help debugging. Regards, Hans > > [1] The split happened as part of a patchset meant to make the DVB > core to use VB2 and provide DMA APIs to it. Unfortunately, the > developer that worked on this project moved to some other project. > The final patch was not applied yet. I have it on my patchwork > queue. I intend to test and apply it sometime this year. > > > >> /** >> * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing >> */ >> @@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb) >> trace_vb2_buf_queue(q, vb); >> >> call_void_vb_qop(vb, buf_queue, vb); >> + >> + vb2_buffer_queued_event(vb); >> } >> >> static int __buf_prepare(struct vb2_buffer *vb, const void *pb) > >
2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>: > On 06/30/17 14:04, Mauro Carvalho Chehab wrote: > > Em Fri, 16 Jun 2017 16:39:10 +0900 > > Gustavo Padovan <gustavo@padovan.org> escreveu: > > > >> From: Gustavo Padovan <gustavo.padovan@collabora.com> > >> > >> Implement the needed pieces to let userspace subscribe for > >> V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the > >> DQEVENT ioctl. > >> > >> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com> > >> --- > >> drivers/media/v4l2-core/v4l2-ctrls.c | 6 +++++- > >> drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++ > >> 2 files changed, 20 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > >> index 5aed7bd..f55b5da 100644 > >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c > >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > >> @@ -3435,8 +3435,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status); > >> int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh, > >> const struct v4l2_event_subscription *sub) > >> { > >> - if (sub->type == V4L2_EVENT_CTRL) > >> + switch (sub->type) { > >> + case V4L2_EVENT_CTRL: > >> return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops); > >> + case V4L2_EVENT_BUF_QUEUED: > >> + return v4l2_event_subscribe(fh, sub, 0, NULL); > >> + } > >> return -EINVAL; > >> } > >> EXPORT_SYMBOL(v4l2_ctrl_subscribe_event); > >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > >> index 29aa9d4..00d9c35 100644 > >> --- a/drivers/media/v4l2-core/videobuf2-core.c > >> +++ b/drivers/media/v4l2-core/videobuf2-core.c > >> @@ -25,6 +25,7 @@ > >> #include <linux/kthread.h> > >> > >> #include <media/videobuf2-core.h> > >> +#include <media/v4l2-event.h> > >> #include <media/v4l2-mc.h> > >> > >> #include <trace/events/vb2.h> > >> @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb) > >> return ret; > >> } > >> > >> +static void vb2_buffer_queued_event(struct vb2_buffer *vb) > >> +{ > >> + struct video_device *vdev = to_video_device(vb->vb2_queue->dev); > >> + struct v4l2_event event; > >> + > >> + memset(&event, 0, sizeof(event)); > >> + event.type = V4L2_EVENT_BUF_QUEUED; > >> + event.u.buf_queued.index = vb->index; > >> + > >> + v4l2_event_queue(vdev, &event); > >> +} > >> + > > > > It doesn't sound right to add a V4L2 event to VB2 core. The hole point > > of splitting the core from V4L2 specific stuff is to allow VB2 to be > > used by non-V4L2 APIs[1]. Please move this to videobuf2-v4l2. > > Good point. So this should be a callback to the higher level. > > One thing I was wondering about: v4l2_event_queue sends the event to all > open filehandles of the video node that subscribed to this event. Is that > what we want? Or should we use v4l2_event_queue_fh to only send it to the > vb2 queue owner? I don't know what is best. I think it is OK to send it > to anyone that is interested. If nothing else it will help debugging. I don't have any preference here, I'll keep it as is - sending events to anyone - if no one objects. Gustavo
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 5aed7bd..f55b5da 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -3435,8 +3435,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status); int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh, const struct v4l2_event_subscription *sub) { - if (sub->type == V4L2_EVENT_CTRL) + switch (sub->type) { + case V4L2_EVENT_CTRL: return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops); + case V4L2_EVENT_BUF_QUEUED: + return v4l2_event_subscribe(fh, sub, 0, NULL); + } return -EINVAL; } EXPORT_SYMBOL(v4l2_ctrl_subscribe_event); diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 29aa9d4..00d9c35 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -25,6 +25,7 @@ #include <linux/kthread.h> #include <media/videobuf2-core.h> +#include <media/v4l2-event.h> #include <media/v4l2-mc.h> #include <trace/events/vb2.h> @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb) return ret; } +static void vb2_buffer_queued_event(struct vb2_buffer *vb) +{ + struct video_device *vdev = to_video_device(vb->vb2_queue->dev); + struct v4l2_event event; + + memset(&event, 0, sizeof(event)); + event.type = V4L2_EVENT_BUF_QUEUED; + event.u.buf_queued.index = vb->index; + + v4l2_event_queue(vdev, &event); +} + /** * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing */ @@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb) trace_vb2_buf_queue(q, vb); call_void_vb_qop(vb, buf_queue, vb); + + vb2_buffer_queued_event(vb); } static int __buf_prepare(struct vb2_buffer *vb, const void *pb)