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

[Laurent Pinchart]

Hi Eric,

On Wed, Dec 01, 2021 at 06:43:02PM +0000, Eric Curtin wrote:
> $ sudo modetest
> trying to open device 'i915'...done
> Encoders:
> id crtc type possible crtcs possible clones
> 238 98 TMDS 0x00000007 0x00000001
> 246 0 TMDS 0x00000007 0x00000002
> 
> Connectors:
> id encoder status name size (mm) modes encoders
> 239 238 connected eDP-1          340x190 1 238
>   modes:
> index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot
>   #0 1920x1080 59.98 1920 1968 2000 2080 1080 1083 1088 1111 138600
> flags: nhsync, nvsync; type: preferred, driver

[snip] (no other connector)

> CRTCs:
> id fb pos size
> 98 259 (0,0) (1920x1080)
>   #0 1920x1080 59.98 1920 1968 2000 2080 1080 1083 1088 1111 138600
> flags: nhsync, nvsync; type: preferred, driver

[snip]

> 167 0 (0,0) (0x0)
>   #0  -nan 0 0 0 0 0 0 0 0 0 flags: ; type:

[snip]

> 236 0 (0,0) (0x0)
>   #0  -nan 0 0 0 0 0 0 0 0 0 flags: ; type:

[snip]

According to this, it should be possible to configure a pipeline using
any CRTC to encoder 238 and connector 239 (eDP-1). I assume that's what
cam is doing, picking CRTC 236.

Let's first validate that this can actually work, can you get something
on the display when running

modetest -s 239 at 236:1920x1080

(if I remember the syntax correctly)

> On Wed, 1 Dec 2021 at 18:31, Laurent Pinchart wrote:
> > 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