[drm_hwcomposer] [PATCH] Take Connection state into account. (v2)

[drm_hwcomposer] [PATCH] Take Connection state into account. (v2)

[Mauro Rossi]

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) {
       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) {
       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

[drm_hwcomposer] [PATCH] Update external connectors list

[Mauro Rossi]

DVID, DVII and VGA are required by discrete and integrated GPUs
---
 drmconnector.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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 {
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [drm_hwcomposer] [PATCH] Update external connectors list

[Robert Foss]

Hey Mauro,

This patch looks good to me apart from the commit message formatting.
If you tell me I can add your SOB, I'll merge it with the below commit message.

On 1/6/18 12:59 AM, Mauro Rossi wrote:
 > DVID, DVII and VGA are required by discrete and integrated GPUs

I would expect something like:

Update external connectors list

VID, DVII and VGA are required by discrete and integrated GPUs.

Signed-off-by: Mauro Rossi <Mauro Rossi <issor.oruam@gmail.com>>

 > ---
 >   drmconnector.cpp | 4 +++-
 >   1 file changed, 3 insertions(+), 1 deletion(-)
 >
 > 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 {
 >
_______________________________________________
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)

[Robert Foss]

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 Bish <jim.bish@intel.com>
Signed-off-by: Mauro Rossi <Mauro Rossi <issor.oruam@gmail.com>

> ---
>   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) {
>         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) {
>         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

Re: [drm_hwcomposer] [PATCH] Take Connection state into account. (v2)

[Sean Paul]

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) {
>        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

>        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)

[Sean Paul]

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

> >        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
> 
> >        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)

[Mauro Rossi]

[-- Attachment #1.1: Type: text/plain, Size: 4051 bytes --]

Il 08/gen/2018 09:46 PM, "Sean Paul" <seanpaul@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



> >        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

[-- Attachment #1.2: Type: text/html, Size: 6350 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
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)

[Sean Paul]

On Thu, Feb 01, 2018 at 05:42:35AM +0100, Mauro Rossi wrote:
> Il 08/gen/2018 09:46 PM, "Sean Paul" <seanpaul@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

Re: [drm_hwcomposer] [PATCH] Take Connection state into account. (v2)

[Daniel Vetter]

On Mon, Jan 8, 2018 at 9:46 PM, Sean Paul <seanpaul@chromium.org> wrote:
> 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.

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 &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
>>
>> >        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