| Message ID | 20190122192040.9719-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thanks for your work. On 2019-01-22 19:20:40 +0000, Kieran Bingham wrote: > The capabilities structure from the kernel can return capabilities of the > device, or potentially more specific device capabilities. > > Handle this with an inline function to return the correct value. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > Jacopo, > > Sending this now to see what you think, as it might be useful to consider this > while you're making adjustments in the area. > > Because this code will be inlined, I expect the compiler to optimise sequential > uses of this operation where the underlying capabilities is not modified. > > Kieran > > > src/libcamera/include/v4l2_device.h | 10 +++++++--- > src/libcamera/v4l2_device.cpp | 7 +++++++ > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index b92e1f1c96c3..90d18b9f2260 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -26,10 +26,14 @@ struct V4L2Capability final : v4l2_capability { > { > return reinterpret_cast<const char *>(v4l2_capability::bus_info); > } > + unsigned int caps() const > + { > + return capabilities & V4L2_CAP_DEVICE_CAPS ? device_caps : capabilities; > + } I can't make up my mind if I like this or not :-) I do see the need for it, something in my thinks this should be handled by a macro as to not make it part of the class itself, but maybe that is just a force of habit :-) If you also have considered these things and still feel this is the way to go I won't object. I do however think the name is somewhat lacking if it's to be part of the class, how about spelling it out capabilities() ? > > - bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; } > - bool isOutput() const { return capabilities & V4L2_CAP_VIDEO_OUTPUT; } > - bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; } > + bool isCapture() const { return caps() & V4L2_CAP_VIDEO_CAPTURE; } > + bool isOutput() const { return caps() & V4L2_CAP_VIDEO_OUTPUT; } > + bool hasStreaming() const { return caps() & V4L2_CAP_STREAMING; } > }; > > class MediaEntity; > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 2b17fa1eb0e8..4d1cc5b6070f 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -49,6 +49,13 @@ LOG_DEFINE_CATEGORY(V4L2) > * \return The string containing the device location > */ > > +/** > + * \fn unsigned int V4L2Capability::caps() > + * \brief Retrieve the capabilities of the device > + * \return The device specific capabilities if V4L2_CAP_DEVICE_CAPS is set or > + * driver capabilities otherwise > + */ > + > /** > * \fn bool V4L2Capability::isCapture() > * \brief Identify if the device is capable of capturing video > -- > 2.17.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On 22/01/2019 23:59, Niklas Söderlund wrote: > Hi Kieran, > > Thanks for your work. > > On 2019-01-22 19:20:40 +0000, Kieran Bingham wrote: >> The capabilities structure from the kernel can return capabilities of the >> device, or potentially more specific device capabilities. >> >> Handle this with an inline function to return the correct value. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> Jacopo, >> >> Sending this now to see what you think, as it might be useful to consider this >> while you're making adjustments in the area. >> >> Because this code will be inlined, I expect the compiler to optimise sequential >> uses of this operation where the underlying capabilities is not modified. >> >> Kieran >> >> >> src/libcamera/include/v4l2_device.h | 10 +++++++--- >> src/libcamera/v4l2_device.cpp | 7 +++++++ >> 2 files changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h >> index b92e1f1c96c3..90d18b9f2260 100644 >> --- a/src/libcamera/include/v4l2_device.h >> +++ b/src/libcamera/include/v4l2_device.h >> @@ -26,10 +26,14 @@ struct V4L2Capability final : v4l2_capability { >> { >> return reinterpret_cast<const char *>(v4l2_capability::bus_info); >> } >> + unsigned int caps() const >> + { >> + return capabilities & V4L2_CAP_DEVICE_CAPS ? device_caps : capabilities; >> + } > > I can't make up my mind if I like this or not :-) > > I do see the need for it, something in my thinks this should be handled > by a macro as to not make it part of the class itself, but maybe that is > just a force of habit :-) Why would it not be part of the class (struct)? The purpose of creating the struct V4L2Capabilities was to group all related code and helpers together in the namespace which applies to the object... > If you also have considered these things and still feel this is the way > to go I won't object. I do however think the name is somewhat lacking if > it's to be part of the class, how about spelling it out capabilities() ? I didn't want to override the existing capabilities variable. 'caps()' is expressed from V4L2_CAP_DEVICE_"CAPS" Equally, I don't want to name it 'device_caps()' because that would also conflict in the naming, but that would be more appropriate than 'capabilities()'. It could be marked 'private' and only used for these inline helpers, but I don't see any harm in keeping it public so that anyone accessing the structure directly can also access the device specific 'caps' if relevant. It's also shorter :-) You know what - now I've tried it locally - it does sort of make sense (device_caps()). These helpers want to parse the device_caps really, and if the V4L2_CAP_DEVICE_CAPS field is not set, then we fall back to the driver capabilities. v2 to post... >> - bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; } >> - bool isOutput() const { return capabilities & V4L2_CAP_VIDEO_OUTPUT; } >> - bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; } >> + bool isCapture() const { return caps() & V4L2_CAP_VIDEO_CAPTURE; } >> + bool isOutput() const { return caps() & V4L2_CAP_VIDEO_OUTPUT; } >> + bool hasStreaming() const { return caps() & V4L2_CAP_STREAMING; } >> }; >> >> class MediaEntity; >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >> index 2b17fa1eb0e8..4d1cc5b6070f 100644 >> --- a/src/libcamera/v4l2_device.cpp >> +++ b/src/libcamera/v4l2_device.cpp >> @@ -49,6 +49,13 @@ LOG_DEFINE_CATEGORY(V4L2) >> * \return The string containing the device location >> */ >> >> +/** >> + * \fn unsigned int V4L2Capability::caps() >> + * \brief Retrieve the capabilities of the device >> + * \return The device specific capabilities if V4L2_CAP_DEVICE_CAPS is set or >> + * driver capabilities otherwise >> + */ >> + >> /** >> * \fn bool V4L2Capability::isCapture() >> * \brief Identify if the device is capable of capturing video >> -- >> 2.17.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h index b92e1f1c96c3..90d18b9f2260 100644 --- a/src/libcamera/include/v4l2_device.h +++ b/src/libcamera/include/v4l2_device.h @@ -26,10 +26,14 @@ struct V4L2Capability final : v4l2_capability { { return reinterpret_cast<const char *>(v4l2_capability::bus_info); } + unsigned int caps() const + { + return capabilities & V4L2_CAP_DEVICE_CAPS ? device_caps : capabilities; + } - bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; } - bool isOutput() const { return capabilities & V4L2_CAP_VIDEO_OUTPUT; } - bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; } + bool isCapture() const { return caps() & V4L2_CAP_VIDEO_CAPTURE; } + bool isOutput() const { return caps() & V4L2_CAP_VIDEO_OUTPUT; } + bool hasStreaming() const { return caps() & V4L2_CAP_STREAMING; } }; class MediaEntity; diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 2b17fa1eb0e8..4d1cc5b6070f 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -49,6 +49,13 @@ LOG_DEFINE_CATEGORY(V4L2) * \return The string containing the device location */ +/** + * \fn unsigned int V4L2Capability::caps() + * \brief Retrieve the capabilities of the device + * \return The device specific capabilities if V4L2_CAP_DEVICE_CAPS is set or + * driver capabilities otherwise + */ + /** * \fn bool V4L2Capability::isCapture() * \brief Identify if the device is capable of capturing video
The capabilities structure from the kernel can return capabilities of the device, or potentially more specific device capabilities. Handle this with an inline function to return the correct value. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- Jacopo, Sending this now to see what you think, as it might be useful to consider this while you're making adjustments in the area. Because this code will be inlined, I expect the compiler to optimise sequential uses of this operation where the underlying capabilities is not modified. Kieran src/libcamera/include/v4l2_device.h | 10 +++++++--- src/libcamera/v4l2_device.cpp | 7 +++++++ 2 files changed, 14 insertions(+), 3 deletions(-)