[libcamera-devel] [PATCH] cam: kms_sink: select valid crtc
Eric Curtin
ecurtin at redhat.com
Wed Dec 1 20:40:14 CET 2021
I can try tomorrow, it's getting closer to bedtime in Ireland.
One thing I don't understand about that for loop logic, is that it breaks
out of the inner-most loop on a valid entry but keeps looping because
of the outer loops. I don't understand why we keep looping.
Behaving like this basically instead, would actually fix it for me even
though it may not be the best fix, it's also faster of course:
/*
* Find a CRTC and plane suitable for the request format and the
* connector at the end of the pipeline. Restrict the search to primary
* planes for now.
*/
for (const DRM::Encoder* encoder : connector_->encoders()) {
for (const DRM::Crtc* crtc : encoder->possibleCrtcs()) {
for (const DRM::Plane* plane : crtc->planes()) {
if (plane->type() != DRM::Plane::TypePrimary)
continue;
if (plane->supportsFormat(format)) {
crtc_ = crtc;
plane_ = plane;
format_ = format;
goto break_all;
}
if (plane->supportsFormat(xFormat)) {
crtc_ = crtc;
plane_ = plane;
format_ = xFormat;
goto break_all;
}
}
}
}
break_all:
Is mise le meas/Regards,
Eric Curtin
On Wed, 1 Dec 2021 at 19:33, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> On Wed, Dec 01, 2021 at 07:05:15PM +0000, Eric Curtin wrote:
> > On Wed, 1 Dec 2021 at 18:55, Laurent Pinchart wrote:
> > > 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
> >
> > Yup sure. This seems to work fine,
> >
> > $ sudo modetest -s 239 at 236:1920x1080
> > trying to open device 'i915'...done
> > setting mode 1920x1080-59.98Hz on connectors 239, crtc 23
>
> So that configuration can work. The question now is why it doesn't work
> with cam. It's a bit hard for me to debug that remotely. It may be a
> matter of having to disable the CRTC that is already connected to the
> connector.
>
> This being said, we can also optimize the KMS sink in cam to pick a CRTC
> that would reuse an existing CRTC -> encoder -> connector route when one
> is available. I think it would also solve (or work around) your problem.
>
> Would you like to give either option a try ?
>
> > > (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
> > >
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list