[libcamera-devel] [PATCH 03/14] libcamera: v4l2_videodevice: Move FD duplication in V4L2Device::Open

[Laurent Pinchart]

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 at 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 at jmondi.org>
> > 
> > >  		return ret;
> > >  	}

-- 
Regards,

Laurent Pinchart