[hwc,v1,RFC] drm_hwcomposer: Flatten composition using writeback connector

Commit Message

Alexandru-Cosmin Gheorghe March 13, 2018, 4:21 p.m. UTC

This patchset tries to add support for using writeback connector to
flatten a scene when it doesn't change for a while. This idea had
been floated around on IRC here [1] and here [2].

Developed on top of the latest writeback series, sent by Liviu here
[3].

Probably the patch should/could be broken in more patches, but since I
want to put this out there to get feedback, I kept them as a single
patch for now.

This change could be summarize as follow:
- Attach a writeback connector to the CRTC that's controlling a
display.
- Detect the scene did not change for a while(60 vblanks).
- Re-commit scene and get the composited scene through the writeback
connector.
- Commit the whole scene as a single plane.

Some areas that I consider important and I could use some
feedback/ideas:

1. Building the pipeline.
Currently, drm_hwcomposer allows to connect just a single connector
to a crtc. For now, I decided to treat the writeback connector as a
separate field inside DrmCrtc. I'm not sure if it's a good idea to try
to handle this in a unified way, since the writeback connector is such
a special connector. Regarding the allocation of writeback connectors,
my idea was to allocate writeback connector to the primary display
first and then continue allocating while respecting the display number. 0
gets a writeback before 1 and so on.

2. Heuristic for triggering the flattening.
I just created a VSyncWorker which will trigger the flattening of the
scene if it doesn't change for 60 consecutive vsyncs. The countdown
gets reset every time ValidateDisplay is called. This is a relatively
basic heuristic, so I'm open to suggestions.

3. Locking scheme corner cases.
The Vsynworker is a separate thread which will contend with
SurfaceFlinger for showing things on the screen. I tried to limit the
race window by resetting the countdown on ValidateDisplay and
explicitely checking that we still need to use the flatten scene before
commiting to get the writeback result or before applying the flattened
scene.

4. Building the DrmDisplayComposition for the flattened scene.
I kind of lost myself  in all types of layers/planes and compositions,
so I'm not sure if I'm correctly building the DrmDisplayComposition
object for the FlattenScene, it works and shows what I expect on the
screen. So, any feedback here is appreciated.

5. I see there is a drmcompositorworker.cpp which implemented the same
idea using the GPU, however that seems to not be used anymore, does
anyone know the rationale behind it?

Some unfinished/untested things:
- Make sure the DrmFrameBuffer allocates one of the formats reported
  in WRITEBACK_PIXEL_FORMATS.
- I'm using a hacked setup where, when needed it, the GL compositing is
  done by Surfaceflinger, so I'm not sure how well this changes are
  getting along with the GLCompositorWorker.

