On Thu, Feb 01, 2018 at 05:42:35AM +0100, Mauro Rossi wrote:
> Il 08/gen/2018 09:46 PM, "Sean Paul" <seanp...@chromium.org> ha scritto:
>
> On Mon, Jan 08, 2018 at 03:41:49PM -0500, Sean Paul wrote:
> > On Sat, Jan 06, 2018 at 12:59:58AM +0100, Mauro Rossi wrote:
> > > Porting of original commit 76fb87e675 of Jim Bish in android-ia master
> to fdo
> > >
> > > Original commit message:
> > > "There are various places where we should be really taking connection
> > > state into account before querying the properties or assuming it
> > > as primary. This patch fixes them."
> > >
> > > (v2) checks on connection state are applied for both internal and
> external
> > > connectors, in order to select the correct primary, as opposed to
> setting,
> > > independently from its state, the first connector as primary
> > >
> > > This is essential to avoid following logcat errors on integrated and
> dedicated GPUs:
> > >
> > > ... 2245 2245 E hwc-drm-resources: Could not find a suitable
> encoder/crtc for display 2
> > > ... 2245 2245 E hwc-drm-resources: Failed CreateDisplayPipe 56 with -19
> > > ... 2245 2245 E hwcomposer-drm: Can't initialize Drm object -19
> > >
> > > Tested with i965 on Sandybridge and nouveau on GT120, GT610
> > > ---
> > > drmresources.cpp | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > index 32dd376..d582cfe 100644
> > > --- a/drmresources.cpp
> > > +++ b/drmresources.cpp
> > > @@ -159,7 +159,7 @@ int DrmResources::Init() {
> > >
> > > // First look for primary amongst internal connectors
> > > for (auto &conn : connectors_) {
> > > - if (conn->internal() && !found_primary) {
> > > + if (conn->state() == DRM_MODE_CONNECTED && conn->internal() &&
> !found_primary) {
>
> One more thing. How do you know this is the right thing to do? What if the
> internal panel is not connected initially, but becomes connected in the
> future?
> IIUC, you'll end up numbering it incorrectly.
>
> Sean
>
>
> Unfortunately I don't have knowledge/experience about the drm mode code,
> but analyzing logs in nouveau with dedicated GPU shows a problem in current
> code.
>
> You are asking if taking connection state into account will work in dynamic
> scenario of plugging/unplugging cable/conn,
> but let's start from acknowledging that current code results in 'no screen
> output', because it bails out too soon, and taking connection state into
> account allows to boot with nouveau, which is the goal of having moved
> drm_hwcomposer to fd.o
>
> IMHO to bo able to boot Android drm_hwcomposer+gbm_gralloc with nouveau is
> a substantial improvement
I completely agree! However, I don't think it's unreasonable to have discussion
around how we fix bugs.
I'm concerned that while this will result in a working setup if everything is
plugged on startup, it creates new problems around hotplugging. For instance, by
gating CreateDisplayPipe on the display being connected, we're no longer mapping
crtc/encoders to displays. I think this will cause problems down the road if
a monitor is plugged into one of these skipped displays. So this patch fixes a
bug by introducing a regression.
Sean
>
>
>
> > > conn->set_display(0);
> > > found_primary = true;
> > > } else {
> > > @@ -170,7 +170,7 @@ int DrmResources::Init() {
> > >
> > > // Then look for primary amongst external connectors
> > > for (auto &conn : connectors_) {
> > > - if (conn->external() && !found_primary) {
> > > + if (conn->state() == DRM_MODE_CONNECTED && conn->external() &&
> !found_primary) {
> >
> > These 2 lines exceed the max character limit. Did you run clang-format?
> >
> > Anyways, I think it'd be nice to add a connected() helper to the connector
> > object which would look cleaner and solve the long lines.
> >
> > Sean
>
>
> Thanks for feedback, we will have a look with Robert to improve coding style
>
> >
> > > conn->set_display(0);
> > > found_primary = true;
> > > }
> > > @@ -288,6 +288,11 @@ int DrmResources::TryEncoderForDisplay(int
> display, DrmEncoder *enc) {
> > >
> > > int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
> > > int display = connector->display();
> > > +
> > > + // skip not connected
> > > + if (connector->state() == DRM_MODE_DISCONNECTED)
> > > + return 0;
> > > +
> > > /* Try to use current setup first */
> > > if (connector->encoder()) {
> > > int ret = TryEncoderForDisplay(display, connector->encoder());
> > > --
> > > 2.14.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
