[libcamera-devel] [PATCH v5] cam: drm: Support /dev/dri cards other than 0
Eric Curtin
ecurtin at redhat.com
Fri Jun 17 10:27:04 CEST 2022
On Fri, 17 Jun 2022 at 07:53, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Eric,
>
> On Thu, Jun 16, 2022 at 10:45:52PM +0100, Eric Curtin wrote:
> > Existing code is hardcoded to card0. Since recent fedora upgrades, we
> > have noticed on more than one machine that card1 is present as the
> > lowest numbered device, could theoretically be higher. This technique
> > tries every file starting with card and continue only when we have
> > successfully opened one. These devices with card1 as the lowest device
> > were simply failing when they do not see a /dev/dri/card0 file present.
> >
> > Reported-by: Ian Mullins <imullins at redhat.com>
> > Tested-by: Ian Mullins <imullins at redhat.com>
> > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > ---
> > Changes in v5:
> > - Split into openCard function
> > - ret initialized to -ENOENT, in case directory is open with no card*
> > - Add another string to simplify
> >
> > Changes in v4:
> > - added Tested-by tag
> > - std::stringified things
> > - changed if conditions to reduce indentations
> > - constexpr sizes now don't include null terminator
> > - just return -ENOENT if no device was successfully opened.
> >
> > Changes in v3:
> > - switch back to memcpy, strncpy causing false compiler issue
> >
> > Changes in v2:
> > - return if no valid card found, or directory could not be opened etc.
> > - memcpy to strncpy for safety
> > - only adjust the numerical bytes for each iteration of loop since
> > /dev/dri/card won't change
> > - initialize ret to zero
> > ---
> > src/cam/drm.cpp | 62 ++++++++++++++++++++++++++++++++++++-------------
> > src/cam/drm.h | 2 +-
> > 2 files changed, 47 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > index 42c5a3b1..39c19aa8 100644
> > --- a/src/cam/drm.cpp
> > +++ b/src/cam/drm.cpp
> > @@ -8,6 +8,7 @@
> > #include "drm.h"
> >
> > #include <algorithm>
> > +#include <dirent.h>
> > #include <errno.h>
> > #include <fcntl.h>
> > #include <iostream>
> > @@ -391,26 +392,55 @@ Device::~Device()
> > drmClose(fd_);
> > }
> >
> > -int Device::init()
> > +int Device::openCard()
> > {
> > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255");
> > - char name[NODE_NAME_MAX];
> > - int ret;
> > + const std::string dirName = "/dev/dri/";
> > + int ret = -ENOENT;
> >
> > /*
> > - * Open the first DRM/KMS device. The libdrm drmOpen*() functions
> > - * require either a module name or a bus ID, which we don't have, so
> > - * bypass them. The automatic module loading and device node creation
> > - * from drmOpen() is of no practical use as any modern system will
> > - * handle that through udev or an equivalent component.
> > - */
> > - snprintf(name, sizeof(name), "/dev/dri/card%u", 0);
> > - fd_ = open(name, O_RDWR | O_CLOEXEC);
> > - if (fd_ < 0) {
> > + * Open the first DRM/KMS device beginning with /dev/dri/card. The
> > + * libdrm drmOpen*() functions require either a module name or a bus ID,
> > + * which we don't have, so bypass them. The automatic module loading and
> > + * device node creation from drmOpen() is of no practical use as any
> > + * modern system will handle that through udev or an equivalent
> > + * component.
> > + */
>
> Weird indentation ?
Yeah you are right, it appeared on my text editor the same as the old
indentation. It's spaces instead of tabs here. It's a pity
clang-format doesn't seem to fix this, nor does checkstyle pick this
up. Have to do it manually, maybe it's because the tools are ignoring
comments indentation, since everything between /* */ is just
beautifying a comment.
>
> > + DIR *folder = opendir(dirName.c_str());
> > + if (!folder) {
> > ret = -errno;
> > - std::cerr
> > - << "Failed to open DRM/KMS device " << name << ": "
> > - << strerror(-ret) << std::endl;
> > + std::cerr << "Failed to open " << dirName
> > + << " directory: " << strerror(-ret) << std::endl;
> > + return ret;
> > + }
> > +
> > + for (struct dirent *res; (res = readdir(folder));) {
> > + if (strncmp(res->d_name, "card", 4)) {
> > + continue;
> > + }
>
> No need for {}. Does checkstyle warn you ?
Nope it doesn't, but I can fix manually.
>
> > +
> > + const std::string devName = dirName + res->d_name;
> > + fd_ = open(devName.c_str(), O_RDWR | O_CLOEXEC);
> > + if (fd_ >= 0) {
> > + ret = 0;
> > + break;
> > + }
> > +
> > + ret = -errno;
> > + std::cerr << "Failed to open DRM/KMS device " << devName << ": "
> > + << strerror(-ret) << std::endl;
> > + }
> > +
> > + closedir(folder);
>
> Looks like fd_ is still not initialized. Maybe it's not an issue
> anymore as you now return earlier in init(), but it would have made the
> code simpler, avoiding you to initialize ret as you could:
>
>
> fd_ = -1;
> for (res; res = readdir(folder)) {
> if (res->d_name != "card)
> continue;
>
> fd_ = open(..);
> if (fd != -1)
> break;
>
> std::cerr << "Failed " ... << strerror(errno);
> }
>
> return fd_ == -1 ? -ENOENT : 0;
>
> Up to you
I could initialize fd_ here also, but it would be superfluous if we
added here (or become superfluous in the constructor if we added
whatever way you want to look at it), I think it's better to have a
single source of truth, if we initialize here, the construct
initialization becomes irrelevant, it may as well be -12312. When a
KMSSink constructor gets called, a Device constructor gets called
(setting fd_ to -1) and dev_.init() is called in the first line of the
KMSSink constructor. In this code, you can pretty much treat
Device::init() and the constructor as one, they get called in quick
succession, but init() gives you a return code which is handy for
bailing out early.
>
> > +
> > + return ret;
> > +}
> > +
> > +int Device::init()
> > +{
> > + int ret = openCard();
> > + if (ret < 0) {
> > + std::cerr << "Failed to open any DRM/KMS device: "
> > + << strerror(-ret) << std::endl;
>
> Duplicated error message ? The openCard() is already verbose enough
> maybe ?
The main reason this error message is here, if there was no message
here. in the case you successfully open /dev/dri/, but there is no
entry in that directory beginning with card*. If we remove this, there
would be no error message in this case.
>
> > return ret;
> > }
> >
> > diff --git a/src/cam/drm.h b/src/cam/drm.h
> > index de57e445..1817d053 100644
> > --- a/src/cam/drm.h
> > +++ b/src/cam/drm.h
> > @@ -312,7 +312,7 @@ private:
> > Device &operator=(const Device &&) = delete;
> >
> > int getResources();
> > -
> > + int openCard();
> > void drmEvent();
> > static void pageFlipComplete(int fd, unsigned int sequence,
> > unsigned int tv_sec, unsigned int tv_usec,
> > --
> > 2.35.3
> >
>
More information about the libcamera-devel
mailing list