[1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-02-23
[2] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-02-09
[3] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html

Signed-off-by: Alexandru Gheorghe <[e-mail]>
---
 drmconnector.cpp         |  36 ++++++++++-
 drmconnector.h           |   8 +++
 drmcrtc.cpp              |  11 +++-
 drmcrtc.h                |   8 ++-
 drmdisplaycompositor.cpp | 164 +++++++++++++++++++++++++++++++++++++++++++++--
 drmdisplaycompositor.h   |  16 +++--
 drmencoder.cpp           |  15 +++++
 drmencoder.h             |   7 +-
 drmhwctwo.cpp            |   1 +
 drmresources.cpp         |  56 +++++++++++++++-
 drmresources.h           |   1 +
 11 files changed, 306 insertions(+), 17 deletions(-)

--
2.7.4

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Comments

Stefan Schake March 13, 2018, 6:20 p.m. UTC | #1

Hey Alexandru,

On Tue, Mar 13, 2018 at 5:21 PM, Alexandru Gheorghe
<[e-mail]> wrote:
> This patchset tries to add support for using writeback connector to
> flatten a scene when it doesn't change for a while. This idea had
> been floated around on IRC here [1] and here [2].
>
> 2. Heuristic for triggering the flattening.
> I just created a VSyncWorker which will trigger the flattening of the
> scene if it doesn't change for 60 consecutive vsyncs. The countdown
> gets reset every time ValidateDisplay is called. This is a relatively
> basic heuristic, so I'm open to suggestions.

I think when Android deems that the display is sufficiently idle, it will
disable VSync through setVsyncEnabled. Presumably we could just trigger the
flattening on an enabled -> disabled transition?

Thanks,
Stefan

Alexandru-Cosmin Gheorghe March 14, 2018, 9:42 a.m. UTC | #2

Hi Stefan,

On Tue, Mar 13, 2018 at 07:20:33PM +0100, Stefan Schake wrote:
> Hey Alexandru,
> 
> On Tue, Mar 13, 2018 at 5:21 PM, Alexandru Gheorghe
> <[e-mail]> wrote:
> > This patchset tries to add support for using writeback connector to
> > flatten a scene when it doesn't change for a while. This idea had
> > been floated around on IRC here [1] and here [2].
> >
> > 2. Heuristic for triggering the flattening.
> > I just created a VSyncWorker which will trigger the flattening of the
> > scene if it doesn't change for 60 consecutive vsyncs. The countdown
> > gets reset every time ValidateDisplay is called. This is a relatively
> > basic heuristic, so I'm open to suggestions.
> 
> I think when Android deems that the display is sufficiently idle, it will
> disable VSync through setVsyncEnabled. Presumably we could just trigger the
> flattening on an enabled -> disabled transition?
> 
> Thanks,
> Stefan

I will look into it thanks.

Sean Paul March 19, 2018, 6:03 p.m. UTC | #3

On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> This patchset tries to add support for using writeback connector to
> flatten a scene when it doesn't change for a while. This idea had
> been floated around on IRC here [1] and here [2].
> 
> Developed on top of the latest writeback series, sent by Liviu here
> [3].
> 
> Probably the patch should/could be broken in more patches, but since I
> want to put this out there to get feedback, I kept them as a single
> patch for now.

Thank you for your patch, it's looking good. Feel free to submit the v2 in
multiple patches. Also note that we've migrated to gitlab, the new tree is
located here:

https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer


> 
> This change could be summarize as follow:
> - Attach a writeback connector to the CRTC that's controlling a
> display.
> - Detect the scene did not change for a while(60 vblanks).
> - Re-commit scene and get the composited scene through the writeback
> connector.
> - Commit the whole scene as a single plane.
> 
> Some areas that I consider important and I could use some
> feedback/ideas:
> 
> 1. Building the pipeline.
> Currently, drm_hwcomposer allows to connect just a single connector
> to a crtc. For now, I decided to treat the writeback connector as a
> separate field inside DrmCrtc. I'm not sure if it's a good idea to try
> to handle this in a unified way, since the writeback connector is such
> a special connector. Regarding the allocation of writeback connectors,
> my idea was to allocate writeback connector to the primary display
> first and then continue allocating while respecting the display number. 0
> gets a writeback before 1 and so on.
> 
> 2. Heuristic for triggering the flattening.
> I just created a VSyncWorker which will trigger the flattening of the
> scene if it doesn't change for 60 consecutive vsyncs. The countdown
> gets reset every time ValidateDisplay is called. This is a relatively
> basic heuristic, so I'm open to suggestions.
> 
> 3. Locking scheme corner cases.
> The Vsynworker is a separate thread which will contend with
> SurfaceFlinger for showing things on the screen. I tried to limit the
> race window by resetting the countdown on ValidateDisplay and
> explicitely checking that we still need to use the flatten scene before
> commiting to get the writeback result or before applying the flattened
> scene.
> 
> 4. Building the DrmDisplayComposition for the flattened scene.
> I kind of lost myself  in all types of layers/planes and compositions,
> so I'm not sure if I'm correctly building the DrmDisplayComposition
> object for the FlattenScene, it works and shows what I expect on the
> screen. So, any feedback here is appreciated.
> 
> 5. I see there is a drmcompositorworker.cpp which implemented the same
> idea using the GPU, however that seems to not be used anymore, does
> anyone know the rationale behind it?
> 
> Some unfinished/untested things:
> - Make sure the DrmFrameBuffer allocates one of the formats reported
>   in WRITEBACK_PIXEL_FORMATS.
> - I'm using a hacked setup where, when needed it, the GL compositing is
>   done by Surfaceflinger, so I'm not sure how well this changes are
>   getting along with the GLCompositorWorker.
> 
> [1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-02-23
> [2] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-02-09
> [3] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html
> 
> Signed-off-by: Alexandru Gheorghe <[e-mail]>
> ---
>  drmconnector.cpp         |  36 ++++++++++-
>  drmconnector.h           |   8 +++
>  drmcrtc.cpp              |  11 +++-
>  drmcrtc.h                |   8 ++-
>  drmdisplaycompositor.cpp | 164 +++++++++++++++++++++++++++++++++++++++++++++--
>  drmdisplaycompositor.h   |  16 +++--
>  drmencoder.cpp           |  15 +++++
>  drmencoder.h             |   7 +-
>  drmhwctwo.cpp            |   1 +
>  drmresources.cpp         |  56 +++++++++++++++-
>  drmresources.h           |   1 +
>  11 files changed, 306 insertions(+), 17 deletions(-)
> 
> diff --git a/drmconnector.cpp b/drmconnector.cpp
> index 145518f..2ed4f23 100644
> --- a/drmconnector.cpp
> +++ b/drmconnector.cpp
> @@ -52,6 +52,23 @@ int DrmConnector::Init() {
>      ALOGE("Could not get CRTC_ID property\n");
>      return ret;
>    }
> +  if (writeback()) {
> +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_PIXEL_FORMATS", &writeback_pixel_formats_);
> +    if (ret) {
> +      ALOGE("Could not get WRITEBACK_PIXEL_FORMATS connector_id = %d\n", id_);
> +      return ret;
> +    }
> +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_FB_ID", &writeback_fb_id_);
> +    if (ret) {
> +      ALOGE("Could not get WRITEBACK_FB_ID connector_id = %d\n", id_);
> +      return ret;
> +    }
> +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_OUT_FENCE_PTR", &writeback_out_fence_);
> +    if (ret) {
> +      ALOGE("Could not get WRITEBACK_OUT_FENCE_PTR connector_id = %d\n", id_);
> +      return ret;
> +    }
> +  }
>    return 0;
>  }
> 
> @@ -78,8 +95,13 @@ bool DrmConnector::external() const {
>           type_ == DRM_MODE_CONNECTOR_VGA;
>  }
> 
> +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> +bool DrmConnector::writeback() const {
> +        return type_ == DRM_MODE_CONNECTOR_WRITEBACK;
> +}
> +
>  bool DrmConnector::valid_type() const {
> -  return internal() || external();
> +  return internal() || external() || writeback();
>  }
> 
>  int DrmConnector::UpdateModes() {
> @@ -130,6 +152,18 @@ const DrmProperty &DrmConnector::crtc_id_property() const {
>    return crtc_id_property_;
>  }
> 
> +const DrmProperty &DrmConnector::writeback_pixel_formats() const {
> +  return writeback_pixel_formats_;
> +}
> +
> +const DrmProperty &DrmConnector::writeback_fb_id() const {
> +  return writeback_fb_id_;
> +}
> +
> +const DrmProperty &DrmConnector::writeback_out_fence() const {
> +  return writeback_out_fence_;
> +}
> +
>  DrmEncoder *DrmConnector::encoder() const {
>    return encoder_;
>  }
> diff --git a/drmconnector.h b/drmconnector.h
> index 5601e06..ad18762 100644
> --- a/drmconnector.h
> +++ b/drmconnector.h
> @@ -28,6 +28,7 @@
>  namespace android {
> 
>  class DrmResources;
> +class DrmCrtc;

Not needed?

> 
>  class DrmConnector {
>   public:
> @@ -46,6 +47,7 @@ class DrmConnector {
> 
>    bool internal() const;
>    bool external() const;
> +  bool writeback() const;
>    bool valid_type() const;
> 
>    int UpdateModes();
> @@ -58,6 +60,9 @@ class DrmConnector {
> 
>    const DrmProperty &dpms_property() const;
>    const DrmProperty &crtc_id_property() const;
> +  const DrmProperty &writeback_pixel_formats() const;
> +  const DrmProperty &writeback_fb_id() const;
> +  const DrmProperty &writeback_out_fence() const;
> 
>    const std::vector<DrmEncoder *> &possible_encoders() const {
>      return possible_encoders_;
> @@ -88,6 +93,9 @@ class DrmConnector {
> 
>    DrmProperty dpms_property_;
>    DrmProperty crtc_id_property_;
> +  DrmProperty writeback_pixel_formats_;
> +  DrmProperty writeback_fb_id_;
> +  DrmProperty writeback_out_fence_;
> 
>    std::vector<DrmEncoder *> possible_encoders_;
>  };
> diff --git a/drmcrtc.cpp b/drmcrtc.cpp
> index 1b354fe..f8c9f25 100644
> --- a/drmcrtc.cpp
> +++ b/drmcrtc.cpp
> @@ -31,7 +31,8 @@ DrmCrtc::DrmCrtc(DrmResources *drm, drmModeCrtcPtr c, unsigned pipe)
>        id_(c->crtc_id),
>        pipe_(pipe),
>        display_(-1),
> -      mode_(&c->mode) {
> +      mode_(&c->mode),
> +      writeback_conn_(nullptr) {
>  }
> 
>  int DrmCrtc::Init() {
> @@ -75,6 +76,14 @@ bool DrmCrtc::can_bind(int display) const {
>    return display_ == -1 || display_ == display;
>  }
> 
> +DrmConnector* DrmCrtc::writeback_conn() const {
> +  return writeback_conn_;
> +}
> +
> +void DrmCrtc::set_writeback_conn(DrmConnector* writeback_conn) {
> +  writeback_conn_ = writeback_conn;
> +}
> +
>  const DrmProperty &DrmCrtc::active_property() const {
>    return active_property_;
>  }
> diff --git a/drmcrtc.h b/drmcrtc.h
> index c5a5599..bbd8d37 100644
> --- a/drmcrtc.h
> +++ b/drmcrtc.h
> @@ -22,11 +22,11 @@
> 
>  #include <stdint.h>
>  #include <xf86drmMode.h>
> -
> +#include "drmconnector.h"

Please retain whitespace (comment applies below as well)

>  namespace android {
> 
>  class DrmResources;
> -
> +class DrmConnector;
>  class DrmCrtc {
>   public:
>    DrmCrtc(DrmResources *drm, drmModeCrtcPtr c, unsigned pipe);
> @@ -39,7 +39,9 @@ class DrmCrtc {
>    unsigned pipe() const;
> 
>    int display() const;
> +  DrmConnector* writeback_conn() const;

I think a better way would be to associate the display with the connector as it
does for a traditional connector. This might be as simple as just adding a
!conn->writeback() check to GetConnectorForDisplay() and adding a new
GetWritebackConnectorForDisplay() function to DrmResources.

>    void set_display(int display);
> +  void set_writeback_conn(DrmConnector*);
> 
>    bool can_bind(int display) const;
> 
> @@ -55,7 +57,7 @@ class DrmCrtc {
>    int display_;
> 
>    DrmMode mode_;
> -
> +  DrmConnector *writeback_conn_;
>    DrmProperty active_property_;
>    DrmProperty mode_property_;
>    DrmProperty out_fence_ptr_property_;
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index e556e86..9783852 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -16,7 +16,6 @@
> 
>  #define ATRACE_TAG ATRACE_TAG_GRAPHICS
>  #define LOG_TAG "hwc-drm-display-compositor"
> -

More unrelated whitespace changes

>  #include "drmdisplaycompositor.h"
> 
>  #include <pthread.h>
> @@ -36,9 +35,24 @@
>  #include "drmplane.h"
>  #include "drmresources.h"
>  #include "glworker.h"
> +static const uint32_t kWaitWritebackFence = 100; //ms
> 
>  namespace android {
> 
> +class CompositorVsyncCallback : public VsyncCallback {
> + public:
> +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> +      :compositor_(compositor) {
> +  }
> +
> +  void Callback(int display, int64_t timestamp) {
> +    compositor_->Vsync(display, timestamp);
> +  }
> +
> + private:
> +  DrmDisplayCompositor *compositor_;
> +};
> +

The GLCompositor already has flattening, so we should tie into that. I assume
writeback is going to be more efficient than using GPU (given that the GPU is
probably turned off in these scenarios), so you're probably safe to use
writeback when available and fall back to GL if it's not.


>  void SquashState::Init(DrmHwcLayer *layers, size_t num_layers) {
>    generation_number_++;
>    valid_history_ = 0;
> @@ -183,7 +197,8 @@ DrmDisplayCompositor::DrmDisplayCompositor()
>        framebuffer_index_(0),
>        squash_framebuffer_index_(0),
>        dump_frames_composited_(0),
> -      dump_last_timestamp_ns_(0) {
> +      dump_last_timestamp_ns_(0),
> +      flatten_countdown_(FLATTEN_COUNTDOWN_INIT) {
>    struct timespec ts;
>    if (clock_gettime(CLOCK_MONOTONIC, &ts))
>      return;
> @@ -193,7 +208,7 @@ DrmDisplayCompositor::DrmDisplayCompositor()
>  DrmDisplayCompositor::~DrmDisplayCompositor() {
>    if (!initialized_)
>      return;
> -
> +  vsync_worker_.Exit();
>    int ret = pthread_mutex_lock(&lock_);
>    if (ret)
>      ALOGE("Failed to acquire compositor lock %d", ret);
> @@ -223,6 +238,9 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
>    }
> 
>    initialized_ = true;
> +  vsync_worker_.Init(drm_, display_);
> +  auto callback = std::make_shared<CompositorVsyncCallback>(this);
> +  vsync_worker_.RegisterCallback(callback);
>    return 0;
>  }
> 
> @@ -482,7 +500,7 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
>  }
> 
>  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> -                                      bool test_only) {
> +                                      bool test_only, DrmDisplayComposition *writeback_comp) {
>    ATRACE_CALL();
> 
>    int ret = 0;
> @@ -491,7 +509,8 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>    std::vector<DrmCompositionPlane> &comp_planes =
>        display_comp->composition_planes();
>    uint64_t out_fences[drm_->crtcs().size()];
> -
> +  int writeback_fence = -1;
> +  DrmFramebuffer *writeback_fb = nullptr;
>    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
>    if (!connector) {
>      ALOGE("Could not locate connector for display %d", display_);
> @@ -508,6 +527,32 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>      ALOGE("Failed to allocate property set");
>      return -ENOMEM;
>    }
> +  DrmConnector *writeback_conn = crtc->writeback_conn();
> +  if (writeback_comp != nullptr) {
> +    if (writeback_conn == nullptr)

FWIW, we use NULL everywhere else, so let's stick with that.

> +      return -EINVAL;
> +    writeback_fb = &framebuffers_[framebuffer_index_];
> +    framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS;
> +    ret = PrepareFramebuffer(*writeback_fb, writeback_comp);
> +    if (ret) {
> +      ALOGE("Failed to prepare framebuffer for pre-composite %d", ret);
> +      return ret;
> +    }
> +    if (writeback_conn->writeback_fb_id().id() == 0 || writeback_conn->writeback_out_fence().id() == 0)
> +      return -EINVAL;
> +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), writeback_conn->writeback_fb_id().id(),
> +                                   writeback_comp->layers().back().buffer->fb_id);
> +    if (ret < 0) {
> +      ALOGE("Failed to add writeback_fb_id");
> +      return ret;
> +    }
> +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), writeback_conn->writeback_out_fence().id(),
> +                                   (uint64_t) &writeback_fence);
> +    if (ret < 0) {
> +      ALOGE("Failed to add writeback_out_fence");
> +      return ret;
> +    }
> +  }
> 
>    if (crtc->out_fence_ptr_property().id() != 0) {
>      ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(),
> @@ -537,6 +582,12 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>        drmModeAtomicFree(pset);
>        return ret;
>      }
> +    if (writeback_conn != nullptr) {
> +      ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), writeback_conn->crtc_id_property().id(), crtc->id());
> +      if (ret < 0) {
> +          ALOGE("Failed to  attach writeback");
> +      }
> +    }
>    }
> 
>    for (DrmCompositionPlane &comp_plane : comp_planes) {
> @@ -691,6 +742,17 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>      if (test_only)
>        flags |= DRM_MODE_ATOMIC_TEST_ONLY;
> 
> +    // There is a race when we recommit a scene to get the writeback result
> +    // to shorten that race, check if we still need that result
> +    bool abort_commit = false;
> +    if (writeback_comp != nullptr) {
> +      AutoLock lock(&lock_, "CommitFrame");
> +      lock.Lock();
> +      abort_commit = !CountdownExpired();
> +    }
> +    if (abort_commit)
> +      return -EINVAL;
> +
>      ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_);
>      if (ret) {
>        if (test_only)
> @@ -729,6 +791,14 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>      display_comp->set_out_fence((int) out_fences[crtc->pipe()]);
>    }
> 
> +  if (writeback_fence >= 0 && writeback_fb != nullptr) {
> +    ret = sync_wait(writeback_fence, kWaitWritebackFence);
> +    if (ret) {
> +        ALOGE("Failed to wait on writeback fence");
> +    }
> +    close(writeback_fence);
> +  }
> +
>    return ret;
>  }
> 
> @@ -784,7 +854,7 @@ void DrmDisplayCompositor::ClearDisplay() {
>  }
> 
>  void DrmDisplayCompositor::ApplyFrame(
> -    std::unique_ptr<DrmDisplayComposition> composition, int status) {
> +    std::unique_ptr<DrmDisplayComposition> composition, int status, bool start_countdown) {
>    int ret = status;
> 
>    if (!ret)
> @@ -807,6 +877,8 @@ void DrmDisplayCompositor::ApplyFrame(
>      ALOGE("Failed to acquire lock for active_composition swap");
> 
>    active_composition_.swap(composition);
> +  flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
> +  vsync_worker_.VSyncControl(start_countdown);
> 
>    if (!ret)
>      ret = pthread_mutex_unlock(&lock_);
> @@ -878,6 +950,65 @@ int DrmDisplayCompositor::ApplyComposition(
>    return ret;
>  }
> 
> +int DrmDisplayCompositor::FlattenScene() {
> +  std::unique_ptr<DrmDisplayComposition> comp = CreateComposition();
> +  DrmDisplayComposition *src = active_composition_.get();
> +  DrmDisplayComposition *dst = comp.get();
> +  std::vector<DrmCompositionPlane> &src_planes = src->composition_planes();
> +  if (src == nullptr || dst == nullptr)
> +    return -EINVAL;
> +  size_t src_planes_with_layer = std::count_if(
> +      src_planes.begin(), src_planes.end(), [](DrmCompositionPlane &p) {
> +        return p.type() != DrmCompositionPlane::Type::kDisable;
> +      });
> +
> +  if (src_planes_with_layer <= 1)
> +    return -EALREADY;
> +  int ret = dst->Init(drm_, src->crtc(), src->importer(), src->planner(),
> +                      src->frame_no());
> +  if (ret)
> +    return ret;
> +  DrmCompositionPlane squashed_comp(DrmCompositionPlane::Type::kPrecomp, NULL,
> +                                    src->crtc());
> +  for (DrmCompositionPlane &comp_plane : src_planes) {
> +    if (comp_plane.plane() == NULL) {
> +      ALOGE("Skipping squash all because of NULL plane");
> +      ret = -EINVAL;
> +    }
> +    if (comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY)
> +      squashed_comp.set_plane(comp_plane.plane());
> +    else
> +      dst->AddPlaneDisable(comp_plane.plane());
> +  }
> +  ret = CommitFrame(active_composition_.get(), 0, dst);
> +  if (ret || dst->layers().size() != 1) {
> +      ALOGE("Failed to flatten scene using writeback");
> +      return -EINVAL;
> +  }
> +  squashed_comp.source_layers().push_back(0);
> +
> +  ret = dst->AddPlaneComposition(std::move(squashed_comp));
> +  if (ret) {
> +    ALOGE("Failed to add flatten scene");
> +    return ret;
> +  }
> +  ret = dst->FinalizeComposition();
> +  if (ret) {
> +    ALOGE("Failed to finalize composition");
> +    return ret;
> +  }
> +  // If countdown it's still expired ApplyFlattenScene
> +  AutoLock lock(&lock_, "FlattenScene");
> +  lock.Lock();
> +  if (CountdownExpired()) {
> +    lock.Unlock();
> +    ApplyFrame(std::move(comp), 0, false);
> +  } else {
> +    return -EAGAIN;
> +  }
> +  return 0;
> +}
> +
>  int DrmDisplayCompositor::SquashAll() {
>    AutoLock lock(&lock_, "compositor");
>    int ret = lock.Lock();
> @@ -1026,6 +1157,27 @@ move_layers_back:
>    return ret;
>  }
> 
> +bool DrmDisplayCompositor::CountdownExpired() const {
> +  return flatten_countdown_ <= 0;
> +}
> +
> +void DrmDisplayCompositor::ResetCountdown() {
> +  AutoLock lock(&lock_, "ResetCountdown");
> +  lock.Lock();
> +  flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
> +}
> +
> +void DrmDisplayCompositor::Vsync(int display, int64_t timestamp) {
> +   AutoLock lock(&lock_, "VSync");
> +   lock.Lock();
> +   flatten_countdown_--;
> +   if (CountdownExpired()) {
> +     lock.Unlock();
> +     int ret = FlattenScene();
> +     ALOGI("Vsync: Flatten scene for display %d at timestamp %" PRIu64 " ret = %d \n", display, timestamp, ret);
> +   }
> +}
> +
>  void DrmDisplayCompositor::Dump(std::ostringstream *out) const {
>    int ret = pthread_mutex_lock(&lock_);
>    if (ret)
> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> index f1965fb..4a5696a 100644
> --- a/drmdisplaycompositor.h
> +++ b/drmdisplaycompositor.h
> @@ -29,11 +29,15 @@
> 
>  #include <hardware/hardware.h>
>  #include <hardware/hwcomposer.h>
> +#include <vsyncworker.h>
> 
>  // One for the front, one for the back, and one for cases where we need to
>  // squash a frame that the hw can't display with hw overlays.
>  #define DRM_DISPLAY_BUFFERS 3
> 
> +// If a scene is still for this number of vblanks flatten it to reduce power consumption.
> +#define FLATTEN_COUNTDOWN_INIT 60
> +
>  namespace android {
> 
>  class GLWorkerCompositor;
> @@ -91,7 +95,8 @@ class DrmDisplayCompositor {
>    int Composite();
>    int SquashAll();
>    void Dump(std::ostringstream *out) const;
> -
> +  void Vsync(int display, int64_t timestamp);
> +  void ResetCountdown();
>    std::tuple<uint32_t, uint32_t, int> GetActiveModeResolution();
> 
>    SquashState *squash_state() {
> @@ -118,15 +123,16 @@ class DrmDisplayCompositor {
>    int ApplySquash(DrmDisplayComposition *display_comp);
>    int ApplyPreComposite(DrmDisplayComposition *display_comp);
>    int PrepareFrame(DrmDisplayComposition *display_comp);
> -  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only);
> +  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only, DrmDisplayComposition *writeback_comp = nullptr);
>    int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst);
>    int ApplyDpms(DrmDisplayComposition *display_comp);
>    int DisablePlanes(DrmDisplayComposition *display_comp);
> 
>    void ClearDisplay();
>    void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
> -                  int status);
> -
> +                  int status, bool start_countdown = true);
> +  int FlattenScene();
> +  bool CountdownExpired() const;
>    std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
> 
>    DrmResources *drm_;
> @@ -155,6 +161,8 @@ class DrmDisplayCompositor {
>    // we need to reset them on every Dump() call.
>    mutable uint64_t dump_frames_composited_;
>    mutable uint64_t dump_last_timestamp_ns_;
> +  VSyncWorker vsync_worker_;
> +  int64_t flatten_countdown_;
>  };
>  }
> 
> diff --git a/drmencoder.cpp b/drmencoder.cpp
> index 3d762f3..3df06a2 100644
> --- a/drmencoder.cpp
> +++ b/drmencoder.cpp
> @@ -27,6 +27,7 @@ DrmEncoder::DrmEncoder(drmModeEncoderPtr e, DrmCrtc *current_crtc,
>                         const std::vector<DrmCrtc *> &possible_crtcs)
>      : id_(e->encoder_id),
>        crtc_(current_crtc),
> +      display_(-1),
>        possible_crtcs_(possible_crtcs) {
>  }
> 
> @@ -40,5 +41,19 @@ DrmCrtc *DrmEncoder::crtc() const {
> 
>  void DrmEncoder::set_crtc(DrmCrtc *crtc) {
>    crtc_ = crtc;
> +  set_display(crtc->display());
>  }
> +
> +int DrmEncoder::display() const {
> +  return display_;
> +}
> +
> +void DrmEncoder::set_display(int display) {
> +  display_ = display;
> +}
> +
> +bool DrmEncoder::can_bind(int display) const {
> +  return display_ == -1 || display_ == display;
> +}
> +
>  }
> diff --git a/drmencoder.h b/drmencoder.h
> index 58ccbfb..6b39505 100644
> --- a/drmencoder.h
> +++ b/drmencoder.h
> @@ -25,6 +25,8 @@
> 
>  namespace android {
> 
> +class DrmCrtc;
> +
>  class DrmEncoder {
>   public:
>    DrmEncoder(drmModeEncoderPtr e, DrmCrtc *current_crtc,
> @@ -36,7 +38,9 @@ class DrmEncoder {
> 
>    DrmCrtc *crtc() const;
>    void set_crtc(DrmCrtc *crtc);
> -
> +  bool can_bind(int display) const;
> +  void set_display(int display);
> +  int display() const;
>    const std::vector<DrmCrtc *> &possible_crtcs() const {
>      return possible_crtcs_;
>    }
> @@ -44,6 +48,7 @@ class DrmEncoder {
>   private:
>    uint32_t id_;
>    DrmCrtc *crtc_;
> +  int display_;
> 
>    std::vector<DrmCrtc *> possible_crtcs_;
>  };
> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> index dfca1a6..65337cc 100644
> --- a/drmhwctwo.cpp
> +++ b/drmhwctwo.cpp
> @@ -700,6 +700,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>          break;
>      }
>    }
> +  compositor_.ResetCountdown();
>    return *num_types ? HWC2::Error::HasChanges : HWC2::Error::None;
>  }
> 
> diff --git a/drmresources.cpp b/drmresources.cpp
> index 32dd376..880cef2 100644
> --- a/drmresources.cpp
> +++ b/drmresources.cpp
> @@ -33,6 +33,8 @@
>  #include <cutils/log.h>
>  #include <cutils/properties.h>
> 
> +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS    4
> +

Presumably this should come from libdrm headers?

>  namespace android {
> 
>  DrmResources::DrmResources() : event_listener_(this) {
> @@ -65,6 +67,11 @@ int DrmResources::Init() {
>      return ret;
>    }
> 
> +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> +  if (ret) {
> +    ALOGI("Failed to set writeback cap %d", ret);
> +    ret = 0;
> +  }
>    drmModeResPtr res = drmModeGetResources(fd());
>    if (!res) {
>      ALOGE("Failed to get DrmResources resources");
> @@ -77,6 +84,7 @@ int DrmResources::Init() {
>        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
> 
>    bool found_primary = false;
> +  int primary_index = 0;
>    int display_num = 1;
> 
>    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> @@ -162,18 +170,22 @@ int DrmResources::Init() {
>      if (conn->internal() && !found_primary) {
>        conn->set_display(0);
>        found_primary = true;
> -    } else {
> +    } else if (conn->external()) {
> +      if (!found_primary) primary_index++;

This is against style guide (and same below).

>        conn->set_display(display_num);
>        ++display_num;
>      }
>    }
> 
>    // Then look for primary amongst external connectors
> +  if (!found_primary) primary_index = 0;
>    for (auto &conn : connectors_) {
>      if (conn->external() && !found_primary) {
>        conn->set_display(0);
>        found_primary = true;
> +      break;
>      }
> +    if (!found_primary) primary_index++;
>    }
> 
>    if (res)
> @@ -220,12 +232,29 @@ int DrmResources::Init() {
>    }
> 
>    for (auto &conn : connectors_) {
> +    if (conn->writeback())
> +      continue;
>      ret = CreateDisplayPipe(conn.get());
>      if (ret) {
>        ALOGE("Failed CreateDisplayPipe %d with %d", conn->id(), ret);
>        return ret;
>      }
>    }
> +  /* Allocate writebacks according to the display number */
> +  /* 0 should get a writeback before 1 */
> +  for (auto &writeback_conn : connectors_) {
> +    if (!writeback_conn->writeback())
> +      continue;
> +    int index = primary_index;
> +    do {
> +      if (connectors_[index]->display() < 0)
> +        continue;
> +      ret = AttachWriteback(writeback_conn.get(), connectors_[index].get());
> +      if (!ret)
> +        break;
> +      index = (index + 1) % connectors_.size();
> +    } while (index != primary_index);
> +  }

I think you can avoid all of the primary_index gymnastics if you just make
allocating the writeback connector part of CreateDisplayPipe (which dovetails
nicely into my suggestion up above to allocate a display to the writeback
connector.

>    return 0;
>  }
> 
> @@ -314,6 +343,31 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
>    return -ENODEV;
>  }
> 
> +/*
> + * Attach writeback connector to the CRTC linked to the display_conn
> + *
> + */
> +int DrmResources::AttachWriteback(DrmConnector *writeback_conn, DrmConnector *display_conn) {
> +  int ret = -EINVAL;
> +  if (display_conn->writeback())
> +    return -EINVAL;
> +  if (!display_conn->encoder() || ! display_conn->encoder()->crtc())
> +    return -EINVAL;
> +  DrmCrtc *display_crtc = display_conn->encoder()->crtc();
> +  if (display_crtc->writeback_conn() != nullptr)
> +    return -EINVAL;
> +  for (DrmEncoder *writeback_enc : writeback_conn->possible_encoders()) {
> +    // 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);
> +      display_crtc->set_writeback_conn(writeback_conn);
> +      ret = 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 4cca48c..ad285ec 100644
> --- a/drmresources.h
> +++ b/drmresources.h
> @@ -78,6 +78,7 @@ class DrmResources {
>                    DrmProperty *property);
> 
>    int CreateDisplayPipe(DrmConnector *connector);
> +  int AttachWriteback(DrmConnector *writeback_conn, DrmConnector *display_conn);
> 
>    UniqueFd fd_;
>    uint32_t mode_id_ = 0;
> --
> 2.7.4
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Alexandru-Cosmin Gheorghe March 20, 2018, 11:26 a.m. UTC | #4

Hi Sean,

Thank you for the feedback.
I will comeback with v2, later on.

On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > This patchset tries to add support for using writeback connector to
> > flatten a scene when it doesn't change for a while. This idea had
> > been floated around on IRC here [1] and here [2].
> > 
> > Developed on top of the latest writeback series, sent by Liviu here
> > [3].
> > 
> > Probably the patch should/could be broken in more patches, but since I
> > want to put this out there to get feedback, I kept them as a single
> > patch for now.
> 
> Thank you for your patch, it's looking good. Feel free to submit the v2 in
> multiple patches. Also note that we've migrated to gitlab, the new tree is
> located here:
> 
> https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer

This seems to be behind
https://anongit.freedesktop.org/git/drm_hwcomposer.git.
Is this location deprecated or does it serve other purposes.


> 
> 
> > 
> > This change could be summarize as follow:
> > - Attach a writeback connector to the CRTC that's controlling a
> > display.
> > - Detect the scene did not change for a while(60 vblanks).
> > - Re-commit scene and get the composited scene through the writeback
> > connector.
> > - Commit the whole scene as a single plane.
> > 
> > Some areas that I consider important and I could use some
> > feedback/ideas:
> > 
> > 1. Building the pipeline.
> > Currently, drm_hwcomposer allows to connect just a single connector
> > to a crtc. For now, I decided to treat the writeback connector as a
> > separate field inside DrmCrtc. I'm not sure if it's a good idea to try
> > to handle this in a unified way, since the writeback connector is such
> > a special connector. Regarding the allocation of writeback connectors,
> > my idea was to allocate writeback connector to the primary display
> > first and then continue allocating while respecting the display number. 0
> > gets a writeback before 1 and so on.
> > 
> > 2. Heuristic for triggering the flattening.
> > I just created a VSyncWorker which will trigger the flattening of the
> > scene if it doesn't change for 60 consecutive vsyncs. The countdown
> > gets reset every time ValidateDisplay is called. This is a relatively
> > basic heuristic, so I'm open to suggestions.
> > 
> > 3. Locking scheme corner cases.
> > The Vsynworker is a separate thread which will contend with
> > SurfaceFlinger for showing things on the screen. I tried to limit the
> > race window by resetting the countdown on ValidateDisplay and
> > explicitely checking that we still need to use the flatten scene before
> > commiting to get the writeback result or before applying the flattened
> > scene.
> > 
> > 4. Building the DrmDisplayComposition for the flattened scene.
> > I kind of lost myself  in all types of layers/planes and compositions,
> > so I'm not sure if I'm correctly building the DrmDisplayComposition
> > object for the FlattenScene, it works and shows what I expect on the
> > screen. So, any feedback here is appreciated.
> > 
> > 5. I see there is a drmcompositorworker.cpp which implemented the same
> > idea using the GPU, however that seems to not be used anymore, does
> > anyone know the rationale behind it?
> > 
> > Some unfinished/untested things:
> > - Make sure the DrmFrameBuffer allocates one of the formats reported
> >   in WRITEBACK_PIXEL_FORMATS.
> > - I'm using a hacked setup where, when needed it, the GL compositing is
> >   done by Surfaceflinger, so I'm not sure how well this changes are
> >   getting along with the GLCompositorWorker.
> > 
> > [1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-02-23
> > [2] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-02-09
> > [3] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html
> > 
> > Signed-off-by: Alexandru Gheorghe <[e-mail]>
> > ---
> >  drmconnector.cpp         |  36 ++++++++++-
> >  drmconnector.h           |   8 +++
> >  drmcrtc.cpp              |  11 +++-
> >  drmcrtc.h                |   8 ++-
> >  drmdisplaycompositor.cpp | 164 +++++++++++++++++++++++++++++++++++++++++++++--
> >  drmdisplaycompositor.h   |  16 +++--
> >  drmencoder.cpp           |  15 +++++
> >  drmencoder.h             |   7 +-
> >  drmhwctwo.cpp            |   1 +
> >  drmresources.cpp         |  56 +++++++++++++++-
> >  drmresources.h           |   1 +
> >  11 files changed, 306 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drmconnector.cpp b/drmconnector.cpp
> > index 145518f..2ed4f23 100644
> > --- a/drmconnector.cpp
> > +++ b/drmconnector.cpp
> > @@ -52,6 +52,23 @@ int DrmConnector::Init() {
> >      ALOGE("Could not get CRTC_ID property\n");
> >      return ret;
> >    }
> > +  if (writeback()) {
> > +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_PIXEL_FORMATS", &writeback_pixel_formats_);
> > +    if (ret) {
> > +      ALOGE("Could not get WRITEBACK_PIXEL_FORMATS connector_id = %d\n", id_);
> > +      return ret;
> > +    }
> > +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_FB_ID", &writeback_fb_id_);
> > +    if (ret) {
> > +      ALOGE("Could not get WRITEBACK_FB_ID connector_id = %d\n", id_);
> > +      return ret;
> > +    }
> > +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_OUT_FENCE_PTR", &writeback_out_fence_);
> > +    if (ret) {
> > +      ALOGE("Could not get WRITEBACK_OUT_FENCE_PTR connector_id = %d\n", id_);
> > +      return ret;
> > +    }
> > +  }
> >    return 0;
> >  }
> > 
> > @@ -78,8 +95,13 @@ bool DrmConnector::external() const {
> >           type_ == DRM_MODE_CONNECTOR_VGA;
> >  }
> > 
> > +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> > +bool DrmConnector::writeback() const {
> > +        return type_ == DRM_MODE_CONNECTOR_WRITEBACK;
> > +}
> > +
> >  bool DrmConnector::valid_type() const {
> > -  return internal() || external();
> > +  return internal() || external() || writeback();
> >  }
> > 
> >  int DrmConnector::UpdateModes() {
> > @@ -130,6 +152,18 @@ const DrmProperty &DrmConnector::crtc_id_property() const {
> >    return crtc_id_property_;
> >  }
> > 
> > +const DrmProperty &DrmConnector::writeback_pixel_formats() const {
> > +  return writeback_pixel_formats_;
> > +}
> > +
> > +const DrmProperty &DrmConnector::writeback_fb_id() const {
> > +  return writeback_fb_id_;
> > +}
> > +
> > +const DrmProperty &DrmConnector::writeback_out_fence() const {
> > +  return writeback_out_fence_;
> > +}
> > +
> >  DrmEncoder *DrmConnector::encoder() const {
> >    return encoder_;
> >  }
> > diff --git a/drmconnector.h b/drmconnector.h
> > index 5601e06..ad18762 100644
> > --- a/drmconnector.h
> > +++ b/drmconnector.h
> > @@ -28,6 +28,7 @@
> >  namespace android {
> > 
> >  class DrmResources;
> > +class DrmCrtc;
> 
> Not needed?
> 
Yes it was, there were some circular includes there, I will
try and fixed them some other way.

> > 
> >  class DrmConnector {
> >   public:
> > @@ -46,6 +47,7 @@ class DrmConnector {
> > 
> >    bool internal() const;
> >    bool external() const;
> > +  bool writeback() const;
> >    bool valid_type() const;
> > 
> >    int UpdateModes();
> > @@ -58,6 +60,9 @@ class DrmConnector {
> > 
> >    const DrmProperty &dpms_property() const;
> >    const DrmProperty &crtc_id_property() const;
> > +  const DrmProperty &writeback_pixel_formats() const;
> > +  const DrmProperty &writeback_fb_id() const;
> > +  const DrmProperty &writeback_out_fence() const;
> > 
> >    const std::vector<DrmEncoder *> &possible_encoders() const {
> >      return possible_encoders_;
> > @@ -88,6 +93,9 @@ class DrmConnector {
> > 
> >    DrmProperty dpms_property_;
> >    DrmProperty crtc_id_property_;
> > +  DrmProperty writeback_pixel_formats_;
> > +  DrmProperty writeback_fb_id_;
> > +  DrmProperty writeback_out_fence_;
> > 
> >    std::vector<DrmEncoder *> possible_encoders_;
> >  };
> > diff --git a/drmcrtc.cpp b/drmcrtc.cpp
> > index 1b354fe..f8c9f25 100644
> > --- a/drmcrtc.cpp
> > +++ b/drmcrtc.cpp
> > @@ -31,7 +31,8 @@ DrmCrtc::DrmCrtc(DrmResources *drm, drmModeCrtcPtr c, unsigned pipe)
> >        id_(c->crtc_id),
> >        pipe_(pipe),
> >        display_(-1),
> > -      mode_(&c->mode) {
> > +      mode_(&c->mode),
> > +      writeback_conn_(nullptr) {
> >  }
> > 
> >  int DrmCrtc::Init() {
> > @@ -75,6 +76,14 @@ bool DrmCrtc::can_bind(int display) const {
> >    return display_ == -1 || display_ == display;
> >  }
> > 
> > +DrmConnector* DrmCrtc::writeback_conn() const {
> > +  return writeback_conn_;
> > +}
> > +
> > +void DrmCrtc::set_writeback_conn(DrmConnector* writeback_conn) {
> > +  writeback_conn_ = writeback_conn;
> > +}
> > +
> >  const DrmProperty &DrmCrtc::active_property() const {
> >    return active_property_;
> >  }
> > diff --git a/drmcrtc.h b/drmcrtc.h
> > index c5a5599..bbd8d37 100644
> > --- a/drmcrtc.h
> > +++ b/drmcrtc.h
> > @@ -22,11 +22,11 @@
> > 
> >  #include <stdint.h>
> >  #include <xf86drmMode.h>
> > -
> > +#include "drmconnector.h"
> 
> Please retain whitespace (comment applies below as well)

Will do, sorry about that.

> 
> >  namespace android {
> > 
> >  class DrmResources;
> > -
> > +class DrmConnector;
> >  class DrmCrtc {
> >   public:
> >    DrmCrtc(DrmResources *drm, drmModeCrtcPtr c, unsigned pipe);
> > @@ -39,7 +39,9 @@ class DrmCrtc {
> >    unsigned pipe() const;
> > 
> >    int display() const;
> > +  DrmConnector* writeback_conn() const;
> 
> I think a better way would be to associate the display with the connector as it
> does for a traditional connector. This might be as simple as just adding a
> !conn->writeback() check to GetConnectorForDisplay() and adding a new
> GetWritebackConnectorForDisplay() function to DrmResources.

Could be done, I will add that in v2.

> 
> >    void set_display(int display);
> > +  void set_writeback_conn(DrmConnector*);
> > 
> >    bool can_bind(int display) const;
> > 
> > @@ -55,7 +57,7 @@ class DrmCrtc {
> >    int display_;
> > 
> >    DrmMode mode_;
> > -
> > +  DrmConnector *writeback_conn_;
> >    DrmProperty active_property_;
> >    DrmProperty mode_property_;
> >    DrmProperty out_fence_ptr_property_;
> > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> > index e556e86..9783852 100644
> > --- a/drmdisplaycompositor.cpp
> > +++ b/drmdisplaycompositor.cpp
> > @@ -16,7 +16,6 @@
> > 
> >  #define ATRACE_TAG ATRACE_TAG_GRAPHICS
> >  #define LOG_TAG "hwc-drm-display-compositor"
> > -
> 
> More unrelated whitespace changes

Will do.

> 
> >  #include "drmdisplaycompositor.h"
> > 
> >  #include <pthread.h>
> > @@ -36,9 +35,24 @@
> >  #include "drmplane.h"
> >  #include "drmresources.h"
> >  #include "glworker.h"
> > +static const uint32_t kWaitWritebackFence = 100; //ms
> > 
> >  namespace android {
> > 
> > +class CompositorVsyncCallback : public VsyncCallback {
> > + public:
> > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > +      :compositor_(compositor) {
> > +  }
> > +
> > +  void Callback(int display, int64_t timestamp) {
> > +    compositor_->Vsync(display, timestamp);
> > +  }
> > +
> > + private:
> > +  DrmDisplayCompositor *compositor_;
> > +};
> > +
> 
> The GLCompositor already has flattening, so we should tie into that. I assume
> writeback is going to be more efficient than using GPU (given that the GPU is
> probably turned off in these scenarios), so you're probably safe to use
> writeback when available and fall back to GL if it's not.
> 
> 

As far as I understood, this was done by the drmcompositorworker.cpp
by calling SquashAll but that's removed from build now. GLCompositor
seems to do compositing only when there are not enough overlays or 
atomic check fails.

Am I missing something ? 
Is it still doing flattening when then scene does not change ?

> >  void SquashState::Init(DrmHwcLayer *layers, size_t num_layers) {
> >    generation_number_++;
> >    valid_history_ = 0;
> > @@ -183,7 +197,8 @@ DrmDisplayCompositor::DrmDisplayCompositor()
> >        framebuffer_index_(0),
> >        squash_framebuffer_index_(0),
> >        dump_frames_composited_(0),
> > -      dump_last_timestamp_ns_(0) {
> > +      dump_last_timestamp_ns_(0),
> > +      flatten_countdown_(FLATTEN_COUNTDOWN_INIT) {
> >    struct timespec ts;
> >    if (clock_gettime(CLOCK_MONOTONIC, &ts))
> >      return;
> > @@ -193,7 +208,7 @@ DrmDisplayCompositor::DrmDisplayCompositor()
> >  DrmDisplayCompositor::~DrmDisplayCompositor() {
> >    if (!initialized_)
> >      return;
> > -
> > +  vsync_worker_.Exit();
> >    int ret = pthread_mutex_lock(&lock_);
> >    if (ret)
> >      ALOGE("Failed to acquire compositor lock %d", ret);
> > @@ -223,6 +238,9 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
> >    }
> > 
> >    initialized_ = true;
> > +  vsync_worker_.Init(drm_, display_);
> > +  auto callback = std::make_shared<CompositorVsyncCallback>(this);
> > +  vsync_worker_.RegisterCallback(callback);
> >    return 0;
> >  }
> > 
> > @@ -482,7 +500,7 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
> >  }
> > 
> >  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> > -                                      bool test_only) {
> > +                                      bool test_only, DrmDisplayComposition *writeback_comp) {
> >    ATRACE_CALL();
> > 
> >    int ret = 0;
> > @@ -491,7 +509,8 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >    std::vector<DrmCompositionPlane> &comp_planes =
> >        display_comp->composition_planes();
> >    uint64_t out_fences[drm_->crtcs().size()];
> > -
> > +  int writeback_fence = -1;
> > +  DrmFramebuffer *writeback_fb = nullptr;
> >    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
> >    if (!connector) {
> >      ALOGE("Could not locate connector for display %d", display_);
> > @@ -508,6 +527,32 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >      ALOGE("Failed to allocate property set");
> >      return -ENOMEM;
> >    }
> > +  DrmConnector *writeback_conn = crtc->writeback_conn();
> > +  if (writeback_comp != nullptr) {
> > +    if (writeback_conn == nullptr)
> 
> FWIW, we use NULL everywhere else, so let's stick with that.

Will do.
> 
> > +      return -EINVAL;
> > +    writeback_fb = &framebuffers_[framebuffer_index_];
> > +    framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS;
> > +    ret = PrepareFramebuffer(*writeback_fb, writeback_comp);
> > +    if (ret) {
> > +      ALOGE("Failed to prepare framebuffer for pre-composite %d", ret);
> > +      return ret;
> > +    }
> > +    if (writeback_conn->writeback_fb_id().id() == 0 || writeback_conn->writeback_out_fence().id() == 0)
> > +      return -EINVAL;
> > +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), writeback_conn->writeback_fb_id().id(),
> > +                                   writeback_comp->layers().back().buffer->fb_id);
> > +    if (ret < 0) {
> > +      ALOGE("Failed to add writeback_fb_id");
> > +      return ret;
> > +    }
> > +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), writeback_conn->writeback_out_fence().id(),
> > +                                   (uint64_t) &writeback_fence);
> > +    if (ret < 0) {
> > +      ALOGE("Failed to add writeback_out_fence");
> > +      return ret;
> > +    }
> > +  }
> > 
> >    if (crtc->out_fence_ptr_property().id() != 0) {
> >      ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(),
> > @@ -537,6 +582,12 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >        drmModeAtomicFree(pset);
> >        return ret;
> >      }
> > +    if (writeback_conn != nullptr) {
> > +      ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), writeback_conn->crtc_id_property().id(), crtc->id());
> > +      if (ret < 0) {
> > +          ALOGE("Failed to  attach writeback");
> > +      }
> > +    }
> >    }
> > 
> >    for (DrmCompositionPlane &comp_plane : comp_planes) {
> > @@ -691,6 +742,17 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >      if (test_only)
> >        flags |= DRM_MODE_ATOMIC_TEST_ONLY;
> > 
> > +    // There is a race when we recommit a scene to get the writeback result
> > +    // to shorten that race, check if we still need that result
> > +    bool abort_commit = false;
> > +    if (writeback_comp != nullptr) {
> > +      AutoLock lock(&lock_, "CommitFrame");
> > +      lock.Lock();
> > +      abort_commit = !CountdownExpired();
> > +    }
> > +    if (abort_commit)
> > +      return -EINVAL;
> > +
> >      ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_);
> >      if (ret) {
> >        if (test_only)
> > @@ -729,6 +791,14 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >      display_comp->set_out_fence((int) out_fences[crtc->pipe()]);
> >    }
> > 
> > +  if (writeback_fence >= 0 && writeback_fb != nullptr) {
> > +    ret = sync_wait(writeback_fence, kWaitWritebackFence);
> > +    if (ret) {
> > +        ALOGE("Failed to wait on writeback fence");
> > +    }
> > +    close(writeback_fence);
> > +  }
> > +
> >    return ret;
> >  }
> > 
> > @@ -784,7 +854,7 @@ void DrmDisplayCompositor::ClearDisplay() {
> >  }
> > 
> >  void DrmDisplayCompositor::ApplyFrame(
> > -    std::unique_ptr<DrmDisplayComposition> composition, int status) {
> > +    std::unique_ptr<DrmDisplayComposition> composition, int status, bool start_countdown) {
> >    int ret = status;
> > 
> >    if (!ret)
> > @@ -807,6 +877,8 @@ void DrmDisplayCompositor::ApplyFrame(
> >      ALOGE("Failed to acquire lock for active_composition swap");
> > 
> >    active_composition_.swap(composition);
> > +  flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
> > +  vsync_worker_.VSyncControl(start_countdown);
> > 
> >    if (!ret)
> >      ret = pthread_mutex_unlock(&lock_);
> > @@ -878,6 +950,65 @@ int DrmDisplayCompositor::ApplyComposition(
> >    return ret;
> >  }
> > 
> > +int DrmDisplayCompositor::FlattenScene() {
> > +  std::unique_ptr<DrmDisplayComposition> comp = CreateComposition();
> > +  DrmDisplayComposition *src = active_composition_.get();
> > +  DrmDisplayComposition *dst = comp.get();
> > +  std::vector<DrmCompositionPlane> &src_planes = src->composition_planes();
> > +  if (src == nullptr || dst == nullptr)
> > +    return -EINVAL;
> > +  size_t src_planes_with_layer = std::count_if(
> > +      src_planes.begin(), src_planes.end(), [](DrmCompositionPlane &p) {
> > +        return p.type() != DrmCompositionPlane::Type::kDisable;
> > +      });
> > +
> > +  if (src_planes_with_layer <= 1)
> > +    return -EALREADY;
> > +  int ret = dst->Init(drm_, src->crtc(), src->importer(), src->planner(),
> > +                      src->frame_no());
> > +  if (ret)
> > +    return ret;
> > +  DrmCompositionPlane squashed_comp(DrmCompositionPlane::Type::kPrecomp, NULL,
> > +                                    src->crtc());
> > +  for (DrmCompositionPlane &comp_plane : src_planes) {
> > +    if (comp_plane.plane() == NULL) {
> > +      ALOGE("Skipping squash all because of NULL plane");
> > +      ret = -EINVAL;
> > +    }
> > +    if (comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY)
> > +      squashed_comp.set_plane(comp_plane.plane());
> > +    else
> > +      dst->AddPlaneDisable(comp_plane.plane());
> > +  }
> > +  ret = CommitFrame(active_composition_.get(), 0, dst);
> > +  if (ret || dst->layers().size() != 1) {
> > +      ALOGE("Failed to flatten scene using writeback");
> > +      return -EINVAL;
> > +  }
> > +  squashed_comp.source_layers().push_back(0);
> > +
> > +  ret = dst->AddPlaneComposition(std::move(squashed_comp));
> > +  if (ret) {
> > +    ALOGE("Failed to add flatten scene");
> > +    return ret;
> > +  }
> > +  ret = dst->FinalizeComposition();
> > +  if (ret) {
> > +    ALOGE("Failed to finalize composition");
> > +    return ret;
> > +  }
> > +  // If countdown it's still expired ApplyFlattenScene
> > +  AutoLock lock(&lock_, "FlattenScene");
> > +  lock.Lock();
> > +  if (CountdownExpired()) {
> > +    lock.Unlock();
> > +    ApplyFrame(std::move(comp), 0, false);
> > +  } else {
> > +    return -EAGAIN;
> > +  }
> > +  return 0;
> > +}
> > +
> >  int DrmDisplayCompositor::SquashAll() {
> >    AutoLock lock(&lock_, "compositor");
> >    int ret = lock.Lock();
> > @@ -1026,6 +1157,27 @@ move_layers_back:
> >    return ret;
> >  }
> > 
> > +bool DrmDisplayCompositor::CountdownExpired() const {
> > +  return flatten_countdown_ <= 0;
> > +}
> > +
> > +void DrmDisplayCompositor::ResetCountdown() {
> > +  AutoLock lock(&lock_, "ResetCountdown");
> > +  lock.Lock();
> > +  flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
> > +}
> > +
> > +void DrmDisplayCompositor::Vsync(int display, int64_t timestamp) {
> > +   AutoLock lock(&lock_, "VSync");
> > +   lock.Lock();
> > +   flatten_countdown_--;
> > +   if (CountdownExpired()) {
> > +     lock.Unlock();
> > +     int ret = FlattenScene();
> > +     ALOGI("Vsync: Flatten scene for display %d at timestamp %" PRIu64 " ret = %d \n", display, timestamp, ret);
> > +   }
> > +}
> > +
> >  void DrmDisplayCompositor::Dump(std::ostringstream *out) const {
> >    int ret = pthread_mutex_lock(&lock_);
> >    if (ret)
> > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> > index f1965fb..4a5696a 100644
> > --- a/drmdisplaycompositor.h
> > +++ b/drmdisplaycompositor.h
> > @@ -29,11 +29,15 @@
> > 
> >  #include <hardware/hardware.h>
> >  #include <hardware/hwcomposer.h>
> > +#include <vsyncworker.h>
> > 
> >  // One for the front, one for the back, and one for cases where we need to
> >  // squash a frame that the hw can't display with hw overlays.
> >  #define DRM_DISPLAY_BUFFERS 3
> > 
> > +// If a scene is still for this number of vblanks flatten it to reduce power consumption.
> > +#define FLATTEN_COUNTDOWN_INIT 60
> > +
> >  namespace android {
> > 
> >  class GLWorkerCompositor;
> > @@ -91,7 +95,8 @@ class DrmDisplayCompositor {
> >    int Composite();
> >    int SquashAll();
> >    void Dump(std::ostringstream *out) const;
> > -
> > +  void Vsync(int display, int64_t timestamp);
> > +  void ResetCountdown();
> >    std::tuple<uint32_t, uint32_t, int> GetActiveModeResolution();
> > 
> >    SquashState *squash_state() {
> > @@ -118,15 +123,16 @@ class DrmDisplayCompositor {
> >    int ApplySquash(DrmDisplayComposition *display_comp);
> >    int ApplyPreComposite(DrmDisplayComposition *display_comp);
> >    int PrepareFrame(DrmDisplayComposition *display_comp);
> > -  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only);
> > +  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only, DrmDisplayComposition *writeback_comp = nullptr);
> >    int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst);
> >    int ApplyDpms(DrmDisplayComposition *display_comp);
> >    int DisablePlanes(DrmDisplayComposition *display_comp);
> > 
> >    void ClearDisplay();
> >    void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
> > -                  int status);
> > -
> > +                  int status, bool start_countdown = true);
> > +  int FlattenScene();
> > +  bool CountdownExpired() const;
> >    std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
> > 
> >    DrmResources *drm_;
> > @@ -155,6 +161,8 @@ class DrmDisplayCompositor {
> >    // we need to reset them on every Dump() call.
> >    mutable uint64_t dump_frames_composited_;
> >    mutable uint64_t dump_last_timestamp_ns_;
> > +  VSyncWorker vsync_worker_;
> > +  int64_t flatten_countdown_;
> >  };
> >  }
> > 
> > diff --git a/drmencoder.cpp b/drmencoder.cpp
> > index 3d762f3..3df06a2 100644
> > --- a/drmencoder.cpp
> > +++ b/drmencoder.cpp
> > @@ -27,6 +27,7 @@ DrmEncoder::DrmEncoder(drmModeEncoderPtr e, DrmCrtc *current_crtc,
> >                         const std::vector<DrmCrtc *> &possible_crtcs)
> >      : id_(e->encoder_id),
> >        crtc_(current_crtc),
> > +      display_(-1),
> >        possible_crtcs_(possible_crtcs) {
> >  }
> > 
> > @@ -40,5 +41,19 @@ DrmCrtc *DrmEncoder::crtc() const {
> > 
> >  void DrmEncoder::set_crtc(DrmCrtc *crtc) {
> >    crtc_ = crtc;
> > +  set_display(crtc->display());
> >  }
> > +
> > +int DrmEncoder::display() const {
> > +  return display_;
> > +}
> > +
> > +void DrmEncoder::set_display(int display) {
> > +  display_ = display;
> > +}
> > +
> > +bool DrmEncoder::can_bind(int display) const {
> > +  return display_ == -1 || display_ == display;
> > +}
> > +
> >  }
> > diff --git a/drmencoder.h b/drmencoder.h
> > index 58ccbfb..6b39505 100644
> > --- a/drmencoder.h
> > +++ b/drmencoder.h
> > @@ -25,6 +25,8 @@
> > 
> >  namespace android {
> > 
> > +class DrmCrtc;
> > +
> >  class DrmEncoder {
> >   public:
> >    DrmEncoder(drmModeEncoderPtr e, DrmCrtc *current_crtc,
> > @@ -36,7 +38,9 @@ class DrmEncoder {
> > 
> >    DrmCrtc *crtc() const;
> >    void set_crtc(DrmCrtc *crtc);
> > -
> > +  bool can_bind(int display) const;
> > +  void set_display(int display);
> > +  int display() const;
> >    const std::vector<DrmCrtc *> &possible_crtcs() const {
> >      return possible_crtcs_;
> >    }
> > @@ -44,6 +48,7 @@ class DrmEncoder {
> >   private:
> >    uint32_t id_;
> >    DrmCrtc *crtc_;
> > +  int display_;
> > 
> >    std::vector<DrmCrtc *> possible_crtcs_;
> >  };
> > diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> > index dfca1a6..65337cc 100644
> > --- a/drmhwctwo.cpp
> > +++ b/drmhwctwo.cpp
> > @@ -700,6 +700,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
> >          break;
> >      }
> >    }
> > +  compositor_.ResetCountdown();
> >    return *num_types ? HWC2::Error::HasChanges : HWC2::Error::None;
> >  }
> > 
> > diff --git a/drmresources.cpp b/drmresources.cpp
> > index 32dd376..880cef2 100644
> > --- a/drmresources.cpp
> > +++ b/drmresources.cpp
> > @@ -33,6 +33,8 @@
> >  #include <cutils/log.h>
> >  #include <cutils/properties.h>
> > 
> > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS    4
> > +
> 
> Presumably this should come from libdrm headers?

Yes it will, this was just a quick fix to be able to compile against
older libdrm.

Btw, this would make drm_hwcomposer compilable only with newer libdrm
version, but I suppose that's acceptable.

> 
> >  namespace android {
> > 
> >  DrmResources::DrmResources() : event_listener_(this) {
> > @@ -65,6 +67,11 @@ int DrmResources::Init() {
> >      return ret;
> >    }
> > 
> > +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > +  if (ret) {
> > +    ALOGI("Failed to set writeback cap %d", ret);
> > +    ret = 0;
> > +  }
> >    drmModeResPtr res = drmModeGetResources(fd());
> >    if (!res) {
> >      ALOGE("Failed to get DrmResources resources");
> > @@ -77,6 +84,7 @@ int DrmResources::Init() {
> >        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
> > 
> >    bool found_primary = false;
> > +  int primary_index = 0;
> >    int display_num = 1;
> > 
> >    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> > @@ -162,18 +170,22 @@ int DrmResources::Init() {
> >      if (conn->internal() && !found_primary) {
> >        conn->set_display(0);
> >        found_primary = true;
> > -    } else {
> > +    } else if (conn->external()) {
> > +      if (!found_primary) primary_index++;
> 
> This is against style guide (and same below).

I suppose that's what happens when you skip some steps from 
"Submitting patches", clang will catch this next time.

> 
> >        conn->set_display(display_num);
> >        ++display_num;
> >      }
> >    }
> > 
> >    // Then look for primary amongst external connectors
> > +  if (!found_primary) primary_index = 0;
> >    for (auto &conn : connectors_) {
> >      if (conn->external() && !found_primary) {
> >        conn->set_display(0);
> >        found_primary = true;
> > +      break;
> >      }
> > +    if (!found_primary) primary_index++;
> >    }
> > 
> >    if (res)
> > @@ -220,12 +232,29 @@ int DrmResources::Init() {
> >    }
> > 
> >    for (auto &conn : connectors_) {
> > +    if (conn->writeback())
> > +      continue;
> >      ret = CreateDisplayPipe(conn.get());
> >      if (ret) {
> >        ALOGE("Failed CreateDisplayPipe %d with %d", conn->id(), ret);
> >        return ret;
> >      }
> >    }
> > +  /* Allocate writebacks according to the display number */
> > +  /* 0 should get a writeback before 1 */
> > +  for (auto &writeback_conn : connectors_) {
> > +    if (!writeback_conn->writeback())
> > +      continue;
> > +    int index = primary_index;
> > +    do {
> > +      if (connectors_[index]->display() < 0)
> > +        continue;
> > +      ret = AttachWriteback(writeback_conn.get(), connectors_[index].get());
> > +      if (!ret)
> > +        break;
> > +      index = (index + 1) % connectors_.size();
> > +    } while (index != primary_index);
> > +  }
> 
> I think you can avoid all of the primary_index gymnastics if you just make
> allocating the writeback connector part of CreateDisplayPipe (which dovetails
> nicely into my suggestion up above to allocate a display to the writeback
> connector.
>

I will give it a try and see what I get. That was my first option at
first, but then I changed my mind and I wanted to make sure primary
display gets assigned a writeback first. Probably, this is a typical
case of premature optimization.

> >    return 0;
> >  }
> > 
> > @@ -314,6 +343,31 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
> >    return -ENODEV;
> >  }
> > 
> > +/*
> > + * Attach writeback connector to the CRTC linked to the display_conn
> > + *
> > + */
> > +int DrmResources::AttachWriteback(DrmConnector *writeback_conn, DrmConnector *display_conn) {
> > +  int ret = -EINVAL;
> > +  if (display_conn->writeback())
> > +    return -EINVAL;
> > +  if (!display_conn->encoder() || ! display_conn->encoder()->crtc())
> > +    return -EINVAL;
> > +  DrmCrtc *display_crtc = display_conn->encoder()->crtc();
> > +  if (display_crtc->writeback_conn() != nullptr)
> > +    return -EINVAL;
> > +  for (DrmEncoder *writeback_enc : writeback_conn->possible_encoders()) {
> > +    // 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);
> > +      display_crtc->set_writeback_conn(writeback_conn);
> > +      ret = 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 4cca48c..ad285ec 100644
> > --- a/drmresources.h
> > +++ b/drmresources.h
> > @@ -78,6 +78,7 @@ class DrmResources {
> >                    DrmProperty *property);
> > 
> >    int CreateDisplayPipe(DrmConnector *connector);
> > +  int AttachWriteback(DrmConnector *writeback_conn, DrmConnector *display_conn);
> > 
> >    UniqueFd fd_;
> >    uint32_t mode_id_ = 0;
> > --
> > 2.7.4
> > 
> > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

Sean Paul March 20, 2018, 1:28 p.m. UTC | #5

On Tue, Mar 20, 2018 at 11:26:39AM +0000, Alexandru-Cosmin Gheorghe wrote:
> Hi Sean,
> 
> Thank you for the feedback.
> I will comeback with v2, later on.
> 
> On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> > On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > > This patchset tries to add support for using writeback connector to
> > > flatten a scene when it doesn't change for a while. This idea had
> > > been floated around on IRC here [1] and here [2].
> > > 
> > > Developed on top of the latest writeback series, sent by Liviu here
> > > [3].
> > > 
> > > Probably the patch should/could be broken in more patches, but since I
> > > want to put this out there to get feedback, I kept them as a single
> > > patch for now.
> > 
> > Thank you for your patch, it's looking good. Feel free to submit the v2 in
> > multiple patches. Also note that we've migrated to gitlab, the new tree is
> > located here:
> > 
> > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer
> 
> This seems to be behind
> https://anongit.freedesktop.org/git/drm_hwcomposer.git.
> Is this location deprecated or does it serve other purposes.
> 

Ah, thanks for catching this. It's a migration glitch, I've made it whole
again.

<snip />

> > >  #include "drmdisplaycompositor.h"
> > > 
> > >  #include <pthread.h>
> > > @@ -36,9 +35,24 @@
> > >  #include "drmplane.h"
> > >  #include "drmresources.h"
> > >  #include "glworker.h"
> > > +static const uint32_t kWaitWritebackFence = 100; //ms
> > > 
> > >  namespace android {
> > > 
> > > +class CompositorVsyncCallback : public VsyncCallback {
> > > + public:
> > > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > > +      :compositor_(compositor) {
> > > +  }
> > > +
> > > +  void Callback(int display, int64_t timestamp) {
> > > +    compositor_->Vsync(display, timestamp);
> > > +  }
> > > +
> > > + private:
> > > +  DrmDisplayCompositor *compositor_;
> > > +};
> > > +
> > 
> > The GLCompositor already has flattening, so we should tie into that. I assume
> > writeback is going to be more efficient than using GPU (given that the GPU is
> > probably turned off in these scenarios), so you're probably safe to use
> > writeback when available and fall back to GL if it's not.
> > 
> > 
> 
> As far as I understood, this was done by the drmcompositorworker.cpp
> by calling SquashAll but that's removed from build now. GLCompositor
> seems to do compositing only when there are not enough overlays or 
> atomic check fails.
> 
> Am I missing something ? 
> Is it still doing flattening when then scene does not change ?

https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/drmcompositorworker.cpp#L62

If the scene doesn't change, everything is squashed.

<snip />

> > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > index 32dd376..880cef2 100644
> > > --- a/drmresources.cpp
> > > +++ b/drmresources.cpp
> > > @@ -33,6 +33,8 @@
> > >  #include <cutils/log.h>
> > >  #include <cutils/properties.h>
> > > 
> > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS    4
> > > +
> > 
> > Presumably this should come from libdrm headers?
> 
> Yes it will, this was just a quick fix to be able to compile against
> older libdrm.
> 
> Btw, this would make drm_hwcomposer compilable only with newer libdrm
> version, but I suppose that's acceptable.

You can use preprocessor checks to determine if
DRM_CLIENT_CAP_WRITEBACK_CONNECTORS is defined, and disable the functionality if
it's not.

> 
> > 
> > >  namespace android {
> > > 
> > >  DrmResources::DrmResources() : event_listener_(this) {
> > > @@ -65,6 +67,11 @@ int DrmResources::Init() {
> > >      return ret;
> > >    }
> > > 
> > > +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > > +  if (ret) {
> > > +    ALOGI("Failed to set writeback cap %d", ret);
> > > +    ret = 0;
> > > +  }
> > >    drmModeResPtr res = drmModeGetResources(fd());
> > >    if (!res) {
> > >      ALOGE("Failed to get DrmResources resources");
> > > @@ -77,6 +84,7 @@ int DrmResources::Init() {
> > >        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
> > > 
> > >    bool found_primary = false;
> > > +  int primary_index = 0;
> > >    int display_num = 1;
> > > 
> > >    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> > > @@ -162,18 +170,22 @@ int DrmResources::Init() {
> > >      if (conn->internal() && !found_primary) {
> > >        conn->set_display(0);
> > >        found_primary = true;
> > > -    } else {
> > > +    } else if (conn->external()) {
> > > +      if (!found_primary) primary_index++;
> > 
> > This is against style guide (and same below).
> 
> I suppose that's what happens when you skip some steps from 
> "Submitting patches", clang will catch this next time.
> 

I might look into adding a hook to gitlab to automatically run this, time
permitting.

Sean

<snip />

Alexandru-Cosmin Gheorghe March 20, 2018, 1:35 p.m. UTC | #6

Hi,

On Tue, Mar 20, 2018 at 09:28:50AM -0400, Sean Paul wrote:
> On Tue, Mar 20, 2018 at 11:26:39AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > Hi Sean,
> > 
> > Thank you for the feedback.
> > I will comeback with v2, later on.
> > 
> > On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> > > On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > > > This patchset tries to add support for using writeback connector to
> > > > flatten a scene when it doesn't change for a while. This idea had
> > > > been floated around on IRC here [1] and here [2].
> > > > 
> > > > Developed on top of the latest writeback series, sent by Liviu here
> > > > [3].
> > > > 
> > > > Probably the patch should/could be broken in more patches, but since I
> > > > want to put this out there to get feedback, I kept them as a single
> > > > patch for now.
> > > 
> > > Thank you for your patch, it's looking good. Feel free to submit the v2 in
> > > multiple patches. Also note that we've migrated to gitlab, the new tree is
> > > located here:
> > > 
> > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer
> > 
> > This seems to be behind
> > https://anongit.freedesktop.org/git/drm_hwcomposer.git.
> > Is this location deprecated or does it serve other purposes.
> > 
> 
> Ah, thanks for catching this. It's a migration glitch, I've made it whole
> again.
> 
> <snip />
> 
> > > >  #include "drmdisplaycompositor.h"
> > > > 
> > > >  #include <pthread.h>
> > > > @@ -36,9 +35,24 @@
> > > >  #include "drmplane.h"
> > > >  #include "drmresources.h"
> > > >  #include "glworker.h"
> > > > +static const uint32_t kWaitWritebackFence = 100; //ms
> > > > 
> > > >  namespace android {
> > > > 
> > > > +class CompositorVsyncCallback : public VsyncCallback {
> > > > + public:
> > > > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > > > +      :compositor_(compositor) {
> > > > +  }
> > > > +
> > > > +  void Callback(int display, int64_t timestamp) {
> > > > +    compositor_->Vsync(display, timestamp);
> > > > +  }
> > > > +
> > > > + private:
> > > > +  DrmDisplayCompositor *compositor_;
> > > > +};
> > > > +
> > > 
> > > The GLCompositor already has flattening, so we should tie into that. I assume
> > > writeback is going to be more efficient than using GPU (given that the GPU is
> > > probably turned off in these scenarios), so you're probably safe to use
> > > writeback when available and fall back to GL if it's not.
> > > 
> > > 
> > 
> > As far as I understood, this was done by the drmcompositorworker.cpp
> > by calling SquashAll but that's removed from build now. GLCompositor
> > seems to do compositing only when there are not enough overlays or 
> > atomic check fails.
> > 
> > Am I missing something ? 
> > Is it still doing flattening when then scene does not change ?
> 
> https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/drmcompositorworker.cpp#L62
> 
> If the scene doesn't change, everything is squashed.

Yeah, but that file it's not even built, see [1].
So, I kind of assumed someone had a reason of deleting it. 
Any idea about that?

[1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/Android.mk#L53

> 
> <snip />
> 
> > > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > > index 32dd376..880cef2 100644
> > > > --- a/drmresources.cpp
> > > > +++ b/drmresources.cpp
> > > > @@ -33,6 +33,8 @@
> > > >  #include <cutils/log.h>
> > > >  #include <cutils/properties.h>
> > > > 
> > > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS    4
> > > > +
> > > 
> > > Presumably this should come from libdrm headers?
> > 
> > Yes it will, this was just a quick fix to be able to compile against
> > older libdrm.
> > 
> > Btw, this would make drm_hwcomposer compilable only with newer libdrm
> > version, but I suppose that's acceptable.
> 
> You can use preprocessor checks to determine if
> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS is defined, and disable the functionality if
> it's not.
> 
> > 
> > > 
> > > >  namespace android {
> > > > 
> > > >  DrmResources::DrmResources() : event_listener_(this) {
> > > > @@ -65,6 +67,11 @@ int DrmResources::Init() {
> > > >      return ret;
> > > >    }
> > > > 
> > > > +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > > > +  if (ret) {
> > > > +    ALOGI("Failed to set writeback cap %d", ret);
> > > > +    ret = 0;
> > > > +  }
> > > >    drmModeResPtr res = drmModeGetResources(fd());
> > > >    if (!res) {
> > > >      ALOGE("Failed to get DrmResources resources");
> > > > @@ -77,6 +84,7 @@ int DrmResources::Init() {
> > > >        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
> > > > 
> > > >    bool found_primary = false;
> > > > +  int primary_index = 0;
> > > >    int display_num = 1;
> > > > 
> > > >    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> > > > @@ -162,18 +170,22 @@ int DrmResources::Init() {
> > > >      if (conn->internal() && !found_primary) {
> > > >        conn->set_display(0);
> > > >        found_primary = true;
> > > > -    } else {
> > > > +    } else if (conn->external()) {
> > > > +      if (!found_primary) primary_index++;
> > > 
> > > This is against style guide (and same below).
> > 
> > I suppose that's what happens when you skip some steps from 
> > "Submitting patches", clang will catch this next time.
> > 
> 
> I might look into adding a hook to gitlab to automatically run this, time
> permitting.
> 
> Sean
> 
> <snip />
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

Sean Paul March 20, 2018, 1:43 p.m. UTC | #7

On Tue, Mar 20, 2018 at 01:35:53PM +0000, Alexandru-Cosmin Gheorghe wrote:
> Hi,
> 
> On Tue, Mar 20, 2018 at 09:28:50AM -0400, Sean Paul wrote:
> > On Tue, Mar 20, 2018 at 11:26:39AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > Hi Sean,
> > > 
> > > Thank you for the feedback.
> > > I will comeback with v2, later on.
> > > 
> > > On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> > > > On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > > > > This patchset tries to add support for using writeback connector to
> > > > > flatten a scene when it doesn't change for a while. This idea had
> > > > > been floated around on IRC here [1] and here [2].
> > > > > 
> > > > > Developed on top of the latest writeback series, sent by Liviu here
> > > > > [3].
> > > > > 
> > > > > Probably the patch should/could be broken in more patches, but since I
> > > > > want to put this out there to get feedback, I kept them as a single
> > > > > patch for now.
> > > > 
> > > > Thank you for your patch, it's looking good. Feel free to submit the v2 in
> > > > multiple patches. Also note that we've migrated to gitlab, the new tree is
> > > > located here:
> > > > 
> > > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer
> > > 
> > > This seems to be behind
> > > https://anongit.freedesktop.org/git/drm_hwcomposer.git.
> > > Is this location deprecated or does it serve other purposes.
> > > 
> > 
> > Ah, thanks for catching this. It's a migration glitch, I've made it whole
> > again.
> > 
> > <snip />
> > 
> > > > >  #include "drmdisplaycompositor.h"
> > > > > 
> > > > >  #include <pthread.h>
> > > > > @@ -36,9 +35,24 @@
> > > > >  #include "drmplane.h"
> > > > >  #include "drmresources.h"
> > > > >  #include "glworker.h"
> > > > > +static const uint32_t kWaitWritebackFence = 100; //ms
> > > > > 
> > > > >  namespace android {
> > > > > 
> > > > > +class CompositorVsyncCallback : public VsyncCallback {
> > > > > + public:
> > > > > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > > > > +      :compositor_(compositor) {
> > > > > +  }
> > > > > +
> > > > > +  void Callback(int display, int64_t timestamp) {
> > > > > +    compositor_->Vsync(display, timestamp);
> > > > > +  }
> > > > > +
> > > > > + private:
> > > > > +  DrmDisplayCompositor *compositor_;
> > > > > +};
> > > > > +
> > > > 
> > > > The GLCompositor already has flattening, so we should tie into that. I assume
> > > > writeback is going to be more efficient than using GPU (given that the GPU is
> > > > probably turned off in these scenarios), so you're probably safe to use
> > > > writeback when available and fall back to GL if it's not.
> > > > 
> > > > 
> > > 
> > > As far as I understood, this was done by the drmcompositorworker.cpp
> > > by calling SquashAll but that's removed from build now. GLCompositor
> > > seems to do compositing only when there are not enough overlays or 
> > > atomic check fails.
> > > 
> > > Am I missing something ? 
> > > Is it still doing flattening when then scene does not change ?
> > 
> > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/drmcompositorworker.cpp#L62
> > 
> > If the scene doesn't change, everything is squashed.
> 
> Yeah, but that file it's not even built, see [1].
> So, I kind of assumed someone had a reason of deleting it. 
> Any idea about that?

Ha, WTF! Yeah, guess that's a regression. 

/me wonders whether we should just rip out the GLCompositor since no one seems
to care about it.

I'll circle back on the review and take a look at how you're doing the
inactivity stuff.

Sean

> 
> [1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/Android.mk#L53
> 
> > 
> > <snip />
> > 
> > > > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > > > index 32dd376..880cef2 100644
> > > > > --- a/drmresources.cpp
> > > > > +++ b/drmresources.cpp
> > > > > @@ -33,6 +33,8 @@
> > > > >  #include <cutils/log.h>
> > > > >  #include <cutils/properties.h>
> > > > > 
> > > > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS    4
> > > > > +
> > > > 
> > > > Presumably this should come from libdrm headers?
> > > 
> > > Yes it will, this was just a quick fix to be able to compile against
> > > older libdrm.
> > > 
> > > Btw, this would make drm_hwcomposer compilable only with newer libdrm
> > > version, but I suppose that's acceptable.
> > 
> > You can use preprocessor checks to determine if
> > DRM_CLIENT_CAP_WRITEBACK_CONNECTORS is defined, and disable the functionality if
> > it's not.
> > 
> > > 
> > > > 
> > > > >  namespace android {
> > > > > 
> > > > >  DrmResources::DrmResources() : event_listener_(this) {
> > > > > @@ -65,6 +67,11 @@ int DrmResources::Init() {
> > > > >      return ret;
> > > > >    }
> > > > > 
> > > > > +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > > > > +  if (ret) {
> > > > > +    ALOGI("Failed to set writeback cap %d", ret);
> > > > > +    ret = 0;
> > > > > +  }
> > > > >    drmModeResPtr res = drmModeGetResources(fd());
> > > > >    if (!res) {
> > > > >      ALOGE("Failed to get DrmResources resources");
> > > > > @@ -77,6 +84,7 @@ int DrmResources::Init() {
> > > > >        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
> > > > > 
> > > > >    bool found_primary = false;
> > > > > +  int primary_index = 0;
> > > > >    int display_num = 1;
> > > > > 
> > > > >    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> > > > > @@ -162,18 +170,22 @@ int DrmResources::Init() {
> > > > >      if (conn->internal() && !found_primary) {
> > > > >        conn->set_display(0);
> > > > >        found_primary = true;
> > > > > -    } else {
> > > > > +    } else if (conn->external()) {
> > > > > +      if (!found_primary) primary_index++;
> > > > 
> > > > This is against style guide (and same below).
> > > 
> > > I suppose that's what happens when you skip some steps from 
> > > "Submitting patches", clang will catch this next time.
> > > 
> > 
> > I might look into adding a hook to gitlab to automatically run this, time
> > permitting.
> > 
> > Sean
> > 
> > <snip />
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Cheers,
> Alex G

Alexandru-Cosmin Gheorghe March 20, 2018, 1:51 p.m. UTC | #8

On Tue, Mar 20, 2018 at 09:43:53AM -0400, Sean Paul wrote:
> On Tue, Mar 20, 2018 at 01:35:53PM +0000, Alexandru-Cosmin Gheorghe wrote:
> > Hi,
> > 
> > On Tue, Mar 20, 2018 at 09:28:50AM -0400, Sean Paul wrote:
> > > On Tue, Mar 20, 2018 at 11:26:39AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > > Hi Sean,
> > > > 
> > > > Thank you for the feedback.
> > > > I will comeback with v2, later on.
> > > > 
> > > > On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> > > > > On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > > > > > This patchset tries to add support for using writeback connector to
> > > > > > flatten a scene when it doesn't change for a while. This idea had
> > > > > > been floated around on IRC here [1] and here [2].
> > > > > > 
> > > > > > Developed on top of the latest writeback series, sent by Liviu here
> > > > > > [3].
> > > > > > 
> > > > > > Probably the patch should/could be broken in more patches, but since I
> > > > > > want to put this out there to get feedback, I kept them as a single
> > > > > > patch for now.
> > > > > 
> > > > > Thank you for your patch, it's looking good. Feel free to submit the v2 in
> > > > > multiple patches. Also note that we've migrated to gitlab, the new tree is
> > > > > located here:
> > > > > 
> > > > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer
> > > > 
> > > > This seems to be behind
> > > > https://anongit.freedesktop.org/git/drm_hwcomposer.git.
> > > > Is this location deprecated or does it serve other purposes.
> > > > 
> > > 
> > > Ah, thanks for catching this. It's a migration glitch, I've made it whole
> > > again.
> > > 
> > > <snip />
> > > 
> > > > > >  #include "drmdisplaycompositor.h"
> > > > > > 
> > > > > >  #include <pthread.h>
> > > > > > @@ -36,9 +35,24 @@
> > > > > >  #include "drmplane.h"
> > > > > >  #include "drmresources.h"
> > > > > >  #include "glworker.h"
> > > > > > +static const uint32_t kWaitWritebackFence = 100; //ms
> > > > > > 
> > > > > >  namespace android {
> > > > > > 
> > > > > > +class CompositorVsyncCallback : public VsyncCallback {
> > > > > > + public:
> > > > > > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > > > > > +      :compositor_(compositor) {
> > > > > > +  }
> > > > > > +
> > > > > > +  void Callback(int display, int64_t timestamp) {
> > > > > > +    compositor_->Vsync(display, timestamp);
> > > > > > +  }
> > > > > > +
> > > > > > + private:
> > > > > > +  DrmDisplayCompositor *compositor_;
> > > > > > +};
> > > > > > +
> > > > > 
> > > > > The GLCompositor already has flattening, so we should tie into that. I assume
> > > > > writeback is going to be more efficient than using GPU (given that the GPU is
> > > > > probably turned off in these scenarios), so you're probably safe to use
> > > > > writeback when available and fall back to GL if it's not.
> > > > > 
> > > > > 
> > > > 
> > > > As far as I understood, this was done by the drmcompositorworker.cpp
> > > > by calling SquashAll but that's removed from build now. GLCompositor
> > > > seems to do compositing only when there are not enough overlays or 
> > > > atomic check fails.
> > > > 
> > > > Am I missing something ? 
> > > > Is it still doing flattening when then scene does not change ?
> > > 
> > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/drmcompositorworker.cpp#L62
> > > 
> > > If the scene doesn't change, everything is squashed.
> > 
> > Yeah, but that file it's not even built, see [1].
> > So, I kind of assumed someone had a reason of deleting it. 
> > Any idea about that?
> 
> Ha, WTF! Yeah, guess that's a regression. 
> 
> /me wonders whether we should just rip out the GLCompositor since no one seems
> to care about it.

I thought about removing/disabling it as well, but it turnout to be
not as easy as I expected, since on validateDisplay we don't do
anything and we do the planning and atomic check on presentDisplay and
then we fallback on GLCompositor if needed.

> 
> I'll circle back on the review and take a look at how you're doing the
> inactivity stuff.

Thanks!.

> 
> Sean
> 
> > 
> > [1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/Android.mk#L53
> > 
> > > 
> > > <snip />
> > > 
> > > > > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > > > > index 32dd376..880cef2 100644
> > > > > > --- a/drmresources.cpp
> > > > > > +++ b/drmresources.cpp
> > > > > > @@ -33,6 +33,8 @@
> > > > > >  #include <cutils/log.h>
> > > > > >  #include <cutils/properties.h>
> > > > > > 
> > > > > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS    4
> > > > > > +
> > > > > 
> > > > > Presumably this should come from libdrm headers?
> > > > 
> > > > Yes it will, this was just a quick fix to be able to compile against
> > > > older libdrm.
> > > > 
> > > > Btw, this would make drm_hwcomposer compilable only with newer libdrm
> > > > version, but I suppose that's acceptable.
> > > 
> > > You can use preprocessor checks to determine if
> > > DRM_CLIENT_CAP_WRITEBACK_CONNECTORS is defined, and disable the functionality if
> > > it's not.
> > > 
> > > > 
> > > > > 
> > > > > >  namespace android {
> > > > > > 
> > > > > >  DrmResources::DrmResources() : event_listener_(this) {
> > > > > > @@ -65,6 +67,11 @@ int DrmResources::Init() {
> > > > > >      return ret;
> > > > > >    }
> > > > > > 
> > > > > > +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > > > > > +  if (ret) {
> > > > > > +    ALOGI("Failed to set writeback cap %d", ret);
> > > > > > +    ret = 0;
> > > > > > +  }
> > > > > >    drmModeResPtr res = drmModeGetResources(fd());
> > > > > >    if (!res) {
> > > > > >      ALOGE("Failed to get DrmResources resources");
> > > > > > @@ -77,6 +84,7 @@ int DrmResources::Init() {
> > > > > >        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
> > > > > > 
> > > > > >    bool found_primary = false;
> > > > > > +  int primary_index = 0;
> > > > > >    int display_num = 1;
> > > > > > 
> > > > > >    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> > > > > > @@ -162,18 +170,22 @@ int DrmResources::Init() {
> > > > > >      if (conn->internal() && !found_primary) {
> > > > > >        conn->set_display(0);
> > > > > >        found_primary = true;
> > > > > > -    } else {
> > > > > > +    } else if (conn->external()) {
> > > > > > +      if (!found_primary) primary_index++;
> > > > > 
> > > > > This is against style guide (and same below).
> > > > 
> > > > I suppose that's what happens when you skip some steps from 
> > > > "Submitting patches", clang will catch this next time.
> > > > 
> > > 
> > > I might look into adding a hook to gitlab to automatically run this, time
> > > permitting.
> > > 
> > > Sean
> > > 
> > > <snip />
> > > -- 
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> > -- 
> > Cheers,
> > Alex G
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

Sean Paul March 20, 2018, 2 p.m. UTC | #9

On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> This patchset tries to add support for using writeback connector to
> flatten a scene when it doesn't change for a while. This idea had
> been floated around on IRC here [1] and here [2].
> 
> Developed on top of the latest writeback series, sent by Liviu here
> [3].
> 
> Probably the patch should/could be broken in more patches, but since I
> want to put this out there to get feedback, I kept them as a single
> patch for now.
> 
> This change could be summarize as follow:
> - Attach a writeback connector to the CRTC that's controlling a
> display.
> - Detect the scene did not change for a while(60 vblanks).
> - Re-commit scene and get the composited scene through the writeback
> connector.
> - Commit the whole scene as a single plane.
> 
> Some areas that I consider important and I could use some
> feedback/ideas:
> 
> 1. Building the pipeline.
> Currently, drm_hwcomposer allows to connect just a single connector
> to a crtc. For now, I decided to treat the writeback connector as a
> separate field inside DrmCrtc. I'm not sure if it's a good idea to try
> to handle this in a unified way, since the writeback connector is such
> a special connector. Regarding the allocation of writeback connectors,
> my idea was to allocate writeback connector to the primary display
> first and then continue allocating while respecting the display number. 0
> gets a writeback before 1 and so on.
> 
> 2. Heuristic for triggering the flattening.
> I just created a VSyncWorker which will trigger the flattening of the
> scene if it doesn't change for 60 consecutive vsyncs. The countdown
> gets reset every time ValidateDisplay is called. This is a relatively
> basic heuristic, so I'm open to suggestions.
> 
> 3. Locking scheme corner cases.
> The Vsynworker is a separate thread which will contend with
> SurfaceFlinger for showing things on the screen. I tried to limit the
> race window by resetting the countdown on ValidateDisplay and
> explicitely checking that we still need to use the flatten scene before
> commiting to get the writeback result or before applying the flattened
> scene.

What are the consequences of the race? It seems like it might be possible that
stale content will be shown over fresh? I think it'd be fine to serialize
vsyncworker and "normal" commits such that the race window is closed instead of
reduced. I think you could do the writeback operation asynchronously and then
commit the result once it's ready, that shouldn't block things up too much.

> 
> 4. Building the DrmDisplayComposition for the flattened scene.
> I kind of lost myself  in all types of layers/planes and compositions,
> so I'm not sure if I'm correctly building the DrmDisplayComposition
> object for the FlattenScene, it works and shows what I expect on the
> screen. So, any feedback here is appreciated.

Yeah, this code needs some love. I had envisioned some Planner-esque interface
where platforms could plug their 2D blocks in and they'd be used by core to
flatten things. This scheme would at least separate the squashing complexity
from compositing. Any interest in taking this on?


> 
> 5. I see there is a drmcompositorworker.cpp which implemented the same
> idea using the GPU, however that seems to not be used anymore, does
> anyone know the rationale behind it?

Let's delete it if it's not used.

Sean

<snip />

Alexandru-Cosmin Gheorghe March 20, 2018, 3:16 p.m. UTC | #10

On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > This patchset tries to add support for using writeback connector to
> > flatten a scene when it doesn't change for a while. This idea had
> > been floated around on IRC here [1] and here [2].
> > 
> > Developed on top of the latest writeback series, sent by Liviu here
> > [3].
> > 
> > Probably the patch should/could be broken in more patches, but since I
> > want to put this out there to get feedback, I kept them as a single
> > patch for now.
> > 
> > This change could be summarize as follow:
> > - Attach a writeback connector to the CRTC that's controlling a
> > display.
> > - Detect the scene did not change for a while(60 vblanks).
> > - Re-commit scene and get the composited scene through the writeback
> > connector.
> > - Commit the whole scene as a single plane.
> > 
> > Some areas that I consider important and I could use some
> > feedback/ideas:
> > 
> > 1. Building the pipeline.
> > Currently, drm_hwcomposer allows to connect just a single connector
> > to a crtc. For now, I decided to treat the writeback connector as a
> > separate field inside DrmCrtc. I'm not sure if it's a good idea to try
> > to handle this in a unified way, since the writeback connector is such
> > a special connector. Regarding the allocation of writeback connectors,
> > my idea was to allocate writeback connector to the primary display
> > first and then continue allocating while respecting the display number. 0
> > gets a writeback before 1 and so on.
> > 
> > 2. Heuristic for triggering the flattening.
> > I just created a VSyncWorker which will trigger the flattening of the
> > scene if it doesn't change for 60 consecutive vsyncs. The countdown
> > gets reset every time ValidateDisplay is called. This is a relatively
> > basic heuristic, so I'm open to suggestions.
> > 
> > 3. Locking scheme corner cases.
> > The Vsynworker is a separate thread which will contend with
> > SurfaceFlinger for showing things on the screen. I tried to limit the
> > race window by resetting the countdown on ValidateDisplay and
> > explicitely checking that we still need to use the flatten scene before
> > commiting to get the writeback result or before applying the flattened
> > scene.
> 
> What are the consequences of the race? It seems like it might be possible that
> stale content will be shown over fresh? 
> I think it'd be fine to serialize
> vsyncworker and "normal" commits such that the race window is closed instead of
> reduced. I think you could do the writeback operation asynchronously and then
> commit the result once it's ready, that shouldn't block things up too much.
> 

Just, to make sure we are on the same page, for Mali DP650 the pipeline
looks like this, I didn't see how it looks for the other hardware.

CRTC ---- encoder ------------ display connector
 |------- writeback enc ------ writeback connector.

There is no asynchronously writeback operation, the scene that you
do writeback for will be shown on the display as well.

Flattening thread:                             
1) Commit original scene + writeback buffer
2) Wait for writeback fence.
3) Commit flattened scene.

Before 1) and 3) I'm doing a locked check where I verify if flattened
scene is still needed and then I release the lock and commit.

Now, I can see a crazy race where immediately after I decided that we
need the flattened scene the flattening thread doesn't get any CPU and
the SurfaceFlinger comes ahead and commits it's new scene followed by
a commit of the old scene.
That's the limitted race window I'm talking about.

The alternative would be to serialize the commits 1) & 3) with
SurfaceFlinger commits, but while 1) or 3) happens surfaceflinger
cannot commit, therefore could potentially miss a VBlank. I suppose
this option is more desireable than the side effect of previously
explained race.


> > 
> > 4. Building the DrmDisplayComposition for the flattened scene.
> > I kind of lost myself  in all types of layers/planes and compositions,
> > so I'm not sure if I'm correctly building the DrmDisplayComposition
> > object for the FlattenScene, it works and shows what I expect on the
> > screen. So, any feedback here is appreciated.
> 
> Yeah, this code needs some love. I had envisioned some Planner-esque interface
> where platforms could plug their 2D blocks in and they'd be used by core to
> flatten things. This scheme would at least separate the squashing complexity
> from compositing. Any interest in taking this on?
> 

I could imagine how that would work with a totally independent 2D
block, not sure if it's doable in my current setup with the writeback
linked to same CRTC as the display, don't you think?

> 
> > 
> > 5. I see there is a drmcompositorworker.cpp which implemented the same
> > idea using the GPU, however that seems to not be used anymore, does
> > anyone know the rationale behind it?
> 
> Let's delete it if it's not used.
> 
> Sean
> 
> <snip />
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

Sean Paul March 20, 2018, 7:53 p.m. UTC | #11

On Tue, Mar 20, 2018 at 03:16:08PM +0000, Alexandru-Cosmin Gheorghe wrote:
> On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> > On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > > This patchset tries to add support for using writeback connector to
> > > flatten a scene when it doesn't change for a while. This idea had
> > > been floated around on IRC here [1] and here [2].
> > > 
> > > Developed on top of the latest writeback series, sent by Liviu here
> > > [3].
> > > 
> > > Probably the patch should/could be broken in more patches, but since I
> > > want to put this out there to get feedback, I kept them as a single
> > > patch for now.
> > > 
> > > This change could be summarize as follow:
> > > - Attach a writeback connector to the CRTC that's controlling a
> > > display.
> > > - Detect the scene did not change for a while(60 vblanks).
> > > - Re-commit scene and get the composited scene through the writeback
> > > connector.
> > > - Commit the whole scene as a single plane.
> > > 
> > > Some areas that I consider important and I could use some
> > > feedback/ideas:
> > > 
> > > 1. Building the pipeline.
> > > Currently, drm_hwcomposer allows to connect just a single connector
> > > to a crtc. For now, I decided to treat the writeback connector as a
> > > separate field inside DrmCrtc. I'm not sure if it's a good idea to try
> > > to handle this in a unified way, since the writeback connector is such
> > > a special connector. Regarding the allocation of writeback connectors,
> > > my idea was to allocate writeback connector to the primary display
> > > first and then continue allocating while respecting the display number. 0
> > > gets a writeback before 1 and so on.
> > > 
> > > 2. Heuristic for triggering the flattening.
> > > I just created a VSyncWorker which will trigger the flattening of the
> > > scene if it doesn't change for 60 consecutive vsyncs. The countdown
> > > gets reset every time ValidateDisplay is called. This is a relatively
> > > basic heuristic, so I'm open to suggestions.
> > > 
> > > 3. Locking scheme corner cases.
> > > The Vsynworker is a separate thread which will contend with
> > > SurfaceFlinger for showing things on the screen. I tried to limit the
> > > race window by resetting the countdown on ValidateDisplay and
> > > explicitely checking that we still need to use the flatten scene before
> > > commiting to get the writeback result or before applying the flattened
> > > scene.
> > 
> > What are the consequences of the race? It seems like it might be possible that
> > stale content will be shown over fresh? 
> > I think it'd be fine to serialize
> > vsyncworker and "normal" commits such that the race window is closed instead of
> > reduced. I think you could do the writeback operation asynchronously and then
> > commit the result once it's ready, that shouldn't block things up too much.
> > 
> 
> Just, to make sure we are on the same page, for Mali DP650 the pipeline
> looks like this, I didn't see how it looks for the other hardware.
> 
> CRTC ---- encoder ------------ display connector
>  |------- writeback enc ------ writeback connector.
> 
> There is no asynchronously writeback operation, the scene that you
> do writeback for will be shown on the display as well.

Ah, hmm, that's not how i was envisioning things working. I suppose that makes
sense since if you drop the display connector from the wb commit it'll be
disconnected from the crtc.

> 
> Flattening thread:                             
> 1) Commit original scene + writeback buffer
> 2) Wait for writeback fence.
> 3) Commit flattened scene.
> 
> Before 1) and 3) I'm doing a locked check where I verify if flattened
> scene is still needed and then I release the lock and commit.
> 
> Now, I can see a crazy race where immediately after I decided that we
> need the flattened scene the flattening thread doesn't get any CPU and
> the SurfaceFlinger comes ahead and commits it's new scene followed by
> a commit of the old scene.
> That's the limitted race window I'm talking about.
> 
> The alternative would be to serialize the commits 1) & 3) with
> SurfaceFlinger commits, but while 1) or 3) happens surfaceflinger
> cannot commit, therefore could potentially miss a VBlank. I suppose
> this option is more desireable than the side effect of previously
> explained race.
> 

So I think you can probably let the kernel serialize things. Take the out fence
from (1) and feed it right back into (3), remove (2) from userspace. Then you
can wrap that function in a lock, and acquire the same lock when doing a "normal"
commit.

> 
> > > 
> > > 4. Building the DrmDisplayComposition for the flattened scene.
> > > I kind of lost myself  in all types of layers/planes and compositions,
> > > so I'm not sure if I'm correctly building the DrmDisplayComposition
> > > object for the FlattenScene, it works and shows what I expect on the
> > > screen. So, any feedback here is appreciated.
> > 
> > Yeah, this code needs some love. I had envisioned some Planner-esque interface
> > where platforms could plug their 2D blocks in and they'd be used by core to
> > flatten things. This scheme would at least separate the squashing complexity
> > from compositing. Any interest in taking this on?
> > 
> 
> I could imagine how that would work with a totally independent 2D
> block, not sure if it's doable in my current setup with the writeback
> linked to same CRTC as the display, don't you think?
> 

Yeah, agreed. It doesn't seem like writeback can be used for squashing layers in
the normal commit path.

Sean

> > 
> > > 
> > > 5. I see there is a drmcompositorworker.cpp which implemented the same
> > > idea using the GPU, however that seems to not be used anymore, does
> > > anyone know the rationale behind it?
> > 
> > Let's delete it if it's not used.
> > 
> > Sean
> > 
> > <snip />
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Cheers,
> Alex G

Rob Clark March 21, 2018, 3:35 a.m. UTC | #12

On Tue, Mar 20, 2018 at 11:16 AM, Alexandru-Cosmin Gheorghe
<[e-mail]> wrote:
> On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
>> On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
>> > This patchset tries to add support for using writeback connector to
>> > flatten a scene when it doesn't change for a while. This idea had
>> > been floated around on IRC here [1] and here [2].
>> >
>> > Developed on top of the latest writeback series, sent by Liviu here
>> > [3].
>> >
>> > Probably the patch should/could be broken in more patches, but since I
>> > want to put this out there to get feedback, I kept them as a single
>> > patch for now.
>> >
>> > This change could be summarize as follow:
>> > - Attach a writeback connector to the CRTC that's controlling a
>> > display.
>> > - Detect the scene did not change for a while(60 vblanks).
>> > - Re-commit scene and get the composited scene through the writeback
>> > connector.
>> > - Commit the whole scene as a single plane.
>> >
>> > Some areas that I consider important and I could use some
>> > feedback/ideas:
>> >
>> > 1. Building the pipeline.
>> > Currently, drm_hwcomposer allows to connect just a single connector
>> > to a crtc. For now, I decided to treat the writeback connector as a
>> > separate field inside DrmCrtc. I'm not sure if it's a good idea to try
>> > to handle this in a unified way, since the writeback connector is such
>> > a special connector. Regarding the allocation of writeback connectors,
>> > my idea was to allocate writeback connector to the primary display
>> > first and then continue allocating while respecting the display number. 0
>> > gets a writeback before 1 and so on.
>> >
>> > 2. Heuristic for triggering the flattening.
>> > I just created a VSyncWorker which will trigger the flattening of the
>> > scene if it doesn't change for 60 consecutive vsyncs. The countdown
>> > gets reset every time ValidateDisplay is called. This is a relatively
>> > basic heuristic, so I'm open to suggestions.
>> >
>> > 3. Locking scheme corner cases.
>> > The Vsynworker is a separate thread which will contend with
>> > SurfaceFlinger for showing things on the screen. I tried to limit the
>> > race window by resetting the countdown on ValidateDisplay and
>> > explicitely checking that we still need to use the flatten scene before
>> > commiting to get the writeback result or before applying the flattened
>> > scene.
>>
>> What are the consequences of the race? It seems like it might be possible that
>> stale content will be shown over fresh?
>> I think it'd be fine to serialize
>> vsyncworker and "normal" commits such that the race window is closed instead of
>> reduced. I think you could do the writeback operation asynchronously and then
>> commit the result once it's ready, that shouldn't block things up too much.
>>
>
> Just, to make sure we are on the same page, for Mali DP650 the pipeline
> looks like this, I didn't see how it looks for the other hardware.
>
> CRTC ---- encoder ------------ display connector
>  |------- writeback enc ------ writeback connector.
>
> There is no asynchronously writeback operation, the scene that you
> do writeback for will be shown on the display as well.
>

fwiw, the msm/mdp5 writeback support I implemented was using a
different CRTC (ie. output going either to display or to wb).. (which
unfortunately implies using different planes).. not sure how much this
case is worth supporting in drm-hwc.

(I think I can do the single CRTC and multiple encoder cases, but it
wasn't really obvious how to get that working with a video mode
display and the hw I was working on didn't have a DSI cmd mode panel)

BR,
-R

> Flattening thread:
> 1) Commit original scene + writeback buffer
> 2) Wait for writeback fence.
> 3) Commit flattened scene.
>
> Before 1) and 3) I'm doing a locked check where I verify if flattened
> scene is still needed and then I release the lock and commit.
>
> Now, I can see a crazy race where immediately after I decided that we
> need the flattened scene the flattening thread doesn't get any CPU and
> the SurfaceFlinger comes ahead and commits it's new scene followed by
> a commit of the old scene.
> That's the limitted race window I'm talking about.
>
> The alternative would be to serialize the commits 1) & 3) with
> SurfaceFlinger commits, but while 1) or 3) happens surfaceflinger
> cannot commit, therefore could potentially miss a VBlank. I suppose
> this option is more desireable than the side effect of previously
> explained race.
>
>
>> >
>> > 4. Building the DrmDisplayComposition for the flattened scene.
>> > I kind of lost myself  in all types of layers/planes and compositions,
>> > so I'm not sure if I'm correctly building the DrmDisplayComposition
>> > object for the FlattenScene, it works and shows what I expect on the
>> > screen. So, any feedback here is appreciated.
>>
>> Yeah, this code needs some love. I had envisioned some Planner-esque interface
>> where platforms could plug their 2D blocks in and they'd be used by core to
>> flatten things. This scheme would at least separate the squashing complexity
>> from compositing. Any interest in taking this on?
>>
>
> I could imagine how that would work with a totally independent 2D
> block, not sure if it's doable in my current setup with the writeback
> linked to same CRTC as the display, don't you think?
>
>>
>> >
>> > 5. I see there is a drmcompositorworker.cpp which implemented the same
>> > idea using the GPU, however that seems to not be used anymore, does
>> > anyone know the rationale behind it?
>>
>> Let's delete it if it's not used.
>>
>> Sean
>>
>> <snip />
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS
>
> --
> Cheers,
> Alex G
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Sean Paul March 21, 2018, 2:01 p.m. UTC | #13

On Tue, Mar 20, 2018 at 11:35:40PM -0400, Rob Clark wrote:
> On Tue, Mar 20, 2018 at 11:16 AM, Alexandru-Cosmin Gheorghe
> <[e-mail]> wrote:
> > On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> >> On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> >> > This patchset tries to add support for using writeback connector to
> >> > flatten a scene when it doesn't change for a while. This idea had
> >> > been floated around on IRC here [1] and here [2].
> >> >
> >> > Developed on top of the latest writeback series, sent by Liviu here
> >> > [3].
> >> >
> >> > Probably the patch should/could be broken in more patches, but since I
> >> > want to put this out there to get feedback, I kept them as a single
> >> > patch for now.
> >> >
> >> > This change could be summarize as follow:
> >> > - Attach a writeback connector to the CRTC that's controlling a
> >> > display.
> >> > - Detect the scene did not change for a while(60 vblanks).
> >> > - Re-commit scene and get the composited scene through the writeback
> >> > connector.
> >> > - Commit the whole scene as a single plane.
> >> >
> >> > Some areas that I consider important and I could use some
> >> > feedback/ideas:
> >> >
> >> > 1. Building the pipeline.
> >> > Currently, drm_hwcomposer allows to connect just a single connector
> >> > to a crtc. For now, I decided to treat the writeback connector as a
> >> > separate field inside DrmCrtc. I'm not sure if it's a good idea to try
> >> > to handle this in a unified way, since the writeback connector is such
> >> > a special connector. Regarding the allocation of writeback connectors,
> >> > my idea was to allocate writeback connector to the primary display
> >> > first and then continue allocating while respecting the display number. 0
> >> > gets a writeback before 1 and so on.
> >> >
> >> > 2. Heuristic for triggering the flattening.
> >> > I just created a VSyncWorker which will trigger the flattening of the
> >> > scene if it doesn't change for 60 consecutive vsyncs. The countdown
> >> > gets reset every time ValidateDisplay is called. This is a relatively
> >> > basic heuristic, so I'm open to suggestions.
> >> >
> >> > 3. Locking scheme corner cases.
> >> > The Vsynworker is a separate thread which will contend with
> >> > SurfaceFlinger for showing things on the screen. I tried to limit the
> >> > race window by resetting the countdown on ValidateDisplay and
> >> > explicitely checking that we still need to use the flatten scene before
> >> > commiting to get the writeback result or before applying the flattened
> >> > scene.
> >>
> >> What are the consequences of the race? It seems like it might be possible that
> >> stale content will be shown over fresh?
> >> I think it'd be fine to serialize
> >> vsyncworker and "normal" commits such that the race window is closed instead of
> >> reduced. I think you could do the writeback operation asynchronously and then
> >> commit the result once it's ready, that shouldn't block things up too much.
> >>
> >
> > Just, to make sure we are on the same page, for Mali DP650 the pipeline
> > looks like this, I didn't see how it looks for the other hardware.
> >
> > CRTC ---- encoder ------------ display connector
> >  |------- writeback enc ------ writeback connector.
> >
> > There is no asynchronously writeback operation, the scene that you
> > do writeback for will be shown on the display as well.
> >
> 
> fwiw, the msm/mdp5 writeback support I implemented was using a
> different CRTC (ie. output going either to display or to wb).. (which
> unfortunately implies using different planes).. not sure how much this
> case is worth supporting in drm-hwc.

Yeah, I wonder whether this is the method of operation we should focus on. It's
unclear whether the restrictions Alex describes above are HW or SW limited, my
impression was SW. Of course, this means we'll need to disable WB if we are
using all crtcs (ie: external display connected), but that seems like a rare
usecase of Android, and one might argue that power savings are not as important
in this case.

Further, if we enable compositing with a dedicated WB pipe, we can also use it
for virtual displays.

Sean

> 
> (I think I can do the single CRTC and multiple encoder cases, but it
> wasn't really obvious how to get that working with a video mode
> display and the hw I was working on didn't have a DSI cmd mode panel)
> 
> BR,
> -R
> 
> > Flattening thread:
> > 1) Commit original scene + writeback buffer
> > 2) Wait for writeback fence.
> > 3) Commit flattened scene.
> >
> > Before 1) and 3) I'm doing a locked check where I verify if flattened
> > scene is still needed and then I release the lock and commit.
> >
> > Now, I can see a crazy race where immediately after I decided that we
> > need the flattened scene the flattening thread doesn't get any CPU and
> > the SurfaceFlinger comes ahead and commits it's new scene followed by
> > a commit of the old scene.
> > That's the limitted race window I'm talking about.
> >
> > The alternative would be to serialize the commits 1) & 3) with
> > SurfaceFlinger commits, but while 1) or 3) happens surfaceflinger
> > cannot commit, therefore could potentially miss a VBlank. I suppose
> > this option is more desireable than the side effect of previously
> > explained race.
> >
> >
> >> >
> >> > 4. Building the DrmDisplayComposition for the flattened scene.
> >> > I kind of lost myself  in all types of layers/planes and compositions,
> >> > so I'm not sure if I'm correctly building the DrmDisplayComposition
> >> > object for the FlattenScene, it works and shows what I expect on the
> >> > screen. So, any feedback here is appreciated.
> >>
> >> Yeah, this code needs some love. I had envisioned some Planner-esque interface
> >> where platforms could plug their 2D blocks in and they'd be used by core to
> >> flatten things. This scheme would at least separate the squashing complexity
> >> from compositing. Any interest in taking this on?
> >>
> >
> > I could imagine how that would work with a totally independent 2D
> > block, not sure if it's doable in my current setup with the writeback
> > linked to same CRTC as the display, don't you think?
> >
> >>
> >> >
> >> > 5. I see there is a drmcompositorworker.cpp which implemented the same
> >> > idea using the GPU, however that seems to not be used anymore, does
> >> > anyone know the rationale behind it?
> >>
> >> Let's delete it if it's not used.
> >>
> >> Sean
> >>
> >> <snip />
> >> --
> >> Sean Paul, Software Engineer, Google / Chromium OS
> >
> > --
> > Cheers,
> > Alex G
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Alexandru-Cosmin Gheorghe March 21, 2018, 3:13 p.m. UTC | #14

Hi Sean & Rob,

On Wed, Mar 21, 2018 at 10:01:07AM -0400, Sean Paul wrote:
> On Tue, Mar 20, 2018 at 11:35:40PM -0400, Rob Clark wrote:
> > On Tue, Mar 20, 2018 at 11:16 AM, Alexandru-Cosmin Gheorghe
> > <[e-mail]> wrote:
> > > On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> > >> On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > >> > This patchset tries to add support for using writeback connector to
> > >> > flatten a scene when it doesn't change for a while. This idea had
> > >> > been floated around on IRC here [1] and here [2].
> > >> >
> > >> > Developed on top of the latest writeback series, sent by Liviu here
> > >> > [3].
> > >> >
> > >> > Probably the patch should/could be broken in more patches, but since I
> > >> > want to put this out there to get feedback, I kept them as a single
> > >> > patch for now.
> > >> >
> > >> > This change could be summarize as follow:
> > >> > - Attach a writeback connector to the CRTC that's controlling a
> > >> > display.
> > >> > - Detect the scene did not change for a while(60 vblanks).
> > >> > - Re-commit scene and get the composited scene through the writeback
> > >> > connector.
> > >> > - Commit the whole scene as a single plane.
> > >> >
> > >> > Some areas that I consider important and I could use some
> > >> > feedback/ideas:
> > >> >
> > >> > 1. Building the pipeline.
> > >> > Currently, drm_hwcomposer allows to connect just a single connector
> > >> > to a crtc. For now, I decided to treat the writeback connector as a
> > >> > separate field inside DrmCrtc. I'm not sure if it's a good idea to try
> > >> > to handle this in a unified way, since the writeback connector is such
> > >> > a special connector. Regarding the allocation of writeback connectors,
> > >> > my idea was to allocate writeback connector to the primary display
> > >> > first and then continue allocating while respecting the display number. 0
> > >> > gets a writeback before 1 and so on.
> > >> >
> > >> > 2. Heuristic for triggering the flattening.
> > >> > I just created a VSyncWorker which will trigger the flattening of the
> > >> > scene if it doesn't change for 60 consecutive vsyncs. The countdown
> > >> > gets reset every time ValidateDisplay is called. This is a relatively
> > >> > basic heuristic, so I'm open to suggestions.
> > >> >
> > >> > 3. Locking scheme corner cases.
> > >> > The Vsynworker is a separate thread which will contend with
> > >> > SurfaceFlinger for showing things on the screen. I tried to limit the
> > >> > race window by resetting the countdown on ValidateDisplay and
> > >> > explicitely checking that we still need to use the flatten scene before
> > >> > commiting to get the writeback result or before applying the flattened
> > >> > scene.
> > >>
> > >> What are the consequences of the race? It seems like it might be possible that
> > >> stale content will be shown over fresh?
> > >> I think it'd be fine to serialize
> > >> vsyncworker and "normal" commits such that the race window is closed instead of
> > >> reduced. I think you could do the writeback operation asynchronously and then
> > >> commit the result once it's ready, that shouldn't block things up too much.
> > >>
> > >
> > > Just, to make sure we are on the same page, for Mali DP650 the pipeline
> > > looks like this, I didn't see how it looks for the other hardware.
> > >
> > > CRTC ---- encoder ------------ display connector
> > >  |------- writeback enc ------ writeback connector.
> > >
> > > There is no asynchronously writeback operation, the scene that you
> > > do writeback for will be shown on the display as well.
> > >
> > 
> > fwiw, the msm/mdp5 writeback support I implemented was using a
> > different CRTC (ie. output going either to display or to wb).. (which
> > unfortunately implies using different planes).. not sure how much this
> > case is worth supporting in drm-hwc.

Which case? With the same CRTC or different CRTCs, I think both could
prove useful.

> 
> Yeah, I wonder whether this is the method of operation we should focus on. It's
> unclear whether the restrictions Alex describes above are HW or SW limited, my
> impression was SW. Of course, this means we'll need to disable WB if we are
> using all crtcs (ie: external display connected), but that seems like a rare
> usecase of Android, and one might argue that power savings are not as important
> in this case.
> 
> Further, if we enable compositing with a dedicated WB pipe, we can also use it
> for virtual displays.
> 
> Sean

For Mali DP-650 this is a HW requirement, we don't have any way of
individually controlling just the writeback functionality without
affecting what's sent to the display.

> 
> > 
> > (I think I can do the single CRTC and multiple encoder cases, but it
> > wasn't really obvious how to get that working with a video mode
> > display and the hw I was working on didn't have a DSI cmd mode panel)
> > 
> > BR,
> > -R
> > 
> > > Flattening thread:
> > > 1) Commit original scene + writeback buffer
> > > 2) Wait for writeback fence.
> > > 3) Commit flattened scene.
> > >
> > > Before 1) and 3) I'm doing a locked check where I verify if flattened
> > > scene is still needed and then I release the lock and commit.
> > >
> > > Now, I can see a crazy race where immediately after I decided that we
> > > need the flattened scene the flattening thread doesn't get any CPU and
> > > the SurfaceFlinger comes ahead and commits it's new scene followed by
> > > a commit of the old scene.
> > > That's the limitted race window I'm talking about.
> > >
> > > The alternative would be to serialize the commits 1) & 3) with
> > > SurfaceFlinger commits, but while 1) or 3) happens surfaceflinger
> > > cannot commit, therefore could potentially miss a VBlank. I suppose
> > > this option is more desireable than the side effect of previously
> > > explained race.
> > >
> > >
> > >> >
> > >> > 4. Building the DrmDisplayComposition for the flattened scene.
> > >> > I kind of lost myself  in all types of layers/planes and compositions,
> > >> > so I'm not sure if I'm correctly building the DrmDisplayComposition
> > >> > object for the FlattenScene, it works and shows what I expect on the
> > >> > screen. So, any feedback here is appreciated.
> > >>
> > >> Yeah, this code needs some love. I had envisioned some Planner-esque interface
> > >> where platforms could plug their 2D blocks in and they'd be used by core to
> > >> flatten things. This scheme would at least separate the squashing complexity
> > >> from compositing. Any interest in taking this on?
> > >>
> > >
> > > I could imagine how that would work with a totally independent 2D
> > > block, not sure if it's doable in my current setup with the writeback
> > > linked to same CRTC as the display, don't you think?
> > >
> > >>
> > >> >
> > >> > 5. I see there is a drmcompositorworker.cpp which implemented the same
> > >> > idea using the GPU, however that seems to not be used anymore, does
> > >> > anyone know the rationale behind it?
> > >>
> > >> Let's delete it if it's not used.
> > >>
> > >> Sean
> > >>
> > >> <snip />
> > >> --
> > >> Sean Paul, Software Engineer, Google / Chromium OS
> > >
> > > --
> > > Cheers,
> > > Alex G
> > > _______________________________________________
> > > dri-devel mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

Liviu Dudau March 21, 2018, 3:39 p.m. UTC | #15

On Wed, Mar 21, 2018 at 10:01:07AM -0400, Sean Paul wrote:
> On Tue, Mar 20, 2018 at 11:35:40PM -0400, Rob Clark wrote:
> > On Tue, Mar 20, 2018 at 11:16 AM, Alexandru-Cosmin Gheorghe
> > <[e-mail]> wrote:
> > > On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> > >> On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > >> > This patchset tries to add support for using writeback connector to
> > >> > flatten a scene when it doesn't change for a while. This idea had
> > >> > been floated around on IRC here [1] and here [2].
> > >> >
> > >> > Developed on top of the latest writeback series, sent by Liviu here
> > >> > [3].
> > >> >
> > >> > Probably the patch should/could be broken in more patches, but since I
> > >> > want to put this out there to get feedback, I kept them as a single
> > >> > patch for now.
> > >> >
> > >> > This change could be summarize as follow:
> > >> > - Attach a writeback connector to the CRTC that's controlling a
> > >> > display.
> > >> > - Detect the scene did not change for a while(60 vblanks).
> > >> > - Re-commit scene and get the composited scene through the writeback
> > >> > connector.
> > >> > - Commit the whole scene as a single plane.
> > >> >
> > >> > Some areas that I consider important and I could use some
> > >> > feedback/ideas:
> > >> >
> > >> > 1. Building the pipeline.
> > >> > Currently, drm_hwcomposer allows to connect just a single connector
> > >> > to a crtc. For now, I decided to treat the writeback connector as a
> > >> > separate field inside DrmCrtc. I'm not sure if it's a good idea to try
> > >> > to handle this in a unified way, since the writeback connector is such
> > >> > a special connector. Regarding the allocation of writeback connectors,
> > >> > my idea was to allocate writeback connector to the primary display
> > >> > first and then continue allocating while respecting the display number. 0
> > >> > gets a writeback before 1 and so on.
> > >> >
> > >> > 2. Heuristic for triggering the flattening.
> > >> > I just created a VSyncWorker which will trigger the flattening of the
> > >> > scene if it doesn't change for 60 consecutive vsyncs. The countdown
> > >> > gets reset every time ValidateDisplay is called. This is a relatively
> > >> > basic heuristic, so I'm open to suggestions.
> > >> >
> > >> > 3. Locking scheme corner cases.
> > >> > The Vsynworker is a separate thread which will contend with
> > >> > SurfaceFlinger for showing things on the screen. I tried to limit the
> > >> > race window by resetting the countdown on ValidateDisplay and
> > >> > explicitely checking that we still need to use the flatten scene before
> > >> > commiting to get the writeback result or before applying the flattened
> > >> > scene.
> > >>
> > >> What are the consequences of the race? It seems like it might be possible that
> > >> stale content will be shown over fresh?
> > >> I think it'd be fine to serialize
> > >> vsyncworker and "normal" commits such that the race window is closed instead of
> > >> reduced. I think you could do the writeback operation asynchronously and then
> > >> commit the result once it's ready, that shouldn't block things up too much.
> > >>
> > >
> > > Just, to make sure we are on the same page, for Mali DP650 the pipeline
> > > looks like this, I didn't see how it looks for the other hardware.
> > >
> > > CRTC ---- encoder ------------ display connector
> > >  |------- writeback enc ------ writeback connector.
> > >
> > > There is no asynchronously writeback operation, the scene that you
> > > do writeback for will be shown on the display as well.
> > >
> > 
> > fwiw, the msm/mdp5 writeback support I implemented was using a
> > different CRTC (ie. output going either to display or to wb).. (which
> > unfortunately implies using different planes).. not sure how much this
> > case is worth supporting in drm-hwc.
> 
> Yeah, I wonder whether this is the method of operation we should focus on. It's
> unclear whether the restrictions Alex describes above are HW or SW limited, my
> impression was SW. Of course, this means we'll need to disable WB if we are
> using all crtcs (ie: external display connected), but that seems like a rare
> usecase of Android, and one might argue that power savings are not as important
> in this case.

When I've inquired on #dri-devel about possible usecase that makes use
of the writeback functionality in the kernel, the flattening of layers
was the one suggested by Sean and this is what Alex has implemented
here. While we could re-focus on virtual displays I feel like this
should be better tackled in a different patchset.

Rob, how is any userspace supposed to drive your implementation? The
framework patches pose no restriction in terms of how the connector gets
attached to a CRTC. Are you saying that for msm/mdp5 we need to have an
atomic commit where only the writeback connector is attached to one of
the possible CRTCs and all the planes are copied to that CRTC for
composition? Does that mean you are going to have a flicker in the
output?

For Mali DP, I need to correct Alex: we can support a CRTC with only the
writeback connected and with no other output, so we can implement
virtual displays easily.

Best regards,
Liviu

> 
> Further, if we enable compositing with a dedicated WB pipe, we can also use it
> for virtual displays.
> 
> Sean
> 
> > 
> > (I think I can do the single CRTC and multiple encoder cases, but it
> > wasn't really obvious how to get that working with a video mode
> > display and the hw I was working on didn't have a DSI cmd mode panel)
> > 
> > BR,
> > -R
> > 
> > > Flattening thread:
> > > 1) Commit original scene + writeback buffer
> > > 2) Wait for writeback fence.
> > > 3) Commit flattened scene.
> > >
> > > Before 1) and 3) I'm doing a locked check where I verify if flattened
> > > scene is still needed and then I release the lock and commit.
> > >
> > > Now, I can see a crazy race where immediately after I decided that we
> > > need the flattened scene the flattening thread doesn't get any CPU and
> > > the SurfaceFlinger comes ahead and commits it's new scene followed by
> > > a commit of the old scene.
> > > That's the limitted race window I'm talking about.
> > >
> > > The alternative would be to serialize the commits 1) & 3) with
> > > SurfaceFlinger commits, but while 1) or 3) happens surfaceflinger
> > > cannot commit, therefore could potentially miss a VBlank. I suppose
> > > this option is more desireable than the side effect of previously
> > > explained race.
> > >
> > >
> > >> >
> > >> > 4. Building the DrmDisplayComposition for the flattened scene.
> > >> > I kind of lost myself  in all types of layers/planes and compositions,
> > >> > so I'm not sure if I'm correctly building the DrmDisplayComposition
> > >> > object for the FlattenScene, it works and shows what I expect on the
> > >> > screen. So, any feedback here is appreciated.
> > >>
> > >> Yeah, this code needs some love. I had envisioned some Planner-esque interface
> > >> where platforms could plug their 2D blocks in and they'd be used by core to
> > >> flatten things. This scheme would at least separate the squashing complexity
> > >> from compositing. Any interest in taking this on?
> > >>
> > >
> > > I could imagine how that would work with a totally independent 2D
> > > block, not sure if it's doable in my current setup with the writeback
> > > linked to same CRTC as the display, don't you think?
> > >
> > >>
> > >> >
> > >> > 5. I see there is a drmcompositorworker.cpp which implemented the same
> > >> > idea using the GPU, however that seems to not be used anymore, does
> > >> > anyone know the rationale behind it?
> > >>
> > >> Let's delete it if it's not used.
> > >>
> > >> Sean
> > >>
> > >> <snip />
> > >> --
> > >> Sean Paul, Software Engineer, Google / Chromium OS
> > >
> > > --
> > > Cheers,
> > > Alex G
> > > _______________________________________________
> > > dri-devel mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

Eric Anholt March 21, 2018, 3:48 p.m. UTC | #16

Rob Clark <[e-mail]> writes:

> On Tue, Mar 20, 2018 at 11:16 AM, Alexandru-Cosmin Gheorghe
> <[e-mail]> wrote:
>> On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
>>> On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
>>> > This patchset tries to add support for using writeback connector to
>>> > flatten a scene when it doesn't change for a while. This idea had
>>> > been floated around on IRC here [1] and here [2].
>>> >
>>> > Developed on top of the latest writeback series, sent by Liviu here
>>> > [3].
>>> >
>>> > Probably the patch should/could be broken in more patches, but since I
>>> > want to put this out there to get feedback, I kept them as a single
>>> > patch for now.
>>> >
>>> > This change could be summarize as follow:
>>> > - Attach a writeback connector to the CRTC that's controlling a
>>> > display.
>>> > - Detect the scene did not change for a while(60 vblanks).
>>> > - Re-commit scene and get the composited scene through the writeback
>>> > connector.
>>> > - Commit the whole scene as a single plane.
>>> >
>>> > Some areas that I consider important and I could use some
>>> > feedback/ideas:
>>> >
>>> > 1. Building the pipeline.
>>> > Currently, drm_hwcomposer allows to connect just a single connector
>>> > to a crtc. For now, I decided to treat the writeback connector as a
>>> > separate field inside DrmCrtc. I'm not sure if it's a good idea to try
>>> > to handle this in a unified way, since the writeback connector is such
>>> > a special connector. Regarding the allocation of writeback connectors,
>>> > my idea was to allocate writeback connector to the primary display
>>> > first and then continue allocating while respecting the display number. 0
>>> > gets a writeback before 1 and so on.
>>> >
>>> > 2. Heuristic for triggering the flattening.
>>> > I just created a VSyncWorker which will trigger the flattening of the
>>> > scene if it doesn't change for 60 consecutive vsyncs. The countdown
>>> > gets reset every time ValidateDisplay is called. This is a relatively
>>> > basic heuristic, so I'm open to suggestions.
>>> >
>>> > 3. Locking scheme corner cases.
>>> > The Vsynworker is a separate thread which will contend with
>>> > SurfaceFlinger for showing things on the screen. I tried to limit the
>>> > race window by resetting the countdown on ValidateDisplay and
>>> > explicitely checking that we still need to use the flatten scene before
>>> > commiting to get the writeback result or before applying the flattened
>>> > scene.
>>>
>>> What are the consequences of the race? It seems like it might be possible that
>>> stale content will be shown over fresh?
>>> I think it'd be fine to serialize
>>> vsyncworker and "normal" commits such that the race window is closed instead of
>>> reduced. I think you could do the writeback operation asynchronously and then
>>> commit the result once it's ready, that shouldn't block things up too much.
>>>
>>
>> Just, to make sure we are on the same page, for Mali DP650 the pipeline
>> looks like this, I didn't see how it looks for the other hardware.
>>
>> CRTC ---- encoder ------------ display connector
>>  |------- writeback enc ------ writeback connector.
>>
>> There is no asynchronously writeback operation, the scene that you
>> do writeback for will be shown on the display as well.
>>
>
> fwiw, the msm/mdp5 writeback support I implemented was using a
> different CRTC (ie. output going either to display or to wb).. (which
> unfortunately implies using different planes).. not sure how much this
> case is worth supporting in drm-hwc.

This is also how VC4's writeback would work.

Patch

diff --git a/drmconnector.cpp b/drmconnector.cpp
index 145518f..2ed4f23 100644
--- a/drmconnector.cpp
+++ b/drmconnector.cpp
@@ -52,6 +52,23 @@  int DrmConnector::Init() {
     ALOGE("Could not get CRTC_ID property\n");
     return ret;
   }
+  if (writeback()) {
+    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_PIXEL_FORMATS", &writeback_pixel_formats_);
+    if (ret) {
+      ALOGE("Could not get WRITEBACK_PIXEL_FORMATS connector_id = %d\n", id_);
+      return ret;
+    }
+    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_FB_ID", &writeback_fb_id_);
+    if (ret) {
+      ALOGE("Could not get WRITEBACK_FB_ID connector_id = %d\n", id_);
+      return ret;
+    }
+    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_OUT_FENCE_PTR", &writeback_out_fence_);
+    if (ret) {
+      ALOGE("Could not get WRITEBACK_OUT_FENCE_PTR connector_id = %d\n", id_);
+      return ret;
+    }
+  }
   return 0;
 }

@@ -78,8 +95,13 @@  bool DrmConnector::external() const {
          type_ == DRM_MODE_CONNECTOR_VGA;
 }

+#define DRM_MODE_CONNECTOR_WRITEBACK   18
+bool DrmConnector::writeback() const {
+        return type_ == DRM_MODE_CONNECTOR_WRITEBACK;
+}
+
 bool DrmConnector::valid_type() const {
-  return internal() || external();
+  return internal() || external() || writeback();
 }

 int DrmConnector::UpdateModes() {
@@ -130,6 +152,18 @@  const DrmProperty &DrmConnector::crtc_id_property() const {
   return crtc_id_property_;
 }

+const DrmProperty &DrmConnector::writeback_pixel_formats() const {
+  return writeback_pixel_formats_;
+}
+
+const DrmProperty &DrmConnector::writeback_fb_id() const {
+  return writeback_fb_id_;
+}
+
+const DrmProperty &DrmConnector::writeback_out_fence() const {
+  return writeback_out_fence_;
+}
+
 DrmEncoder *DrmConnector::encoder() const {
   return encoder_;
 }
diff --git a/drmconnector.h b/drmconnector.h
index 5601e06..ad18762 100644
--- a/drmconnector.h
+++ b/drmconnector.h
@@ -28,6 +28,7 @@ 
 namespace android {

 class DrmResources;
+class DrmCrtc;

 class DrmConnector {
  public:
@@ -46,6 +47,7 @@  class DrmConnector {

   bool internal() const;
   bool external() const;
+  bool writeback() const;
   bool valid_type() const;

   int UpdateModes();
@@ -58,6 +60,9 @@  class DrmConnector {

   const DrmProperty &dpms_property() const;
   const DrmProperty &crtc_id_property() const;
+  const DrmProperty &writeback_pixel_formats() const;
+  const DrmProperty &writeback_fb_id() const;
+  const DrmProperty &writeback_out_fence() const;

   const std::vector<DrmEncoder *> &possible_encoders() const {
     return possible_encoders_;
@@ -88,6 +93,9 @@  class DrmConnector {

   DrmProperty dpms_property_;
   DrmProperty crtc_id_property_;
+  DrmProperty writeback_pixel_formats_;
+  DrmProperty writeback_fb_id_;
+  DrmProperty writeback_out_fence_;

   std::vector<DrmEncoder *> possible_encoders_;
 };
diff --git a/drmcrtc.cpp b/drmcrtc.cpp
index 1b354fe..f8c9f25 100644
--- a/drmcrtc.cpp
+++ b/drmcrtc.cpp
@@ -31,7 +31,8 @@  DrmCrtc::DrmCrtc(DrmResources *drm, drmModeCrtcPtr c, unsigned pipe)
       id_(c->crtc_id),
       pipe_(pipe),
       display_(-1),
-      mode_(&c->mode) {
+      mode_(&c->mode),
+      writeback_conn_(nullptr) {
 }

 int DrmCrtc::Init() {
@@ -75,6 +76,14 @@  bool DrmCrtc::can_bind(int display) const {
   return display_ == -1 || display_ == display;
 }

+DrmConnector* DrmCrtc::writeback_conn() const {
+  return writeback_conn_;
+}
+
+void DrmCrtc::set_writeback_conn(DrmConnector* writeback_conn) {
+  writeback_conn_ = writeback_conn;
+}
+
 const DrmProperty &DrmCrtc::active_property() const {
   return active_property_;
 }
diff --git a/drmcrtc.h b/drmcrtc.h
index c5a5599..bbd8d37 100644
--- a/drmcrtc.h
+++ b/drmcrtc.h
@@ -22,11 +22,11 @@ 

 #include <stdint.h>
 #include <xf86drmMode.h>
-
+#include "drmconnector.h"
 namespace android {

 class DrmResources;
-
+class DrmConnector;
 class DrmCrtc {
  public:
   DrmCrtc(DrmResources *drm, drmModeCrtcPtr c, unsigned pipe);
@@ -39,7 +39,9 @@  class DrmCrtc {
   unsigned pipe() const;

   int display() const;
+  DrmConnector* writeback_conn() const;
   void set_display(int display);
+  void set_writeback_conn(DrmConnector*);

   bool can_bind(int display) const;

@@ -55,7 +57,7 @@  class DrmCrtc {
   int display_;

   DrmMode mode_;
-
+  DrmConnector *writeback_conn_;
   DrmProperty active_property_;
   DrmProperty mode_property_;
   DrmProperty out_fence_ptr_property_;
diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index e556e86..9783852 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -16,7 +16,6 @@ 

 #define ATRACE_TAG ATRACE_TAG_GRAPHICS
 #define LOG_TAG "hwc-drm-display-compositor"
-
 #include "drmdisplaycompositor.h"

 #include <pthread.h>
@@ -36,9 +35,24 @@ 
 #include "drmplane.h"
 #include "drmresources.h"
 #include "glworker.h"
+static const uint32_t kWaitWritebackFence = 100; //ms

 namespace android {

+class CompositorVsyncCallback : public VsyncCallback {
+ public:
+  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
+      :compositor_(compositor) {
+  }
+
+  void Callback(int display, int64_t timestamp) {
+    compositor_->Vsync(display, timestamp);
+  }
+
+ private:
+  DrmDisplayCompositor *compositor_;
+};
+
 void SquashState::Init(DrmHwcLayer *layers, size_t num_layers) {
   generation_number_++;
   valid_history_ = 0;
@@ -183,7 +197,8 @@  DrmDisplayCompositor::DrmDisplayCompositor()
       framebuffer_index_(0),
       squash_framebuffer_index_(0),
       dump_frames_composited_(0),
-      dump_last_timestamp_ns_(0) {
+      dump_last_timestamp_ns_(0),
+      flatten_countdown_(FLATTEN_COUNTDOWN_INIT) {
   struct timespec ts;
   if (clock_gettime(CLOCK_MONOTONIC, &ts))
     return;
@@ -193,7 +208,7 @@  DrmDisplayCompositor::DrmDisplayCompositor()
 DrmDisplayCompositor::~DrmDisplayCompositor() {
   if (!initialized_)
     return;
-
+  vsync_worker_.Exit();
   int ret = pthread_mutex_lock(&lock_);
   if (ret)
     ALOGE("Failed to acquire compositor lock %d", ret);
@@ -223,6 +238,9 @@  int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
   }

   initialized_ = true;
+  vsync_worker_.Init(drm_, display_);
+  auto callback = std::make_shared<CompositorVsyncCallback>(this);
+  vsync_worker_.RegisterCallback(callback);
   return 0;
 }

@@ -482,7 +500,7 @@  int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
 }

 int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
-                                      bool test_only) {
+                                      bool test_only, DrmDisplayComposition *writeback_comp) {
   ATRACE_CALL();

   int ret = 0;
@@ -491,7 +509,8 @@  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
   std::vector<DrmCompositionPlane> &comp_planes =
       display_comp->composition_planes();
   uint64_t out_fences[drm_->crtcs().size()];
-
+  int writeback_fence = -1;
+  DrmFramebuffer *writeback_fb = nullptr;
   DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
   if (!connector) {
     ALOGE("Could not locate connector for display %d", display_);
@@ -508,6 +527,32 @@  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
     ALOGE("Failed to allocate property set");
     return -ENOMEM;
   }
+  DrmConnector *writeback_conn = crtc->writeback_conn();
+  if (writeback_comp != nullptr) {
+    if (writeback_conn == nullptr)
+      return -EINVAL;
+    writeback_fb = &framebuffers_[framebuffer_index_];
+    framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS;
+    ret = PrepareFramebuffer(*writeback_fb, writeback_comp);
+    if (ret) {
+      ALOGE("Failed to prepare framebuffer for pre-composite %d", ret);
+      return ret;
+    }
+    if (writeback_conn->writeback_fb_id().id() == 0 || writeback_conn->writeback_out_fence().id() == 0)
+      return -EINVAL;
+    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), writeback_conn->writeback_fb_id().id(),
+                                   writeback_comp->layers().back().buffer->fb_id);
+    if (ret < 0) {
+      ALOGE("Failed to add writeback_fb_id");
+      return ret;
+    }
+    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), writeback_conn->writeback_out_fence().id(),
+                                   (uint64_t) &writeback_fence);
+    if (ret < 0) {
+      ALOGE("Failed to add writeback_out_fence");
+      return ret;
+    }
+  }

   if (crtc->out_fence_ptr_property().id() != 0) {
     ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(),
@@ -537,6 +582,12 @@  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
       drmModeAtomicFree(pset);
       return ret;
     }
+    if (writeback_conn != nullptr) {
+      ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), writeback_conn->crtc_id_property().id(), crtc->id());
+      if (ret < 0) {
+          ALOGE("Failed to  attach writeback");
+      }
+    }
   }

   for (DrmCompositionPlane &comp_plane : comp_planes) {
@@ -691,6 +742,17 @@  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
     if (test_only)
       flags |= DRM_MODE_ATOMIC_TEST_ONLY;

+    // There is a race when we recommit a scene to get the writeback result
+    // to shorten that race, check if we still need that result
+    bool abort_commit = false;
+    if (writeback_comp != nullptr) {
+      AutoLock lock(&lock_, "CommitFrame");
+      lock.Lock();
+      abort_commit = !CountdownExpired();
+    }
+    if (abort_commit)
+      return -EINVAL;
+
     ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_);
     if (ret) {
       if (test_only)
@@ -729,6 +791,14 @@  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
     display_comp->set_out_fence((int) out_fences[crtc->pipe()]);
   }

+  if (writeback_fence >= 0 && writeback_fb != nullptr) {
+    ret = sync_wait(writeback_fence, kWaitWritebackFence);
+    if (ret) {
+        ALOGE("Failed to wait on writeback fence");
+    }
+    close(writeback_fence);
+  }
+
   return ret;
 }

@@ -784,7 +854,7 @@  void DrmDisplayCompositor::ClearDisplay() {
 }

 void DrmDisplayCompositor::ApplyFrame(
-    std::unique_ptr<DrmDisplayComposition> composition, int status) {
+    std::unique_ptr<DrmDisplayComposition> composition, int status, bool start_countdown) {
   int ret = status;

   if (!ret)
@@ -807,6 +877,8 @@  void DrmDisplayCompositor::ApplyFrame(
     ALOGE("Failed to acquire lock for active_composition swap");

   active_composition_.swap(composition);
+  flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
+  vsync_worker_.VSyncControl(start_countdown);

   if (!ret)
     ret = pthread_mutex_unlock(&lock_);
@@ -878,6 +950,65 @@  int DrmDisplayCompositor::ApplyComposition(
   return ret;
 }

+int DrmDisplayCompositor::FlattenScene() {
+  std::unique_ptr<DrmDisplayComposition> comp = CreateComposition();
+  DrmDisplayComposition *src = active_composition_.get();
+  DrmDisplayComposition *dst = comp.get();
+  std::vector<DrmCompositionPlane> &src_planes = src->composition_planes();
+  if (src == nullptr || dst == nullptr)
+    return -EINVAL;
+  size_t src_planes_with_layer = std::count_if(
+      src_planes.begin(), src_planes.end(), [](DrmCompositionPlane &p) {
+        return p.type() != DrmCompositionPlane::Type::kDisable;
+      });
+
+  if (src_planes_with_layer <= 1)
+    return -EALREADY;
+  int ret = dst->Init(drm_, src->crtc(), src->importer(), src->planner(),
+                      src->frame_no());
+  if (ret)
+    return ret;
+  DrmCompositionPlane squashed_comp(DrmCompositionPlane::Type::kPrecomp, NULL,
+                                    src->crtc());
+  for (DrmCompositionPlane &comp_plane : src_planes) {
+    if (comp_plane.plane() == NULL) {
+      ALOGE("Skipping squash all because of NULL plane");
+      ret = -EINVAL;
+    }
+    if (comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY)
+      squashed_comp.set_plane(comp_plane.plane());
+    else
+      dst->AddPlaneDisable(comp_plane.plane());
+  }
+  ret = CommitFrame(active_composition_.get(), 0, dst);
+  if (ret || dst->layers().size() != 1) {
+      ALOGE("Failed to flatten scene using writeback");
+      return -EINVAL;
+  }
+  squashed_comp.source_layers().push_back(0);
+
+  ret = dst->AddPlaneComposition(std::move(squashed_comp));
+  if (ret) {
+    ALOGE("Failed to add flatten scene");
+    return ret;
+  }
+  ret = dst->FinalizeComposition();
+  if (ret) {
+    ALOGE("Failed to finalize composition");
+    return ret;
+  }
+  // If countdown it's still expired ApplyFlattenScene
+  AutoLock lock(&lock_, "FlattenScene");
+  lock.Lock();
+  if (CountdownExpired()) {
+    lock.Unlock();
+    ApplyFrame(std::move(comp), 0, false);
+  } else {
+    return -EAGAIN;
+  }
+  return 0;
+}
+
 int DrmDisplayCompositor::SquashAll() {
   AutoLock lock(&lock_, "compositor");
   int ret = lock.Lock();
@@ -1026,6 +1157,27 @@  move_layers_back:
   return ret;
 }

+bool DrmDisplayCompositor::CountdownExpired() const {
+  return flatten_countdown_ <= 0;
+}
+
+void DrmDisplayCompositor::ResetCountdown() {
+  AutoLock lock(&lock_, "ResetCountdown");
+  lock.Lock();
+  flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
+}
+
+void DrmDisplayCompositor::Vsync(int display, int64_t timestamp) {
+   AutoLock lock(&lock_, "VSync");
+   lock.Lock();
+   flatten_countdown_--;
+   if (CountdownExpired()) {
+     lock.Unlock();
+     int ret = FlattenScene();
+     ALOGI("Vsync: Flatten scene for display %d at timestamp %" PRIu64 " ret = %d \n", display, timestamp, ret);
+   }
+}
+
 void DrmDisplayCompositor::Dump(std::ostringstream *out) const {
   int ret = pthread_mutex_lock(&lock_);
   if (ret)
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index f1965fb..4a5696a 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -29,11 +29,15 @@ 

 #include <hardware/hardware.h>
 #include <hardware/hwcomposer.h>
+#include <vsyncworker.h>

 // One for the front, one for the back, and one for cases where we need to
 // squash a frame that the hw can't display with hw overlays.
 #define DRM_DISPLAY_BUFFERS 3

+// If a scene is still for this number of vblanks flatten it to reduce power consumption.
+#define FLATTEN_COUNTDOWN_INIT 60
+
 namespace android {

 class GLWorkerCompositor;
@@ -91,7 +95,8 @@  class DrmDisplayCompositor {
   int Composite();
   int SquashAll();
   void Dump(std::ostringstream *out) const;
-
+  void Vsync(int display, int64_t timestamp);
+  void ResetCountdown();
   std::tuple<uint32_t, uint32_t, int> GetActiveModeResolution();

   SquashState *squash_state() {
@@ -118,15 +123,16 @@  class DrmDisplayCompositor {
   int ApplySquash(DrmDisplayComposition *display_comp);
   int ApplyPreComposite(DrmDisplayComposition *display_comp);
   int PrepareFrame(DrmDisplayComposition *display_comp);
-  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only);
+  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only, DrmDisplayComposition *writeback_comp = nullptr);
   int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst);
   int ApplyDpms(DrmDisplayComposition *display_comp);
   int DisablePlanes(DrmDisplayComposition *display_comp);

   void ClearDisplay();
   void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
-                  int status);
-
+                  int status, bool start_countdown = true);
+  int FlattenScene();
+  bool CountdownExpired() const;
   std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);

   DrmResources *drm_;
@@ -155,6 +161,8 @@  class DrmDisplayCompositor {
   // we need to reset them on every Dump() call.
   mutable uint64_t dump_frames_composited_;
   mutable uint64_t dump_last_timestamp_ns_;
+  VSyncWorker vsync_worker_;
+  int64_t flatten_countdown_;
 };
 }

diff --git a/drmencoder.cpp b/drmencoder.cpp
index 3d762f3..3df06a2 100644
--- a/drmencoder.cpp
+++ b/drmencoder.cpp
@@ -27,6 +27,7 @@  DrmEncoder::DrmEncoder(drmModeEncoderPtr e, DrmCrtc *current_crtc,
                        const std::vector<DrmCrtc *> &possible_crtcs)
     : id_(e->encoder_id),
       crtc_(current_crtc),
+      display_(-1),
       possible_crtcs_(possible_crtcs) {
 }

@@ -40,5 +41,19 @@  DrmCrtc *DrmEncoder::crtc() const {

 void DrmEncoder::set_crtc(DrmCrtc *crtc) {
   crtc_ = crtc;
+  set_display(crtc->display());
 }
+
+int DrmEncoder::display() const {
+  return display_;
+}
+
+void DrmEncoder::set_display(int display) {
+  display_ = display;
+}
+
+bool DrmEncoder::can_bind(int display) const {
+  return display_ == -1 || display_ == display;
+}
+
 }
diff --git a/drmencoder.h b/drmencoder.h
index 58ccbfb..6b39505 100644
--- a/drmencoder.h
+++ b/drmencoder.h
@@ -25,6 +25,8 @@ 

 namespace android {

+class DrmCrtc;
+
 class DrmEncoder {
  public:
   DrmEncoder(drmModeEncoderPtr e, DrmCrtc *current_crtc,
@@ -36,7 +38,9 @@  class DrmEncoder {

   DrmCrtc *crtc() const;
   void set_crtc(DrmCrtc *crtc);
-
+  bool can_bind(int display) const;
+  void set_display(int display);
+  int display() const;
   const std::vector<DrmCrtc *> &possible_crtcs() const {
     return possible_crtcs_;
   }
@@ -44,6 +48,7 @@  class DrmEncoder {
  private:
   uint32_t id_;
   DrmCrtc *crtc_;
+  int display_;

   std::vector<DrmCrtc *> possible_crtcs_;
 };
diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
index dfca1a6..65337cc 100644
--- a/drmhwctwo.cpp
+++ b/drmhwctwo.cpp
@@ -700,6 +700,7 @@  HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
         break;
     }
   }
+  compositor_.ResetCountdown();
   return *num_types ? HWC2::Error::HasChanges : HWC2::Error::None;
 }

diff --git a/drmresources.cpp b/drmresources.cpp
index 32dd376..880cef2 100644
--- a/drmresources.cpp
+++ b/drmresources.cpp
@@ -33,6 +33,8 @@ 
 #include <cutils/log.h>
 #include <cutils/properties.h>

+#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS    4
+
 namespace android {

 DrmResources::DrmResources() : event_listener_(this) {
@@ -65,6 +67,11 @@  int DrmResources::Init() {
     return ret;
   }

+  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
+  if (ret) {
+    ALOGI("Failed to set writeback cap %d", ret);
+    ret = 0;
+  }
   drmModeResPtr res = drmModeGetResources(fd());
   if (!res) {
     ALOGE("Failed to get DrmResources resources");
@@ -77,6 +84,7 @@  int DrmResources::Init() {
       std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);

   bool found_primary = false;
+  int primary_index = 0;
   int display_num = 1;

   for (int i = 0; !ret && i < res->count_crtcs; ++i) {
@@ -162,18 +170,22 @@  int DrmResources::Init() {
     if (conn->internal() && !found_primary) {
       conn->set_display(0);
       found_primary = true;
-    } else {
+    } else if (conn->external()) {
+      if (!found_primary) primary_index++;
       conn->set_display(display_num);
       ++display_num;
     }
   }

   // Then look for primary amongst external connectors
+  if (!found_primary) primary_index = 0;
   for (auto &conn : connectors_) {
     if (conn->external() && !found_primary) {
       conn->set_display(0);
       found_primary = true;
+      break;
     }
+    if (!found_primary) primary_index++;
   }

   if (res)
@@ -220,12 +232,29 @@  int DrmResources::Init() {
   }

   for (auto &conn : connectors_) {
+    if (conn->writeback())
+      continue;
     ret = CreateDisplayPipe(conn.get());
     if (ret) {
       ALOGE("Failed CreateDisplayPipe %d with %d", conn->id(), ret);
       return ret;
     }
   }
+  /* Allocate writebacks according to the display number */
+  /* 0 should get a writeback before 1 */
+  for (auto &writeback_conn : connectors_) {
+    if (!writeback_conn->writeback())
+      continue;
+    int index = primary_index;
+    do {
+      if (connectors_[index]->display() < 0)
+        continue;
+      ret = AttachWriteback(writeback_conn.get(), connectors_[index].get());
+      if (!ret)
+        break;
+      index = (index + 1) % connectors_.size();
+    } while (index != primary_index);
+  }
   return 0;
 }

@@ -314,6 +343,31 @@  int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
   return -ENODEV;
 }

+/*
+ * Attach writeback connector to the CRTC linked to the display_conn
+ *
+ */
+int DrmResources::AttachWriteback(DrmConnector *writeback_conn, DrmConnector *display_conn) {
+  int ret = -EINVAL;
+  if (display_conn->writeback())
+    return -EINVAL;
+  if (!display_conn->encoder() || ! display_conn->encoder()->crtc())
+    return -EINVAL;
+  DrmCrtc *display_crtc = display_conn->encoder()->crtc();
+  if (display_crtc->writeback_conn() != nullptr)
+    return -EINVAL;
+  for (DrmEncoder *writeback_enc : writeback_conn->possible_encoders()) {
+    // 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);
+      display_crtc->set_writeback_conn(writeback_conn);
+      ret = 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 4cca48c..ad285ec 100644
--- a/drmresources.h
+++ b/drmresources.h
@@ -78,6 +78,7 @@  class DrmResources {
                   DrmProperty *property);

   int CreateDisplayPipe(DrmConnector *connector);
+  int AttachWriteback(DrmConnector *writeback_conn, DrmConnector *display_conn);

   UniqueFd fd_;
   uint32_t mode_id_ = 0;