[libcamera-devel,v2] libcamera: v4l2device: Obtain device capabilities
diff mbox series

Message ID 20190123134530.26993-1-kieran.bingham@ideasonboard.com
State Accepted
Commit e5163f54d47c4f2c167eba5e40f8df62229e8ad5
Headers show
Series
  • [libcamera-devel,v2] libcamera: v4l2device: Obtain device capabilities
Related show

Commit Message

Kieran Bingham Jan. 23, 2019, 1:45 p.m. UTC 
The capabilities structure from the kernel can return capabilities of the
driver, or potentially more specific device capabilities.

Handle this with an inline function 'device_caps()' to return the device
specific capabilities when available, or fall back to the driver capabilities
otherwise.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/include/v4l2_device.h | 12 +++++++++---
 src/libcamera/v4l2_device.cpp       |  7 +++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Niklas Söderlund Jan. 23, 2019, 2:19 p.m. UTC | #1 
Hi Kieran,

Thanks for your work.

On 2019-01-23 13:45:30 +0000, Kieran Bingham wrote:
> The capabilities structure from the kernel can return capabilities of the
> driver, or potentially more specific device capabilities.
> 
> Handle this with an inline function 'device_caps()' to return the device
> specific capabilities when available, or fall back to the driver capabilities
> otherwise.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

I like device_caps() better then caps() :-)

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/include/v4l2_device.h | 12 +++++++++---
>  src/libcamera/v4l2_device.cpp       |  7 +++++++
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index b92e1f1c96c3..cb3601ca070c 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -26,10 +26,16 @@ struct V4L2Capability final : v4l2_capability {
>  	{
>  		return reinterpret_cast<const char *>(v4l2_capability::bus_info);
>  	}
> +	unsigned int device_caps() const
> +	{
> +		return capabilities & V4L2_CAP_DEVICE_CAPS
> +				    ? v4l2_capability::device_caps
> +				    : v4l2_capability::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 device_caps() & V4L2_CAP_VIDEO_CAPTURE; }
> +	bool isOutput() const { return device_caps() & V4L2_CAP_VIDEO_OUTPUT; }
> +	bool hasStreaming() const { return device_caps() & V4L2_CAP_STREAMING; }
>  };
>  
>  class MediaEntity;
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 2b17fa1eb0e8..0ce2b187f2f0 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::device_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.19.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Jan. 23, 2019, 2:21 p.m. UTC | #2 
Hi Niklas,

On 23/01/2019 14:19, Niklas Söderlund wrote:
> Hi Kieran,
> 
> Thanks for your work.
> 
> On 2019-01-23 13:45:30 +0000, Kieran Bingham wrote:
>> The capabilities structure from the kernel can return capabilities of the
>> driver, or potentially more specific device capabilities.
>>
>> Handle this with an inline function 'device_caps()' to return the device
>> specific capabilities when available, or fall back to the driver capabilities
>> otherwise.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> I like device_caps() better then caps() :-)

Thanks,

I won't push this until I hear that Jacopo is aware of it as it will
force a rebase for his patches.

--
Kieran


> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
>> ---
>>  src/libcamera/include/v4l2_device.h | 12 +++++++++---
>>  src/libcamera/v4l2_device.cpp       |  7 +++++++
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>> index b92e1f1c96c3..cb3601ca070c 100644
>> --- a/src/libcamera/include/v4l2_device.h
>> +++ b/src/libcamera/include/v4l2_device.h
>> @@ -26,10 +26,16 @@ struct V4L2Capability final : v4l2_capability {
>>  	{
>>  		return reinterpret_cast<const char *>(v4l2_capability::bus_info);
>>  	}
>> +	unsigned int device_caps() const
>> +	{
>> +		return capabilities & V4L2_CAP_DEVICE_CAPS
>> +				    ? v4l2_capability::device_caps
>> +				    : v4l2_capability::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 device_caps() & V4L2_CAP_VIDEO_CAPTURE; }
>> +	bool isOutput() const { return device_caps() & V4L2_CAP_VIDEO_OUTPUT; }
>> +	bool hasStreaming() const { return device_caps() & V4L2_CAP_STREAMING; }
>>  };
>>  
>>  class MediaEntity;
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 2b17fa1eb0e8..0ce2b187f2f0 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::device_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.19.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>
Jacopo Mondi Jan. 23, 2019, 2:29 p.m. UTC | #3 
Hi,

