[libcamera-devel,v3] cam: kms_sink: Remove limitation that camera and display must match
diff mbox series

Message ID 20220329130221.31972-1-ecurtin@redhat.com
State Accepted
Headers show
Series
  • [libcamera-devel,v3] cam: kms_sink: Remove limitation that camera and display must match
Related show

Commit Message

Eric Curtin March 29, 2022, 1:02 p.m. UTC 
There is a limitation that requires input and output to be pixel
for pixel identical in terms of height and width. Remove this
limitation to enable more hardware that doesn't match. Just start
drawing from top right 0, 0 corner.

Signed-off-by: Eric Curtin <ecurtin@redhat.com>
---
Changes in v2:
- Tested and support drawing from negative pixel range
  kernel parameter (video=960x540@60) was useful here
Changes in v3:
- Much simplified version of the patch where we just attempt to
  draw from point 0, 0. Only in the case where we do not find a
  matching mode. Can expand to do centralization, scaling, etc.
  in further patches if needs be.
---
 src/cam/kms_sink.cpp | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Eric Curtin April 15, 2022, 11:18 a.m. UTC | #1 
Another friendly reminder this adds value. The existing use cases are
unchanged (when identical capture and display resolutions are
displayed it uses that), but in the cases before in which we would
have failed, we attempt to draw from 0,0.

It's happens to me quite frequently that displays and cameras don't
match pixel for pixel.

Is mise le meas/Regards,

Eric Curtin


On Tue, 29 Mar 2022 at 14:02, Eric Curtin <ecurtin@redhat.com> wrote:
>
> There is a limitation that requires input and output to be pixel
> for pixel identical in terms of height and width. Remove this
> limitation to enable more hardware that doesn't match. Just start
> drawing from top right 0, 0 corner.
>
> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> ---
> Changes in v2:
> - Tested and support drawing from negative pixel range
>   kernel parameter (video=960x540@60) was useful here
> Changes in v3:
> - Much simplified version of the patch where we just attempt to
>   draw from point 0, 0. Only in the case where we do not find a
>   matching mode. Can expand to do centralization, scaling, etc.
>   in further patches if needs be.
> ---
>  src/cam/kms_sink.cpp | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index da579846..5847c4e1 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -119,17 +119,21 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
>                                                       mode.vdisplay == cfg.size.height;
>                                        });
>         if (iter == modes.end()) {
> -               std::cerr
> -                       << "No mode matching " << cfg.size.toString()
> -                       << std::endl;
> -               return -EINVAL;
> +                if (modes.empty()) {
> +                       std::cerr << "No modes\n";
> +                       return -EINVAL;
> +                }
> +
> +                mode_ = &modes[0];
>         }
> +        else {
> +                mode_ = &*iter;
> +        }
>
>         int ret = configurePipeline(cfg.pixelFormat);
>         if (ret < 0)
>                 return ret;
>
> -       mode_ = &*iter;
>         size_ = cfg.size;
>         stride_ = cfg.stride;
>
> @@ -297,12 +301,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)
>                 drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
>                 drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
>                 drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> -               drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
> -               drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
> +               drmRequest->addProperty(plane_, "SRC_W", size_.width << 16);
> +               drmRequest->addProperty(plane_, "SRC_H", size_.height << 16);
>                 drmRequest->addProperty(plane_, "CRTC_X", 0);
>                 drmRequest->addProperty(plane_, "CRTC_Y", 0);
> -               drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> -               drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> +               drmRequest->addProperty(plane_, "CRTC_W", size_.width);
> +               drmRequest->addProperty(plane_, "CRTC_H", size_.height);
>
>                 flags |= DRM::AtomicRequest::FlagAllowModeset;
>         }
> --
> 2.35.1
>
Laurent Pinchart April 15, 2022, 4:20 p.m. UTC | #2 
Hi Eric,

Thank you for the patch.

