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

[Mauro Rossi <issor.oruam@xxxxxxxxx>]

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

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