On Wed, Jan 23, 2019 at 02:21:33PM +0000, Kieran Bingham wrote:
> Hi Niklas,
>
> On 23/01/2019 14:19, Niklas Söderlund wrote:
> > Hi Kieran,
> >
> > Thanks for your work.
> >
> > On 2019-01-23 13:45:30 +0000, Kieran Bingham wrote:
> >> The capabilities structure from the kernel can return capabilities of the
> >> driver, or potentially more specific device capabilities.
> >>
> >> Handle this with an inline function 'device_caps()' to return the device
> >> specific capabilities when available, or fall back to the driver capabilities
> >> otherwise.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > I like device_caps() better then caps() :-)
>
> Thanks,
>
> I won't push this until I hear that Jacopo is aware of it as it will
> force a rebase for his patches.

As 'device_caps' are a very specific type of capabilities, I like
caps() better than device_caps(). That said, please go ahead and push
and thanks for waiting.

Thanks
  j


>
> --
> Kieran
>
>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >
> >> ---
> >>  src/libcamera/include/v4l2_device.h | 12 +++++++++---
> >>  src/libcamera/v4l2_device.cpp       |  7 +++++++
> >>  2 files changed, 16 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >> index b92e1f1c96c3..cb3601ca070c 100644
> >> --- a/src/libcamera/include/v4l2_device.h
> >> +++ b/src/libcamera/include/v4l2_device.h
> >> @@ -26,10 +26,16 @@ struct V4L2Capability final : v4l2_capability {
> >>  	{
> >>  		return reinterpret_cast<const char *>(v4l2_capability::bus_info);
> >>  	}
> >> +	unsigned int device_caps() const
> >> +	{
> >> +		return capabilities & V4L2_CAP_DEVICE_CAPS
> >> +				    ? v4l2_capability::device_caps
> >> +				    : v4l2_capability::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 device_caps() & V4L2_CAP_VIDEO_CAPTURE; }
> >> +	bool isOutput() const { return device_caps() & V4L2_CAP_VIDEO_OUTPUT; }
> >> +	bool hasStreaming() const { return device_caps() & V4L2_CAP_STREAMING; }
> >>  };
> >>
> >>  class MediaEntity;
> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >> index 2b17fa1eb0e8..0ce2b187f2f0 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::device_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.19.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
Kieran Bingham Jan. 23, 2019, 2:35 p.m. UTC | #4 
On 23/01/2019 14:29, Jacopo Mondi wrote:
> Hi,
>>> I like device_caps() better then caps() :-)
>>
>> Thanks,
>>
>> I won't push this until I hear that Jacopo is aware of it as it will
>> force a rebase for his patches.
> 
> As 'device_caps' are a very specific type of capabilities, I like
> caps() better than device_caps(). That said, please go ahead and push
> and thanks for waiting.

Done.

--
Kieran
Laurent Pinchart Jan. 23, 2019, 4:09 p.m. UTC | #5 
Hi Niklas,

Thank you for the patch.

On Wed, Jan 23, 2019 at 01:45:30PM +0000, Kieran Bingham wrote:
> The capabilities structure from the kernel can return capabilities of the
> driver, or potentially more specific device capabilities.
> 
> Handle this with an inline function 'device_caps()' to return the device
> specific capabilities when available, or fall back to the driver capabilities
> otherwise.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/include/v4l2_device.h | 12 +++++++++---
>  src/libcamera/v4l2_device.cpp       |  7 +++++++
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index b92e1f1c96c3..cb3601ca070c 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -26,10 +26,16 @@ struct V4L2Capability final : v4l2_capability {
>  	{
>  		return reinterpret_cast<const char *>(v4l2_capability::bus_info);
>  	}
> +	unsigned int device_caps() const
> +	{
> +		return capabilities & V4L2_CAP_DEVICE_CAPS
> +				    ? v4l2_capability::device_caps
> +				    : v4l2_capability::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 device_caps() & V4L2_CAP_VIDEO_CAPTURE; }
> +	bool isOutput() const { return device_caps() & V4L2_CAP_VIDEO_OUTPUT; }
> +	bool hasStreaming() const { return device_caps() & V4L2_CAP_STREAMING; }

We're probably reaching the limit of what inline functions should do. If
another layer of indirection is needed we may want to de-inline some of
the functions. It's fine for now.

>  };
>  
>  class MediaEntity;
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 2b17fa1eb0e8..0ce2b187f2f0 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::device_caps()
> + * \brief Retrieve the capabilities of the device
> + * \return The device specific capabilities if V4L2_CAP_DEVICE_CAPS is set or
> + * 	   driver capabilities otherwise

