| Message ID | 20190414013506.10515-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Sun, Apr 14, 2019 at 03:34:59AM +0200, Niklas Söderlund wrote: > To gain better control of when a file descriptor is open to the media > device and reduce the work needed in pipeline handler implementations, > handle the file descriptor in acquire() and release(). > > This changes the current behavior where a file descriptor is only open > when requested by the pipeline handler to that one is always open as > long a media device is acquired. This new behavior is desired to allow > implementing exclusive access to a pipeline handler between processes. I think this is fine. It might interfere with power handling on the kernel side, but I don't think the kernel should perform power management on open/close of media devices, so it's an issue that would need to be handled there. > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/include/media_device.h | 2 +- > src/libcamera/media_device.cpp | 33 ++++++++++---------- > src/libcamera/pipeline/ipu3/ipu3.cpp | 25 +-------------- > test/media_device/media_device_link_test.cpp | 7 ----- > test/v4l2_subdevice/v4l2_subdevice_test.cpp | 10 +----- > 5 files changed, 19 insertions(+), 58 deletions(-) > > diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h > index 9f038093b2b22fc7..883985055eb419dd 100644 > --- a/src/libcamera/include/media_device.h > +++ b/src/libcamera/include/media_device.h > @@ -27,7 +27,7 @@ public: > ~MediaDevice(); > > bool acquire(); > - void release() { acquired_ = false; } > + void release(); > bool busy() const { return acquired_; } > > int open(); > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > index 54706bb73a7591d5..644c6143fb15e1fa 100644 > --- a/src/libcamera/media_device.cpp > +++ b/src/libcamera/media_device.cpp > @@ -39,10 +39,9 @@ LOG_DEFINE_CATEGORY(MediaDevice) > * instance. > * > * The instance is created with an empty media graph. Before performing any > - * other operation, it must be opened with the open() function and the media > - * graph populated by calling populate(). Instances of MediaEntity, MediaPad and > - * MediaLink are created to model the media graph, and stored in a map indexed > - * by object id. > + * other operation, it must be populate by calling populate(). Instances of > + * MediaEntity, MediaPad and MediaLink are created to model the media graph, > + * and stored in a map indexed by object id. > * > * The graph is valid once successfully populated, as reported by the valid() > * function. It can be queried to list all entities(), or entities can be > @@ -50,12 +49,6 @@ LOG_DEFINE_CATEGORY(MediaDevice) > * entity to entity through pads and links as exposed by the corresponding > * classes. > * > - * An open media device will keep an open file handle for the underlying media > - * controller device node. It can be closed at any time with a call to close(). > - * This will not invalidate the media graph and all cached media objects remain > - * valid and can be accessed normally. The device can then be later reopened if > - * needed to perform other operations that interact with the device node. > - * > * Media device can be claimed for exclusive use with acquire(), released with > * release() and tested with busy(). This mechanism is aimed at pipeline > * managers to claim media devices they support during enumeration. > @@ -65,8 +58,8 @@ LOG_DEFINE_CATEGORY(MediaDevice) > * \brief Construct a MediaDevice > * \param deviceNode The media device node path > * > - * Once constructed the media device is invalid, and must be opened and > - * populated with open() and populate() before the media graph can be queried. > + * Once constructed the media device is invalid, and must be populated with > + * populate() before the media graph can be queried. > */ > MediaDevice::MediaDevice(const std::string &deviceNode) > : deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false) > @@ -106,15 +99,22 @@ bool MediaDevice::acquire() > if (acquired_) > return false; > > + if (open()) > + return false; > + I think you should document that an acquired media device will keep an fd open until the device is released. > acquired_ = true; > return true; > } > > /** > - * \fn MediaDevice::release() > * \brief Release a device previously claimed for exclusive use > * \sa acquire(), busy() > */ > +void MediaDevice::release() > +{ > + close(); > + acquired_ = false; > +} > > /** > * \fn MediaDevice::busy() > @@ -127,10 +127,6 @@ bool MediaDevice::acquire() > /** > * \brief Open a media device and retrieve device information I think you should remove " and retrieve device information" in the previous patch that moves information retrieval to populate(). > * > - * Before populating the media graph or performing any operation that interact > - * with the device node associated with the media device, the device node must > - * be opened. > - * And maybe removing "populating the media graph or " in a previous patch too ? > * If the device is already open the function returns -EBUSY. > * > * \return 0 on success or a negative error code otherwise > @@ -201,6 +197,9 @@ int MediaDevice::populate() > __u64 version = -1; > int ret; > > + if (fd_ != -1) > + return -EBUSY; > + Can this happen ? If you think the check is useful, I would log an error message, and update the method's documentation. > clear(); > > ret = open(); > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index d8de6b3c36314fc0..a87eff8dfec6d143 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -468,23 +468,10 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > /* > * Disable all links that are enabled by default on CIO2, as camera > * creation enables all valid links it finds. > - * > - * Close the CIO2 media device after, as links are enabled and should > - * not need to be changed after. > */ > - if (cio2MediaDev_->open()) > + if (cio2MediaDev_->disableLinks()) > return false; > > - if (cio2MediaDev_->disableLinks()) { > - cio2MediaDev_->close(); > - return false; > - } > - > - if (imguMediaDev_->open()) { > - cio2MediaDev_->close(); > - return false; > - } > - > /* > * FIXME: enabled links in one ImgU instance interfere with capture > * operations on the other one. This can be easily triggered by > @@ -516,9 +503,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > ret = registerCameras(); > > error: You can remove the error label and return directly instead of using goto. > - cio2MediaDev_->close(); > - imguMediaDev_->close(); > - > return ret == 0; > } > > @@ -961,11 +945,6 @@ int ImgUDevice::enableLinks(bool enable) > std::string inputName = name_ + " input"; > int ret; > > - /* \todo Establish rules to handle media devices open/close. */ > - ret = media_->open(); > - if (ret) > - return ret; > - > ret = linkSetup(inputName, 0, name_, PAD_INPUT, enable); > if (ret) > goto done; > @@ -981,8 +960,6 @@ int ImgUDevice::enableLinks(bool enable) > ret = linkSetup(name_, PAD_STAT, statName, 0, enable); > > done: Same here. > - media_->close(); > - > return ret; > } > > diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp > index 4b8de3bee722e912..3989da43ca9ec30f 100644 > --- a/test/media_device/media_device_link_test.cpp > +++ b/test/media_device/media_device_link_test.cpp > @@ -57,12 +57,6 @@ class MediaDeviceLinkTest : public Test > return TestSkip; > } > > - if (dev_->open()) { > - cerr << "Failed to open media device at " > - << dev_->deviceNode() << endl; > - return TestFail; > - } > - > return 0; > } > > @@ -238,7 +232,6 @@ class MediaDeviceLinkTest : public Test > > void cleanup() > { > - dev_->close(); > dev_->release(); > } > > diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp > index 21335efbad4d5668..31f3465a7ba3a5b1 100644 > --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp > +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp > @@ -51,14 +51,6 @@ int V4L2SubdeviceTest::init() > return TestSkip; > } > > - int ret = media_->open(); > - if (ret) { > - cerr << "Unable to open media device: " << media_->deviceNode() > - << ": " << strerror(ret) << endl; > - media_->release(); > - return TestSkip; > - } > - > MediaEntity *videoEntity = media_->getEntityByName("Scaler"); > if (!videoEntity) { > cerr << "Unable to find media entity 'Scaler'" << endl; > @@ -67,7 +59,7 @@ int V4L2SubdeviceTest::init() > } > > scaler_ = new V4L2Subdevice(videoEntity); > - ret = scaler_->open(); > + int ret = scaler_->open(); > if (ret) { > cerr << "Unable to open video subdevice " > << scaler_->deviceNode() << endl;
diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h index 9f038093b2b22fc7..883985055eb419dd 100644 --- a/src/libcamera/include/media_device.h +++ b/src/libcamera/include/media_device.h @@ -27,7 +27,7 @@ public: ~MediaDevice(); bool acquire(); - void release() { acquired_ = false; } + void release(); bool busy() const { return acquired_; } int open(); diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp index 54706bb73a7591d5..644c6143fb15e1fa 100644 --- a/src/libcamera/media_device.cpp +++ b/src/libcamera/media_device.cpp @@ -39,10 +39,9 @@ LOG_DEFINE_CATEGORY(MediaDevice) * instance. * * The instance is created with an empty media graph. Before performing any - * other operation, it must be opened with the open() function and the media - * graph populated by calling populate(). Instances of MediaEntity, MediaPad and - * MediaLink are created to model the media graph, and stored in a map indexed - * by object id. + * other operation, it must be populate by calling populate(). Instances of + * MediaEntity, MediaPad and MediaLink are created to model the media graph, + * and stored in a map indexed by object id. * * The graph is valid once successfully populated, as reported by the valid() * function. It can be queried to list all entities(), or entities can be @@ -50,12 +49,6 @@ LOG_DEFINE_CATEGORY(MediaDevice) * entity to entity through pads and links as exposed by the corresponding * classes. * - * An open media device will keep an open file handle for the underlying media - * controller device node. It can be closed at any time with a call to close(). - * This will not invalidate the media graph and all cached media objects remain - * valid and can be accessed normally. The device can then be later reopened if - * needed to perform other operations that interact with the device node. - * * Media device can be claimed for exclusive use with acquire(), released with * release() and tested with busy(). This mechanism is aimed at pipeline * managers to claim media devices they support during enumeration. @@ -65,8 +58,8 @@ LOG_DEFINE_CATEGORY(MediaDevice) * \brief Construct a MediaDevice * \param deviceNode The media device node path * - * Once constructed the media device is invalid, and must be opened and - * populated with open() and populate() before the media graph can be queried. + * Once constructed the media device is invalid, and must be populated with + * populate() before the media graph can be queried. */ MediaDevice::MediaDevice(const std::string &deviceNode) : deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false) @@ -106,15 +99,22 @@ bool MediaDevice::acquire() if (acquired_) return false; + if (open()) + return false; + acquired_ = true; return true; } /** - * \fn MediaDevice::release() * \brief Release a device previously claimed for exclusive use * \sa acquire(), busy() */ +void MediaDevice::release() +{ + close(); + acquired_ = false; +} /** * \fn MediaDevice::busy() @@ -127,10 +127,6 @@ bool MediaDevice::acquire() /** * \brief Open a media device and retrieve device information * - * Before populating the media graph or performing any operation that interact - * with the device node associated with the media device, the device node must - * be opened. - * * If the device is already open the function returns -EBUSY. * * \return 0 on success or a negative error code otherwise @@ -201,6 +197,9 @@ int MediaDevice::populate() __u64 version = -1; int ret; + if (fd_ != -1) + return -EBUSY; + clear(); ret = open(); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index d8de6b3c36314fc0..a87eff8dfec6d143 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -468,23 +468,10 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) /* * Disable all links that are enabled by default on CIO2, as camera * creation enables all valid links it finds. - * - * Close the CIO2 media device after, as links are enabled and should - * not need to be changed after. */ - if (cio2MediaDev_->open()) + if (cio2MediaDev_->disableLinks()) return false; - if (cio2MediaDev_->disableLinks()) { - cio2MediaDev_->close(); - return false; - } - - if (imguMediaDev_->open()) { - cio2MediaDev_->close(); - return false; - } - /* * FIXME: enabled links in one ImgU instance interfere with capture * operations on the other one. This can be easily triggered by @@ -516,9 +503,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) ret = registerCameras(); error: - cio2MediaDev_->close(); - imguMediaDev_->close(); - return ret == 0; } @@ -961,11 +945,6 @@ int ImgUDevice::enableLinks(bool enable) std::string inputName = name_ + " input"; int ret; - /* \todo Establish rules to handle media devices open/close. */ - ret = media_->open(); - if (ret) - return ret; - ret = linkSetup(inputName, 0, name_, PAD_INPUT, enable); if (ret) goto done; @@ -981,8 +960,6 @@ int ImgUDevice::enableLinks(bool enable) ret = linkSetup(name_, PAD_STAT, statName, 0, enable); done: - media_->close(); - return ret; } diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp index 4b8de3bee722e912..3989da43ca9ec30f 100644 --- a/test/media_device/media_device_link_test.cpp +++ b/test/media_device/media_device_link_test.cpp @@ -57,12 +57,6 @@ class MediaDeviceLinkTest : public Test return TestSkip; } - if (dev_->open()) { - cerr << "Failed to open media device at " - << dev_->deviceNode() << endl; - return TestFail; - } - return 0; } @@ -238,7 +232,6 @@ class MediaDeviceLinkTest : public Test void cleanup() { - dev_->close(); dev_->release(); } diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp index 21335efbad4d5668..31f3465a7ba3a5b1 100644 --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp @@ -51,14 +51,6 @@ int V4L2SubdeviceTest::init() return TestSkip; } - int ret = media_->open(); - if (ret) { - cerr << "Unable to open media device: " << media_->deviceNode() - << ": " << strerror(ret) << endl; - media_->release(); - return TestSkip; - } - MediaEntity *videoEntity = media_->getEntityByName("Scaler"); if (!videoEntity) { cerr << "Unable to find media entity 'Scaler'" << endl; @@ -67,7 +59,7 @@ int V4L2SubdeviceTest::init() } scaler_ = new V4L2Subdevice(videoEntity); - ret = scaler_->open(); + int ret = scaler_->open(); if (ret) { cerr << "Unable to open video subdevice " << scaler_->deviceNode() << endl;
To gain better control of when a file descriptor is open to the media device and reduce the work needed in pipeline handler implementations, handle the file descriptor in acquire() and release(). This changes the current behavior where a file descriptor is only open when requested by the pipeline handler to that one is always open as long a media device is acquired. This new behavior is desired to allow implementing exclusive access to a pipeline handler between processes. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/include/media_device.h | 2 +- src/libcamera/media_device.cpp | 33 ++++++++++---------- src/libcamera/pipeline/ipu3/ipu3.cpp | 25 +-------------- test/media_device/media_device_link_test.cpp | 7 ----- test/v4l2_subdevice/v4l2_subdevice_test.cpp | 10 +----- 5 files changed, 19 insertions(+), 58 deletions(-)