| Message ID | 20211202102355.178664-1-ecurtin@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Thu, 2 Dec 2021 at 10:26, Eric Curtin <ecurtin@redhat.com> wrote: > > Once we have found a suitable plane and CRTC to auto-select, break out > of all loops, not just the inner-most loop. > > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration") > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > src/cam/kms_sink.cpp | 27 ++++++++++++++++----------- > src/cam/kms_sink.h | 1 + > 2 files changed, 17 insertions(+), 11 deletions(-) > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > index d30fba78..44a0f07b 100644 > --- a/src/cam/kms_sink.cpp > +++ b/src/cam/kms_sink.cpp > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > return 0; > } > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format) > { > /* > - * If the requested format has an alpha channel, also consider the X > - * variant. > - */ > + * If the requested format has an alpha channel, also consider the X > + * variant. > + */ > libcamera::PixelFormat xFormat; > > switch (format) { > @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > } > > /* > - * Find a CRTC and plane suitable for the request format and the > - * connector at the end of the pipeline. Restrict the search to primary > - * planes for now. > - */ > + * Find a CRTC and plane suitable for the request format and the > + * connector at the end of the pipeline. Restrict the search to primary > + * planes for now. > + */ > for (const DRM::Encoder *encoder : connector_->encoders()) { > for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) { > for (const DRM::Plane *plane : crtc->planes()) { > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > crtc_ = crtc; > plane_ = plane; > format_ = format; > - break; > + return crtc; > } > > if (plane->supportsFormat(xFormat)) { > crtc_ = crtc; > plane_ = plane; > format_ = xFormat; > - break; > + return crtc; > } > } > } > } > > - if (!crtc_) { > + return nullptr; > +} > + > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > +{ > + if (!findCrtc(format)) { > std::cerr > << "Unable to find display pipeline for format " > << format.toString() << std::endl; > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h > index 1e4290ad..a8a29399 100644 > --- a/src/cam/kms_sink.h > +++ b/src/cam/kms_sink.h > @@ -47,6 +47,7 @@ private: > libcamera::Request *camRequest_; > }; > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format); > int configurePipeline(const libcamera::PixelFormat &format); > void requestComplete(DRM::AtomicRequest *request); > > -- > 2.33.1 > Almost forgot about this patch, just a friendly reminder.
Hi Eric, Quoting Eric Curtin (2021-12-02 10:23:55) > Once we have found a suitable plane and CRTC to auto-select, break out > of all loops, not just the inner-most loop. > > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration") Is this an optimisation, or a bug fix ? Does this make a specific change in the outcome? The fixes tag implies it does, so it might be nice to hear here what happens without this patch, but it sounds like it's a worthwhile update. I haven't been able to test capture direct to DRM yet, as I haven't seemed to find a suitable match for camera and display, but I hope to try this again soon. Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it can't match configurations. Oh well, I'll try something else later. It's a shame we can't handle this with strides to be able to still display the image as it's smaller. > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > src/cam/kms_sink.cpp | 27 ++++++++++++++++----------- > src/cam/kms_sink.h | 1 + > 2 files changed, 17 insertions(+), 11 deletions(-) > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > index d30fba78..44a0f07b 100644 > --- a/src/cam/kms_sink.cpp > +++ b/src/cam/kms_sink.cpp > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > return 0; > } > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format) > { > /* > - * If the requested format has an alpha channel, also consider the X > - * variant. > - */ > + * If the requested format has an alpha channel, also consider the X > + * variant. > + */ I think those changes broke the indentation... Looks like the tabs are replaced by spaces? > libcamera::PixelFormat xFormat; > > switch (format) { > @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > } > > /* > - * Find a CRTC and plane suitable for the request format and the > - * connector at the end of the pipeline. Restrict the search to primary > - * planes for now. > - */ > + * Find a CRTC and plane suitable for the request format and the > + * connector at the end of the pipeline. Restrict the search to primary > + * planes for now. > + */ Same here, But splitting this to a new function looks reasonable to me. So with the comments repaired, which could be done while applying too: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > for (const DRM::Encoder *encoder : connector_->encoders()) { > for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) { > for (const DRM::Plane *plane : crtc->planes()) { > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > crtc_ = crtc; > plane_ = plane; > format_ = format; > - break; > + return crtc; > } > > if (plane->supportsFormat(xFormat)) { > crtc_ = crtc; > plane_ = plane; > format_ = xFormat; > - break; > + return crtc; > } > } > } > } > > - if (!crtc_) { > + return nullptr; > +} > + > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > +{ > + if (!findCrtc(format)) { > std::cerr > << "Unable to find display pipeline for format " > << format.toString() << std::endl; > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h > index 1e4290ad..a8a29399 100644 > --- a/src/cam/kms_sink.h > +++ b/src/cam/kms_sink.h > @@ -47,6 +47,7 @@ private: > libcamera::Request *camRequest_; > }; > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format); > int configurePipeline(const libcamera::PixelFormat &format); > void requestComplete(DRM::AtomicRequest *request); > > -- > 2.33.1 >
On Mon, 31 Jan 2022 at 23:52, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Eric, > > Quoting Eric Curtin (2021-12-02 10:23:55) > > Once we have found a suitable plane and CRTC to auto-select, break out > > of all loops, not just the inner-most loop. > > > > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration") > > Is this an optimisation, or a bug fix ? Does this make a specific change > in the outcome? For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on my hardware, unless we break out of the loop the first time around. But probably something else at play here, since as you say, this should just be an optimization. It was briefly discussed on another email thread in libcamera-devel months ago. > > The fixes tag implies it does, so it might be nice to hear here what > happens without this patch, but it sounds like it's a worthwhile update. > > I haven't been able to test capture direct to DRM yet, as I haven't > seemed to find a suitable match for camera and display, but I hope to > try this again soon. > > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it > can't match configurations. Oh well, I'll try something else later. > It's a shame we can't handle this with strides to be able to still > display the image as it's smaller. I actually removed this limitation on the version we've been writing, which is 90% "cam" reference implementation stripped and repurposed for red hat's needs, it's gonna be executed by plymouth. So the commits on December 7th here: https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp allow smaller resolution camera outputs to run on larger displays, so a 1080p image such as in your case should be centralized and take up 1/4 of your 4k screen. It's not scaling, but it was an easy way to remove a limitation for me that enabled me to test on more hardware. Could be easily added to "cam" also. I've tested "twincam" on two machines, chances are, if you build and run it on your machine on a suitable tty, it might "just work". > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > src/cam/kms_sink.cpp | 27 ++++++++++++++++----------- > > src/cam/kms_sink.h | 1 + > > 2 files changed, 17 insertions(+), 11 deletions(-) > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > index d30fba78..44a0f07b 100644 > > --- a/src/cam/kms_sink.cpp > > +++ b/src/cam/kms_sink.cpp > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > return 0; > > } > > > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format) > > { > > /* > > - * If the requested format has an alpha channel, also consider the X > > - * variant. > > - */ > > + * If the requested format has an alpha channel, also consider the X > > + * variant. > > + */ > > I think those changes broke the indentation... Looks like the tabs are > replaced by spaces? Unintentional, I'll fix. But I think I auto-formatted using the checked in .clang-format file: https://github.com/kbingham/libcamera/blob/master/.clang-format > > > libcamera::PixelFormat xFormat; > > > > switch (format) { > > @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > } > > > > /* > > - * Find a CRTC and plane suitable for the request format and the > > - * connector at the end of the pipeline. Restrict the search to primary > > - * planes for now. > > - */ > > + * Find a CRTC and plane suitable for the request format and the > > + * connector at the end of the pipeline. Restrict the search to primary > > + * planes for now. > > + */ > > Same here, > > But splitting this to a new function looks reasonable to me. > > So with the comments repaired, which could be done while applying too: I can resend patch. Thanks for reviewing. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > for (const DRM::Encoder *encoder : connector_->encoders()) { > > for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) { > > for (const DRM::Plane *plane : crtc->planes()) { > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > crtc_ = crtc; > > plane_ = plane; > > format_ = format; > > - break; > > + return crtc; > > } > > > > if (plane->supportsFormat(xFormat)) { > > crtc_ = crtc; > > plane_ = plane; > > format_ = xFormat; > > - break; > > + return crtc; > > } > > } > > } > > } > > > > - if (!crtc_) { > > + return nullptr; > > +} > > + > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > +{ > > + if (!findCrtc(format)) { > > std::cerr > > << "Unable to find display pipeline for format " > > << format.toString() << std::endl; > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h > > index 1e4290ad..a8a29399 100644 > > --- a/src/cam/kms_sink.h > > +++ b/src/cam/kms_sink.h > > @@ -47,6 +47,7 @@ private: > > libcamera::Request *camRequest_; > > }; > > > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format); > > int configurePipeline(const libcamera::PixelFormat &format); > > void requestComplete(DRM::AtomicRequest *request); > > > > -- > > 2.33.1 > > >
Hi Eric, Quoting Eric Curtin (2022-02-01 22:16:35) > On Mon, 31 Jan 2022 at 23:52, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Hi Eric, > > > > Quoting Eric Curtin (2021-12-02 10:23:55) > > > Once we have found a suitable plane and CRTC to auto-select, break out > > > of all loops, not just the inner-most loop. > > > > > > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration") > > > > Is this an optimisation, or a bug fix ? Does this make a specific change > > in the outcome? > > For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on > my hardware, unless we break out of the loop the first time around. But probably > something else at play here, since as you say, this should just be an > optimization. Ok, well a fixes tag is probably enough then. > It was briefly discussed on another email thread in libcamera-devel months ago. > > > > > The fixes tag implies it does, so it might be nice to hear here what > > happens without this patch, but it sounds like it's a worthwhile update. > > > > I haven't been able to test capture direct to DRM yet, as I haven't > > seemed to find a suitable match for camera and display, but I hope to > > try this again soon. > > > > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it > > can't match configurations. Oh well, I'll try something else later. > > It's a shame we can't handle this with strides to be able to still > > display the image as it's smaller. > > I actually removed this limitation on the version we've been writing, > which is 90% > "cam" reference implementation stripped and repurposed for red hat's needs, it's > gonna be executed by plymouth. > > So the commits on December 7th here: > > https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp > > allow smaller resolution camera outputs to run on larger displays, so a 1080p > image such as in your case should be centralized and take up 1/4 of your 4k > screen. It's not scaling, but it was an easy way to remove a limitation for me > that enabled me to test on more hardware. Could be easily added to "cam" > also. Oh - yes please, could you add that! Then I would actually be able to test things :-) I think it's absolutely fine to not scale, and simply allow the image to be presented if it fits. I'd even say if the image is bigger than the display it would be nice if it still got displayed, centrally and simply cropped by the display output size. That would be far more useful than requiring an exact match IMO. > I've tested "twincam" on two machines, chances are, if you build and run it > on your machine on a suitable tty, it might "just work". Aha! Thankyou, I wasn't aware that you're actually building an application here. Can I add this to my list of applications supported by libcamera? What are the goals of twincam? ... Just run and display cameras without a window manager/GUI? (That sounds like exactly something I've been after/wanting to make) > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > --- > > > src/cam/kms_sink.cpp | 27 ++++++++++++++++----------- > > > src/cam/kms_sink.h | 1 + > > > 2 files changed, 17 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > index d30fba78..44a0f07b 100644 > > > --- a/src/cam/kms_sink.cpp > > > +++ b/src/cam/kms_sink.cpp > > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > return 0; > > > } > > > > > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format) > > > { > > > /* > > > - * If the requested format has an alpha channel, also consider the X > > > - * variant. > > > - */ > > > + * If the requested format has an alpha channel, also consider the X > > > + * variant. > > > + */ > > > > I think those changes broke the indentation... Looks like the tabs are > > replaced by spaces? > > Unintentional, I'll fix. But I think I auto-formatted using the > checked in .clang-format file: > > https://github.com/kbingham/libcamera/blob/master/.clang-format I would expect that to know we use tabs not spaces... hrmmm We have a script that handles formatting called ./utils/checkstyle.py, and that can be enabled as a post-commit hook. cp ./utils/hooks/post-commit .git/hooks/ > > > > > libcamera::PixelFormat xFormat; > > > > > > switch (format) { > > > @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > } > > > > > > /* > > > - * Find a CRTC and plane suitable for the request format and the > > > - * connector at the end of the pipeline. Restrict the search to primary > > > - * planes for now. > > > - */ > > > + * Find a CRTC and plane suitable for the request format and the > > > + * connector at the end of the pipeline. Restrict the search to primary > > > + * planes for now. > > > + */ > > > > Same here, > > > > But splitting this to a new function looks reasonable to me. > > > > So with the comments repaired, which could be done while applying too: > > I can resend patch. Thanks for reviewing. > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > for (const DRM::Encoder *encoder : connector_->encoders()) { > > > for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) { > > > for (const DRM::Plane *plane : crtc->planes()) { > > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > crtc_ = crtc; > > > plane_ = plane; > > > format_ = format; > > > - break; > > > + return crtc; > > > } > > > > > > if (plane->supportsFormat(xFormat)) { > > > crtc_ = crtc; > > > plane_ = plane; > > > format_ = xFormat; > > > - break; > > > + return crtc; > > > } > > > } > > > } > > > } > > > > > > - if (!crtc_) { > > > + return nullptr; > > > +} > > > + > > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > +{ > > > + if (!findCrtc(format)) { > > > std::cerr > > > << "Unable to find display pipeline for format " > > > << format.toString() << std::endl; > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h > > > index 1e4290ad..a8a29399 100644 > > > --- a/src/cam/kms_sink.h > > > +++ b/src/cam/kms_sink.h > > > @@ -47,6 +47,7 @@ private: > > > libcamera::Request *camRequest_; > > > }; > > > > > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format); > > > int configurePipeline(const libcamera::PixelFormat &format); > > > void requestComplete(DRM::AtomicRequest *request); > > > > > > -- > > > 2.33.1 > > > > > >
On Wed, 2 Feb 2022 at 09:32, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Eric, > > Quoting Eric Curtin (2022-02-01 22:16:35) > > On Mon, 31 Jan 2022 at 23:52, Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > Hi Eric, > > > > > > Quoting Eric Curtin (2021-12-02 10:23:55) > > > > Once we have found a suitable plane and CRTC to auto-select, break out > > > > of all loops, not just the inner-most loop. > > > > > > > > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration") > > > > > > Is this an optimisation, or a bug fix ? Does this make a specific change > > > in the outcome? > > > > For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on > > my hardware, unless we break out of the loop the first time around. But probably > > something else at play here, since as you say, this should just be an > > optimization. > > Ok, well a fixes tag is probably enough then. > > > It was briefly discussed on another email thread in libcamera-devel months ago. > > > > > > > > The fixes tag implies it does, so it might be nice to hear here what > > > happens without this patch, but it sounds like it's a worthwhile update. > > > > > > I haven't been able to test capture direct to DRM yet, as I haven't > > > seemed to find a suitable match for camera and display, but I hope to > > > try this again soon. > > > > > > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it > > > can't match configurations. Oh well, I'll try something else later. > > > It's a shame we can't handle this with strides to be able to still > > > display the image as it's smaller. > > > > I actually removed this limitation on the version we've been writing, > > which is 90% > > "cam" reference implementation stripped and repurposed for red hat's needs, it's > > gonna be executed by plymouth. > > > > So the commits on December 7th here: > > > > https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp > > > > allow smaller resolution camera outputs to run on larger displays, so a 1080p > > image such as in your case should be centralized and take up 1/4 of your 4k > > screen. It's not scaling, but it was an easy way to remove a limitation for me > > that enabled me to test on more hardware. Could be easily added to "cam" > > also. > > Oh - yes please, could you add that! Then I would actually be able to > test things :-) > > I think it's absolutely fine to not scale, and simply allow the image to > be presented if it fits. I'd even say if the image is bigger than the > display it would be nice if it still got displayed, centrally and simply > cropped by the display output size. It will attempt to display large images on smaller displays, not sure the math or code is correct for that though, haven't tried that. Would be useful also like you say. > > That would be far more useful than requiring an exact match IMO. > > > I've tested "twincam" on two machines, chances are, if you build and run it > > on your machine on a suitable tty, it might "just work". > > Aha! Thankyou, I wasn't aware that you're actually building an > application here. > > Can I add this to my list of applications supported by libcamera? Yes, of course. > > What are the goals of twincam? ... Just run and display cameras without > a window manager/GUI? (That sounds like exactly something I've been > after/wanting to make) It's part of Red Hat's automotive effort, as there's a two second goal we have to hit to start our camera from cold boot, including firmware loading, everything. But I'm open minded if people find it useful for other things and it proves useful for other use cases, great! So that's one goal the two second thing, but after that we will see. Some things off the top of my mind are scaling, pixel format converters, etc. Want to support many different types of camera of course, at present I only have UVC camera to test with unfortunately. But would like to test with CSI/MIPI camera on aarch64, as that's more commonly used in automotive I think. There is also a separate gstreamer effort at Red Hat as you know I guess, we will likely try to transition to that once boot is complete. But everything is open to change of course. > > > > > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > --- > > > > src/cam/kms_sink.cpp | 27 ++++++++++++++++----------- > > > > src/cam/kms_sink.h | 1 + > > > > 2 files changed, 17 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > > index d30fba78..44a0f07b 100644 > > > > --- a/src/cam/kms_sink.cpp > > > > +++ b/src/cam/kms_sink.cpp > > > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > > return 0; > > > > } > > > > > > > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format) > > > > { > > > > /* > > > > - * If the requested format has an alpha channel, also consider the X > > > > - * variant. > > > > - */ > > > > + * If the requested format has an alpha channel, also consider the X > > > > + * variant. > > > > + */ > > > > > > I think those changes broke the indentation... Looks like the tabs are > > > replaced by spaces? > > > > Unintentional, I'll fix. But I think I auto-formatted using the > > checked in .clang-format file: > > > > https://github.com/kbingham/libcamera/blob/master/.clang-format > > I would expect that to know we use tabs not spaces... hrmmm > > We have a script that handles formatting called ./utils/checkstyle.py, > and that can be enabled as a post-commit hook. Thanks for letting me know > > cp ./utils/hooks/post-commit .git/hooks/ > > > > > > > > libcamera::PixelFormat xFormat; > > > > > > > > switch (format) { > > > > @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > } > > > > > > > > /* > > > > - * Find a CRTC and plane suitable for the request format and the > > > > - * connector at the end of the pipeline. Restrict the search to primary > > > > - * planes for now. > > > > - */ > > > > + * Find a CRTC and plane suitable for the request format and the > > > > + * connector at the end of the pipeline. Restrict the search to primary > > > > + * planes for now. > > > > + */ > > > > > > Same here, > > > > > > But splitting this to a new function looks reasonable to me. > > > > > > So with the comments repaired, which could be done while applying too: > > > > I can resend patch. Thanks for reviewing. > > > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > for (const DRM::Encoder *encoder : connector_->encoders()) { > > > > for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) { > > > > for (const DRM::Plane *plane : crtc->planes()) { > > > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > crtc_ = crtc; > > > > plane_ = plane; > > > > format_ = format; > > > > - break; > > > > + return crtc; > > > > } > > > > > > > > if (plane->supportsFormat(xFormat)) { > > > > crtc_ = crtc; > > > > plane_ = plane; > > > > format_ = xFormat; > > > > - break; > > > > + return crtc; > > > > } > > > > } > > > > } > > > > } > > > > > > > > - if (!crtc_) { > > > > + return nullptr; > > > > +} > > > > + > > > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > +{ > > > > + if (!findCrtc(format)) { > > > > std::cerr > > > > << "Unable to find display pipeline for format " > > > > << format.toString() << std::endl; > > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h > > > > index 1e4290ad..a8a29399 100644 > > > > --- a/src/cam/kms_sink.h > > > > +++ b/src/cam/kms_sink.h > > > > @@ -47,6 +47,7 @@ private: > > > > libcamera::Request *camRequest_; > > > > }; > > > > > > > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format); > > > > int configurePipeline(const libcamera::PixelFormat &format); > > > > void requestComplete(DRM::AtomicRequest *request); > > > > > > > > -- > > > > 2.33.1 > > > > > > > > > >
Hi Eric, On Tue, Feb 01, 2022 at 10:16:35PM +0000, Eric Curtin wrote: > On Mon, 31 Jan 2022 at 23:52, Kieran Bingham wrote: > > Quoting Eric Curtin (2021-12-02 10:23:55) > > > Once we have found a suitable plane and CRTC to auto-select, break out > > > of all loops, not just the inner-most loop. > > > > > > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration") > > > > Is this an optimisation, or a bug fix ? Does this make a specific change > > in the outcome? > > For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on > my hardware, unless we break out of the loop the first time around. But probably > something else at play here, since as you say, this should just be an > optimization. With the existing code base, we pick the last suitable CRTC, while with this patch, we'll pick the first one. Breaking from the loop once a suitable CRTC is found is a good idea, but if it fixes an issue with your setup, it means that at least one of the CRTCs that are considered suitable is actually not. We may thus break your platform inadvertently in the future when reworking this code. It would be good to go to the bottom of this, and fix the CRTC selection to not rely on luck. It can be done separately from this patch. > It was briefly discussed on another email thread in libcamera-devel months ago. > > > The fixes tag implies it does, so it might be nice to hear here what > > happens without this patch, but it sounds like it's a worthwhile update. > > > > I haven't been able to test capture direct to DRM yet, as I haven't > > seemed to find a suitable match for camera and display, but I hope to > > try this again soon. > > > > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it > > can't match configurations. Oh well, I'll try something else later. > > It's a shame we can't handle this with strides to be able to still > > display the image as it's smaller. > > I actually removed this limitation on the version we've been writing, > which is 90% > "cam" reference implementation stripped and repurposed for red hat's needs, it's > gonna be executed by plymouth. > > So the commits on December 7th here: > > https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp > > allow smaller resolution camera outputs to run on larger displays, so a 1080p > image such as in your case should be centralized and take up 1/4 of your 4k > screen. It's not scaling, but it was an easy way to remove a limitation for me > that enabled me to test on more hardware. Could be easily added to "cam" > also. > > I've tested "twincam" on two machines, chances are, if you build and run it > on your machine on a suitable tty, it might "just work". > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > --- > > > src/cam/kms_sink.cpp | 27 ++++++++++++++++----------- > > > src/cam/kms_sink.h | 1 + > > > 2 files changed, 17 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > index d30fba78..44a0f07b 100644 > > > --- a/src/cam/kms_sink.cpp > > > +++ b/src/cam/kms_sink.cpp > > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > return 0; > > > } > > > > > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format) > > > { [snip] > > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > crtc_ = crtc; > > > plane_ = plane; > > > format_ = format; > > > - break; > > > + return crtc; > > > } > > > > > > if (plane->supportsFormat(xFormat)) { > > > crtc_ = crtc; > > > plane_ = plane; > > > format_ = xFormat; > > > - break; > > > + return crtc; > > > } > > > } > > > } > > > } > > > > > > - if (!crtc_) { > > > + return nullptr; The function returns a DRM::Crtc and is called findCrtc(), but it also finds a plane, selects a formatn and store them all in member variables. That's a bit confusing. I would rename the function to findPipeline() (for lack of a better name) and make it return an integer (0 on success, -EPIPE on error) or a std::tuple<DRM::Crtc *, DRM::Plane *, libcamera::PixelFormat>. The former is likely easier. Another option is diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp index d30fba783c96..aaffa372f050 100644 --- a/src/cam/kms_sink.cpp +++ b/src/cam/kms_sink.cpp @@ -174,14 +174,14 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) crtc_ = crtc; plane_ = plane; format_ = format; - break; + goto found; } if (plane->supportsFormat(xFormat)) { crtc_ = crtc; plane_ = plane; format_ = xFormat; - break; + goto found; } } } @@ -195,6 +195,7 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) return -EPIPE; } +found: std::cout << "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id() << ", connector " << connector_->name() which would be even simpler. It would cause issues if we were to add local variables after the goto statements and before the label, but we can worry about that later. > > > +} > > > + > > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > +{ > > > + if (!findCrtc(format)) { > > > std::cerr > > > << "Unable to find display pipeline for format " > > > << format.toString() << std::endl; > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h > > > index 1e4290ad..a8a29399 100644 > > > --- a/src/cam/kms_sink.h > > > +++ b/src/cam/kms_sink.h > > > @@ -47,6 +47,7 @@ private: > > > libcamera::Request *camRequest_; > > > }; > > > > > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format); > > > int configurePipeline(const libcamera::PixelFormat &format); > > > void requestComplete(DRM::AtomicRequest *request); > > >
On Wed, 2 Feb 2022 at 14:35, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Eric, > > On Tue, Feb 01, 2022 at 10:16:35PM +0000, Eric Curtin wrote: > > On Mon, 31 Jan 2022 at 23:52, Kieran Bingham wrote: > > > Quoting Eric Curtin (2021-12-02 10:23:55) > > > > Once we have found a suitable plane and CRTC to auto-select, break out > > > > of all loops, not just the inner-most loop. > > > > > > > > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration") > > > > > > Is this an optimisation, or a bug fix ? Does this make a specific change > > > in the outcome? > > > > For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on > > my hardware, unless we break out of the loop the first time around. But probably > > something else at play here, since as you say, this should just be an > > optimization. > > With the existing code base, we pick the last suitable CRTC, while with > this patch, we'll pick the first one. Breaking from the loop once a > suitable CRTC is found is a good idea, but if it fixes an issue with > your setup, it means that at least one of the CRTCs that are considered > suitable is actually not. We may thus break your platform inadvertently > in the future when reworking this code. It would be good to go to the > bottom of this, and fix the CRTC selection to not rely on luck. It can > be done separately from this patch. > > > It was briefly discussed on another email thread in libcamera-devel months ago. > > > > > The fixes tag implies it does, so it might be nice to hear here what > > > happens without this patch, but it sounds like it's a worthwhile update. > > > > > > I haven't been able to test capture direct to DRM yet, as I haven't > > > seemed to find a suitable match for camera and display, but I hope to > > > try this again soon. > > > > > > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it > > > can't match configurations. Oh well, I'll try something else later. > > > It's a shame we can't handle this with strides to be able to still > > > display the image as it's smaller. > > > > I actually removed this limitation on the version we've been writing, > > which is 90% > > "cam" reference implementation stripped and repurposed for red hat's needs, it's > > gonna be executed by plymouth. > > > > So the commits on December 7th here: > > > > https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp > > > > allow smaller resolution camera outputs to run on larger displays, so a 1080p > > image such as in your case should be centralized and take up 1/4 of your 4k > > screen. It's not scaling, but it was an easy way to remove a limitation for me > > that enabled me to test on more hardware. Could be easily added to "cam" > > also. > > > > I've tested "twincam" on two machines, chances are, if you build and run it > > on your machine on a suitable tty, it might "just work". > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > --- > > > > src/cam/kms_sink.cpp | 27 ++++++++++++++++----------- > > > > src/cam/kms_sink.h | 1 + > > > > 2 files changed, 17 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > > index d30fba78..44a0f07b 100644 > > > > --- a/src/cam/kms_sink.cpp > > > > +++ b/src/cam/kms_sink.cpp > > > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > > return 0; > > > > } > > > > > > > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format) > > > > { > > [snip] > > > > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > crtc_ = crtc; > > > > plane_ = plane; > > > > format_ = format; > > > > - break; > > > > + return crtc; > > > > } > > > > > > > > if (plane->supportsFormat(xFormat)) { > > > > crtc_ = crtc; > > > > plane_ = plane; > > > > format_ = xFormat; > > > > - break; > > > > + return crtc; > > > > } > > > > } > > > > } > > > > } > > > > > > > > - if (!crtc_) { > > > > + return nullptr; > > The function returns a DRM::Crtc and is called findCrtc(), but it also > finds a plane, selects a formatn and store them all in member variables. > That's a bit confusing. I would rename the function to findPipeline() > (for lack of a better name) and make it return an integer (0 on success, > -EPIPE on error) or a std::tuple<DRM::Crtc *, DRM::Plane *, > libcamera::PixelFormat>. The former is likely easier. Yup, I was thinking of changing the name in the next version of the patch funnily enough, because you're right, it does more than select ctrc. I'll do the int return, just to keep it simple. > > Another option is > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > index d30fba783c96..aaffa372f050 100644 > --- a/src/cam/kms_sink.cpp > +++ b/src/cam/kms_sink.cpp > @@ -174,14 +174,14 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > crtc_ = crtc; > plane_ = plane; > format_ = format; > - break; > + goto found; > } > > if (plane->supportsFormat(xFormat)) { > crtc_ = crtc; > plane_ = plane; > format_ = xFormat; > - break; > + goto found; > } > } > } > @@ -195,6 +195,7 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > return -EPIPE; > } > > +found: > std::cout > << "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id() > << ", connector " << connector_->name() > > which would be even simpler. It would cause issues if we were to add > local variables after the goto statements and before the label, but we > can worry about that later. Yeah I actually started with this, changed it just in case there were some goto haters reviewing this, writing a function with returns is how to write a goto without writing a goto. I'll write the function just to keep the functions a little smaller. > > > > > +} > > > > + > > > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > +{ > > > > + if (!findCrtc(format)) { > > > > std::cerr > > > > << "Unable to find display pipeline for format " > > > > << format.toString() << std::endl; > > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h > > > > index 1e4290ad..a8a29399 100644 > > > > --- a/src/cam/kms_sink.h > > > > +++ b/src/cam/kms_sink.h > > > > @@ -47,6 +47,7 @@ private: > > > > libcamera::Request *camRequest_; > > > > }; > > > > > > > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format); > > > > int configurePipeline(const libcamera::PixelFormat &format); > > > > void requestComplete(DRM::AtomicRequest *request); > > > > > > -- > Regards, > > Laurent Pinchart >
Hi Eric, On Wed, Feb 02, 2022 at 05:04:10PM +0000, Eric Curtin wrote: > On Wed, 2 Feb 2022 at 14:35, Laurent Pinchart wrote: > > On Tue, Feb 01, 2022 at 10:16:35PM +0000, Eric Curtin wrote: > > > On Mon, 31 Jan 2022 at 23:52, Kieran Bingham wrote: > > > > Quoting Eric Curtin (2021-12-02 10:23:55) > > > > > Once we have found a suitable plane and CRTC to auto-select, break out > > > > > of all loops, not just the inner-most loop. > > > > > > > > > > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration") > > > > > > > > Is this an optimisation, or a bug fix ? Does this make a specific change > > > > in the outcome? > > > > > > For me, on my hardware, it's a bug fix, since 1de0f90dd432, it won't work on > > > my hardware, unless we break out of the loop the first time around. But probably > > > something else at play here, since as you say, this should just be an > > > optimization. > > > > With the existing code base, we pick the last suitable CRTC, while with > > this patch, we'll pick the first one. Breaking from the loop once a > > suitable CRTC is found is a good idea, but if it fixes an issue with > > your setup, it means that at least one of the CRTCs that are considered > > suitable is actually not. We may thus break your platform inadvertently > > in the future when reworking this code. It would be good to go to the > > bottom of this, and fix the CRTC selection to not rely on luck. It can > > be done separately from this patch. > > > > > It was briefly discussed on another email thread in libcamera-devel months ago. > > > > > > > The fixes tag implies it does, so it might be nice to hear here what > > > > happens without this patch, but it sounds like it's a worthwhile update. > > > > > > > > I haven't been able to test capture direct to DRM yet, as I haven't > > > > seemed to find a suitable match for camera and display, but I hope to > > > > try this again soon. > > > > > > > > Aha, it seems I'm cursed by a 1080p Camera, and a 4K display, and so it > > > > can't match configurations. Oh well, I'll try something else later. > > > > It's a shame we can't handle this with strides to be able to still > > > > display the image as it's smaller. > > > > > > I actually removed this limitation on the version we've been writing, > > > which is 90% > > > "cam" reference implementation stripped and repurposed for red hat's needs, it's > > > gonna be executed by plymouth. > > > > > > So the commits on December 7th here: > > > > > > https://github.com/ericcurtin/twincam/commits/main/kms_sink.cpp > > > > > > allow smaller resolution camera outputs to run on larger displays, so a 1080p > > > image such as in your case should be centralized and take up 1/4 of your 4k > > > screen. It's not scaling, but it was an easy way to remove a limitation for me > > > that enabled me to test on more hardware. Could be easily added to "cam" > > > also. > > > > > > I've tested "twincam" on two machines, chances are, if you build and run it > > > on your machine on a suitable tty, it might "just work". > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > > --- > > > > > src/cam/kms_sink.cpp | 27 ++++++++++++++++----------- > > > > > src/cam/kms_sink.h | 1 + > > > > > 2 files changed, 17 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > > > > index d30fba78..44a0f07b 100644 > > > > > --- a/src/cam/kms_sink.cpp > > > > > +++ b/src/cam/kms_sink.cpp > > > > > @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > > > return 0; > > > > > } > > > > > > > > > > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > > +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format) > > > > > { > > > > [snip] > > > > > > > @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > > crtc_ = crtc; > > > > > plane_ = plane; > > > > > format_ = format; > > > > > - break; > > > > > + return crtc; > > > > > } > > > > > > > > > > if (plane->supportsFormat(xFormat)) { > > > > > crtc_ = crtc; > > > > > plane_ = plane; > > > > > format_ = xFormat; > > > > > - break; > > > > > + return crtc; > > > > > } > > > > > } > > > > > } > > > > > } > > > > > > > > > > - if (!crtc_) { > > > > > + return nullptr; > > > > The function returns a DRM::Crtc and is called findCrtc(), but it also > > finds a plane, selects a formatn and store them all in member variables. > > That's a bit confusing. I would rename the function to findPipeline() > > (for lack of a better name) and make it return an integer (0 on success, > > -EPIPE on error) or a std::tuple<DRM::Crtc *, DRM::Plane *, > > libcamera::PixelFormat>. The former is likely easier. > > Yup, I was thinking of changing the name in the next version of the > patch funnily enough, because you're right, it does more than select > ctrc. I'll do the int return, just to keep it simple. selectPipeline() could be a better name than findPipeline() as the function selects the components, it doesn't just look them up. > > Another option is > > > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp > > index d30fba783c96..aaffa372f050 100644 > > --- a/src/cam/kms_sink.cpp > > +++ b/src/cam/kms_sink.cpp > > @@ -174,14 +174,14 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > crtc_ = crtc; > > plane_ = plane; > > format_ = format; > > - break; > > + goto found; > > } > > > > if (plane->supportsFormat(xFormat)) { > > crtc_ = crtc; > > plane_ = plane; > > format_ = xFormat; > > - break; > > + goto found; > > } > > } > > } > > @@ -195,6 +195,7 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > return -EPIPE; > > } > > > > +found: > > std::cout > > << "Using KMS plane " << plane_->id() << ", CRTC " << crtc_->id() > > << ", connector " << connector_->name() > > > > which would be even simpler. It would cause issues if we were to add > > local variables after the goto statements and before the label, but we > > can worry about that later. > > Yeah I actually started with this, changed it just in case there were some > goto haters reviewing this, writing a function with returns is how to write > a goto without writing a goto. I'll write the function just to keep the > functions a little smaller. OK. > > > > > +} > > > > > + > > > > > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format) > > > > > +{ > > > > > + if (!findCrtc(format)) { > > > > > std::cerr > > > > > << "Unable to find display pipeline for format " > > > > > << format.toString() << std::endl; > > > > > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h > > > > > index 1e4290ad..a8a29399 100644 > > > > > --- a/src/cam/kms_sink.h > > > > > +++ b/src/cam/kms_sink.h > > > > > @@ -47,6 +47,7 @@ private: > > > > > libcamera::Request *camRequest_; > > > > > }; > > > > > > > > > > + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format); > > > > > int configurePipeline(const libcamera::PixelFormat &format); > > > > > void requestComplete(DRM::AtomicRequest *request); > > > > >
diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp index d30fba78..44a0f07b 100644 --- a/src/cam/kms_sink.cpp +++ b/src/cam/kms_sink.cpp @@ -136,12 +136,12 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) return 0; } -int KMSSink::configurePipeline(const libcamera::PixelFormat &format) +const DRM::Crtc *KMSSink::findCrtc(const libcamera::PixelFormat &format) { /* - * If the requested format has an alpha channel, also consider the X - * variant. - */ + * If the requested format has an alpha channel, also consider the X + * variant. + */ libcamera::PixelFormat xFormat; switch (format) { @@ -160,10 +160,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) } /* - * Find a CRTC and plane suitable for the request format and the - * connector at the end of the pipeline. Restrict the search to primary - * planes for now. - */ + * Find a CRTC and plane suitable for the request format and the + * connector at the end of the pipeline. Restrict the search to primary + * planes for now. + */ for (const DRM::Encoder *encoder : connector_->encoders()) { for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) { for (const DRM::Plane *plane : crtc->planes()) { @@ -174,20 +174,25 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format) crtc_ = crtc; plane_ = plane; format_ = format; - break; + return crtc; } if (plane->supportsFormat(xFormat)) { crtc_ = crtc; plane_ = plane; format_ = xFormat; - break; + return crtc; } } } } - if (!crtc_) { + return nullptr; +} + +int KMSSink::configurePipeline(const libcamera::PixelFormat &format) +{ + if (!findCrtc(format)) { std::cerr << "Unable to find display pipeline for format " << format.toString() << std::endl; diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h index 1e4290ad..a8a29399 100644 --- a/src/cam/kms_sink.h +++ b/src/cam/kms_sink.h @@ -47,6 +47,7 @@ private: libcamera::Request *camRequest_; }; + const DRM::Crtc *findCrtc(const libcamera::PixelFormat &format); int configurePipeline(const libcamera::PixelFormat &format); void requestComplete(DRM::AtomicRequest *request);
Once we have found a suitable plane and CRTC to auto-select, break out of all loops, not just the inner-most loop. Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration") Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- src/cam/kms_sink.cpp | 27 ++++++++++++++++----------- src/cam/kms_sink.h | 1 + 2 files changed, 17 insertions(+), 11 deletions(-)