[libcamera-devel,v2,1/6] libcamera: v4l2_device: Add setFd()
diff mbox series

Message ID 20190809150459.14421-2-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • V4L2 M2M Support (+RPi PoC)
Related show

Commit Message

Kieran Bingham Aug. 9, 2019, 3:04 p.m. UTC 
Provide a means for V4L2 device instances to set the fd_ explicitly.
This allows for V4L2Devices to operate when they must use an external
open() call.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/v4l2_device.h |  1 +
 src/libcamera/v4l2_device.cpp       | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

Comments

Laurent Pinchart Aug. 9, 2019, 10:48 p.m. UTC | #1 
Hi Kieran,

Thank you for the patch.

On Fri, Aug 09, 2019 at 04:04:54PM +0100, Kieran Bingham wrote:
> Provide a means for V4L2 device instances to set the fd_ explicitly.
> This allows for V4L2Devices to operate when they must use an external
> open() call.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_device.h |  1 +
>  src/libcamera/v4l2_device.cpp       | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index e7e9829cb05f..7c3af5f6ebe0 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -39,6 +39,7 @@ protected:
>  	int ioctl(unsigned long request, void *argp);
>  
>  	int fd() { return fd_; }
> +	int setFd(int fd);
>  
>  private:
>  	void listControls();
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 9a00566a532b..ba4c0d725752 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -308,6 +308,32 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
>   * \return The V4L2 device file descriptor, -1 if the device node is not open
>   */
>  
> +/**
> + * \brief Set the file descriptor of a V4L2 device
> + * \param[in] fd The file descriptor handle
> + *
> + * This method allows a device to provide an already opened file descriptor
> + * referring to the V4L2 device node, instead of opening it with open(). This
> + * can be used for V4L2 M2M devices where a single video device node is used for
> + * both the output and capture devices, or when receiving an open file
> + * descriptor in a context that doesn't have permission to open the device node
> + * itself.
> + *
> + * This method and the open() method are mutually exclusive, only one of the two
> + * shall be used for a V4L2Device instance.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2Device::setFd(int fd)
> +{
> +	if (fd_ != -1)
> +		return -1;

Just one more comment, would -EBUSY be more appropriate ?

> +
> +	fd_ = fd;
> +
> +	return 0;
> +}
> +
>  /*
>   * \brief List and store information about all controls supported by the
>   * V4L2 device
Kieran Bingham Aug. 12, 2019, 7:20 a.m. UTC | #2 
On 09/08/2019 23:48, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Aug 09, 2019 at 04:04:54PM +0100, Kieran Bingham wrote:
>> Provide a means for V4L2 device instances to set the fd_ explicitly.
>> This allows for V4L2Devices to operate when they must use an external
>> open() call.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  src/libcamera/include/v4l2_device.h |  1 +
>>  src/libcamera/v4l2_device.cpp       | 26 ++++++++++++++++++++++++++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>> index e7e9829cb05f..7c3af5f6ebe0 100644
>> --- a/src/libcamera/include/v4l2_device.h
>> +++ b/src/libcamera/include/v4l2_device.h
>> @@ -39,6 +39,7 @@ protected:
>>  	int ioctl(unsigned long request, void *argp);
>>  
>>  	int fd() { return fd_; }
>> +	int setFd(int fd);
>>  
>>  private:
>>  	void listControls();
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 9a00566a532b..ba4c0d725752 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -308,6 +308,32 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
>>   * \return The V4L2 device file descriptor, -1 if the device node is not open
>>   */
>>  
>> +/**
>> + * \brief Set the file descriptor of a V4L2 device
>> + * \param[in] fd The file descriptor handle
>> + *
>> + * This method allows a device to provide an already opened file descriptor
>> + * referring to the V4L2 device node, instead of opening it with open(). This
>> + * can be used for V4L2 M2M devices where a single video device node is used for
>> + * both the output and capture devices, or when receiving an open file
>> + * descriptor in a context that doesn't have permission to open the device node
>> + * itself.
>> + *
>> + * This method and the open() method are mutually exclusive, only one of the two
>> + * shall be used for a V4L2Device instance.
>> + *
>> + * \return 0 on success or a negative error code otherwise
>> + */
>> +int V4L2Device::setFd(int fd)
>> +{
>> +	if (fd_ != -1)
>> +		return -1;
> 
> Just one more comment, would -EBUSY be more appropriate ?

Yes, probably. I'll update.

> 
>> +
>> +	fd_ = fd;
>> +
>> +	return 0;
>> +}
>> +
>>  /*
>>   * \brief List and store information about all controls supported by the
>>   * V4L2 device
>
Kieran Bingham Aug. 12, 2019, 9:13 a.m. UTC | #3 
Hi Laurent,

On 12/08/2019 08:20, Kieran Bingham wrote:
> 
> 
> On 09/08/2019 23:48, Laurent Pinchart wrote:
>> Hi Kieran,
>>
>> Thank you for the patch.
>>
>> On Fri, Aug 09, 2019 at 04:04:54PM +0100, Kieran Bingham wrote:
>>> Provide a means for V4L2 device instances to set the fd_ explicitly.
>>> This allows for V4L2Devices to operate when they must use an external
>>> open() call.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  src/libcamera/include/v4l2_device.h |  1 +
>>>  src/libcamera/v4l2_device.cpp       | 26 ++++++++++++++++++++++++++
>>>  2 files changed, 27 insertions(+)
>>>
>>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>>> index e7e9829cb05f..7c3af5f6ebe0 100644
>>> --- a/src/libcamera/include/v4l2_device.h
>>> +++ b/src/libcamera/include/v4l2_device.h
>>> @@ -39,6 +39,7 @@ protected:
>>>  	int ioctl(unsigned long request, void *argp);
>>>  
>>>  	int fd() { return fd_; }
>>> +	int setFd(int fd);
>>>  
>>>  private:
>>>  	void listControls();
>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>>> index 9a00566a532b..ba4c0d725752 100644
>>> --- a/src/libcamera/v4l2_device.cpp
>>> +++ b/src/libcamera/v4l2_device.cpp
>>> @@ -308,6 +308,32 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
>>>   * \return The V4L2 device file descriptor, -1 if the device node is not open
>>>   */
>>>  
>>> +/**
>>> + * \brief Set the file descriptor of a V4L2 device
>>> + * \param[in] fd The file descriptor handle
>>> + *
>>> + * This method allows a device to provide an already opened file descriptor
>>> + * referring to the V4L2 device node, instead of opening it with open(). This
>>> + * can be used for V4L2 M2M devices where a single video device node is used for
>>> + * both the output and capture devices, or when receiving an open file
>>> + * descriptor in a context that doesn't have permission to open the device node
>>> + * itself.
>>> + *
>>> + * This method and the open() method are mutually exclusive, only one of the two
>>> + * shall be used for a V4L2Device instance.
>>> + *
>>> + * \return 0 on success or a negative error code otherwise
>>> + */
>>> +int V4L2Device::setFd(int fd)
>>> +{
>>> +	if (fd_ != -1)
>>> +		return -1;
>>
>> Just one more comment, would -EBUSY be more appropriate ?
> 
> Yes, probably. I'll update.

This should really be:

	if (isOpen())
		return -EBUSY;

to match the open() call.

Then I think setFd() should move up in the file to between open() and
close() too.

I'll update for the next version.


>>
>>> +
>>> +	fd_ = fd;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  /*
>>>   * \brief List and store information about all controls supported by the
>>>   * V4L2 device
>>
>
Laurent Pinchart Aug. 12, 2019, 10:06 a.m. UTC | #4 
Hi Kieran,

On Mon, Aug 12, 2019 at 10:13:01AM +0100, Kieran Bingham wrote:
> On 12/08/2019 08:20, Kieran Bingham wrote:
> > On 09/08/2019 23:48, Laurent Pinchart wrote:
> >> On Fri, Aug 09, 2019 at 04:04:54PM +0100, Kieran Bingham wrote:
> >>> Provide a means for V4L2 device instances to set the fd_ explicitly.
> >>> This allows for V4L2Devices to operate when they must use an external
> >>> open() call.
> >>>
> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  src/libcamera/include/v4l2_device.h |  1 +
> >>>  src/libcamera/v4l2_device.cpp       | 26 ++++++++++++++++++++++++++
> >>>  2 files changed, 27 insertions(+)
> >>>
> >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >>> index e7e9829cb05f..7c3af5f6ebe0 100644
> >>> --- a/src/libcamera/include/v4l2_device.h
> >>> +++ b/src/libcamera/include/v4l2_device.h
> >>> @@ -39,6 +39,7 @@ protected:
> >>>  	int ioctl(unsigned long request, void *argp);
> >>>  
> >>>  	int fd() { return fd_; }
> >>> +	int setFd(int fd);
> >>>  
> >>>  private:
> >>>  	void listControls();
> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >>> index 9a00566a532b..ba4c0d725752 100644
> >>> --- a/src/libcamera/v4l2_device.cpp
> >>> +++ b/src/libcamera/v4l2_device.cpp
> >>> @@ -308,6 +308,32 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> >>>   * \return The V4L2 device file descriptor, -1 if the device node is not open
> >>>   */
> >>>  
> >>> +/**
> >>> + * \brief Set the file descriptor of a V4L2 device
> >>> + * \param[in] fd The file descriptor handle
> >>> + *
> >>> + * This method allows a device to provide an already opened file descriptor
> >>> + * referring to the V4L2 device node, instead of opening it with open(). This
> >>> + * can be used for V4L2 M2M devices where a single video device node is used for
> >>> + * both the output and capture devices, or when receiving an open file
> >>> + * descriptor in a context that doesn't have permission to open the device node
> >>> + * itself.
> >>> + *
> >>> + * This method and the open() method are mutually exclusive, only one of the two
> >>> + * shall be used for a V4L2Device instance.
> >>> + *
> >>> + * \return 0 on success or a negative error code otherwise
> >>> + */
> >>> +int V4L2Device::setFd(int fd)
> >>> +{
> >>> +	if (fd_ != -1)
> >>> +		return -1;
> >>
> >> Just one more comment, would -EBUSY be more appropriate ?
> > 
> > Yes, probably. I'll update.
> 
> This should really be:
> 
> 	if (isOpen())
> 		return -EBUSY;
> 
> to match the open() call.
> 
> Then I think setFd() should move up in the file to between open() and
> close() too.

Sounds good to me, you can keep my Rb.

> I'll update for the next version.
> 
> >>> +
> >>> +	fd_ = fd;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>  /*
> >>>   * \brief List and store information about all controls supported by the
> >>>   * V4L2 device

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
index e7e9829cb05f..7c3af5f6ebe0 100644
--- a/src/libcamera/include/v4l2_device.h
+++ b/src/libcamera/include/v4l2_device.h
@@ -39,6 +39,7 @@  protected:
 	int ioctl(unsigned long request, void *argp);
 
 	int fd() { return fd_; }
+	int setFd(int fd);
 
 private:
 	void listControls();
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 9a00566a532b..ba4c0d725752 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -308,6 +308,32 @@  int V4L2Device::ioctl(unsigned long request, void *argp)
  * \return The V4L2 device file descriptor, -1 if the device node is not open
  */
 
+/**
+ * \brief Set the file descriptor of a V4L2 device
+ * \param[in] fd The file descriptor handle
+ *
+ * This method allows a device to provide an already opened file descriptor
+ * referring to the V4L2 device node, instead of opening it with open(). This
+ * can be used for V4L2 M2M devices where a single video device node is used for
+ * both the output and capture devices, or when receiving an open file
+ * descriptor in a context that doesn't have permission to open the device node
+ * itself.
+ *
+ * This method and the open() method are mutually exclusive, only one of the two
+ * shall be used for a V4L2Device instance.
+ *
+ * \return 0 on success or a negative error code otherwise
+ */
+int V4L2Device::setFd(int fd)
+{
+	if (fd_ != -1)
+		return -1;
+
+	fd_ = fd;
+
+	return 0;
+}
+
 /*
  * \brief List and store information about all controls supported by the
  * V4L2 device