From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Foss Subject: Re: [drm_hwcomposer PATCH] Take Connection state into account. (v2) Date: Wed, 03 Jan 2018 16:25:19 +0100 Message-ID: <1514993119.8410.8.camel@collabora.com> References: <20180103101007.27037-1-issor.oruam@gmail.com> <1514978193.5740.16.camel@collabora.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1485832536==" Return-path: Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227]) by gabe.freedesktop.org (Postfix) with ESMTPS id 400776E004 for ; Wed, 3 Jan 2018 15:25:23 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Mauro Rossi Cc: ML dri-devel , "Yu, Qiang" , Jim Bish List-Id: dri-devel@lists.freedesktop.org --===============1485832536== Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-OhTO6aCW81VPp4KQk0Au" --=-OhTO6aCW81VPp4KQk0Au Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hey Mauro, On Wed, 2018-01-03 at 13:40 +0100, Mauro Rossi wrote: >=20 >=20 > 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. > >=20 > >=20 > > 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 > >=20 > > 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. >=20 > Hi Robert, >=20 > 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 >=20 > So changes in drmresources.cpp belong to solving "Could not find a > suitable encoder/crtc for display X" problem,=20 > which is a show stopper for enabling mainline graphics in android-x86 >=20 > Other developers observed independently that the current > implementation is "embedded oriented" and only checks the first > display output,=20 > isn't it? As far as I remember it prioritizes the internal connections, if none are found, it will use the external ones. > =20 > 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. >=20 > 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. >=20 > =20 > > > > > > (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=3DNone. > > > TEST=3DSystem 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 > >=20 > > 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). >=20 > Sorry I don't have a signature, as usually my patches need sign-off > from professionals, > I'm a (crash test dummy) hobbyist..really :-) >=20 > 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. >=20 > =20 > > The BUG and TEST fields are not strictly required either, but > > aren't a > > problem. >=20 > 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! > =20 > > > --- > > > 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_ =3D=3D DRM_MODE_CONNECTOR_HDMIA; > > > + return type_ =3D=3D DRM_MODE_CONNECTOR_HDMIA || type_ =3D=3D > > > DRM_MODE_CONNECTOR_DisplayPort || > > > + type_ =3D=3D DRM_MODE_CONNECTOR_DVID || type_ =3D=3D > > > DRM_MODE_CONNECTOR_DVII || > > > + type_ =3D=3D DRM_MODE_CONNECTOR_VGA; > > > } > >=20 > > 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. >=20 > 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 >=20 > Mauro I won't, but it's ok the way it is, making small atomic patches is preferable in general. >=20 > > > > > > 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 &conn : connectors_) { > > > - if (conn->internal() && !found_primary) { > > > + if (conn->state() =3D=3D DRM_MODE_CONNECTED && conn->internal() > > && > > > !found_primary) { > > > conn->set_display(0); > > > found_primary =3D 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() =3D=3D DRM_MODE_CONNECTED && conn->external() > > && > > > !found_primary) { > > > conn->set_display(0); > > > found_primary =3D true; > > > } > > > @@ -288,6 +288,11 @@ int DrmResources::TryEncoderForDisplay(int > > > display, DrmEncoder *enc) { > > > > > > int DrmResources::CreateDisplayPipe(DrmConnector *connector) { > > > int display =3D connector->display(); > > > + > > > + // skip not connected > > > + if (connector->state() =3D=3D DRM_MODE_DISCONNECTED) > > > + return 0; > > > + > > > /* Try to use current setup first */ > > > if (connector->encoder()) { > > > int ret =3D TryEncoderForDisplay(display, connector- > > >encoder()); > >=20 >=20 > [1] https://github.com/android-ia/external-drm_hwcomposer/commit/76fb > 87e675a20449d1261fccba5303aee7be3dba=20 >=20 --=-OhTO6aCW81VPp4KQk0Au Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQIcBAABCAAGBQJaTPXfAAoJELeaC2oR7vcn7tkQAIO3DvyO8LfEJv7zwKT51zDv MkWmJpXoJnxuqx6KHSiHyXcqD8w7+At4Sbql7fxq3xaiW2LCHtc42s/8AzS0/9l8 GppyP3qvebB34u3hs84RIHpJL+NHhcfGTjXXwtg11X8w3BvbHRdAsqpz1YZD0NHA 2lYBWj6SbfoIdp8PIKcGkZjUbMw7l0HQ0Akg4KeQ//9oqkga4u8TmOPPKhNuOHdi DbvEQ3aeY38XhLozmEH2BvGwIdCIpfpta0egY+as1x70hL9wKC+qHxuDDWOw61GV zsyygSwHl8XxnkqeG2xkxGVg5XTuciVwn76MQ0TL2IPAj4dKxt3kU5T0As2x0MMl g2Wjstu3B4uWh5epsneaEXE3DylRNXCGJQubHCGcxO0w/jkp3fWSmRddBOAaQn21 fp4BVG/BOAzajYT8x8oCaxuARH8/hdyeBxqSNaQdpFFO3Vg3Segs70+aje9TUTFM 74P2hotenvGyLytNyRS+P11+c3zY5Qu9kHbdYUZog9B/VrPfyBy9ql28vxWSWenz wSaGopPVmuEdnJhwaS+Oj6Egrd30kQYYSO/GViUI3spLz8+w9Ur0oFBrJKdYpyK7 0IsE3r+3rVZ1VojryrtC0MjabvEcNW1nWJzCw3lLLso6t9XkbJ0pNCHedkEtBuUL aYmnfbhp3KAmZG2/RRSK =bWfd -----END PGP SIGNATURE----- --=-OhTO6aCW81VPp4KQk0Au-- --===============1485832536== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1485832536==--