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

Message ID 20200519032505.17307-8-laurent.pinchart@ideasonboard.com
State Changes Requested
Delegated to: Laurent Pinchart
Headers show
Series
  • libcamera: Add DRM/KMS viewfinder display to cam
Related show

Commit Message

Laurent Pinchart May 19, 2020, 3:25 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

Kieran Bingham June 5, 2020, 1:01 p.m. UTC | #1 
Hi Laurent,

On 19/05/2020 04:25, Laurent Pinchart wrote:
> Not all display controllers support enabling the display without any
> active plane. Delay display enabling to the first frame.

Presumably this patch can squash directly into patch 6/8 ?

> 
> 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(-)
> 
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index 85f244ea5413..a769255c022e 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -184,23 +184,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;
>  }
>  
> @@ -244,10 +227,17 @@ bool KMSSink::consumeBuffer(const libcamera::Stream *stream,
>  
>  	DRM::FrameBuffer *drmBuffer = iter->second.get();
>  
> +	unsigned int flags = DRM::AtomicRequest::FlagAsync;
>  	DRM::AtomicRequest *request = new DRM::AtomicRequest(&dev_);
>  	request->addProperty(plane_, "FB_ID", drmBuffer->id());
>  
> -	if (!planeInitialized_) {
> +	if (!active_ && !queued_) {
> +		/* Enable the display pipeline on the first frame. */
> +		request->addProperty(connector_, "CRTC_ID", crtc_->id());
> +
> +		request->addProperty(crtc_, "ACTIVE", 1);
> +		request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> +
>  		request->addProperty(plane_, "CRTC_ID", crtc_->id());
>  		request->addProperty(plane_, "SRC_X", 0 << 16);
>  		request->addProperty(plane_, "SRC_Y", 0 << 16);
> @@ -257,7 +247,8 @@ bool KMSSink::consumeBuffer(const libcamera::Stream *stream,
>  		request->addProperty(plane_, "CRTC_Y", 0);
>  		request->addProperty(plane_, "CRTC_W", mode_->hdisplay);
>  		request->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> -		planeInitialized_ = true;
> +
> +		flags |= DRM::AtomicRequest::FlagAllowModeset;
>  	}
>  
>  	pending_ = std::make_unique<Request>(request, buffer);
> @@ -265,7 +256,7 @@ bool KMSSink::consumeBuffer(const libcamera::Stream *stream,
>  	std::lock_guard<std::mutex> lock(lock_);
>  
>  	if (!queued_) {
> -		int ret = request->commit(DRM::AtomicRequest::FlagAsync);
> +		int ret = request->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 ee257a071c24..0879bb559e5a 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_;
>
Laurent Pinchart June 5, 2020, 2:59 p.m. UTC | #2 
Hi Kieran,

On Fri, Jun 05, 2020 at 02:01:15PM +0100, Kieran Bingham wrote:
> On 19/05/2020 04:25, Laurent Pinchart wrote:
> > Not all display controllers support enabling the display without any
> > active plane. Delay display enabling to the first frame.
> 
> Presumably this patch can squash directly into patch 6/8 ?

It could, I think I mentioned that in the cover letter. I've kept it
separate to ease review, and also to make it easier to revert the patch
should we reconsider the approach in the future, for instance with a
first frame containing a splash screen (or a black 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(-)
> > 
> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > index 85f244ea5413..a769255c022e 100644
> > --- a/src/cam/kms_sink.cpp
> > +++ b/src/cam/kms_sink.cpp
> > @@ -184,23 +184,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;
> >  }
> >  
> > @@ -244,10 +227,17 @@ bool KMSSink::consumeBuffer(const libcamera::Stream *stream,
> >  
> >  	DRM::FrameBuffer *drmBuffer = iter->second.get();
> >  
> > +	unsigned int flags = DRM::AtomicRequest::FlagAsync;
> >  	DRM::AtomicRequest *request = new DRM::AtomicRequest(&dev_);
> >  	request->addProperty(plane_, "FB_ID", drmBuffer->id());
> >  
> > -	if (!planeInitialized_) {
> > +	if (!active_ && !queued_) {
> > +		/* Enable the display pipeline on the first frame. */
> > +		request->addProperty(connector_, "CRTC_ID", crtc_->id());
> > +
> > +		request->addProperty(crtc_, "ACTIVE", 1);
> > +		request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> > +
> >  		request->addProperty(plane_, "CRTC_ID", crtc_->id());
> >  		request->addProperty(plane_, "SRC_X", 0 << 16);
> >  		request->addProperty(plane_, "SRC_Y", 0 << 16);
> > @@ -257,7 +247,8 @@ bool KMSSink::consumeBuffer(const libcamera::Stream *stream,
> >  		request->addProperty(plane_, "CRTC_Y", 0);
> >  		request->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> >  		request->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> > -		planeInitialized_ = true;
> > +
> > +		flags |= DRM::AtomicRequest::FlagAllowModeset;
> >  	}
> >  
> >  	pending_ = std::make_unique<Request>(request, buffer);
> > @@ -265,7 +256,7 @@ bool KMSSink::consumeBuffer(const libcamera::Stream *stream,
> >  	std::lock_guard<std::mutex> lock(lock_);
> >  
> >  	if (!queued_) {
> > -		int ret = request->commit(DRM::AtomicRequest::FlagAsync);
> > +		int ret = request->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 ee257a071c24..0879bb559e5a 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 85f244ea5413..a769255c022e 100644
--- a/src/cam/kms_sink.cpp
+++ b/src/cam/kms_sink.cpp
@@ -184,23 +184,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;
 }
 
@@ -244,10 +227,17 @@  bool KMSSink::consumeBuffer(const libcamera::Stream *stream,
 
 	DRM::FrameBuffer *drmBuffer = iter->second.get();
 
+	unsigned int flags = DRM::AtomicRequest::FlagAsync;
 	DRM::AtomicRequest *request = new DRM::AtomicRequest(&dev_);
 	request->addProperty(plane_, "FB_ID", drmBuffer->id());
 
-	if (!planeInitialized_) {
+	if (!active_ && !queued_) {
+		/* Enable the display pipeline on the first frame. */
+		request->addProperty(connector_, "CRTC_ID", crtc_->id());
+
+		request->addProperty(crtc_, "ACTIVE", 1);
+		request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
+
 		request->addProperty(plane_, "CRTC_ID", crtc_->id());
 		request->addProperty(plane_, "SRC_X", 0 << 16);
 		request->addProperty(plane_, "SRC_Y", 0 << 16);
@@ -257,7 +247,8 @@  bool KMSSink::consumeBuffer(const libcamera::Stream *stream,
 		request->addProperty(plane_, "CRTC_Y", 0);
 		request->addProperty(plane_, "CRTC_W", mode_->hdisplay);
 		request->addProperty(plane_, "CRTC_H", mode_->vdisplay);
-		planeInitialized_ = true;
+
+		flags |= DRM::AtomicRequest::FlagAllowModeset;
 	}
 
 	pending_ = std::make_unique<Request>(request, buffer);
@@ -265,7 +256,7 @@  bool KMSSink::consumeBuffer(const libcamera::Stream *stream,
 	std::lock_guard<std::mutex> lock(lock_);
 
 	if (!queued_) {
-		int ret = request->commit(DRM::AtomicRequest::FlagAsync);
+		int ret = request->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 ee257a071c24..0879bb559e5a 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_;