[libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards other than 0
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jun 3 22:58:45 CEST 2022
Hello,
On Fri, Jun 03, 2022 at 08:36:33AM +0200, Jacopo Mondi wrote:
> On Thu, Jun 02, 2022 at 02:50:33PM +0100, Eric Curtin wrote:
> > On Thu, 2 Jun 2022 at 09:41, Jacopo Mondi wrote:
> > > On Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote:
> > > > On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi wrote:
> > > > > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi wrote:
> > > > > > > On Wed, Jun 01, 2022 at 04:23:45PM +0100, Eric Curtin via libcamera-devel 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>
> > > > > > > > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > > > > > > > ---
> > > > > > > > src/cam/drm.cpp | 37 +++++++++++++++++++++++++++----------
> > > > > > > > 1 file changed, 27 insertions(+), 10 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > > > > > > > index 42c5a3b1..5a322819 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>
> > > > > > > > @@ -393,8 +394,10 @@ Device::~Device()
> > > > > > > >
> > > > > > > > int Device::init()
> > > > > > > > {
> > > > > > > > - constexpr size_t NODE_NAME_MAX = sizeof("/dev/dri/card255");
> > > > > > > > - char name[NODE_NAME_MAX];
> > > > > > > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/");
> > > > > > > > + constexpr size_t BASE_NAME_MAX = sizeof("card255");
> > > > > > > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;
> > > > > > > > + char name[NODE_NAME_MAX] = "/dev/dri/";
> > > > > > > > int ret;
> > > > > > > >
> > > > > > > > /*
> > > > > > > > @@ -404,14 +407,28 @@ int Device::init()
> > > > > > > > * 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) {
> > > > > > > > - ret = -errno;
> > > > > > > > - std::cerr
> > > > > > > > - << "Failed to open DRM/KMS device " << name << ": "
> > > > > > > > - << strerror(-ret) << std::endl;
> > > > > > > > - return ret;
> > > > > > > > + DIR *folder = opendir(name);
> > > > > > > > + if (folder) {
> > > > > > > > + for (struct dirent *res; (res = readdir(folder));) {
> > > > > > > > + if (strlen(res->d_name) > 4 &&
> > > > > > >
> > > > > > > I feel this might be a bit simplified, maybe using std::filesystem
> > > > > >
> > > > > > If ultimately demanded or required, I'll change to std::filesystem, a
> > > > > > quick grep through the codebase shows that we use opendir and other
> > > > > > similar C code in all instances except for one case in the Android
> > > > > > code though. And I would like to keep this code lean and in C if
> > > > > > possible. In fact in V2 I might make this even leaner and just write
> > > > > > the bytes after /dev/dri/card when needed rather than /dev/dri/ and
> > > > > > remove the strlen.
> > > > >
> > > > > We have moved rather recently to C++17 for the internal code, where
> > > > > std::filesystem has been introduced. That's maybe why it's not that
> > > > > used.
> > > >
> > > > I think std::filesystem makes a lot of sense where you have to support
> > > > many platforms, Linux, Windows, MacOS, etc. and the underlying
> > > > implementation of the filesystem calls are different, in the
> > > > multi-platform case I could understand why it doesn't make sense to
> > > > maintain multiple versions for each platform. But when your only
> > > > platform is Linux, I feel like it's easier to call the C code directly
> > > > and you get the added benefit of knowing what kind of open calls, etc.
> > >
> > > What is the benefit in knowing that you use opendir + open plus a
> > > bunch of string comparison API and one memcpy, compared to the C++
> > > stdlib ?
> > >
> > > I see your code doing pretty much canonical things, nothing special
> > > that benefits from knowing exactly what lib C functions are used.
> > >
> > > > are used. I feel you don't gain much with the std::filesystem here,
> > > > it's still just a loop with a string comparison. I think it adds a
> > > > little complexity here even, like when you see:
> > > >
> > > > dir.path().string().c_str()
> > >
> > > So, is
> > >
> > > + constexpr size_t DIR_NAME_MAX = sizeof("/dev/dri/");
> > > + constexpr size_t BASE_NAME_MAX = sizeof("card255");
> > > + constexpr size_t NODE_NAME_MAX = DIR_NAME_MAX + BASE_NAME_MAX - 1;
> > > + char name[NODE_NAME_MAX] = "/dev/dri/";
> > >
> > > + DIR *folder = opendir(name);
> > > + if (folder) {
> > > + for (struct dirent *res; (res = readdir(folder));) {
> > > + if (strlen(res->d_name) > 4 &&
> > > + !strncmp(res->d_name, "card", 4)) {
> > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,
> > > + BASE_NAME_MAX);
> > > +
> > > + fd_ = open(name, O_RDWR | O_CLOEXEC);
> > >
> > > easier to parse than
> >
> > Tbf, I think the std::filesystem examples are primarily easier to
> > parse because of the use of std::string, rather than the use of
> > std::filesystem. std::filesystem results in us replacing:
> >
> > DIR *folder = opendir(name);
> > if (folder) {
> > for (struct dirent *res; (res = readdir(folder));) {
> >
> > ...
> >
> > closedir(folder);
> >
> > with:
> >
> > const std::filesystem::path dri("/dev/dri");
> > for (const auto &dir : std::filesystem::directory_iterator(dri)) {
> >
> > with std::filesystem, it concerns me to read this:
> >
> > https://en.cppreference.com/w/cpp/filesystem/path/path
> >
> > "Exceptions
> >
> > 2, 4-8) May throw implementation-defined exceptions."
> >
> > I'm running this in a scenario where "/dev/dri" is often not available
> > yet (an early starting camera for automotive), I want to know exactly
> > what happens in that case ("man opendir" clearly states it returns
> > null in that case), I think it will throw an exception, but based on
> > the documentation I don't know for sure. Exception handling is slower
> > also. I also don't know if it's path or directory_iterator that opens
> > the directory, that's important so it can be handled correctly.
The C++ filesystem API indeed seems a bit underspecified to me here. I
would in practice expect the only exceptions to be thrown by
path::path() to be std::bad_alloc or exceptions related to character
encoding, which shouldn't be much of a concern. A path object can refer
to a file or directory that doesn't exist, that won't cause the path
constructor to throw an exception.
The directory_iterator() constructor, however, will thrown an exception
if the path doesn't exist or is not a directory. A call to
fs::is_directory() would prevent that.
I think I'm with Jacopo on this, not really due to std::filesystem as
such, but more about usage of std::string. There's nothing
performance-sensitive here, so I'd favour the readability and
maintainability that std::string provides.
This being said, I'll review v3.
> You're right, it would require an
>
> if (!exists(dri))
> return -ENOENT;
>
> before the for loop
>
> > The pre-existing code uses a "char name[NODE_NAME_MAX];" over an
> > std::string, I actually think that's ok in this case, because the API
> > defined that as the max size.
> >
> > >
> > > + const std::filesystem::path dri("/dev/dri");
> > > + for (const auto &dir : std::filesystem::directory_iterator(dri)) {
> > > + const std::string &direntry = dir.path().filename().string();
> > > +
> > > + if (direntry.find("card") == std::string::npos)
> > > + continue;
> > > +
> > > + fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);
> > >
> > > I guess it's a matter of tastes. I generally don't like C++ syntax, but in
> > > this case the benefits are evident. In example reviewing what that memcpy
> > > exactly does (and let alone the fact you have to go through a copy) took me
> > > a few minutes.
> > >
> > > >
> > > > and you just need to pass a simple string in there.
> > > >
> > > > Once you start using a C++ feature you never go back, I always have
> > > > that little fear also.
> > > >
> > >
> > > Where do you have to go back to ?
> > >
> > > Anyway, it's a little patch and it's not worth a long discussion.
> > >
> > > If you understand C and you prefer it, that's fine. But I might have
> > > missed why any of the above arguments is actually relevant.
> > >
> > > Up to you ;)
> > >
> > > > > Up to you. However I don't find the previous version much leaner
> > > > > compared to the version I suggested, at least from a readability point
> > > > > of view. Why would you like to keep this a much as C code as possible
> > > > > if I may ask ?
> > > > >
> > > > > > >
> > > > > > > const std::filesystem::path dri("/dev/dri");
> > > > > > > for (const auto &dir : std::filesystem::directory_iterator(dri)) {
> > > > > > > const std::string &direntry = dir.path().filename().string();
> > > > > > >
> > > > > > > if (direntry.find("card") == std::string::npos)
> > > > > > > continue;
> > > > > > >
> > > > > > > fd_ = open(dir.path().string().c_str(), O_RDWR | O_CLOEXEC);
> > > > > > >
> > > > > > > ...
> > > > > > > }
> > > > > > >
> > > > > > > > + !strncmp(res->d_name, "card", 4)) {
> > > > > > > > + memcpy(name + DIR_NAME_MAX - 1, res->d_name,
> > > > > > > > + BASE_NAME_MAX);
> > > > > > > > + fd_ = open(name, O_RDWR | O_CLOEXEC);
> > > > > > > > + if (fd_ < 0) {
> > > > > > > > + ret = -errno;
> > > > > > > > + std::cerr
> > > > > > > > + << "Failed to open DRM/KMS device "
> > > > > > > > + << name << ": "
> > > > > > > > + << strerror(-ret) << std::endl;
> > > > > > > > + continue;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + closedir(folder);
> > > > > > >
> > > > > > > What if no card is found ?
> > > > > > > Should fd_ be initialized and here checked ?
> > > > > >
> > > > > > Thanks, I need one more fd_ < 0 comparison and return alright. Nice spot!
> > > > > >
> > > > > > > > }
> > > > > > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list