| Message ID | 20250529082326.25156-1-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2025-05-29 09:23:26) > Device::openCard() in the cam DRM helpers looks for a /dev/dri/card* > device that can be opened and that doesn't fail when asked about > DRM_CAP_DUMB_BUFFER capability (regardless whether the capability is > supported by the device). > > There can be matching devices that are not display devices. This can > lead to selection of such a device and inability to use KMS output with > `cam' application. The ultimate goal is to display something on the > device and later the KMS sink will fail if there is no connector > attached to the device (although it can actually fail earlier, when > trying to set DRM_CLIENT_CAP_ATOMIC capability if this is not > supported). > > Let's avoid selecting devices without connectors, CRTCs or encoders. > The added check makes the original check for DRM_CAP_DUMB_BUFFER API > most likely unnecessary, let's remove it. > > Changes in v3: > - Added checks for the presence of CRTCs and encoders. > - Removed the check for DRM_CAP_DUMB_BUFFER. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/apps/cam/drm.cpp | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/src/apps/cam/drm.cpp b/src/apps/cam/drm.cpp > index 47bbb6b05..c40c31410 100644 > --- a/src/apps/cam/drm.cpp > +++ b/src/apps/cam/drm.cpp > @@ -450,8 +450,6 @@ int Device::openCard() > } > > for (struct dirent *res; (res = readdir(folder));) { > - uint64_t cap; > - > if (strncmp(res->d_name, "card", 4)) > continue; > > @@ -464,16 +462,16 @@ int Device::openCard() > continue; > } > > - /* > - * Skip devices that don't support the modeset API, to avoid > - * selecting a DRM device corresponding to a GPU. There is no > - * modeset capability, but the kernel returns an error for most > - * caps if mode setting isn't support by the driver. The > - * DRM_CAP_DUMB_BUFFER capability is one of those, other would > - * do as well. The capability value itself isn't relevant. > - */ > - ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap); > - if (ret < 0) { > + /* Skip devices without connectors. */ This all sounds reasonable to me, though this comment now skips devices without connectors/crts/encoders. Perhaps we should update it while applying? /* Skip non-display devices */ Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + std::unique_ptr<drmModeRes, decltype(&drmModeFreeResources)> resources{ > + drmModeGetResources(fd_), > + &drmModeFreeResources > + }; > + if (!resources || > + resources->count_connectors <= 0 || > + resources->count_crtcs <= 0 || > + resources->count_encoders <= 0) { > + resources.reset(); > drmClose(fd_); > fd_ = -1; > continue; > -- > 2.49.0 >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2025-05-29 09:23:26) >> Device::openCard() in the cam DRM helpers looks for a /dev/dri/card* >> device that can be opened and that doesn't fail when asked about > >> DRM_CAP_DUMB_BUFFER capability (regardless whether the capability is >> supported by the device). >> >> There can be matching devices that are not display devices. This can >> lead to selection of such a device and inability to use KMS output with >> `cam' application. The ultimate goal is to display something on the >> device and later the KMS sink will fail if there is no connector >> attached to the device (although it can actually fail earlier, when >> trying to set DRM_CLIENT_CAP_ATOMIC capability if this is not >> supported). >> >> Let's avoid selecting devices without connectors, CRTCs or encoders. >> The added check makes the original check for DRM_CAP_DUMB_BUFFER API >> most likely unnecessary, let's remove it. >> >> Changes in v3: >> - Added checks for the presence of CRTCs and encoders. >> - Removed the check for DRM_CAP_DUMB_BUFFER. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/apps/cam/drm.cpp | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/src/apps/cam/drm.cpp b/src/apps/cam/drm.cpp >> index 47bbb6b05..c40c31410 100644 >> --- a/src/apps/cam/drm.cpp >> +++ b/src/apps/cam/drm.cpp >> @@ -450,8 +450,6 @@ int Device::openCard() >> } >> >> for (struct dirent *res; (res = readdir(folder));) { >> - uint64_t cap; >> - >> if (strncmp(res->d_name, "card", 4)) >> continue; >> >> @@ -464,16 +462,16 @@ int Device::openCard() >> continue; >> } >> >> - /* >> - * Skip devices that don't support the modeset API, to avoid >> - * selecting a DRM device corresponding to a GPU. There is no >> - * modeset capability, but the kernel returns an error for most >> - * caps if mode setting isn't support by the driver. The >> - * DRM_CAP_DUMB_BUFFER capability is one of those, other would >> - * do as well. The capability value itself isn't relevant. >> - */ >> - ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap); >> - if (ret < 0) { >> + /* Skip devices without connectors. */ > > This all sounds reasonable to me, though this comment now skips devices > without connectors/crts/encoders. > > Perhaps we should update it while applying? > > /* Skip non-display devices */ Ah, yes, please. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> + std::unique_ptr<drmModeRes, decltype(&drmModeFreeResources)> resources{ >> + drmModeGetResources(fd_), >> + &drmModeFreeResources >> + }; >> + if (!resources || >> + resources->count_connectors <= 0 || >> + resources->count_crtcs <= 0 || >> + resources->count_encoders <= 0) { >> + resources.reset(); >> drmClose(fd_); >> fd_ = -1; >> continue; >> -- >> 2.49.0 >>
On jeu., mai 29, 2025 at 15:08, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Quoting Milan Zamazal (2025-05-29 09:23:26) >> Device::openCard() in the cam DRM helpers looks for a /dev/dri/card* >> device that can be opened and that doesn't fail when asked about >> DRM_CAP_DUMB_BUFFER capability (regardless whether the capability is >> supported by the device). >> >> There can be matching devices that are not display devices. This can >> lead to selection of such a device and inability to use KMS output with >> `cam' application. The ultimate goal is to display something on the >> device and later the KMS sink will fail if there is no connector >> attached to the device (although it can actually fail earlier, when >> trying to set DRM_CLIENT_CAP_ATOMIC capability if this is not >> supported). >> >> Let's avoid selecting devices without connectors, CRTCs or encoders. >> The added check makes the original check for DRM_CAP_DUMB_BUFFER API >> most likely unnecessary, let's remove it. >> >> Changes in v3: >> - Added checks for the presence of CRTCs and encoders. >> - Removed the check for DRM_CAP_DUMB_BUFFER. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/apps/cam/drm.cpp | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/src/apps/cam/drm.cpp b/src/apps/cam/drm.cpp >> index 47bbb6b05..c40c31410 100644 >> --- a/src/apps/cam/drm.cpp >> +++ b/src/apps/cam/drm.cpp >> @@ -450,8 +450,6 @@ int Device::openCard() >> } >> >> for (struct dirent *res; (res = readdir(folder));) { >> - uint64_t cap; >> - >> if (strncmp(res->d_name, "card", 4)) >> continue; >> >> @@ -464,16 +462,16 @@ int Device::openCard() >> continue; >> } >> >> - /* >> - * Skip devices that don't support the modeset API, to avoid >> - * selecting a DRM device corresponding to a GPU. There is no >> - * modeset capability, but the kernel returns an error for most >> - * caps if mode setting isn't support by the driver. The >> - * DRM_CAP_DUMB_BUFFER capability is one of those, other would >> - * do as well. The capability value itself isn't relevant. >> - */ >> - ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap); >> - if (ret < 0) { >> + /* Skip devices without connectors. */ > > This all sounds reasonable to me, though this comment now skips devices > without connectors/crts/encoders. > > Perhaps we should update it while applying? > > /* Skip non-display devices */ I agree with the fixup. If it's applied, then: Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> + std::unique_ptr<drmModeRes, decltype(&drmModeFreeResources)> resources{ >> + drmModeGetResources(fd_), >> + &drmModeFreeResources >> + }; >> + if (!resources || >> + resources->count_connectors <= 0 || >> + resources->count_crtcs <= 0 || >> + resources->count_encoders <= 0) { >> + resources.reset(); >> drmClose(fd_); >> fd_ = -1; >> continue; >> -- >> 2.49.0 >>
On Fri, May 30, 2025 at 09:05:41AM +0200, Mattijs Korpershoek wrote: > On jeu., mai 29, 2025 at 15:08, Kieran Bingham wrote: > > Quoting Milan Zamazal (2025-05-29 09:23:26) > >> Device::openCard() in the cam DRM helpers looks for a /dev/dri/card* > >> device that can be opened and that doesn't fail when asked about > >> DRM_CAP_DUMB_BUFFER capability (regardless whether the capability is > >> supported by the device). > >> > >> There can be matching devices that are not display devices. This can > >> lead to selection of such a device and inability to use KMS output with > >> `cam' application. The ultimate goal is to display something on the > >> device and later the KMS sink will fail if there is no connector > >> attached to the device (although it can actually fail earlier, when > >> trying to set DRM_CLIENT_CAP_ATOMIC capability if this is not > >> supported). > >> > >> Let's avoid selecting devices without connectors, CRTCs or encoders. > >> The added check makes the original check for DRM_CAP_DUMB_BUFFER API > >> most likely unnecessary, let's remove it. > >> > >> Changes in v3: > >> - Added checks for the presence of CRTCs and encoders. > >> - Removed the check for DRM_CAP_DUMB_BUFFER. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/apps/cam/drm.cpp | 22 ++++++++++------------ > >> 1 file changed, 10 insertions(+), 12 deletions(-) > >> > >> diff --git a/src/apps/cam/drm.cpp b/src/apps/cam/drm.cpp > >> index 47bbb6b05..c40c31410 100644 > >> --- a/src/apps/cam/drm.cpp > >> +++ b/src/apps/cam/drm.cpp > >> @@ -450,8 +450,6 @@ int Device::openCard() > >> } > >> > >> for (struct dirent *res; (res = readdir(folder));) { > >> - uint64_t cap; > >> - > >> if (strncmp(res->d_name, "card", 4)) > >> continue; > >> > >> @@ -464,16 +462,16 @@ int Device::openCard() > >> continue; > >> } > >> > >> - /* > >> - * Skip devices that don't support the modeset API, to avoid > >> - * selecting a DRM device corresponding to a GPU. There is no > >> - * modeset capability, but the kernel returns an error for most > >> - * caps if mode setting isn't support by the driver. The > >> - * DRM_CAP_DUMB_BUFFER capability is one of those, other would > >> - * do as well. The capability value itself isn't relevant. > >> - */ > >> - ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap); > >> - if (ret < 0) { > >> + /* Skip devices without connectors. */ > > > > This all sounds reasonable to me, though this comment now skips devices > > without connectors/crts/encoders. > > > > Perhaps we should update it while applying? > > > > /* Skip non-display devices */ > > I agree with the fixup. I would expand it a bit /* * Skip non-display devices. While this could in theory be done * by checking for support of the mode setting API, some * out-of-tree render-only GPU drivers (namely powervr) * incorrectly set the DRIVER_MODESET driver feature. Check for * the presence of at least one CRTC, encoder and connector * instead. */ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > If it's applied, then: > > Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> + std::unique_ptr<drmModeRes, decltype(&drmModeFreeResources)> resources{ > >> + drmModeGetResources(fd_), > >> + &drmModeFreeResources > >> + }; > >> + if (!resources || > >> + resources->count_connectors <= 0 || > >> + resources->count_crtcs <= 0 || > >> + resources->count_encoders <= 0) { > >> + resources.reset(); > >> drmClose(fd_); > >> fd_ = -1; > >> continue;
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Fri, May 30, 2025 at 09:05:41AM +0200, Mattijs Korpershoek wrote: >> On jeu., mai 29, 2025 at 15:08, Kieran Bingham wrote: >> > Quoting Milan Zamazal (2025-05-29 09:23:26) > >> >> Device::openCard() in the cam DRM helpers looks for a /dev/dri/card* >> >> device that can be opened and that doesn't fail when asked about >> >> DRM_CAP_DUMB_BUFFER capability (regardless whether the capability is >> >> supported by the device). >> >> >> >> There can be matching devices that are not display devices. This can >> >> lead to selection of such a device and inability to use KMS output with >> >> `cam' application. The ultimate goal is to display something on the >> >> device and later the KMS sink will fail if there is no connector >> >> attached to the device (although it can actually fail earlier, when >> >> trying to set DRM_CLIENT_CAP_ATOMIC capability if this is not >> >> supported). >> >> >> >> Let's avoid selecting devices without connectors, CRTCs or encoders. >> >> The added check makes the original check for DRM_CAP_DUMB_BUFFER API >> >> most likely unnecessary, let's remove it. >> >> >> >> Changes in v3: >> >> - Added checks for the presence of CRTCs and encoders. >> >> - Removed the check for DRM_CAP_DUMB_BUFFER. >> >> >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> src/apps/cam/drm.cpp | 22 ++++++++++------------ >> >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> >> >> diff --git a/src/apps/cam/drm.cpp b/src/apps/cam/drm.cpp >> >> index 47bbb6b05..c40c31410 100644 >> >> --- a/src/apps/cam/drm.cpp >> >> +++ b/src/apps/cam/drm.cpp >> >> @@ -450,8 +450,6 @@ int Device::openCard() >> >> } >> >> >> >> for (struct dirent *res; (res = readdir(folder));) { >> >> - uint64_t cap; >> >> - >> >> if (strncmp(res->d_name, "card", 4)) >> >> continue; >> >> >> >> @@ -464,16 +462,16 @@ int Device::openCard() >> >> continue; >> >> } >> >> >> >> - /* >> >> - * Skip devices that don't support the modeset API, to avoid >> >> - * selecting a DRM device corresponding to a GPU. There is no >> >> - * modeset capability, but the kernel returns an error for most >> >> - * caps if mode setting isn't support by the driver. The >> >> - * DRM_CAP_DUMB_BUFFER capability is one of those, other would >> >> - * do as well. The capability value itself isn't relevant. >> >> - */ >> >> - ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap); >> >> - if (ret < 0) { >> >> + /* Skip devices without connectors. */ >> > >> > This all sounds reasonable to me, though this comment now skips devices >> > without connectors/crts/encoders. >> > >> > Perhaps we should update it while applying? >> > >> > /* Skip non-display devices */ >> >> I agree with the fixup. > > I would expand it a bit > > /* > * Skip non-display devices. While this could in theory be done > * by checking for support of the mode setting API, some > * out-of-tree render-only GPU drivers (namely powervr) > * incorrectly set the DRIVER_MODESET driver feature. Check for > * the presence of at least one CRTC, encoder and connector > * instead. > */ Yes, thank you, done in v4 (the only change there). > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> If it's applied, then: >> >> Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org> >> >> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> > >> >> + std::unique_ptr<drmModeRes, decltype(&drmModeFreeResources)> resources{ >> >> + drmModeGetResources(fd_), >> >> + &drmModeFreeResources >> >> + }; >> >> + if (!resources || >> >> + resources->count_connectors <= 0 || >> >> + resources->count_crtcs <= 0 || >> >> + resources->count_encoders <= 0) { >> >> + resources.reset(); >> >> drmClose(fd_); >> >> fd_ = -1; >> >> continue;
diff --git a/src/apps/cam/drm.cpp b/src/apps/cam/drm.cpp index 47bbb6b05..c40c31410 100644 --- a/src/apps/cam/drm.cpp +++ b/src/apps/cam/drm.cpp @@ -450,8 +450,6 @@ int Device::openCard() } for (struct dirent *res; (res = readdir(folder));) { - uint64_t cap; - if (strncmp(res->d_name, "card", 4)) continue; @@ -464,16 +462,16 @@ int Device::openCard() continue; } - /* - * Skip devices that don't support the modeset API, to avoid - * selecting a DRM device corresponding to a GPU. There is no - * modeset capability, but the kernel returns an error for most - * caps if mode setting isn't support by the driver. The - * DRM_CAP_DUMB_BUFFER capability is one of those, other would - * do as well. The capability value itself isn't relevant. - */ - ret = drmGetCap(fd_, DRM_CAP_DUMB_BUFFER, &cap); - if (ret < 0) { + /* Skip devices without connectors. */ + std::unique_ptr<drmModeRes, decltype(&drmModeFreeResources)> resources{ + drmModeGetResources(fd_), + &drmModeFreeResources + }; + if (!resources || + resources->count_connectors <= 0 || + resources->count_crtcs <= 0 || + resources->count_encoders <= 0) { + resources.reset(); drmClose(fd_); fd_ = -1; continue;
Device::openCard() in the cam DRM helpers looks for a /dev/dri/card* device that can be opened and that doesn't fail when asked about DRM_CAP_DUMB_BUFFER capability (regardless whether the capability is supported by the device). There can be matching devices that are not display devices. This can lead to selection of such a device and inability to use KMS output with `cam' application. The ultimate goal is to display something on the device and later the KMS sink will fail if there is no connector attached to the device (although it can actually fail earlier, when trying to set DRM_CLIENT_CAP_ATOMIC capability if this is not supported). Let's avoid selecting devices without connectors, CRTCs or encoders. The added check makes the original check for DRM_CAP_DUMB_BUFFER API most likely unnecessary, let's remove it. Changes in v3: - Added checks for the presence of CRTCs and encoders. - Removed the check for DRM_CAP_DUMB_BUFFER. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/apps/cam/drm.cpp | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)