[libcamera-devel] [PATCH] cam: kms_sink: select valid crtc

[Laurent Pinchart]

Hi Eric,

On Wed, Dec 01, 2021 at 06:03:43PM +0000, Eric Curtin wrote:
> Hi Laurent,
> 
> Thanks for welcoming me.
> 
> Another suggested solution would be welcome, let me describe
> what happens, it's been reproduced on multiple machines.
> 
> So here the the CRTCs output of modetest on my machine:
> 
> CRTCs:
> id      fb      pos     size
> 98      256     (0,0)   (1920x1080)
>   #0 1920x1080 59.98 1920 1968 2000 2080 1080 1083 1088 1111 138600
> flags: nhsync, nvsync; type: preferred, driver
> ...
> 167     0       (0,0)   (0x0)
>   #0  -nan 0 0 0 0 0 0 0 0 0 flags: ; type:
> ...
> 236     0       (0,0)   (0x0)
>   #0  -nan 0 0 0 0 0 0 0 0 0 flags: ; type:
> 
> So these lines are executed 3 times:
> 
> if (plane->supportsFormat(format)) {
>     crtc_ = crtc;
> 
> crtc_ = 98, break out of loop, crtc_ = 167, break out of loop, crtc_ =
> 236, break out of loop.
> 
> So crtc_ is set in the end to be the one with an id of 236, which doesn't
> display anything on my machine but 98 does.

The issue isn't so much the selected CRTC I believe, but the selected
connector (of course the CRTC plays a role there). Could you share the
full DRM device topology (CRTCs, encoders, connectors) ?

> A alternative to fix this would be apprecitaion!
> 
> I run like this:
> 
> build/src/cam/cam -c1 -DeDP-1 -spixelformat=YUYV -C0
> 
> On Wed, 1 Dec 2021 at 17:14, Laurent Pinchart wrote:
> >
> > Hi Eric,
> >
> > Thank you for the patch, and welcome to libcamera.
> >
> > On Wed, Dec 01, 2021 at 04:44:13PM +0000, Eric Curtin wrote:
> > > When looping through possibleCrtcs on my machine, the only valid crtc is
> > > actually the first one. This can be identified by checking if the clock
> > > variable is non-zero.
> > >
> > > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > > ---
> > >  src/cam/drm.cpp      | 2 +-
> > >  src/cam/drm.h        | 2 ++
> > >  src/cam/kms_sink.cpp | 4 ++++
> > >  3 files changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > > index f2530091..808e5a87 100644
> > > --- a/src/cam/drm.cpp
> > > +++ b/src/cam/drm.cpp
> > > @@ -128,7 +128,7 @@ std::unique_ptr<Blob> Mode::toBlob(Device *dev) const
> > >  }
> > >
> > >  Crtc::Crtc(Device *dev, const drmModeCrtc *crtc, unsigned int index)
> > > -     : Object(dev, crtc->crtc_id, Object::TypeCrtc), index_(index)
> > > +     : Object(dev, crtc->crtc_id, Object::TypeCrtc), index_(index), crtc_(crtc)
> > >  {
> > >  }
> > >
> > > diff --git a/src/cam/drm.h b/src/cam/drm.h
> > > index de57e445..d05e6d13 100644
> > > --- a/src/cam/drm.h
> > > +++ b/src/cam/drm.h
> > > @@ -149,12 +149,14 @@ public:
> > >       Crtc(Device *dev, const drmModeCrtc *crtc, unsigned int index);
> > >
> > >       unsigned int index() const { return index_; }
> > > +     uint32_t clock() const { return crtc_->mode.clock; }
> > >       const std::vector<const Plane *> &planes() const { return planes_; }
> > >
> > >  private:
> > >       friend Device;
> > >
> > >       unsigned int index_;
> > > +     const drmModeCrtc *crtc_;
> > >       std::vector<const Plane *> planes_;
> > >  };
> > >
> > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > > index d30fba78..aa0b7a3c 100644
> > > --- a/src/cam/kms_sink.cpp
> > > +++ b/src/cam/kms_sink.cpp
> > > @@ -166,6 +166,10 @@ int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > >        */
> > >       for (const DRM::Encoder *encoder : connector_->encoders()) {
> > >               for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {
> > > +                     if (!crtc->clock()) {
> > > +                             continue;
> > > +                     }
> >
> > I don't think this is quite right. When the Crtc instance is created,
> > drmModeCrtc.mode contains the currently configured mode. Any CRTC that
> > is not enabled when the cam application is started will thus be skipped,
> > even if it can be used.
> >
> > On a side note, no need for curly braces.
> >
> > > +
> > >                       for (const DRM::Plane *plane : crtc->planes()) {
> > >                               if (plane->type() != DRM::Plane::TypePrimary)
> > >                                       continue;
> >

-- 
Regards,

Laurent Pinchart