[libcamera-devel] [PATCH v3] cam: kms-sink: Once we have found a suitable pipeline to select, return
Eric Curtin
ecurtin at redhat.com
Tue Feb 8 09:09:57 CET 2022
On Mon, 7 Feb 2022 at 22:28, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On Mon, Feb 07, 2022 at 03:01:36PM +0000, Eric Curtin wrote:
> > The break only breaks out of the innermost loop which is not the most
> > optimal. It also breaks cam on my machines.
>
> I'd expand this a bit.
>
> cam: kms_sink: Use the first suitable pipeline found
>
> When searching for a suitable pipeline, we mistakenly only break from
> the inner loop. This results in the last suitable output being selected.
> Pick the first one instead.
>
> > Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration")
> > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > ---
> >
> > Changes in v3:
> > - Add "cam: kms_sink:" tag
> >
> > Changes in v2:
> > - Change function name to selectPipeline
> > - Return int rather than pointer
> > - Formatting
> > - Mention in commit message that it fixes a bug on my machine also
> >
> > src/cam/kms_sink.cpp | 18 ++++++++++++------
> > src/cam/kms_sink.h | 1 +
> > 2 files changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > index d30fba78..973cd370 100644
> > --- a/src/cam/kms_sink.cpp
> > +++ b/src/cam/kms_sink.cpp
> > @@ -136,7 +136,7 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
> > return 0;
> > }
> >
> > -int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > +int KMSSink::selectPipeline(const libcamera::PixelFormat &format)
> > {
> > /*
> > * If the requested format has an alpha channel, also consider the X
> > @@ -174,25 +174,31 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > crtc_ = crtc;
> > plane_ = plane;
> > format_ = format;
> > - break;
> > + return 0;
> > }
> >
> > if (plane->supportsFormat(xFormat)) {
> > crtc_ = crtc;
> > plane_ = plane;
> > format_ = xFormat;
> > - break;
> > + return 0;
> > }
> > }
> > }
> > }
> >
> > - if (!crtc_) {
> > + return -EPIPE;
> > +}
> > +
> > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > +{
> > + const int ret = selectPipeline(format);
> > + if (ret) {
> > std::cerr
> > << "Unable to find display pipeline for format "
> > << format.toString() << std::endl;
> >
> > - return -EPIPE;
> > + return ret;
> > }
> >
> > std::cout
> > @@ -200,7 +206,7 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > << ", connector " << connector_->name()
> > << " (" << connector_->id() << ")" << std::endl;
> >
> > - return 0;
> > + return ret;
>
> This change isn't needed.
>
> With these changes,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> If you agree with those changes there's no need to resubmit, I'll make
> those small modifications before pushing.
Meant to reply all, sounds good to me.
>
> > }
> >
> > int KMSSink::start()
> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> > index 1e4290ad..4a0a872c 100644
> > --- a/src/cam/kms_sink.h
> > +++ b/src/cam/kms_sink.h
> > @@ -47,6 +47,7 @@ private:
> > libcamera::Request *camRequest_;
> > };
> >
> > + int selectPipeline(const libcamera::PixelFormat &format);
> > int configurePipeline(const libcamera::PixelFormat &format);
> > void requestComplete(DRM::AtomicRequest *request);
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list