[libcamera-devel,v2,7/8] cam: kms_sink: Enable display on first frame
diff mbox series

Message ID 20210730010306.19956-8-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Add DRM/KMS viewfinder display to cam
Related show

Commit Message

Laurent Pinchart July 30, 2021, 1:03 a.m. UTC 
Not all display controllers support enabling the display without any
active plane. Delay display enabling to the first frame.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/cam/kms_sink.cpp | 31 +++++++++++--------------------
 src/cam/kms_sink.h   |  2 --
 2 files changed, 11 insertions(+), 22 deletions(-)

Comments

Paul Elder Aug. 4, 2021, 7:01 a.m. UTC | #1 
Hi Laurent,

On Fri, Jul 30, 2021 at 04:03:05AM +0300, Laurent Pinchart wrote:
> Not all display controllers support enabling the display without any
> active plane. Delay display enabling to the first frame.

I don't have enough experience or information to judge whether or not
the other choices (as you outlined in the cover letter) are better :/

But from what you explained, I think this is a fine solution.

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I assume this will be squashed into 6/8 after you've confirmed that this
is the direction that you want?

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/cam/kms_sink.cpp | 31 +++++++++++--------------------
>  src/cam/kms_sink.h   |  2 --
>  2 files changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index b8f86dcb6f4e..2c767cb40885 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -185,23 +185,6 @@ int KMSSink::start()
>  		return ret;
>  	}
>  
> -	/* Enable the display pipeline with no plane to start with. */
> -	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> -
> -	request->addProperty(connector_, "CRTC_ID", crtc_->id());
> -	request->addProperty(crtc_, "ACTIVE", 1);
> -	request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> -
> -	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> -	if (ret < 0) {
> -		std::cerr
> -			<< "Failed to enable display pipeline: "
> -			<< strerror(-ret) << std::endl;
> -		return ret;
> -	}
> -
> -	planeInitialized_ = false;
> -
>  	return 0;
>  }
>  
> @@ -245,10 +228,17 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)
>  
>  	DRM::FrameBuffer *drmBuffer = iter->second.get();
>  
> +	unsigned int flags = DRM::AtomicRequest::FlagAsync;
>  	DRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);
>  	drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
>  
> -	if (!planeInitialized_) {
> +	if (!active_ && !queued_) {
> +		/* Enable the display pipeline on the first frame. */
> +		drmRequest->addProperty(connector_, "CRTC_ID", crtc_->id());
> +
> +		drmRequest->addProperty(crtc_, "ACTIVE", 1);
> +		drmRequest->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> +
>  		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
>  		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
>  		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> @@ -258,7 +248,8 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)
>  		drmRequest->addProperty(plane_, "CRTC_Y", 0);
>  		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
>  		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> -		planeInitialized_ = true;
> +
> +		flags |= DRM::AtomicRequest::FlagAllowModeset;
>  	}
>  
>  	pending_ = std::make_unique<Request>(drmRequest, camRequest);
> @@ -266,7 +257,7 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)
>  	std::lock_guard<std::mutex> lock(lock_);
>  
>  	if (!queued_) {
> -		int ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);
> +		int ret = drmRequest->commit(flags);
>  		if (ret < 0)
>  			std::cerr
>  				<< "Failed to commit atomic request: "
> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> index 7b6ffcede28c..8536ef2e532d 100644
> --- a/src/cam/kms_sink.h
> +++ b/src/cam/kms_sink.h
> @@ -63,8 +63,6 @@ private:
>  	libcamera::Size size_;
>  	unsigned int stride_;
>  
> -	bool planeInitialized_;
> -
>  	std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
>  
>  	std::mutex lock_;
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Aug. 4, 2021, 8:58 a.m. UTC | #2 
Hi Paul,

On Wed, Aug 04, 2021 at 04:01:28PM +0900, paul.elder@ideasonboard.com wrote:
> On Fri, Jul 30, 2021 at 04:03:05AM +0300, Laurent Pinchart wrote:
> > Not all display controllers support enabling the display without any
> > active plane. Delay display enabling to the first frame.
> 
> I don't have enough experience or information to judge whether or not
> the other choices (as you outlined in the cover letter) are better :/
> 
> But from what you explained, I think this is a fine solution.

I think so too :-)

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I assume this will be squashed into 6/8 after you've confirmed that this
> is the direction that you want?

I can do that, yes, but I'm tempted to keep it separate so that the
history could provide useful information about why the code is
implemented this way.

> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> > ---
> >  src/cam/kms_sink.cpp | 31 +++++++++++--------------------
> >  src/cam/kms_sink.h   |  2 --
> >  2 files changed, 11 insertions(+), 22 deletions(-)
> > 
> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > index b8f86dcb6f4e..2c767cb40885 100644
> > --- a/src/cam/kms_sink.cpp
> > +++ b/src/cam/kms_sink.cpp
> > @@ -185,23 +185,6 @@ int KMSSink::start()
> >  		return ret;
> >  	}
> >  
> > -	/* Enable the display pipeline with no plane to start with. */
> > -	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> > -
> > -	request->addProperty(connector_, "CRTC_ID", crtc_->id());
> > -	request->addProperty(crtc_, "ACTIVE", 1);
> > -	request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> > -
> > -	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> > -	if (ret < 0) {
> > -		std::cerr
> > -			<< "Failed to enable display pipeline: "
> > -			<< strerror(-ret) << std::endl;
> > -		return ret;
> > -	}
> > -
> > -	planeInitialized_ = false;
> > -
> >  	return 0;
> >  }
> >  
> > @@ -245,10 +228,17 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)
> >  
> >  	DRM::FrameBuffer *drmBuffer = iter->second.get();
> >  
> > +	unsigned int flags = DRM::AtomicRequest::FlagAsync;
> >  	DRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);
> >  	drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
> >  
> > -	if (!planeInitialized_) {
> > +	if (!active_ && !queued_) {
> > +		/* Enable the display pipeline on the first frame. */
> > +		drmRequest->addProperty(connector_, "CRTC_ID", crtc_->id());
> > +
> > +		drmRequest->addProperty(crtc_, "ACTIVE", 1);
> > +		drmRequest->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> > +
> >  		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
> >  		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
> >  		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> > @@ -258,7 +248,8 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)
> >  		drmRequest->addProperty(plane_, "CRTC_Y", 0);
> >  		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> >  		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> > -		planeInitialized_ = true;
> > +
> > +		flags |= DRM::AtomicRequest::FlagAllowModeset;
> >  	}
> >  
> >  	pending_ = std::make_unique<Request>(drmRequest, camRequest);
> > @@ -266,7 +257,7 @@ bool KMSSink::consumeRequest(libcamera::Request *camRequest)
> >  	std::lock_guard<std::mutex> lock(lock_);
> >  
> >  	if (!queued_) {
> > -		int ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);
> > +		int ret = drmRequest->commit(flags);
> >  		if (ret < 0)
> >  			std::cerr
> >  				<< "Failed to commit atomic request: "
> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> > index 7b6ffcede28c..8536ef2e532d 100644
> > --- a/src/cam/kms_sink.h
> > +++ b/src/cam/kms_sink.h
> > @@ -63,8 +63,6 @@ private:
> >  	libcamera::Size size_;
> >  	unsigned int stride_;
> >  
> > -	bool planeInitialized_;
> > -
> >  	std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
> >  
> >  	std::mutex lock_;

Patch
diff mbox series 

diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
index b8f86dcb6f4e..2c767cb40885 100644
--- a/src/cam/kms_sink.cpp
+++ b/src/cam/kms_sink.cpp
@@ -185,23 +185,6 @@  int KMSSink::start()
 		return ret;
 	}
 
-	/* Enable the display pipeline with no plane to start with. */
-	request = std::make_unique<DRM::AtomicRequest>(&dev_);
-
-	request->addProperty(connector_, "CRTC_ID", crtc_->id());
-	request->addProperty(crtc_, "ACTIVE", 1);
-	request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
-
-	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
-	if (ret < 0) {
-		std::cerr
-			<< "Failed to enable display pipeline: "
-			<< strerror(-ret) << std::endl;
-		return ret;
-	}
-
-	planeInitialized_ = false;
-
 	return 0;
 }
 
@@ -245,10 +228,17 @@  bool KMSSink::consumeRequest(libcamera::Request *camRequest)
 
 	DRM::FrameBuffer *drmBuffer = iter->second.get();
 
+	unsigned int flags = DRM::AtomicRequest::FlagAsync;
 	DRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);
 	drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
 
-	if (!planeInitialized_) {
+	if (!active_ && !queued_) {
+		/* Enable the display pipeline on the first frame. */
+		drmRequest->addProperty(connector_, "CRTC_ID", crtc_->id());
+
+		drmRequest->addProperty(crtc_, "ACTIVE", 1);
+		drmRequest->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
+
 		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
 		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
 		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
@@ -258,7 +248,8 @@  bool KMSSink::consumeRequest(libcamera::Request *camRequest)
 		drmRequest->addProperty(plane_, "CRTC_Y", 0);
 		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
 		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
-		planeInitialized_ = true;
+
+		flags |= DRM::AtomicRequest::FlagAllowModeset;
 	}
 
 	pending_ = std::make_unique<Request>(drmRequest, camRequest);
@@ -266,7 +257,7 @@  bool KMSSink::consumeRequest(libcamera::Request *camRequest)
 	std::lock_guard<std::mutex> lock(lock_);
 
 	if (!queued_) {
-		int ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);
+		int ret = drmRequest->commit(flags);
 		if (ret < 0)
 			std::cerr
 				<< "Failed to commit atomic request: "
diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
index 7b6ffcede28c..8536ef2e532d 100644
--- a/src/cam/kms_sink.h
+++ b/src/cam/kms_sink.h
@@ -63,8 +63,6 @@  private:
 	libcamera::Size size_;
 	unsigned int stride_;
 
-	bool planeInitialized_;
-
 	std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
 
 	std::mutex lock_;