Re: [PATCH hwc v2 09/18] drm_hwcomposer: Handle writeback connectors

[Sean Paul <seanpaul@chromium.org>]

On Wed, Apr 11, 2018 at 04:22:20PM +0100, Alexandru Gheorghe wrote:
> When writeback connectors are available assign them to displays, in
> order to be able to use them for flattening of the current displayed
> scene. The pipeline for each display will look like this:
> 
> CRTC ---- encoder ------------ display connector.
>  |------- writeback enc ------ writeback connector.
> 
> However, the writeback connector will be later used/enabled only if
> one of the following conditions are met:
>  - Could be a clone of the display connector, as pointed by the
>    possible_clones property.
>  - The display_connector is disconnected, so we are safe to use it for
>    flattening the scene that's already presented on another display.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmresources.cpp | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  drmresources.h   |  3 +++
>  2 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/drmresources.cpp b/drmresources.cpp
> index 39f50be..fef6835 100644
> --- a/drmresources.cpp
> +++ b/drmresources.cpp
> @@ -64,6 +64,14 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
>      return ret;
>    }
>  
> +#ifdef DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> +  if (ret) {
> +    ALOGI("Failed to set writeback cap %d", ret);
> +    ret = 0;
> +  }
> +#endif
> +
>    drmModeResPtr res = drmModeGetResources(fd());
>    if (!res) {
>      ALOGE("Failed to get DrmResources resources");
> @@ -169,7 +177,7 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
>        conn->set_display(0);
>        displays_[0] = 0;
>        found_primary = true;
> -    } else {
> +    } else if (conn->external()) {
>        conn->set_display(display_num);
>        displays_[display_num] = display_num;
>        ++display_num;
> @@ -230,6 +238,8 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
>    }
>  
>    for (auto &conn : connectors_) {
> +    if (conn->writeback())
> +      continue;
>      ret = CreateDisplayPipe(conn.get());
>      if (ret) {
>        ALOGE("Failed CreateDisplayPipe %d with %d", conn->id(), ret);
> @@ -245,7 +255,15 @@ bool DrmResources::HandlesDisplay(int display) const {
>  
>  DrmConnector *DrmResources::GetConnectorForDisplay(int display) const {
>    for (auto &conn : connectors_) {
> -    if (conn->display() == display)
> +    if (conn->display() == display && !conn->writeback())
> +      return conn.get();
> +  }
> +  return NULL;
> +}
> +
> +DrmConnector *DrmResources::GetWritebackConnectorForDisplay(int display) const {
> +  for (auto &conn : connectors_) {
> +    if (conn->display() == display && conn->writeback())
>        return conn.get();
>    }
>    return NULL;
> @@ -280,6 +298,7 @@ int DrmResources::TryEncoderForDisplay(int display, DrmEncoder *enc) {
>    DrmCrtc *crtc = enc->crtc();
>    if (crtc && crtc->can_bind(display)) {
>      crtc->set_display(display);
> +    enc->set_display(display);
>      return 0;
>    }
>  
> @@ -306,6 +325,7 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
>    if (connector->encoder()) {
>      int ret = TryEncoderForDisplay(display, connector->encoder());
>      if (!ret) {
> +      AttachWriteback(connector);

AttachWriteback returns int, but you throw it away here. Additionally,
AttachWriteback always follows a successful TryEncoderForDisplay, so it makes
sense to just call it from there.

>        return 0;
>      } else if (ret != -EAGAIN) {
>        ALOGE("Could not set mode %d/%d", display, ret);
> @@ -317,6 +337,7 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
>      int ret = TryEncoderForDisplay(display, enc);
>      if (!ret) {
>        connector->set_encoder(enc);
> +      AttachWriteback(connector);
>        return 0;
>      } else if (ret != -EAGAIN) {
>        ALOGE("Could not set mode %d/%d", display, ret);
> @@ -328,6 +349,43 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
>    return -ENODEV;
>  }
>  
> +/*
> + * Attach writeback connector to the CRTC linked to the display_conn
> + *
> + */
> +int DrmResources::AttachWriteback(DrmConnector *display_conn) {
> +  int ret = -EINVAL;

This isn't really used, just return the error code directly at the bottom.

> +  if (display_conn->writeback())
> +    return -EINVAL;

This condition would benefit from a log

> +  DrmEncoder *display_enc = display_conn->encoder();
> +  if (!display_enc)
> +    return -EINVAL;
> +  DrmCrtc *display_crtc = display_enc->crtc();
> +  if (!display_crtc)
> +    return -EINVAL;

Are these possible given this is only called after a successful
TryEncoderForDisplay()?

> +  if (GetWritebackConnectorForDisplay(display_crtc->display()) != NULL)
> +    return -EINVAL;

Again, logging would be useful.

> +  for (auto &writeback_conn : connectors_) {
> +    if (writeback_conn->display() >= 0 || !writeback_conn->writeback())

There doesn't seem to be any situation where you iterate through connectors_ and
you don't have some type of writeback() check. So it seems like it'd make sense
to track these in different vectors.

> +      continue;
> +    for (DrmEncoder *writeback_enc : writeback_conn->possible_encoders()) {
> +      for (DrmCrtc *possible_crtc : writeback_enc->possible_crtcs()) {
> +        if (possible_crtc != display_crtc)
> +          continue;
> +        // Use just encoders which had not been bound already
> +        if (writeback_enc->can_bind(display_crtc->display())) {
> +          writeback_enc->set_crtc(display_crtc);
> +          writeback_conn->set_encoder(writeback_enc);
> +          writeback_conn->set_display(display_crtc->display());
> +          writeback_conn->UpdateModes();
> +          return 0;
> +        }
> +      }
> +    }
> +  }
> +  return ret;
> +}
> +
>  int DrmResources::CreatePropertyBlob(void *data, size_t length,
>                                       uint32_t *blob_id) {
>    struct drm_mode_create_blob create_blob;
> diff --git a/drmresources.h b/drmresources.h
> index 4cdcd87..4fb17fc 100644
> --- a/drmresources.h
> +++ b/drmresources.h
> @@ -59,6 +59,8 @@ class DrmResources {
>    }
>  
>    DrmConnector *GetConnectorForDisplay(int display) const;
> +  DrmConnector *GetWritebackConnectorForDisplay(int display) const;
> +  DrmConnector *FindWritebackConnector(int display) const;
>    DrmCrtc *GetCrtcForDisplay(int display) const;
>    DrmPlane *GetPlane(uint32_t id) const;
>    DrmEventListener *event_listener();
> @@ -84,6 +86,7 @@ class DrmResources {
>                    DrmProperty *property);
>  
>    int CreateDisplayPipe(DrmConnector *connector);
> +  int AttachWriteback(DrmConnector *display_conn);
>  
>    UniqueFd fd_;
>    uint32_t mode_id_ = 0;
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel