[libcamera-devel] [PATCH v3] cam: kms-sink: Once we have found a suitable pipeline to select, return

[Kieran Bingham]

Quoting Laurent Pinchart (2022-02-07 22:28:16)
> 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;
> 

In fact that was the only part that stopped me sending a tag yesterday
as I didn't spend the time to go look up if this return ret made sense.

> This change isn't needed.
> 
> With these changes,
> 

With that, 

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 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.
> 
> >  }
> >  
> >  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