[libcamera-devel] [PATCH] cam: drm: Skip DRM devices not capable of supporting the atomic API

[Laurent Pinchart]

Hi Jacopo,

On Wed, Sep 28, 2022 at 08:10:02AM +0200, Jacopo Mondi wrote:
> On Wed, Sep 28, 2022 at 02:26:28AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The DRM helper picks the first DRM card that it can open. On platforms
> > that have a standalone GPU, this risks selecting a device corresponding
> > to the GPU instead of the display controller. Fix this by skipping
> > devices that don't support the KMS atomic API. Some legacy display
> > controllers would be skipped as well, but libcamera doesn't run on those
> > systems anyway, and the DRM helper doesn't support the legacy KMS
> > modeset API in any case.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/cam/drm.cpp | 31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > index b0602c942853..b2a6b0bba9e8 100644
> > --- a/src/cam/drm.cpp
> > +++ b/src/cam/drm.cpp
> > @@ -430,7 +430,8 @@ int Device::init()
> >  int Device::openCard()
> >  {
> >  	const std::string dirName = "/dev/dri/";
> > -	int ret = -ENOENT;
> > +	bool found = false;
> > +	int ret;
> >
> >  	/*
> >  	 * Open the first DRM/KMS device beginning with /dev/dri/card. The
> > @@ -449,24 +450,38 @@ int Device::openCard()
> >  	}
> >
> >  	for (struct dirent *res; (res = readdir(folder));) {
> > +		uint64_t cap;
> > +
> >  		if (strncmp(res->d_name, "card", 4))
> >  			continue;
> >
> >  		const std::string devName = dirName + res->d_name;
> >  		fd_ = open(devName.c_str(), O_RDWR | O_CLOEXEC);
> > -		if (fd_ >= 0) {
> > -			ret = 0;
> > -			break;
> > +		if (fd_ < 0) {
> > +			ret = -errno;
> 
> As we don't propagate errors up, can't you use errno directly ?

The first calls to operator<<() may modify the value of errno before the
strerror() call is evaluated. See
https://en.cppreference.com/w/cpp/language/eval_order:

"Order of evaluation of any part of any expression, including order of
evaluation of function arguments is unspecified (with some exceptions
listed below). The compiler can evaluate operands and other
subexpressions in any order, and may choose another order when the same
expression is evaluated again."

> > +			std::cerr << "Failed to open DRM/KMS device " << devName << ": "
> > +				  << strerror(-ret) << std::endl;
> > +			continue;
> >  		}
> >
> > -		ret = -errno;
> > -		std::cerr << "Failed to open DRM/KMS device " << devName << ": "
> > -			  << strerror(-ret) << std::endl;
> > +		/*
> > +		 * Skip devices that don't support the atomic API, to avoid
> > +		 * selecting a DRM device corresponding to a GPU.
> > +		 */
> > +		ret = drmGetCap(fd_, DRM_CLIENT_CAP_ATOMIC, &cap);
> 
> This is the only reference I found of drmGetCap:
> https://patchwork.kernel.org/project/dri-devel/patch/1298251639-21282-1-git-send-email-skeggsb@gmail.com/
> 
> Can it return a negative value ?

https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L1383 for
the implementation of the function, which just calls DRM_IOCTL_GET_CAP.
That ends up in drm_getcap() on the kernel side, which, for
DRM_CLIENT_CAP_ATOMIC, ... works by chance, as DRM_IOCTL_GET_CAP take a
DRM_CAP_* argument, not a DRM_CLIENT_CAP_* argument.
DRM_CLIENT_CAP_ATOMIC is equal to 3, which is
DRM_CAP_DUMB_PREFERRED_DEPTH. I'll fix this to use DRM_CAP_DUMB_BUFFER.
It doesn't matter much what cap we read, the important part is to pick
one that returns an error for non-KMS drivers.

> > +		if (ret < 0) {
> > +			drmClose(fd_);
> > +			fd_ = -1;
> > +			continue;
> > +		}
> > +
> > +		found = true;
> > +		break;
> >  	}
> >
> >  	closedir(folder);
> >
> > -	return ret;
> > +	return found ? 0 : -ENOENT;
> >  }
> >
> >  int Device::getResources()

-- 
Regards,

Laurent Pinchart