[libcamera-devel] [PATCH] cam: drm: Support /dev/dri cards other than 0
Jacopo Mondi
jacopo at jmondi.org
Fri Jun 3 08:36:33 CEST 2022
Hi Eric
On Thu, Jun 02, 2022 at 02:50:33PM +0100, Eric Curtin wrote:
> On Thu, 2 Jun 2022 at 09:41, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Eric
> >
> > On Thu, Jun 02, 2022 at 09:04:12AM +0100, Eric Curtin wrote:
> > > On Thu, 2 Jun 2022 at 08:07, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > >
> > > > Hi Eric,
> > > >
> > > > On Wed, Jun 01, 2022 at 09:41:53PM +0100, Eric Curtin wrote:
> > > > > On Wed, 1 Jun 2022 at 18:26, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > > > >
> > > > > > Hi Eric
> > > > > >
> > > > > > 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.
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 ?
> > > >
> > > > Thanks
> > > > j
> > > >
> > > > > >
> > > > > > 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!
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > > j
> > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > /*
> > > > > > > --
> > > > > > > 2.35.3
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
More information about the libcamera-devel
mailing list