[PATCH hwc v1] [RFC] drm_hwcomposer: Flatten composition using writeback connector
Alexandru-Cosmin Gheorghe
Alexandru-Cosmin.Gheorghe at arm.com
Tue Mar 20 11:26:39 UTC 2018
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 <alexandru-cosmin.gheorghe at arm.com>
> > ---
> > 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
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
--
Cheers,
Alex G
More information about the dri-devel
mailing list