[libcamera-devel,03/14] libcamera: v4l2_videodevice: Move FD duplication in V4L2Device::Open
diff mbox series

Message ID 20220908184850.1874303-4-xavier.roumegue@oss.nxp.com
State Changes Requested
Headers show
Series
  • Add dw100 dewarper support to simple/rkisp1 pipeline
Related show

Commit Message

Xavier Roumegue Sept. 8, 2022, 6:48 p.m. UTC 
This ensures that the V4L2Device controls attributes have been enumerated
in its open() method in case of file descriptor duplication.

Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
---
 src/libcamera/v4l2_videodevice.cpp | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

Comments

Jacopo Mondi Sept. 14, 2022, 9:19 a.m. UTC | #1 
Hello Xavier,

On Thu, Sep 08, 2022 at 08:48:39PM +0200, Xavier Roumegue via libcamera-devel wrote:
> This ensures that the V4L2Device controls attributes have been enumerated
> in its open() method in case of file descriptor duplication.
>

Do I get this right or the actual issue is the controls are not
enumerated for M2M devices because V4L2Device::open() (and consequentially
V4L2Device::listControls()) is never called for them ?

If that's the case, I would squash 2 and 3 and clarify this in the
commit message.

> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 955e1508..d0427fef 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -661,17 +661,9 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)
>  {
>  	int ret;
>
> -	UniqueFD newFd = handle.dup();
> -	if (!newFd.isValid()) {
> -		ret = -errno;
> -		LOG(V4L2, Error) << "Failed to duplicate file handle: "
> -				 << strerror(-ret);
> -		return ret;
> -	}
> -
> -	ret = V4L2Device::setFd(std::move(newFd));
> +	ret = V4L2Device::open(handle);
>  	if (ret < 0) {
> -		LOG(V4L2, Error) << "Failed to set file handle: "
> +		LOG(V4L2, Error) << "Failed to open file handle: "
>  				 << strerror(-ret);

V4L2Device::open() already complains out loud in case of failure. I
would drop the error message here.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

>  		return ret;
>  	}
> --
> 2.37.3
>
Laurent Pinchart Oct. 3, 2022, 2:14 p.m. UTC | #2 
On Wed, Sep 14, 2022 at 11:19:01AM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hello Xavier,
> 
> On Thu, Sep 08, 2022 at 08:48:39PM +0200, Xavier Roumegue via libcamera-devel wrote:
> > This ensures that the V4L2Device controls attributes have been enumerated
> > in its open() method in case of file descriptor duplication.
> 
> Do I get this right or the actual issue is the controls are not
> enumerated for M2M devices because V4L2Device::open() (and consequentially
> V4L2Device::listControls()) is never called for them ?
> 
> If that's the case, I would squash 2 and 3 and clarify this in the
> commit message.

I agree. And as setFd() is now used internally only, I would turn it
from a protected to a private function.

I think I would prefer keeping the file handle duplication in
V4L2VideoDevice::open() though. I'll propose a patch.

> > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 955e1508..d0427fef 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -661,17 +661,9 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)
> >  {
> >  	int ret;
> >
> > -	UniqueFD newFd = handle.dup();
> > -	if (!newFd.isValid()) {
> > -		ret = -errno;
> > -		LOG(V4L2, Error) << "Failed to duplicate file handle: "
> > -				 << strerror(-ret);
> > -		return ret;
> > -	}
> > -
> > -	ret = V4L2Device::setFd(std::move(newFd));
> > +	ret = V4L2Device::open(handle);
> >  	if (ret < 0) {
> > -		LOG(V4L2, Error) << "Failed to set file handle: "
> > +		LOG(V4L2, Error) << "Failed to open file handle: "
> >  				 << strerror(-ret);
> 
> V4L2Device::open() already complains out loud in case of failure. I
> would drop the error message here.
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> >  		return ret;
> >  	}
Laurent Pinchart Oct. 3, 2022, 7:07 p.m. UTC | #3 
On Mon, Oct 03, 2022 at 05:14:10PM +0300, Laurent Pinchart via libcamera-devel wrote:
> On Wed, Sep 14, 2022 at 11:19:01AM +0200, Jacopo Mondi via libcamera-devel wrote:
> > Hello Xavier,
> > 
> > On Thu, Sep 08, 2022 at 08:48:39PM +0200, Xavier Roumegue via libcamera-devel wrote:
> > > This ensures that the V4L2Device controls attributes have been enumerated
> > > in its open() method in case of file descriptor duplication.
> > 
> > Do I get this right or the actual issue is the controls are not
> > enumerated for M2M devices because V4L2Device::open() (and consequentially
> > V4L2Device::listControls()) is never called for them ?
> > 
> > If that's the case, I would squash 2 and 3 and clarify this in the
> > commit message.
> 
> I agree. And as setFd() is now used internally only, I would turn it
> from a protected to a private function.
> 
> I think I would prefer keeping the file handle duplication in
> V4L2VideoDevice::open() though. I'll propose a patch.

Done in "[PATCH 0/2] libcamera: v4l2_device: Fix control enumeration for
M2M devices".

> > > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> > > ---
> > >  src/libcamera/v4l2_videodevice.cpp | 12 ++----------
> > >  1 file changed, 2 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 955e1508..d0427fef 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -661,17 +661,9 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)
> > >  {
> > >  	int ret;
> > >
> > > -	UniqueFD newFd = handle.dup();
> > > -	if (!newFd.isValid()) {
> > > -		ret = -errno;
> > > -		LOG(V4L2, Error) << "Failed to duplicate file handle: "
> > > -				 << strerror(-ret);
> > > -		return ret;
> > > -	}
> > > -
> > > -	ret = V4L2Device::setFd(std::move(newFd));
> > > +	ret = V4L2Device::open(handle);
> > >  	if (ret < 0) {
> > > -		LOG(V4L2, Error) << "Failed to set file handle: "
> > > +		LOG(V4L2, Error) << "Failed to open file handle: "
> > >  				 << strerror(-ret);
> > 
> > V4L2Device::open() already complains out loud in case of failure. I
> > would drop the error message here.
> > 
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > >  		return ret;
> > >  	}

Patch
diff mbox series 

diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 955e1508..d0427fef 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -661,17 +661,9 @@  int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)
 {
 	int ret;
 
-	UniqueFD newFd = handle.dup();
-	if (!newFd.isValid()) {
-		ret = -errno;
-		LOG(V4L2, Error) << "Failed to duplicate file handle: "
-				 << strerror(-ret);
-		return ret;
-	}
-
-	ret = V4L2Device::setFd(std::move(newFd));
+	ret = V4L2Device::open(handle);
 	if (ret < 0) {
-		LOG(V4L2, Error) << "Failed to set file handle: "
+		LOG(V4L2, Error) << "Failed to open file handle: "
 				 << strerror(-ret);
 		return ret;
 	}