No need for special indentation here. This doesn't require a fix in a
follow-up patch, but you could fix it if you touch this code later.

> + */
> +
>  /**
>   * \fn bool V4L2Capability::isCapture()
>   * \brief Identify if the device is capable of capturing video
Laurent Pinchart Jan. 23, 2019, 4:10 p.m. UTC | #6 
On Wed, Jan 23, 2019 at 06:09:09PM +0200, Laurent Pinchart wrote:
> Hi Niklas,

Or maybe Kieran. This clearly shows I should automate name extraction
from the sender :-)

> Thank you for the patch.
> 
> On Wed, Jan 23, 2019 at 01:45:30PM +0000, Kieran Bingham wrote:
> > The capabilities structure from the kernel can return capabilities of the
> > driver, or potentially more specific device capabilities.
> > 
> > Handle this with an inline function 'device_caps()' to return the device
> > specific capabilities when available, or fall back to the driver capabilities
> > otherwise.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/libcamera/include/v4l2_device.h | 12 +++++++++---
> >  src/libcamera/v4l2_device.cpp       |  7 +++++++
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index b92e1f1c96c3..cb3601ca070c 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -26,10 +26,16 @@ struct V4L2Capability final : v4l2_capability {
> >  	{
> >  		return reinterpret_cast<const char *>(v4l2_capability::bus_info);
> >  	}
> > +	unsigned int device_caps() const
> > +	{
> > +		return capabilities & V4L2_CAP_DEVICE_CAPS
> > +				    ? v4l2_capability::device_caps
> > +				    : v4l2_capability::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 device_caps() & V4L2_CAP_VIDEO_CAPTURE; }
> > +	bool isOutput() const { return device_caps() & V4L2_CAP_VIDEO_OUTPUT; }
> > +	bool hasStreaming() const { return device_caps() & V4L2_CAP_STREAMING; }
> 
> We're probably reaching the limit of what inline functions should do. If
> another layer of indirection is needed we may want to de-inline some of
> the functions. It's fine for now.
> 
> >  };
> >  
> >  class MediaEntity;
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 2b17fa1eb0e8..0ce2b187f2f0 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::device_caps()
> > + * \brief Retrieve the capabilities of the device
> > + * \return The device specific capabilities if V4L2_CAP_DEVICE_CAPS is set or
> > + * 	   driver capabilities otherwise
> 
> No need for special indentation here. This doesn't require a fix in a
> follow-up patch, but you could fix it if you touch this code later.
> 
> > + */
> > +
> >  /**
> >   * \fn bool V4L2Capability::isCapture()
> >   * \brief Identify if the device is capable of capturing video

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
index b92e1f1c96c3..cb3601ca070c 100644
--- a/src/libcamera/include/v4l2_device.h
+++ b/src/libcamera/include/v4l2_device.h
@@ -26,10 +26,16 @@  struct V4L2Capability final : v4l2_capability {
 	{
 		return reinterpret_cast<const char *>(v4l2_capability::bus_info);
 	}
+	unsigned int device_caps() const
+	{
+		return capabilities & V4L2_CAP_DEVICE_CAPS
+				    ? v4l2_capability::device_caps
+				    : v4l2_capability::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 device_caps() & V4L2_CAP_VIDEO_CAPTURE; }
+	bool isOutput() const { return device_caps() & V4L2_CAP_VIDEO_OUTPUT; }
+	bool hasStreaming() const { return device_caps() & V4L2_CAP_STREAMING; }
 };
 
 class MediaEntity;
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 2b17fa1eb0e8..0ce2b187f2f0 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::device_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