On Tue, Mar 29, 2022 at 02:02:21PM +0100, Eric Curtin via libcamera-devel wrote:
> There is a limitation that requires input and output to be pixel
> for pixel identical in terms of height and width. Remove this
> limitation to enable more hardware that doesn't match. Just start
> drawing from top right 0, 0 corner.

Isn't that the top-left corner ?

> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> ---
> Changes in v2:
> - Tested and support drawing from negative pixel range
>   kernel parameter (video=960x540@60) was useful here
> Changes in v3:
> - Much simplified version of the patch where we just attempt to
>   draw from point 0, 0. Only in the case where we do not find a
>   matching mode. Can expand to do centralization, scaling, etc.
>   in further patches if needs be.
> ---
>  src/cam/kms_sink.cpp | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index da579846..5847c4e1 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -119,17 +119,21 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
>  						      mode.vdisplay == cfg.size.height;
>  				       });
>  	if (iter == modes.end()) {
> -		std::cerr
> -			<< "No mode matching " << cfg.size.toString()
> -			<< std::endl;
> -		return -EINVAL;
> +                if (modes.empty()) {

There are spaces used for indentation.

> +        		std::cerr << "No modes\n";
> +        		return -EINVAL;
> +                }
> +
> +                mode_ = &modes[0];
>  	}
> +        else {
> +                mode_ = &*iter;
> +        }
>  
>  	int ret = configurePipeline(cfg.pixelFormat);
>  	if (ret < 0)
>  		return ret;
>  
> -	mode_ = &*iter;
>  	size_ = cfg.size;
>  	stride_ = cfg.stride;
>  
> @@ -297,12 +301,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)
>  		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
>  		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
>  		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> -		drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
> -		drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
> +		drmRequest->addProperty(plane_, "SRC_W", size_.width << 16);
> +		drmRequest->addProperty(plane_, "SRC_H", size_.height << 16);
>  		drmRequest->addProperty(plane_, "CRTC_X", 0);
>  		drmRequest->addProperty(plane_, "CRTC_Y", 0);
> -		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> -		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> +		drmRequest->addProperty(plane_, "CRTC_W", size_.width);
> +		drmRequest->addProperty(plane_, "CRTC_H", size_.height);

Not all drivers will support this. That's fine, it's not a regression as
they wouldn't work anyway, but I'm annoyed that things will fail only
when trying to display the first frame, and without a clear error
message :-(

Ideally we should try to probe the device to see what it supports (which
could include scaling) and then pick the best option in
KMSSink::configure().

>  
>  		flags |= DRM::AtomicRequest::FlagAllowModeset;
>  	}
Eric Curtin April 19, 2022, 2:24 p.m. UTC | #3 
On Fri, 15 Apr 2022 at 17:21, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On Tue, Mar 29, 2022 at 02:02:21PM +0100, Eric Curtin via libcamera-devel wrote:
> > There is a limitation that requires input and output to be pixel
> > for pixel identical in terms of height and width. Remove this
> > limitation to enable more hardware that doesn't match. Just start
> > drawing from top right 0, 0 corner.
>
> Isn't that the top-left corner ?

Yes sorry will change commit message.

