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

Message ID 20220207150136.22584-1-ecurtin@redhat.com
State Accepted
Headers show
Series
  • [libcamera-devel,v3] cam: kms-sink: Once we have found a suitable pipeline to select, return
Related show

Commit Message

Eric Curtin Feb. 7, 2022, 3:01 p.m. UTC 
The break only breaks out of the innermost loop which is not the most
optimal. It also breaks cam on my machines.

Fixes: 1de0f90dd432 ("cam: kms_sink: Print display pipelineconfiguration")
Signed-off-by: Eric Curtin <ecurtin@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(-)

Comments

Laurent Pinchart Feb. 7, 2022, 10:28 p.m. UTC | #1 
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@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@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);
>
Eric Curtin Feb. 8, 2022, 8:09 a.m. UTC | #2 
On Mon, 7 Feb 2022 at 22:28, Laurent Pinchart
<laurent.pinchart@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@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@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
>
Kieran Bingham Feb. 8, 2022, 9:33 a.m. UTC | #3 
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@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@ideasonboard.com>

> Reviewed-by: Laurent Pinchart <laurent.pinchart@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

Patch
diff mbox series 

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;
 }
 
 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);