[libcamera-devel] [PATCH v2 7/8] cam: kms_sink: Enable display on first frame

[Laurent Pinchart]

Hi Paul,

On Wed, Aug 04, 2021 at 04:01:28PM +0900, paul.elder at 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 at 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 at 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