>
> > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > ---
> > Changes in v2:
> > - Tested and support drawing from negative pixel range
> >   kernel parameter (video=960x540@60) was useful here
> > Changes in v3:
> > - Much simplified version of the patch where we just attempt to
> >   draw from point 0, 0. Only in the case where we do not find a
> >   matching mode. Can expand to do centralization, scaling, etc.
> >   in further patches if needs be.
> > ---
> >  src/cam/kms_sink.cpp | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > index da579846..5847c4e1 100644
> > --- a/src/cam/kms_sink.cpp
> > +++ b/src/cam/kms_sink.cpp
> > @@ -119,17 +119,21 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
> >                                                     mode.vdisplay == cfg.size.height;
> >                                      });
> >       if (iter == modes.end()) {
> > -             std::cerr
> > -                     << "No mode matching " << cfg.size.toString()
> > -                     << std::endl;
> > -             return -EINVAL;
> > +                if (modes.empty()) {
>
> There are spaces used for indentation.

Will change

>
> > +                     std::cerr << "No modes\n";
> > +                     return -EINVAL;
> > +                }
> > +
> > +                mode_ = &modes[0];
> >       }
> > +        else {
> > +                mode_ = &*iter;
> > +        }
> >
> >       int ret = configurePipeline(cfg.pixelFormat);
> >       if (ret < 0)
> >               return ret;
> >
> > -     mode_ = &*iter;
> >       size_ = cfg.size;
> >       stride_ = cfg.stride;
> >
> > @@ -297,12 +301,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)
> >               drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
> >               drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
> >               drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> > -             drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
> > -             drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
> > +             drmRequest->addProperty(plane_, "SRC_W", size_.width << 16);
> > +             drmRequest->addProperty(plane_, "SRC_H", size_.height << 16);
> >               drmRequest->addProperty(plane_, "CRTC_X", 0);
> >               drmRequest->addProperty(plane_, "CRTC_Y", 0);
> > -             drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> > -             drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> > +             drmRequest->addProperty(plane_, "CRTC_W", size_.width);
> > +             drmRequest->addProperty(plane_, "CRTC_H", size_.height);
>
> Not all drivers will support this. That's fine, it's not a regression as
> they wouldn't work anyway, but I'm annoyed that things will fail only
> when trying to display the first frame, and without a clear error
> message :-(

Yeah I tried to focus on just making sure this isn't a regression,
because for me, this helps me test this neat minimal KMS sink with no
dependencies on things like SDL and/or Qt. If people want to extend
after this, with various probing etc., great! Actually SDL is probably
a good reference there if anyone wants to tackle that, they seem to
have it figured out.

>
> Ideally we should try to probe the device to see what it supports (which
> could include scaling) and then pick the best option in
> KMSSink::configure().

Could I leave this to someone if they wanted to extend this? I was
trying to do the most minimal change to get this sink testable for
non-resolution identical devices. I need to see frames on the screen,
that's pretty much it, it's a testing tool for me.

I have zero issue with people altering this in future if they want to
do more probing, etc.

>
> >
> >               flags |= DRM::AtomicRequest::FlagAllowModeset;
> >       }
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
index da579846..5847c4e1 100644
--- a/src/cam/kms_sink.cpp
+++ b/src/cam/kms_sink.cpp
@@ -119,17 +119,21 @@  int KMSSink::configure(const libcamera::CameraConfiguration &config)
 						      mode.vdisplay == cfg.size.height;
 				       });
 	if (iter == modes.end()) {
-		std::cerr
-			<< "No mode matching " << cfg.size.toString()
-			<< std::endl;
-		return -EINVAL;
+                if (modes.empty()) {
+        		std::cerr << "No modes\n";
+        		return -EINVAL;
+                }
+
+                mode_ = &modes[0];
 	}
+        else {
+                mode_ = &*iter;
+        }
 
 	int ret = configurePipeline(cfg.pixelFormat);
 	if (ret < 0)
 		return ret;
 
-	mode_ = &*iter;
 	size_ = cfg.size;
 	stride_ = cfg.stride;
 
@@ -297,12 +301,12 @@  bool KMSSink::processRequest(libcamera::Request *camRequest)
 		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
 		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
 		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
-		drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
-		drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
+		drmRequest->addProperty(plane_, "SRC_W", size_.width << 16);
+		drmRequest->addProperty(plane_, "SRC_H", size_.height << 16);
 		drmRequest->addProperty(plane_, "CRTC_X", 0);
 		drmRequest->addProperty(plane_, "CRTC_Y", 0);
-		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
-		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
+		drmRequest->addProperty(plane_, "CRTC_W", size_.width);
+		drmRequest->addProperty(plane_, "CRTC_H", size_.height);
 
 		flags |= DRM::AtomicRequest::FlagAllowModeset;
 	}