| Message ID | 20190226162641.12116-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-02-26 17:26:34 +0100, Jacopo Mondi wrote: > Store the media entity backing the V4L2Subdevice and add a deviceName() > method to retrieve the human readable name of the subdevice, which is > created using the name of the associated media entity. > > Use the deviceName() in error messages where appropriate, as the entity > name is sometimes more informative than the device node path. I would keep the device node in the error messages instead of the device name. As a user diagnosing the problem I would find it easier to associate the errno to a device node rather then a name. Also going from the device node to the device name is easier then the reveres using v4l2 tools. Possibly you could print both. With this fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_subdevice.h | 8 +++++--- > src/libcamera/v4l2_subdevice.cpp | 17 ++++++++++++----- > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > index 82fa6685ab52..eac699a06109 100644 > --- a/src/libcamera/include/v4l2_subdevice.h > +++ b/src/libcamera/include/v4l2_subdevice.h > @@ -9,9 +9,10 @@ > > #include <string> > > +#include "media_object.h" > + > namespace libcamera { > > -class MediaEntity; > struct Rectangle; > > struct V4L2SubdeviceFormat { > @@ -31,7 +32,8 @@ public: > bool isOpen() const; > void close(); > > - std::string deviceNode() const { return deviceNode_; } > + std::string deviceNode() const { return entity_->deviceNode(); } > + std::string deviceName() const { return entity_->name(); } > > int setCrop(unsigned int pad, Rectangle *rect); > int setCompose(unsigned int pad, Rectangle *rect); > @@ -43,7 +45,7 @@ private: > int setSelection(unsigned int pad, unsigned int target, > Rectangle *rect); > > - std::string deviceNode_; > + const MediaEntity *entity_; > int fd_; > }; > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index b436f73cc75f..56ecf3851cb0 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -88,7 +88,7 @@ LOG_DEFINE_CATEGORY(V4L2Subdev) > * path > */ > V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) > - : deviceNode_(entity->deviceNode()), fd_(-1) > + : entity_(entity), fd_(-1) > { > } > > @@ -106,11 +106,11 @@ int V4L2Subdevice::open() > return -EBUSY; > } > > - ret = ::open(deviceNode_.c_str(), O_RDWR); > + ret = ::open(deviceNode().c_str(), O_RDWR); > if (ret < 0) { > ret = -errno; > LOG(V4L2Subdev, Error) > - << "Failed to open V4L2 subdevice '" << deviceNode_ > + << "Failed to open V4L2 subdevice '" << deviceNode() > << "': " << strerror(-ret); > return ret; > } > @@ -147,6 +147,13 @@ void V4L2Subdevice::close() > * \return The subdevice's device node system path > */ > > +/** > + * \fn V4L2Subdevice::deviceName() > + * \brief Retrieve the name of the media entity associated with the subdevice > + * > + * \return The name of the media entity the subdevice is associated to > + */ > + > /** > * \brief Set a crop rectangle on one of the V4L2 subdevice pads > * \param[in] pad The 0-indexed pad number the rectangle is to be applied to > @@ -189,7 +196,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format) > ret = -errno; > LOG(V4L2Subdev, Error) > << "Unable to get format on pad " << pad > - << " of " << deviceNode_ << ": " << strerror(-ret); > + << " of " << deviceName() << ": " << strerror(-ret); > return ret; > } > > @@ -255,7 +262,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > ret = -errno; > LOG(V4L2Subdev, Error) > << "Unable to set rectangle " << target << " on pad " > - << pad << " of " << deviceNode_ << ": " > + << pad << " of " << deviceName() << ": " > << strerror(-ret); > return ret; > } > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 26/02/2019 16:26, Jacopo Mondi wrote: > Store the media entity backing the V4L2Subdevice and add a deviceName() > method to retrieve the human readable name of the subdevice, which is > created using the name of the associated media entity. > > Use the deviceName() in error messages where appropriate, as the entity > name is sometimes more informative than the device node path. I did something similar locally in the V4L2Device perhaps I it might be useful in mainline. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_subdevice.h | 8 +++++--- > src/libcamera/v4l2_subdevice.cpp | 17 ++++++++++++----- > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > index 82fa6685ab52..eac699a06109 100644 > --- a/src/libcamera/include/v4l2_subdevice.h > +++ b/src/libcamera/include/v4l2_subdevice.h > @@ -9,9 +9,10 @@ > > #include <string> > > +#include "media_object.h" > + > namespace libcamera { > > -class MediaEntity; > struct Rectangle; > > struct V4L2SubdeviceFormat { > @@ -31,7 +32,8 @@ public: > bool isOpen() const; > void close(); > > - std::string deviceNode() const { return deviceNode_; } > + std::string deviceNode() const { return entity_->deviceNode(); } > + std::string deviceName() const { return entity_->name(); } deviceName() calling name() bothers me a little. But I'm sure it's fine. Perhaps we might do a global s/deviceName()/name()/ or the inverse sometime. > > int setCrop(unsigned int pad, Rectangle *rect); > int setCompose(unsigned int pad, Rectangle *rect); > @@ -43,7 +45,7 @@ private: > int setSelection(unsigned int pad, unsigned int target, > Rectangle *rect); > > - std::string deviceNode_; > + const MediaEntity *entity_; > int fd_; > }; > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index b436f73cc75f..56ecf3851cb0 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -88,7 +88,7 @@ LOG_DEFINE_CATEGORY(V4L2Subdev) > * path > */ > V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) > - : deviceNode_(entity->deviceNode()), fd_(-1) > + : entity_(entity), fd_(-1) > { > } > > @@ -106,11 +106,11 @@ int V4L2Subdevice::open() > return -EBUSY; > } > > - ret = ::open(deviceNode_.c_str(), O_RDWR); > + ret = ::open(deviceNode().c_str(), O_RDWR); > if (ret < 0) { > ret = -errno; > LOG(V4L2Subdev, Error) > - << "Failed to open V4L2 subdevice '" << deviceNode_ > + << "Failed to open V4L2 subdevice '" << deviceNode() > << "': " << strerror(-ret); > return ret; > } > @@ -147,6 +147,13 @@ void V4L2Subdevice::close() > * \return The subdevice's device node system path > */ > > +/** > + * \fn V4L2Subdevice::deviceName() > + * \brief Retrieve the name of the media entity associated with the subdevice > + * > + * \return The name of the media entity the subdevice is associated to > + */ > + > /** > * \brief Set a crop rectangle on one of the V4L2 subdevice pads > * \param[in] pad The 0-indexed pad number the rectangle is to be applied to > @@ -189,7 +196,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format) > ret = -errno; > LOG(V4L2Subdev, Error) > << "Unable to get format on pad " << pad > - << " of " << deviceNode_ << ": " << strerror(-ret); > + << " of " << deviceName() << ": " << strerror(-ret); > return ret; > } > > @@ -255,7 +262,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > ret = -errno; > LOG(V4L2Subdev, Error) > << "Unable to set rectangle " << target << " on pad " > - << pad << " of " << deviceNode_ << ": " > + << pad << " of " << deviceName() << ": "> << strerror(-ret); > return ret; > } >
Hi Niklas, On Tue, Feb 26, 2019 at 08:31:37PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2019-02-26 17:26:34 +0100, Jacopo Mondi wrote: > > Store the media entity backing the V4L2Subdevice and add a deviceName() > > method to retrieve the human readable name of the subdevice, which is > > created using the name of the associated media entity. > > > > Use the deviceName() in error messages where appropriate, as the entity > > name is sometimes more informative than the device node path. > > I would keep the device node in the error messages instead of the device > name. As a user diagnosing the problem I would find it easier to > associate the errno to a device node rather then a name. Also going from > the device node to the device name is easier then the reveres using v4l2 > tools. Possibly you could print both. I would agree if we were talking about video device nodes, but here we're talking about subdevices. It's very easy to go from entities from the devnode path with media-ctl, and subdevices are inspected with usually with media-ctl that presents them by their entities name. Having the crude v4l-subdev path, is much less informative imho. > > With this fixed, > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/v4l2_subdevice.h | 8 +++++--- > > src/libcamera/v4l2_subdevice.cpp | 17 ++++++++++++----- > > 2 files changed, 17 insertions(+), 8 deletions(-) > > > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > > index 82fa6685ab52..eac699a06109 100644 > > --- a/src/libcamera/include/v4l2_subdevice.h > > +++ b/src/libcamera/include/v4l2_subdevice.h > > @@ -9,9 +9,10 @@ > > > > #include <string> > > > > +#include "media_object.h" > > + > > namespace libcamera { > > > > -class MediaEntity; > > struct Rectangle; > > > > struct V4L2SubdeviceFormat { > > @@ -31,7 +32,8 @@ public: > > bool isOpen() const; > > void close(); > > > > - std::string deviceNode() const { return deviceNode_; } > > + std::string deviceNode() const { return entity_->deviceNode(); } > > + std::string deviceName() const { return entity_->name(); } > > > > int setCrop(unsigned int pad, Rectangle *rect); > > int setCompose(unsigned int pad, Rectangle *rect); > > @@ -43,7 +45,7 @@ private: > > int setSelection(unsigned int pad, unsigned int target, > > Rectangle *rect); > > > > - std::string deviceNode_; > > + const MediaEntity *entity_; > > int fd_; > > }; > > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > index b436f73cc75f..56ecf3851cb0 100644 > > --- a/src/libcamera/v4l2_subdevice.cpp > > +++ b/src/libcamera/v4l2_subdevice.cpp > > @@ -88,7 +88,7 @@ LOG_DEFINE_CATEGORY(V4L2Subdev) > > * path > > */ > > V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) > > - : deviceNode_(entity->deviceNode()), fd_(-1) > > + : entity_(entity), fd_(-1) > > { > > } > > > > @@ -106,11 +106,11 @@ int V4L2Subdevice::open() > > return -EBUSY; > > } > > > > - ret = ::open(deviceNode_.c_str(), O_RDWR); > > + ret = ::open(deviceNode().c_str(), O_RDWR); > > if (ret < 0) { > > ret = -errno; > > LOG(V4L2Subdev, Error) > > - << "Failed to open V4L2 subdevice '" << deviceNode_ > > + << "Failed to open V4L2 subdevice '" << deviceNode() > > << "': " << strerror(-ret); > > return ret; > > } > > @@ -147,6 +147,13 @@ void V4L2Subdevice::close() > > * \return The subdevice's device node system path > > */ > > > > +/** > > + * \fn V4L2Subdevice::deviceName() > > + * \brief Retrieve the name of the media entity associated with the subdevice > > + * > > + * \return The name of the media entity the subdevice is associated to > > + */ > > + > > /** > > * \brief Set a crop rectangle on one of the V4L2 subdevice pads > > * \param[in] pad The 0-indexed pad number the rectangle is to be applied to > > @@ -189,7 +196,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format) > > ret = -errno; > > LOG(V4L2Subdev, Error) > > << "Unable to get format on pad " << pad > > - << " of " << deviceNode_ << ": " << strerror(-ret); > > + << " of " << deviceName() << ": " << strerror(-ret); > > return ret; > > } > > > > @@ -255,7 +262,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > > ret = -errno; > > LOG(V4L2Subdev, Error) > > << "Unable to set rectangle " << target << " on pad " > > - << pad << " of " << deviceNode_ << ": " > > + << pad << " of " << deviceName() << ": " > > << strerror(-ret); > > return ret; > > } > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, Thank you for the patch. On Tue, Feb 26, 2019 at 05:26:34PM +0100, Jacopo Mondi wrote: > Store the media entity backing the V4L2Subdevice and add a deviceName() > method to retrieve the human readable name of the subdevice, which is > created using the name of the associated media entity. > > Use the deviceName() in error messages where appropriate, as the entity > name is sometimes more informative than the device node path. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/include/v4l2_subdevice.h | 8 +++++--- > src/libcamera/v4l2_subdevice.cpp | 17 ++++++++++++----- > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > index 82fa6685ab52..eac699a06109 100644 > --- a/src/libcamera/include/v4l2_subdevice.h > +++ b/src/libcamera/include/v4l2_subdevice.h > @@ -9,9 +9,10 @@ > > #include <string> > > +#include "media_object.h" > + > namespace libcamera { > > -class MediaEntity; > struct Rectangle; > > struct V4L2SubdeviceFormat { > @@ -31,7 +32,8 @@ public: > bool isOpen() const; > void close(); > > - std::string deviceNode() const { return deviceNode_; } > + std::string deviceNode() const { return entity_->deviceNode(); } > + std::string deviceName() const { return entity_->name(); } > > int setCrop(unsigned int pad, Rectangle *rect); > int setCompose(unsigned int pad, Rectangle *rect); > @@ -43,7 +45,7 @@ private: > int setSelection(unsigned int pad, unsigned int target, > Rectangle *rect); > > - std::string deviceNode_; > + const MediaEntity *entity_; > int fd_; > }; > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index b436f73cc75f..56ecf3851cb0 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -88,7 +88,7 @@ LOG_DEFINE_CATEGORY(V4L2Subdev) > * path > */ > V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) > - : deviceNode_(entity->deviceNode()), fd_(-1) > + : entity_(entity), fd_(-1) > { > } > > @@ -106,11 +106,11 @@ int V4L2Subdevice::open() > return -EBUSY; > } > > - ret = ::open(deviceNode_.c_str(), O_RDWR); > + ret = ::open(deviceNode().c_str(), O_RDWR); > if (ret < 0) { > ret = -errno; > LOG(V4L2Subdev, Error) > - << "Failed to open V4L2 subdevice '" << deviceNode_ > + << "Failed to open V4L2 subdevice '" << deviceNode() > << "': " << strerror(-ret); > return ret; > } > @@ -147,6 +147,13 @@ void V4L2Subdevice::close() > * \return The subdevice's device node system path > */ > > +/** > + * \fn V4L2Subdevice::deviceName() > + * \brief Retrieve the name of the media entity associated with the subdevice > + * > + * \return The name of the media entity the subdevice is associated to > + */ > + > /** > * \brief Set a crop rectangle on one of the V4L2 subdevice pads > * \param[in] pad The 0-indexed pad number the rectangle is to be applied to > @@ -189,7 +196,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format) > ret = -errno; > LOG(V4L2Subdev, Error) > << "Unable to get format on pad " << pad > - << " of " << deviceNode_ << ": " << strerror(-ret); > + << " of " << deviceName() << ": " << strerror(-ret); > return ret; > } > > @@ -255,7 +262,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > ret = -errno; > LOG(V4L2Subdev, Error) > << "Unable to set rectangle " << target << " on pad " > - << pad << " of " << deviceNode_ << ": " > + << pad << " of " << deviceName() << ": " > << strerror(-ret); > return ret; > }
diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h index 82fa6685ab52..eac699a06109 100644 --- a/src/libcamera/include/v4l2_subdevice.h +++ b/src/libcamera/include/v4l2_subdevice.h @@ -9,9 +9,10 @@ #include <string> +#include "media_object.h" + namespace libcamera { -class MediaEntity; struct Rectangle; struct V4L2SubdeviceFormat { @@ -31,7 +32,8 @@ public: bool isOpen() const; void close(); - std::string deviceNode() const { return deviceNode_; } + std::string deviceNode() const { return entity_->deviceNode(); } + std::string deviceName() const { return entity_->name(); } int setCrop(unsigned int pad, Rectangle *rect); int setCompose(unsigned int pad, Rectangle *rect); @@ -43,7 +45,7 @@ private: int setSelection(unsigned int pad, unsigned int target, Rectangle *rect); - std::string deviceNode_; + const MediaEntity *entity_; int fd_; }; diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index b436f73cc75f..56ecf3851cb0 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -88,7 +88,7 @@ LOG_DEFINE_CATEGORY(V4L2Subdev) * path */ V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity) - : deviceNode_(entity->deviceNode()), fd_(-1) + : entity_(entity), fd_(-1) { } @@ -106,11 +106,11 @@ int V4L2Subdevice::open() return -EBUSY; } - ret = ::open(deviceNode_.c_str(), O_RDWR); + ret = ::open(deviceNode().c_str(), O_RDWR); if (ret < 0) { ret = -errno; LOG(V4L2Subdev, Error) - << "Failed to open V4L2 subdevice '" << deviceNode_ + << "Failed to open V4L2 subdevice '" << deviceNode() << "': " << strerror(-ret); return ret; } @@ -147,6 +147,13 @@ void V4L2Subdevice::close() * \return The subdevice's device node system path */ +/** + * \fn V4L2Subdevice::deviceName() + * \brief Retrieve the name of the media entity associated with the subdevice + * + * \return The name of the media entity the subdevice is associated to + */ + /** * \brief Set a crop rectangle on one of the V4L2 subdevice pads * \param[in] pad The 0-indexed pad number the rectangle is to be applied to @@ -189,7 +196,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format) ret = -errno; LOG(V4L2Subdev, Error) << "Unable to get format on pad " << pad - << " of " << deviceNode_ << ": " << strerror(-ret); + << " of " << deviceName() << ": " << strerror(-ret); return ret; } @@ -255,7 +262,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, ret = -errno; LOG(V4L2Subdev, Error) << "Unable to set rectangle " << target << " on pad " - << pad << " of " << deviceNode_ << ": " + << pad << " of " << deviceName() << ": " << strerror(-ret); return ret; }
Store the media entity backing the V4L2Subdevice and add a deviceName() method to retrieve the human readable name of the subdevice, which is created using the name of the associated media entity. Use the deviceName() in error messages where appropriate, as the entity name is sometimes more informative than the device node path. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_subdevice.h | 8 +++++--- src/libcamera/v4l2_subdevice.cpp | 17 ++++++++++++----- 2 files changed, 17 insertions(+), 8 deletions(-)