Hey Mauro, On Wed, 2018-01-03 at 13:40 +0100, Mauro Rossi wrote: > > > 2018-01-03 12:16 GMT+01:00 Robert Foss <robert.f...@collabora.com>: > > 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 <jim.b...@intel.com>
> > >
> > > (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.
> Cheers
>
> Mauro
I won't, but it's ok the way it is, making small atomic patches is
preferable in general.
>
> > >
> > > 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() == 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());
> >
>
> [1] https://github.com/android-ia/external-drm_hwcomposer/commit/76fb
> 87e675a20449d1261fccba5303aee7be3dba
>
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
