[{"id":13603,"web_url":"https://patchwork.libcamera.org/comment/13603/","msgid":"<20201104153324.2hsb5rzfb2hg7tgt@uno.localdomain>","date":"2020-11-04T15:33:24","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: v4l2_device: Move\n\tstart of frame detection to V4L2Device","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Oct 28, 2020 at 02:00:43AM +0100, Niklas Söderlund wrote:\n> The V4L2_EVENT_FRAME_SYNC event may occur on both V4L2 video-devices\n> (V4L2VideoDevice) and sub-devices (V4L2Subdevice). Move the start of\n> frame detection to the common base class of the two, V4L2Device.\n>\n> There is no functional change.\n>\n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Laurent Pinchart \n> ---\n> * Changes since v1\n> - Mark eventAvailable() private instead of protected.\n> - Remove a blank line.\n> - Call setFd() instead of duplicating it.\n> ---\n> include/libcamera/internal/v4l2_device.h | 13 +++-\n> include/libcamera/internal/v4l2_videodevice.h | 8 --\n> src/libcamera/v4l2_device.cpp | 76 ++++++++++++++++++-\n> src/libcamera/v4l2_videodevice.cpp | 75 +-----------------\n> 4 files changed, 87 insertions(+), 85 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 722fb72207d74111..295f08f0be6f1980 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -13,11 +13,15 @@\n>\n> #include \n>\n> +#include \n> +\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/v4l2_controls.h\"\n>\n> namespace libcamera {\n>\n> +class EventNotifier;\n> +\n> class V4L2Device : protected Loggable\n> {\n> public:\n> @@ -34,6 +38,9 @@ public:\n> \tconst std::string &deviceNode() const { return deviceNode_; }\n> \tstd::string devicePath() const;\n>\n> +\tint setFrameStartEnabled(bool enable);\n> +\tSignal frameStart;\n> +\n> protected:\n> \tV4L2Device(const std::string &deviceNode);\n> \t~V4L2Device();\n> @@ -44,18 +51,22 @@ protected:\n> \tint ioctl(unsigned long request, void *argp);\n>\n> \tint fd() { return fd_; }\n> -\n\nIntentional ?\nI see we have an empty line between the different sections in other\nheaders\n\n> private:\n> \tvoid listControls();\n> \tvoid updateControls(ControlList *ctrls,\n> \t\t\t const struct v4l2_ext_control *v4l2Ctrls,\n> \t\t\t unsigned int count);\n>\n> +\tvoid eventAvailable(EventNotifier *notifier);\n> +\n> \tstd::map controlInfo_;\n> \tstd::vector> controlIds_;\n> \tControlInfoMap controls_;\n> \tstd::string deviceNode_;\n> \tint fd_;\n> +\n> +\tEventNotifier *fdEventNotifier_;\n> +\tbool frameStartEnabled_;\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 53f6a2d5515b9bb3..cf705ec9cd6ad118 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -204,9 +204,6 @@ public:\n> \tint queueBuffer(FrameBuffer *buffer);\n> \tSignal bufferReady;\n>\n> -\tint setFrameStartEnabled(bool enable);\n> -\tSignal frameStart;\n> -\n> \tint streamOn();\n> \tint streamOff();\n>\n> @@ -240,8 +237,6 @@ private:\n> \tvoid bufferAvailable(EventNotifier *notifier);\n> \tFrameBuffer *dequeueBuffer();\n>\n> -\tvoid eventAvailable(EventNotifier *notifier);\n> -\n> \tV4L2Capability caps_;\n>\n> \tenum v4l2_buf_type bufferType_;\n> @@ -251,9 +246,6 @@ private:\n> \tstd::map queuedBuffers_;\n>\n> \tEventNotifier *fdBufferNotifier_;\n> -\tEventNotifier *fdEventNotifier_;\n> -\n> -\tbool frameStartEnabled_;\n> };\n>\n> class V4L2M2MDevice\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 31d4dad0ee47b734..36fe62b04003551e 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -16,6 +16,8 @@\n> #include \n> #include \n>\n> +#include \n> +\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/sysfs.h\"\n> #include \"libcamera/internal/utils.h\"\n> @@ -52,7 +54,8 @@ LOG_DEFINE_CATEGORY(V4L2)\n> * at open() time, and the \\a logTag to prefix log messages with.\n> */\n> V4L2Device::V4L2Device(const std::string &deviceNode)\n> -\t: deviceNode_(deviceNode), fd_(-1)\n> +\t: deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),\n> +\t frameStartEnabled_(false)\n> {\n> }\n>\n> @@ -87,7 +90,7 @@ int V4L2Device::open(unsigned int flags)\n> \t\treturn ret;\n> \t}\n>\n> -\tfd_ = ret;\n> +\tsetFd(ret);\n\nIs this related to this patch ?\nAnyway, the change looks good:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> \tlistControls();\n>\n> @@ -117,6 +120,10 @@ int V4L2Device::setFd(int fd)\n>\n> \tfd_ = fd;\n>\n> +\tfdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);\n> +\tfdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);\n> +\tfdEventNotifier_->setEnabled(false);\n> +\n> \treturn 0;\n> }\n>\n> @@ -130,6 +137,8 @@ void V4L2Device::close()\n> \tif (!isOpen())\n> \t\treturn;\n>\n> +\tdelete fdEventNotifier_;\n> +\n> \tif (::close(fd_) < 0)\n> \t\tLOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> \t\t\t\t << strerror(errno);\n> @@ -396,6 +405,40 @@ std::string V4L2Device::devicePath() const\n> \treturn path;\n> }\n>\n> +/**\n> + * \\brief Enable or disable frame start event notification\n> + * \\param[in] enable True to enable frame start events, false to disable them\n> + *\n> + * This function enables or disables generation of frame start events. Once\n> + * enabled, the events are signalled through the frameStart signal.\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + */\n> +int V4L2Device::setFrameStartEnabled(bool enable)\n> +{\n> +\tif (frameStartEnabled_ == enable)\n> +\t\treturn 0;\n> +\n> +\tstruct v4l2_event_subscription event{};\n> +\tevent.type = V4L2_EVENT_FRAME_SYNC;\n> +\n> +\tunsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT\n> +\t\t\t : VIDIOC_UNSUBSCRIBE_EVENT;\n> +\tint ret = ioctl(request, &event);\n> +\tif (enable && ret)\n> +\t\treturn ret;\n> +\n> +\tfdEventNotifier_->setEnabled(enable);\n> +\tframeStartEnabled_ = enable;\n> +\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * \\var V4L2Device::frameStart\n> + * \\brief A Signal emitted when capture of a frame has started\n> + */\n> +\n> /**\n> * \\brief Perform an IOCTL system call on the device node\n> * \\param[in] request The IOCTL request code\n> @@ -519,4 +562,33 @@ void V4L2Device::updateControls(ControlList *ctrls,\n> \t}\n> }\n>\n> +/**\n> + * \\brief Slot to handle V4L2 events from the V4L2 device\n> + * \\param[in] notifier The event notifier\n> + *\n> + * When this slot is called, a V4L2 event is available to be dequeued from the\n> + * device.\n> + */\n> +void V4L2Device::eventAvailable([[maybe_unused]] EventNotifier *notifier)\n> +{\n> +\tstruct v4l2_event event{};\n> +\tint ret = ioctl(VIDIOC_DQEVENT, &event);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Failed to dequeue event, disabling event notifier\";\n> +\t\tfdEventNotifier_->setEnabled(false);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (event.type != V4L2_EVENT_FRAME_SYNC) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Spurious event (\" << event.type\n> +\t\t\t<< \"), disabling event notifier\";\n> +\t\tfdEventNotifier_->setEnabled(false);\n> +\t\treturn;\n> +\t}\n> +\n> +\tframeStart.emit(event.u.frame_sync.frame_sequence);\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 36d7d9a0f27a103f..012d9bc73f30ad09 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -472,8 +472,7 @@ const std::string V4L2DeviceFormat::toString() const\n> * \\param[in] deviceNode The file-system path to the video device node\n> */\n> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> -\t: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr),\n> -\t fdEventNotifier_(nullptr), frameStartEnabled_(false)\n> +\t: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr)\n> {\n> \t/*\n> \t * We default to an MMAP based CAPTURE video device, however this will\n> @@ -566,10 +565,6 @@ int V4L2VideoDevice::open()\n> \tfdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> \tfdBufferNotifier_->setEnabled(false);\n>\n> -\tfdEventNotifier_ = new EventNotifier(fd(), EventNotifier::Exception);\n> -\tfdEventNotifier_->activated.connect(this, &V4L2VideoDevice::eventAvailable);\n> -\tfdEventNotifier_->setEnabled(false);\n> -\n> \tLOG(V4L2, Debug)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> @@ -659,10 +654,6 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> \tfdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> \tfdBufferNotifier_->setEnabled(false);\n>\n> -\tfdEventNotifier_ = new EventNotifier(fd(), EventNotifier::Exception);\n> -\tfdEventNotifier_->activated.connect(this, &V4L2VideoDevice::eventAvailable);\n> -\tfdEventNotifier_->setEnabled(false);\n> -\n> \tLOG(V4L2, Debug)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> @@ -680,7 +671,6 @@ void V4L2VideoDevice::close()\n>\n> \treleaseBuffers();\n> \tdelete fdBufferNotifier_;\n> -\tdelete fdEventNotifier_;\n>\n> \tV4L2Device::close();\n> }\n> @@ -1533,74 +1523,11 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \treturn buffer;\n> }\n>\n> -/**\n> - * \\brief Slot to handle V4L2 events from the V4L2 video device\n> - * \\param[in] notifier The event notifier\n> - *\n> - * When this slot is called, a V4L2 event is available to be dequeued from the\n> - * device.\n> - */\n> -void V4L2VideoDevice::eventAvailable([[maybe_unused]] EventNotifier *notifier)\n> -{\n> -\tstruct v4l2_event event{};\n> -\tint ret = ioctl(VIDIOC_DQEVENT, &event);\n> -\tif (ret < 0) {\n> -\t\tLOG(V4L2, Error)\n> -\t\t\t<< \"Failed to dequeue event, disabling event notifier\";\n> -\t\tfdEventNotifier_->setEnabled(false);\n> -\t\treturn;\n> -\t}\n> -\n> -\tif (event.type != V4L2_EVENT_FRAME_SYNC) {\n> -\t\tLOG(V4L2, Error)\n> -\t\t\t<< \"Spurious event (\" << event.type\n> -\t\t\t<< \"), disabling event notifier\";\n> -\t\tfdEventNotifier_->setEnabled(false);\n> -\t\treturn;\n> -\t}\n> -\n> -\tframeStart.emit(event.u.frame_sync.frame_sequence);\n> -}\n> -\n> /**\n> * \\var V4L2VideoDevice::bufferReady\n> * \\brief A Signal emitted when a framebuffer completes\n> */\n>\n> -/**\n> - * \\brief Enable or disable frame start event notification\n> - * \\param[in] enable True to enable frame start events, false to disable them\n> - *\n> - * This function enables or disables generation of frame start events. Once\n> - * enabled, the events are signalled through the frameStart signal.\n> - *\n> - * \\return 0 on success, a negative error code otherwise\n> - */\n> -int V4L2VideoDevice::setFrameStartEnabled(bool enable)\n> -{\n> -\tif (frameStartEnabled_ == enable)\n> -\t\treturn 0;\n> -\n> -\tstruct v4l2_event_subscription event{};\n> -\tevent.type = V4L2_EVENT_FRAME_SYNC;\n> -\n> -\tunsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT\n> -\t\t\t : VIDIOC_UNSUBSCRIBE_EVENT;\n> -\tint ret = ioctl(request, &event);\n> -\tif (enable && ret)\n> -\t\treturn ret;\n> -\n> -\tfdEventNotifier_->setEnabled(enable);\n> -\tframeStartEnabled_ = enable;\n> -\n> -\treturn ret;\n> -}\n> -\n> -/**\n> - * \\var V4L2VideoDevice::frameStart\n> - * \\brief A Signal emitted when capture of a frame has started\n> - */\n> -\n> /**\n> * \\brief Start the video stream\n> * \\return 0 on success or a negative error code otherwise\n> --\n> 2.29.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 594B0BDB89\n\tfor ;\n\tWed, 4 Nov 2020 15:33:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D70D062C6C;\n\tWed, 4 Nov 2020 16:33:25 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 55D9762B91\n\tfor ;\n\tWed, 4 Nov 2020 16:33:25 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id EB9CE4000A;\n\tWed, 4 Nov 2020 15:33:23 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 4 Nov 2020 16:33:24 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201104153324.2hsb5rzfb2hg7tgt@uno.localdomain>","References":"<20201028010051.3830668-1-niklas.soderlund@ragnatech.se>\n\t<20201028010051.3830668-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201028010051.3830668-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: v4l2_device: Move\n\tstart of frame detection to V4L2Device","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13630,"web_url":"https://patchwork.libcamera.org/comment/13630/","msgid":"<20201106144834.GH3195686@oden.dyn.berto.se>","date":"2020-11-06T14:48:34","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: v4l2_device: Move\n\tstart of frame detection to V4L2Device","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your comments.\n\nOn 2020-11-04 16:33:24 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Oct 28, 2020 at 02:00:43AM +0100, Niklas Söderlund wrote:\n> > The V4L2_EVENT_FRAME_SYNC event may occur on both V4L2 video-devices\n> > (V4L2VideoDevice) and sub-devices (V4L2Subdevice). Move the start of\n> > frame detection to the common base class of the two, V4L2Device.\n> >\n> > There is no functional change.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > * Changes since v1\n> > - Mark eventAvailable() private instead of protected.\n> > - Remove a blank line.\n> > - Call setFd() instead of duplicating it.\n> > ---\n> > include/libcamera/internal/v4l2_device.h | 13 +++-\n> > include/libcamera/internal/v4l2_videodevice.h | 8 --\n> > src/libcamera/v4l2_device.cpp | 76 ++++++++++++++++++-\n> > src/libcamera/v4l2_videodevice.cpp | 75 +-----------------\n> > 4 files changed, 87 insertions(+), 85 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index 722fb72207d74111..295f08f0be6f1980 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -13,11 +13,15 @@\n> >\n> > #include \n> >\n> > +#include \n> > +\n> > #include \"libcamera/internal/log.h\"\n> > #include \"libcamera/internal/v4l2_controls.h\"\n> >\n> > namespace libcamera {\n> >\n> > +class EventNotifier;\n> > +\n> > class V4L2Device : protected Loggable\n> > {\n> > public:\n> > @@ -34,6 +38,9 @@ public:\n> > \tconst std::string &deviceNode() const { return deviceNode_; }\n> > \tstd::string devicePath() const;\n> >\n> > +\tint setFrameStartEnabled(bool enable);\n> > +\tSignal frameStart;\n> > +\n> > protected:\n> > \tV4L2Device(const std::string &deviceNode);\n> > \t~V4L2Device();\n> > @@ -44,18 +51,22 @@ protected:\n> > \tint ioctl(unsigned long request, void *argp);\n> >\n> > \tint fd() { return fd_; }\n> > -\n> \n> Intentional ?\n> I see we have an empty line between the different sections in other\n> headers\n\nNope, this is a typo, will fix.\n\n> \n> > private:\n> > \tvoid listControls();\n> > \tvoid updateControls(ControlList *ctrls,\n> > \t\t\t const struct v4l2_ext_control *v4l2Ctrls,\n> > \t\t\t unsigned int count);\n> >\n> > +\tvoid eventAvailable(EventNotifier *notifier);\n> > +\n> > \tstd::map controlInfo_;\n> > \tstd::vector> controlIds_;\n> > \tControlInfoMap controls_;\n> > \tstd::string deviceNode_;\n> > \tint fd_;\n> > +\n> > +\tEventNotifier *fdEventNotifier_;\n> > +\tbool frameStartEnabled_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index 53f6a2d5515b9bb3..cf705ec9cd6ad118 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -204,9 +204,6 @@ public:\n> > \tint queueBuffer(FrameBuffer *buffer);\n> > \tSignal bufferReady;\n> >\n> > -\tint setFrameStartEnabled(bool enable);\n> > -\tSignal frameStart;\n> > -\n> > \tint streamOn();\n> > \tint streamOff();\n> >\n> > @@ -240,8 +237,6 @@ private:\n> > \tvoid bufferAvailable(EventNotifier *notifier);\n> > \tFrameBuffer *dequeueBuffer();\n> >\n> > -\tvoid eventAvailable(EventNotifier *notifier);\n> > -\n> > \tV4L2Capability caps_;\n> >\n> > \tenum v4l2_buf_type bufferType_;\n> > @@ -251,9 +246,6 @@ private:\n> > \tstd::map queuedBuffers_;\n> >\n> > \tEventNotifier *fdBufferNotifier_;\n> > -\tEventNotifier *fdEventNotifier_;\n> > -\n> > -\tbool frameStartEnabled_;\n> > };\n> >\n> > class V4L2M2MDevice\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 31d4dad0ee47b734..36fe62b04003551e 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -16,6 +16,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > #include \"libcamera/internal/log.h\"\n> > #include \"libcamera/internal/sysfs.h\"\n> > #include \"libcamera/internal/utils.h\"\n> > @@ -52,7 +54,8 @@ LOG_DEFINE_CATEGORY(V4L2)\n> > * at open() time, and the \\a logTag to prefix log messages with.\n> > */\n> > V4L2Device::V4L2Device(const std::string &deviceNode)\n> > -\t: deviceNode_(deviceNode), fd_(-1)\n> > +\t: deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),\n> > +\t frameStartEnabled_(false)\n> > {\n> > }\n> >\n> > @@ -87,7 +90,7 @@ int V4L2Device::open(unsigned int flags)\n> > \t\treturn ret;\n> > \t}\n> >\n> > -\tfd_ = ret;\n> > +\tsetFd(ret);\n> \n> Is this related to this patch ?\n\nYes, as setFd() is extended below to create and enable the notifier we \ncan either duplicate that code here or call setFd().\n\n> Anyway, the change looks good:\n> Reviewed-by: Jacopo Mondi \n\nThanks!\n\n> \n> Thanks\n> j\n> \n> >\n> > \tlistControls();\n> >\n> > @@ -117,6 +120,10 @@ int V4L2Device::setFd(int fd)\n> >\n> > \tfd_ = fd;\n> >\n> > +\tfdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);\n> > +\tfdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);\n> > +\tfdEventNotifier_->setEnabled(false);\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -130,6 +137,8 @@ void V4L2Device::close()\n> > \tif (!isOpen())\n> > \t\treturn;\n> >\n> > +\tdelete fdEventNotifier_;\n> > +\n> > \tif (::close(fd_) < 0)\n> > \t\tLOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> > \t\t\t\t << strerror(errno);\n> > @@ -396,6 +405,40 @@ std::string V4L2Device::devicePath() const\n> > \treturn path;\n> > }\n> >\n> > +/**\n> > + * \\brief Enable or disable frame start event notification\n> > + * \\param[in] enable True to enable frame start events, false to disable them\n> > + *\n> > + * This function enables or disables generation of frame start events. Once\n> > + * enabled, the events are signalled through the frameStart signal.\n> > + *\n> > + * \\return 0 on success, a negative error code otherwise\n> > + */\n> > +int V4L2Device::setFrameStartEnabled(bool enable)\n> > +{\n> > +\tif (frameStartEnabled_ == enable)\n> > +\t\treturn 0;\n> > +\n> > +\tstruct v4l2_event_subscription event{};\n> > +\tevent.type = V4L2_EVENT_FRAME_SYNC;\n> > +\n> > +\tunsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT\n> > +\t\t\t : VIDIOC_UNSUBSCRIBE_EVENT;\n> > +\tint ret = ioctl(request, &event);\n> > +\tif (enable && ret)\n> > +\t\treturn ret;\n> > +\n> > +\tfdEventNotifier_->setEnabled(enable);\n> > +\tframeStartEnabled_ = enable;\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +/**\n> > + * \\var V4L2Device::frameStart\n> > + * \\brief A Signal emitted when capture of a frame has started\n> > + */\n> > +\n> > /**\n> > * \\brief Perform an IOCTL system call on the device node\n> > * \\param[in] request The IOCTL request code\n> > @@ -519,4 +562,33 @@ void V4L2Device::updateControls(ControlList *ctrls,\n> > \t}\n> > }\n> >\n> > +/**\n> > + * \\brief Slot to handle V4L2 events from the V4L2 device\n> > + * \\param[in] notifier The event notifier\n> > + *\n> > + * When this slot is called, a V4L2 event is available to be dequeued from the\n> > + * device.\n> > + */\n> > +void V4L2Device::eventAvailable([[maybe_unused]] EventNotifier *notifier)\n> > +{\n> > +\tstruct v4l2_event event{};\n> > +\tint ret = ioctl(VIDIOC_DQEVENT, &event);\n> > +\tif (ret < 0) {\n> > +\t\tLOG(V4L2, Error)\n> > +\t\t\t<< \"Failed to dequeue event, disabling event notifier\";\n> > +\t\tfdEventNotifier_->setEnabled(false);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tif (event.type != V4L2_EVENT_FRAME_SYNC) {\n> > +\t\tLOG(V4L2, Error)\n> > +\t\t\t<< \"Spurious event (\" << event.type\n> > +\t\t\t<< \"), disabling event notifier\";\n> > +\t\tfdEventNotifier_->setEnabled(false);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tframeStart.emit(event.u.frame_sync.frame_sequence);\n> > +}\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 36d7d9a0f27a103f..012d9bc73f30ad09 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -472,8 +472,7 @@ const std::string V4L2DeviceFormat::toString() const\n> > * \\param[in] deviceNode The file-system path to the video device node\n> > */\n> > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> > -\t: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr),\n> > -\t fdEventNotifier_(nullptr), frameStartEnabled_(false)\n> > +\t: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr)\n> > {\n> > \t/*\n> > \t * We default to an MMAP based CAPTURE video device, however this will\n> > @@ -566,10 +565,6 @@ int V4L2VideoDevice::open()\n> > \tfdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> > \tfdBufferNotifier_->setEnabled(false);\n> >\n> > -\tfdEventNotifier_ = new EventNotifier(fd(), EventNotifier::Exception);\n> > -\tfdEventNotifier_->activated.connect(this, &V4L2VideoDevice::eventAvailable);\n> > -\tfdEventNotifier_->setEnabled(false);\n> > -\n> > \tLOG(V4L2, Debug)\n> > \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> > \t\t<< caps_.driver() << \": \" << caps_.card();\n> > @@ -659,10 +654,6 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> > \tfdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> > \tfdBufferNotifier_->setEnabled(false);\n> >\n> > -\tfdEventNotifier_ = new EventNotifier(fd(), EventNotifier::Exception);\n> > -\tfdEventNotifier_->activated.connect(this, &V4L2VideoDevice::eventAvailable);\n> > -\tfdEventNotifier_->setEnabled(false);\n> > -\n> > \tLOG(V4L2, Debug)\n> > \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> > \t\t<< caps_.driver() << \": \" << caps_.card();\n> > @@ -680,7 +671,6 @@ void V4L2VideoDevice::close()\n> >\n> > \treleaseBuffers();\n> > \tdelete fdBufferNotifier_;\n> > -\tdelete fdEventNotifier_;\n> >\n> > \tV4L2Device::close();\n> > }\n> > @@ -1533,74 +1523,11 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > \treturn buffer;\n> > }\n> >\n> > -/**\n> > - * \\brief Slot to handle V4L2 events from the V4L2 video device\n> > - * \\param[in] notifier The event notifier\n> > - *\n> > - * When this slot is called, a V4L2 event is available to be dequeued from the\n> > - * device.\n> > - */\n> > -void V4L2VideoDevice::eventAvailable([[maybe_unused]] EventNotifier *notifier)\n> > -{\n> > -\tstruct v4l2_event event{};\n> > -\tint ret = ioctl(VIDIOC_DQEVENT, &event);\n> > -\tif (ret < 0) {\n> > -\t\tLOG(V4L2, Error)\n> > -\t\t\t<< \"Failed to dequeue event, disabling event notifier\";\n> > -\t\tfdEventNotifier_->setEnabled(false);\n> > -\t\treturn;\n> > -\t}\n> > -\n> > -\tif (event.type != V4L2_EVENT_FRAME_SYNC) {\n> > -\t\tLOG(V4L2, Error)\n> > -\t\t\t<< \"Spurious event (\" << event.type\n> > -\t\t\t<< \"), disabling event notifier\";\n> > -\t\tfdEventNotifier_->setEnabled(false);\n> > -\t\treturn;\n> > -\t}\n> > -\n> > -\tframeStart.emit(event.u.frame_sync.frame_sequence);\n> > -}\n> > -\n> > /**\n> > * \\var V4L2VideoDevice::bufferReady\n> > * \\brief A Signal emitted when a framebuffer completes\n> > */\n> >\n> > -/**\n> > - * \\brief Enable or disable frame start event notification\n> > - * \\param[in] enable True to enable frame start events, false to disable them\n> > - *\n> > - * This function enables or disables generation of frame start events. Once\n> > - * enabled, the events are signalled through the frameStart signal.\n> > - *\n> > - * \\return 0 on success, a negative error code otherwise\n> > - */\n> > -int V4L2VideoDevice::setFrameStartEnabled(bool enable)\n> > -{\n> > -\tif (frameStartEnabled_ == enable)\n> > -\t\treturn 0;\n> > -\n> > -\tstruct v4l2_event_subscription event{};\n> > -\tevent.type = V4L2_EVENT_FRAME_SYNC;\n> > -\n> > -\tunsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT\n> > -\t\t\t : VIDIOC_UNSUBSCRIBE_EVENT;\n> > -\tint ret = ioctl(request, &event);\n> > -\tif (enable && ret)\n> > -\t\treturn ret;\n> > -\n> > -\tfdEventNotifier_->setEnabled(enable);\n> > -\tframeStartEnabled_ = enable;\n> > -\n> > -\treturn ret;\n> > -}\n> > -\n> > -/**\n> > - * \\var V4L2VideoDevice::frameStart\n> > - * \\brief A Signal emitted when capture of a frame has started\n> > - */\n> > -\n> > /**\n> > * \\brief Start the video stream\n> > * \\return 0 on success or a negative error code otherwise\n> > --\n> > 2.29.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 840ECBE082\n\tfor ;\n\tFri, 6 Nov 2020 14:48:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1792562D2D;\n\tFri, 6 Nov 2020 15:48:38 +0100 (CET)","from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 89CB7622AA\n\tfor ;\n\tFri, 6 Nov 2020 15:48:36 +0100 (CET)","by mail-lf1-x142.google.com with SMTP id b1so2264674lfp.11\n\tfor ;\n\tFri, 06 Nov 2020 06:48:36 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tp12sm189306lfr.66.2020.11.06.06.48.34\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 06 Nov 2020 06:48:34 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"zntmsWan\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=oCRcBYsC1cCKRcfsxDUP+e5f3R8783+Vwp7CR5qy470=;\n\tb=zntmsWanr1ekX2z+AfnxBjU6YYwGZk6X3nbFy24IunFIqgEWFWMy8dQCRRLu41DWqh\n\tNcOOQHFaMQQfjX7QhOrvZbfZZaSE5XT7m6Y4bOSyFO8QOVWLKyHQNAHPzXFxWtviem11\n\t3Oz0ke26wsCWvRd4XSzZEKtmc0T7MxNATUWSh76vkhxavUKStDTMIqNNLelhfs/h2Plt\n\tBGZOoX7OXd3QPXAJYuMbXAUXf/tDL7YLPK1wq3MlXnaQoGoqxCeS6H6rqwDpFGdQ2H7R\n\txmM2DpPLrcUk5TkJoqXRi2uv9oiWpBDgJJslT/G5nX2fXF7BDxw2KpPS1yTEj11Hrrp/\n\tvlFw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=oCRcBYsC1cCKRcfsxDUP+e5f3R8783+Vwp7CR5qy470=;\n\tb=teQ2Gz4TjK1ckmkolfZWAMWpeWU9UNXMWlOo7SFYsUhg/M10KyHRJot0IlySrCB050\n\tCd6HlMk0CXsrlEZzmu1htHwo3cVCF7P/Fzx8fDWI+BfmOetSMjpMN6axs9CsGTizZCyf\n\tT7Ul4KtZp6AnOQPIFyFxug0fcHkRN2sF3KXCm/fmWDWjVAsnvWxnz4Et595uGwaCBX+N\n\tbJXEYdSO94s2+nyNp5SIMfDpD0BQWzeMZN6oiMnePgy6ehQSbphs+rz56l/YwJ+ywahX\n\tHJSCoisCim5NX1ToCExe22P4z8eJbzkVbDrdXcxcWgeJ8gChgb98KwEnott/zJS+hwDm\n\tY7IQ==","X-Gm-Message-State":"AOAM531N9m0R/ELDL67cETQaj6+a+J+HG1XHDORctAs5nT6rDgIyiutF\n\t3OV6wZm0SIyBYb/IGDWpJDuVeQ==","X-Google-Smtp-Source":"ABdhPJwFjzftxM1hcoZIPKJMo40V6sscqQQNT3+ceFkRYix2JRsfB5s+bTMfVLZdGEQlpAZsUIzpYA==","X-Received":"by 2002:a05:6512:1087:: with SMTP id\n\tj7mr994716lfg.122.1604674115809; \n\tFri, 06 Nov 2020 06:48:35 -0800 (PST)","Date":"Fri, 6 Nov 2020 15:48:34 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20201106144834.GH3195686@oden.dyn.berto.se>","References":"<20201028010051.3830668-1-niklas.soderlund@ragnatech.se>\n\t<20201028010051.3830668-2-niklas.soderlund@ragnatech.se>\n\t<20201104153324.2hsb5rzfb2hg7tgt@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201104153324.2hsb5rzfb2hg7tgt@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: v4l2_device: Move\n\tstart of frame detection to V4L2Device","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]