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

["Bish, Jim" <jim.bish@intel.com>]

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

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<mailto: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?

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?

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.
it would be proper to keep the original author signoff but ok to add additional signoff/testing, etc.  I am not so worried about credit for this just patch but good to following proper procedures.


>
> (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<mailto: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


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


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


>
>  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/76fb87e675a20449d1261fccba5303aee7be3dba


[-- Attachment #1.2: Type: text/html, Size: 8816 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