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

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

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

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.
> 
> (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).
The BUG and TEST fields are not strictly required either, but aren't a
problem.

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

>  
>  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());

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