Re: [drm_hwcomposer] [PATCH] Take Connection state into account. (v2)
On Mon, Jan 8, 2018 at 9:46 PM, Sean Paulwrote: > 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 : 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. I'm not sure how consistent it all is, but the internal panel can report as disconnected when e.g. the lid is closed, on at least some drivers. So sounds like a real scenario to me. If the panel isn't even there, then the driver shouldn't even register the connector (or it's a driver bug). So maybe just don't filter the internal connectors? -Daniel > > 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 : 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 >> >> >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 > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [drm_hwcomposer] [PATCH] Take Connection state into account. (v2)
On Thu, Feb 01, 2018 at 05:42:35AM +0100, Mauro Rossi wrote: > Il 08/gen/2018 09:46 PM, "Sean Paul"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 : 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 : 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
Re: [drm_hwcomposer] [PATCH] Take Connection state into account. (v2)
Il 08/gen/2018 09:46 PM, "Sean Paul"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 : 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 > >conn->set_display(0); > >found_primary = true; > > } else { > > @@ -170,7 +170,7 @@ int DrmResources::Init() { > > > >// Then look for primary amongst external connectors > >for (auto : 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [drm_hwcomposer] [PATCH] Take Connection state into account. (v2)
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 : 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 > >conn->set_display(0); > >found_primary = true; > > } else { > > @@ -170,7 +170,7 @@ int DrmResources::Init() { > > > >// Then look for primary amongst external connectors > >for (auto : 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 > > >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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [drm_hwcomposer] [PATCH] Take Connection state into account. (v2)
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 : connectors_) { > -if (conn->internal() && !found_primary) { > +if (conn->state() == DRM_MODE_CONNECTED && conn->internal() && > !found_primary) { >conn->set_display(0); >found_primary = true; > } else { > @@ -170,7 +170,7 @@ int DrmResources::Init() { > >// Then look for primary amongst external connectors >for (auto : 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 >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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [drm_hwcomposer] [PATCH] Take Connection state into account. (v2)
Hey Mauro! Thanks for the v2, I would like to merge this, but the commit message is a little bit wonky still :) Let me clean it up for you, and if you're fine with me adding your S-o-B I'll push it. Also, if you want to avoid the slow mailing list back and forth, I would happily help out over IRC too. You can find me on freenode with the nick robertfoss. #dri-devel is also a good channel for this kind of work. On 1/6/18 12:59 AM, 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 This is what I would expect the commit message to look like: Take Connection state into account 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. 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 Signed-off-by: Jim BishSigned-off-by: Mauro Rossi --- 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 : connectors_) { -if (conn->internal() && !found_primary) { +if (conn->state() == DRM_MODE_CONNECTED && conn->internal() && !found_primary) { conn->set_display(0); found_primary = true; } else { @@ -170,7 +170,7 @@ int DrmResources::Init() { // Then look for primary amongst external connectors for (auto : connectors_) { -if (conn->external() && !found_primary) { +if (conn->state() == DRM_MODE_CONNECTED && conn->external() && !found_primary) { 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()); ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[drm_hwcomposer] [PATCH] Take Connection state into account. (v2)
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 : connectors_) { -if (conn->internal() && !found_primary) { +if (conn->state() == DRM_MODE_CONNECTED && conn->internal() && !found_primary) { conn->set_display(0); found_primary = true; } else { @@ -170,7 +170,7 @@ int DrmResources::Init() { // Then look for primary amongst external connectors for (auto : connectors_) { -if (conn->external() && !found_primary) { +if (conn->state() == DRM_MODE_CONNECTED && conn->external() && !found_primary) { 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
Re: [drm_hwcomposer PATCH] Take Connection state into account. (v2)
On Wed, Jan 3, 2018 at 6:40 AM, Mauro Rossiwrote: > > > 2018-01-03 12:16 GMT+01:00 Robert Foss : >> >> Hey Mauro, >> >> Thanks for the patch! It builds and looks good to me, but I have some >> suggestions however. >> >> >> On Wed, 2018-01-03 at 11:10 +0100, Mauro Rossi wrote: >> > These changes avoid following logcat error 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 >> >> It isn't quite clear clear which errors belong to which changes, >> to make this patch a bit more bisectable it would be nice to see >> independent commits created for each error. > > > Hi Robert, > > In this case I honestly do not think that splitting would add too much > value, > original commit (v1) is well documented in [1] and tackles with bug in > drmresources.cpp > which currently fails to find workable crtc -> encoder -> display output > combination > for integrated and dedicated GPUs. Besides that it was also adding > DisplayPort drm mode connector type > > So changes in drmresources.cpp belong to solving "Could not find a suitable > encoder/crtc for display X" problem, > which is a show stopper for enabling mainline graphics in android-x86 > > Other developers observed independently that the current implementation is > "embedded oriented" and only checks the first display output, > isn't it? > > The only change I did in drmconnector.cpp is to account for the additional > external drm mode connectors types > which were missing (DVID, DVII, VGA) . One question on my side: Do we need > more? > > Besides these minor types lists discussions, I would propose you to check > with Jim Bish if he should have the credit for the patch > and review the coding of his changes. > > >> >> > >> > (v1) 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. >> > >> > BUG=None. >> > TEST=System boots up and shows UI. >> > >> > (v1) Signed-off-by: Jim Bish >> > >> > (v2) porting of original commit 76fb87e675 of android-ia master >> > with additional external connector types (DisplayPort, DVID, DVII, >> > VGA) >> > Tested with i965 on Sandybridge and nouveau on GT120, GT610 >> >> The commit message isn't really following the usual format. It doesn't >> need to include a changelog (that is typically placed between the "---" >> markers in the email instead), >> and it is missing a signoff from the submitter (you). > > > Sorry I don't have a signature, as usually my patches need sign-off from > professionals, > I'm a (crash test dummy) hobbyist..really :-) That's not what S-o-b means. > Maybe Rob Herring, Qiang Yu or Chih-Wei could review and sign-off the > proposed patch The S-o-b is the Developer's Certificate of Origin[1]. It's tracking the author(s) and vouching that the code has the appropriate license. It can be the original author (Jim), modifier (you), and/or committer (Robert or me). Rob [1] https://developercertificate.org/ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [drm_hwcomposer PATCH] Take Connection state into account. (v2)
2018-01-03 12:16 GMT+01:00 Robert Foss: > Hey Mauro, > > Thanks for the patch! It builds and looks good to me, but I have some > suggestions however. > > > On Wed, 2018-01-03 at 11:10 +0100, Mauro Rossi wrote: > > These changes avoid following logcat error 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 > > It isn't quite clear clear which errors belong to which changes, > to make this patch a bit more bisectable it would be nice to see > independent commits created for each error. > Hi Robert, In this case I honestly do not think that splitting would add too much value, original commit (v1) is well documented in [1] and tackles with bug in drmresources.cpp which currently fails to find workable crtc -> encoder -> display output combination for integrated and dedicated GPUs. Besides that it was also adding DisplayPort drm mode connector type So changes in drmresources.cpp belong to solving "Could not find a suitable encoder/crtc for display X" problem, which is a show stopper for enabling mainline graphics in android-x86 Other developers observed independently that the current implementation is "embedded oriented" and only checks the first display output, isn't it? The only change I did in drmconnector.cpp is to account for the additional external drm mode connectors types which were missing (DVID, DVII, VGA) . One question on my side: Do we need more? Besides these minor types lists discussions, I would propose you to check with Jim Bish if he should have the credit for the patch and review the coding of his changes. > > > > (v1) 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. > > > > BUG=None. > > TEST=System boots up and shows UI. > > > > (v1) Signed-off-by: Jim Bish > > > > (v2) porting of original commit 76fb87e675 of android-ia master > > with additional external connector types (DisplayPort, DVID, DVII, > > VGA) > > Tested with i965 on Sandybridge and nouveau on GT120, GT610 > > The commit message isn't really following the usual format. It doesn't > need to include a changelog (that is typically placed between the "---" > markers in the email instead), > and it is missing a signoff from the submitter (you). > Sorry I don't have a signature, as usually my patches need sign-off from professionals, I'm a (crash test dummy) hobbyist..really :-) Maybe Rob Herring, Qiang Yu or Chih-Wei could review and sign-off the proposed patch > The BUG and TEST fields are not strictly required either, but aren't a > problem. > That part is the original Jim Bish commit message [1], my changes version is (v2) Sorry if I was not clear enought > > > --- > > drmconnector.cpp | 4 +++- > > drmresources.cpp | 9 +++-- > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drmconnector.cpp b/drmconnector.cpp > > index 247f56b..145518f 100644 > > --- a/drmconnector.cpp > > +++ b/drmconnector.cpp > > @@ -73,7 +73,9 @@ bool DrmConnector::internal() const { > > } > > > > bool DrmConnector::external() const { > > - return type_ == DRM_MODE_CONNECTOR_HDMIA; > > + return type_ == DRM_MODE_CONNECTOR_HDMIA || type_ == > > DRM_MODE_CONNECTOR_DisplayPort || > > + type_ == DRM_MODE_CONNECTOR_DVID || type_ == > > DRM_MODE_CONNECTOR_DVII || > > + type_ == DRM_MODE_CONNECTOR_VGA; > > } > > The changes to external() should probably be broken out into it's own > commit, since external() is called elsewhere changes to it _could_ > introduce issues. > Will you check the code, involving Jim Bish if necessary, about these potential issues? I am available to perform changes/tests, but the original maker will be better. Cheers Mauro > > > > bool DrmConnector::valid_type() const { > > 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 : connectors_) { > > -if (conn->internal() && !found_primary) { > > +if (conn->state() == DRM_MODE_CONNECTED && conn->internal() && > > !found_primary) { > >conn->set_display(0); > >found_primary = true; > > } else { > > @@ -170,7 +170,7 @@ int DrmResources::Init() { > > > >// Then look for primary amongst external connectors > >for (auto : connectors_) { > > -if (conn->external() && !found_primary) { > > +if (conn->state() == DRM_MODE_CONNECTED && conn->external() && > > !found_primary) { > >conn->set_display(0); > >found_primary = true; > > } > > @@
[drm_hwcomposer PATCH] Take Connection state into account. (v2)
These changes avoid following logcat error 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 (v1) 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. BUG=None. TEST=System boots up and shows UI. (v1) Signed-off-by: Jim Bish(v2) porting of original commit 76fb87e675 of android-ia master with additional external connector types (DisplayPort, DVID, DVII, VGA) Tested with i965 on Sandybridge and nouveau on GT120, GT610 --- drmconnector.cpp | 4 +++- drmresources.cpp | 9 +++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/drmconnector.cpp b/drmconnector.cpp index 247f56b..145518f 100644 --- a/drmconnector.cpp +++ b/drmconnector.cpp @@ -73,7 +73,9 @@ bool DrmConnector::internal() const { } bool DrmConnector::external() const { - return type_ == DRM_MODE_CONNECTOR_HDMIA; + return type_ == DRM_MODE_CONNECTOR_HDMIA || type_ == DRM_MODE_CONNECTOR_DisplayPort || + type_ == DRM_MODE_CONNECTOR_DVID || type_ == DRM_MODE_CONNECTOR_DVII || + type_ == DRM_MODE_CONNECTOR_VGA; } bool DrmConnector::valid_type() const { 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 : connectors_) { -if (conn->internal() && !found_primary) { +if (conn->state() == DRM_MODE_CONNECTED && conn->internal() && !found_primary) { conn->set_display(0); found_primary = true; } else { @@ -170,7 +170,7 @@ int DrmResources::Init() { // Then look for primary amongst external connectors for (auto : connectors_) { -if (conn->external() && !found_primary) { +if (conn->state() == DRM_MODE_CONNECTED && conn->external() && !found_primary) { 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
Re: [drm_hwcomposer PATCH] Take Connection state into account. (v2)
On Wed, 2018-01-03 at 13:40 +0100, Mauro Rossi wrote: 2018-01-03 12:16 GMT+01:00 Robert Foss>: Hey Mauro, Thanks for the patch! It builds and looks good to me, but I have some suggestions however. On Wed, 2018-01-03 at 11:10 +0100, Mauro Rossi wrote: > These changes avoid following logcat error 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 It isn't quite clear clear which errors belong to which changes, to make this patch a bit more bisectable it would be nice to see independent commits created for each error. Hi Robert, In this case I honestly do not think that splitting would add too much value, original commit (v1) is well documented in [1] and tackles with bug in drmresources.cpp which currently fails to find workable crtc -> encoder -> display output combination for integrated and dedicated GPUs. Besides that it was also adding DisplayPort drm mode connector type So changes in drmresources.cpp belong to solving "Could not find a suitable encoder/crtc for display X" problem, which is a show stopper for enabling mainline graphics in android-x86 Other developers observed independently that the current implementation is "embedded oriented" and only checks the first display output, isn't it? The only change I did in drmconnector.cpp is to account for the additional external drm mode connectors types which were missing (DVID, DVII, VGA) . One question on my side: Do we need more? Besides these minor types lists discussions, I would propose you to check with Jim Bish if he should have the credit for the patch and review the coding of his changes. it would be proper to keep the original author signoff but ok to add additional signoff/testing, etc. I am not so worried about credit for this just patch but good to following proper procedures. > > (v1) 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. > > BUG=None. > TEST=System boots up and shows UI. > > (v1) Signed-off-by: Jim Bish > > > (v2) porting of original commit 76fb87e675 of android-ia master > with additional external connector types (DisplayPort, DVID, DVII, > VGA) > Tested with i965 on Sandybridge and nouveau on GT120, GT610 The commit message isn't really following the usual format. It doesn't need to include a changelog (that is typically placed between the "---" markers in the email instead), and it is missing a signoff from the submitter (you). Sorry I don't have a signature, as usually my patches need sign-off from professionals, I'm a (crash test dummy) hobbyist..really :-) Maybe Rob Herring, Qiang Yu or Chih-Wei could review and sign-off the proposed patch The BUG and TEST fields are not strictly required either, but aren't a problem. That part is the original Jim Bish commit message [1], my changes version is (v2) Sorry if I was not clear enought > --- > drmconnector.cpp | 4 +++- > drmresources.cpp | 9 +++-- > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drmconnector.cpp b/drmconnector.cpp > index 247f56b..145518f 100644 > --- a/drmconnector.cpp > +++ b/drmconnector.cpp > @@ -73,7 +73,9 @@ bool DrmConnector::internal() const { > } > > bool DrmConnector::external() const { > - return type_ == DRM_MODE_CONNECTOR_HDMIA; > + return type_ == DRM_MODE_CONNECTOR_HDMIA || type_ == > DRM_MODE_CONNECTOR_DisplayPort || > + type_ == DRM_MODE_CONNECTOR_DVID || type_ == > DRM_MODE_CONNECTOR_DVII || > + type_ == DRM_MODE_CONNECTOR_VGA; > } The changes to external() should probably be broken out into it's own commit, since external() is called elsewhere changes to it _could_ introduce issues. Will you check the code, involving Jim Bish if necessary, about these potential issues? I am available to perform changes/tests, but the original maker will be better. Cheers Mauro > > bool DrmConnector::valid_type() const { > 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 : connectors_) { > -if (conn->internal() && !found_primary) { > +if (conn->state() == DRM_MODE_CONNECTED && conn->internal() && > !found_primary) { >conn->set_display(0); >found_primary = true; > } else { > @@ -170,7 +170,7 @@ int DrmResources::Init() { > >// Then look for primary amongst external connectors >for (auto : connectors_) { > -if (conn->external() && !found_primary) { > +if (conn->state() ==
Re: [drm_hwcomposer PATCH] Take Connection state into account. (v2)
Hey Mauro, On Wed, 2018-01-03 at 13:40 +0100, Mauro Rossi wrote: > > > 2018-01-03 12:16 GMT+01:00 Robert Foss: > > Hey Mauro, > > / > > Thanks for the patch! It builds and looks good to me, but I have > > some > > suggestions however. > > > > > > On Wed, 2018-01-03 at 11:10 +0100, Mauro Rossi wrote: > > > These changes avoid following logcat error 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 > > > > It isn't quite clear clear which errors belong to which changes, > > to make this patch a bit more bisectable it would be nice to see > > independent commits created for each error. > > Hi Robert, > > In this case I honestly do not think that splitting would add too > much value, > original commit (v1) is well documented in [1] and tackles with bug > in drmresources.cpp > which currently fails to find workable crtc -> encoder -> display > output combination > for integrated and dedicated GPUs. Besides that it was also adding > DisplayPort drm mode connector type > > So changes in drmresources.cpp belong to solving "Could not find a > suitable encoder/crtc for display X" problem, > which is a show stopper for enabling mainline graphics in android-x86 > > Other developers observed independently that the current > implementation is "embedded oriented" and only checks the first > display output, > isn't it? As far as I remember it prioritizes the internal connections, if none are found, it will use the external ones. > > The only change I did in drmconnector.cpp is to account for the > additional external drm mode connectors types > which were missing (DVID, DVII, VGA) . One question on my side: Do we > need more? I think they can be added as need be, that's been the process this far. > > Besides these minor types lists discussions, I would propose you to > check with Jim Bish if he should have the credit for the patch > and review the coding of his changes. So, I think we could just have both of you SOBs in the commit message, and the would be fine. > > > > > > > > (v1) 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. > > > > > > BUG=None. > > > TEST=System boots up and shows UI. > > > > > > (v1) Signed-off-by: Jim Bish > > > > > > (v2) porting of original commit 76fb87e675 of android-ia master > > > with additional external connector types (DisplayPort, DVID, > > DVII, > > > VGA) > > > Tested with i965 on Sandybridge and nouveau on GT120, GT610 > > > > The commit message isn't really following the usual format. It > > doesn't > > need to include a changelog (that is typically placed between the > > "---" > > markers in the email instead), > > and it is missing a signoff from the submitter (you). > > Sorry I don't have a signature, as usually my patches need sign-off > from professionals, > I'm a (crash test dummy) hobbyist..really :-) > > Maybe Rob Herring, Qiang Yu or Chih-Wei could review and sign-off > the proposed patch If you've tested or modified the code I would encourage you to add your Signoff-by or Tested-by. > > > > The BUG and TEST fields are not strictly required either, but > > aren't a > > problem. > > That part is the original Jim Bish commit message [1], my changes > version is (v2) > Sorry if I was not clear enought That's quite alright. So let's clean up this commit message, and add your Tested-by or Signoff-by (depending on if you changed any of the code) and then I'll merge it. Lastly, thanks for having a look at this, your contributions are very welcome! > > > > --- > > > drmconnector.cpp | 4 +++- > > > drmresources.cpp | 9 +++-- > > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/drmconnector.cpp b/drmconnector.cpp > > > index 247f56b..145518f 100644 > > > --- a/drmconnector.cpp > > > +++ b/drmconnector.cpp > > > @@ -73,7 +73,9 @@ bool DrmConnector::internal() const { > > > } > > > > > > bool DrmConnector::external() const { > > > - return type_ == DRM_MODE_CONNECTOR_HDMIA; > > > + return type_ == DRM_MODE_CONNECTOR_HDMIA || type_ == > > > DRM_MODE_CONNECTOR_DisplayPort || > > > + type_ == DRM_MODE_CONNECTOR_DVID || type_ == > > > DRM_MODE_CONNECTOR_DVII || > > > + type_ == DRM_MODE_CONNECTOR_VGA; > > > } > > > > The changes to external() should probably be broken out into it's > > own > > commit, since external() is called elsewhere changes to it _could_ > > introduce issues. > > Will you check the code, involving Jim Bish if necessary, about these > potential issues? > I am available to perform changes/tests, but the original maker will > be better.
Re: [drm_hwcomposer PATCH] Take Connection state into account. (v2)
Hey Mauro, Thanks for the patch! It builds and looks good to me, but I have some suggestions however. On Wed, 2018-01-03 at 11:10 +0100, Mauro Rossi wrote: > These changes avoid following logcat error 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 It isn't quite clear clear which errors belong to which changes, to make this patch a bit more bisectable it would be nice to see independent commits created for each error. > > (v1) 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. > > BUG=None. > TEST=System boots up and shows UI. > > (v1) Signed-off-by: Jim Bish> > (v2) porting of original commit 76fb87e675 of android-ia master > with additional external connector types (DisplayPort, DVID, DVII, > VGA) > Tested with i965 on Sandybridge and nouveau on GT120, GT610 The commit message isn't really following the usual format. It doesn't need to include a changelog (that is typically placed between the "---" markers in the email instead), and it is missing a signoff from the submitter (you). The BUG and TEST fields are not strictly required either, but aren't a problem. > --- > drmconnector.cpp | 4 +++- > drmresources.cpp | 9 +++-- > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drmconnector.cpp b/drmconnector.cpp > index 247f56b..145518f 100644 > --- a/drmconnector.cpp > +++ b/drmconnector.cpp > @@ -73,7 +73,9 @@ bool DrmConnector::internal() const { > } > > bool DrmConnector::external() const { > - return type_ == DRM_MODE_CONNECTOR_HDMIA; > + return type_ == DRM_MODE_CONNECTOR_HDMIA || type_ == > DRM_MODE_CONNECTOR_DisplayPort || > + type_ == DRM_MODE_CONNECTOR_DVID || type_ == > DRM_MODE_CONNECTOR_DVII || > + type_ == DRM_MODE_CONNECTOR_VGA; > } The changes to external() should probably be broken out into it's own commit, since external() is called elsewhere changes to it _could_ introduce issues. > > bool DrmConnector::valid_type() const { > 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 : connectors_) { > -if (conn->internal() && !found_primary) { > +if (conn->state() == DRM_MODE_CONNECTED && conn->internal() && > !found_primary) { >conn->set_display(0); >found_primary = true; > } else { > @@ -170,7 +170,7 @@ int DrmResources::Init() { > >// Then look for primary amongst external connectors >for (auto : connectors_) { > -if (conn->external() && !found_primary) { > +if (conn->state() == DRM_MODE_CONNECTED && conn->external() && > !found_primary) { >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()); signature.asc Description: This is a digitally signed message part ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel