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

[Robert Foss <robert.foss@collabora.com>]

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

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.foss@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.bish@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 
> 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 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