| Message ID | 1523460149-1740-18-git-send-email-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 11, 2018 at 04:22:28PM +0100, Alexandru Gheorghe wrote: > Flatten scene on the same CRTC as the one driving the display. > The active composition is played back to the display with a buffer > attached to the writeback connector. > Then we build a composition that has only one plane enabled and that > uses the result of the writeback as the input. > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > --- > drmdisplaycompositor.cpp | 203 +++++++++++++++++++++++++++++++++++++++++++++-- > drmdisplaycompositor.h | 7 +- > 2 files changed, 204 insertions(+), 6 deletions(-) > > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp > index e535e8a..cb670e6 100644 > --- a/drmdisplaycompositor.cpp > +++ b/drmdisplaycompositor.cpp > @@ -36,6 +36,7 @@ > #include "drmplane.h" > #include "drmresources.h" > #include "glworker.h" > +static const uint32_t kWaitWritebackFence = 100; // ms > > namespace android { > > @@ -523,7 +524,9 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) { > } > > int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > - bool test_only) { > + bool test_only, > + DrmDisplayComposition *writeback_comp, > + DrmConnector *writeback_conn) { > ATRACE_CALL(); > > int ret = 0; > @@ -532,6 +535,7 @@ 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; > > DrmConnector *connector = drm_->GetConnectorForDisplay(display_); > if (!connector) { > @@ -550,9 +554,37 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > return -ENOMEM; > } > > + if (writeback_comp != NULL) { > + if (writeback_conn == NULL) > + return -EINVAL; > + if (writeback_conn->writeback_fb_id().id() == 0 || > + writeback_conn->writeback_out_fence().id() == 0) { > + ALOGE("Writeback properties don't exit"); > + return -EINVAL; > + } > + if (writeback_comp->layers().size() != 1) { > + ALOGE("Invalid number of layers for writeback composition"); > + 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); Upcasting int to u64 isn't a great idea, please go the other way (as we do with out_fences). > + if (ret < 0) { > + ALOGE("Failed to add writeback_out_fence"); > + return ret; > + } > + } This would be more readable if it was split off into a function. > if (crtc->out_fence_ptr_property().id() != 0) { > - ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(), > - (uint64_t) &out_fences[crtc->pipe()]); > + ret = drmModeAtomicAddProperty(pset, crtc->id(), > + crtc->out_fence_ptr_property().id(), > + (uint64_t)&out_fences[crtc->pipe()]); > if (ret < 0) { > ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret); > drmModeAtomicFree(pset); > @@ -580,6 +612,15 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > } > } > > + if (writeback_conn != NULL) { > + ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), > + writeback_conn->crtc_id_property().id(), > + crtc->id()); > + if (ret < 0) { > + ALOGE("Failed to attach writeback"); > + } > + } Can you do this above with the rest of the writeback properties? > + > for (DrmCompositionPlane &comp_plane : comp_planes) { > DrmPlane *plane = comp_plane.plane(); > DrmCrtc *crtc = comp_plane.crtc(); > @@ -729,8 +770,18 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > if (!ret) { > uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET; > - if (test_only) > + if (test_only) { > flags |= DRM_MODE_ATOMIC_TEST_ONLY; > + } else { > + if (writeback_comp != NULL) { > + if (!CountdownExpired() && active_composition_) { Given that we're holding the lock throughout this function, can't you just abort at the start? > + ALOGE("Writeback composition not needed, abort commit"); > + drmModeAtomicFree(pset); > + return -EINVAL; > + }; > + flags |= DRM_MODE_ATOMIC_NONBLOCK; I'm guessing this is the cause of the active_composition race you're fixing earlier in the series? Could you please pull this out and squash it into that patch as a "Use non-blocking commits" standalone? It might be useful for testing, and this is something that's substantial enough to warrant the additional visibility of its own patch. > + } > + } > > ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_); > if (ret) { > @@ -769,6 +820,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > if (crtc->out_fence_ptr_property().id()) { > display_comp->set_out_fence((int) out_fences[crtc->pipe()]); > } > + if (writeback_fence >= 0) { > + if (writeback_comp->layers().size() != 1) { > + ALOGE("Invalid numbers of layer for writeback_comp"); > + return -EINVAL; > + } You already test this above? > + writeback_comp->layers()[0].acquire_fence.Set(writeback_fence); > + } > > return ret; > } > @@ -837,6 +895,8 @@ void DrmDisplayCompositor::ApplyFrame( > if (active_composition_) > active_composition_->SignalCompositionDone(); > active_composition_.swap(composition); > + flatten_countdown_ = FLATTEN_COUNTDOWN_INIT; > + vsync_worker_.VSyncControl(!writeback); > } > } > > @@ -913,8 +973,141 @@ int DrmDisplayCompositor::ApplyComposition( > return ret; > } > > +int DrmDisplayCompositor::WritebackComposite(DrmDisplayComposition *src, > + DrmDisplayComposition *dst, > + DrmConnector *writeback_conn) { > + int ret = 0; > + if (src == NULL || dst == NULL) > + return -EINVAL; > + std::vector<DrmCompositionPlane> &src_planes = src->composition_planes(); > + 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 (!squashed_comp.plane() && > + comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY) > + squashed_comp.set_plane(comp_plane.plane()); > + else > + dst->AddPlaneDisable(comp_plane.plane()); > + } > + > + DrmFramebuffer *writeback_fb = NULL; > + AutoLock lock(&lock_, __FUNCTION__); > + if ((ret = lock.Lock())) Same comments regarding assignment in a conditional. > + return ret; > + writeback_fb = &framebuffers_[framebuffer_index_]; > + framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS; > + ret = PrepareFramebuffer(*writeback_fb, dst, mode_.mode.h_display(), > + mode_.mode.v_display()); > + if (ret) { > + ALOGE("Failed to prepare destination buffer"); > + return ret; > + } > + lock.Unlock(); > + ret = CommitFrame(src, true, dst, writeback_conn); > + if (ret) { > + ALOGE("Atomic check failed"); > + return ret; > + } > + if ((ret = lock.Lock())) All of these locks and unlocks are going to cause races, as mentioned below, please re-evaluate. > + return ret; > + if (!CountdownExpired() && active_composition_) { > + ALOGE("Writeback composition not needed abort"); > + return -EINVAL; > + } > + ret = CommitFrame(src, false, dst, writeback_conn); > + lock.Unlock(); > + if (ret || dst->layers().size() != 1) { > + ALOGE("Failed to flatten scene using writeback"); > + return -EINVAL; > + } > + squashed_comp.source_layers().push_back(0); > + ret = > + sync_wait(dst->layers()[0].acquire_fence.Release(), kWaitWritebackFence); line break > + if (ret) { > + ALOGE("Failed to wait on writeback fence"); > + return ret; > + } > + 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; > + } > + return 0; > +} > + > +int DrmDisplayCompositor::FlattenSynchronously(DrmConnector *writeback_conn) { > + if (writeback_conn->display() != display_) { > + ALOGE("Cannot flatten synchronously on different display"); > + return -EINVAL; > + } You check this right before calling, so this isn't needed. > + ALOGI("FlattenSynchronously using the same display"); I think you should downgrade this log level. > + int ret = 0; > + /* Flattened composition with only one layer that is built > + * using the writeback connector > + */ > + std::unique_ptr<DrmDisplayComposition> writeback_comp = > + CreateInitializedComposition(); > + /* Copy of the active_composition_, we need a copy because > + * if we use the active composition we have to hold the lock > + * for the entire sequence of flattening. > + */ > + std::unique_ptr<DrmDisplayComposition> copy_comp = > + CreateInitializedComposition(); > + > + if (!copy_comp || !writeback_comp) > + return -EINVAL; > + AutoLock lock(&lock_, __FUNCTION__); > + if ((ret = lock.Lock())) Assignments in if statements make me nervous. Since you don't need the declaration above, just do: int ret = lock.Lock(); if (ret) > + return ret; > + if (CountdownExpired()) { > + ret = copy_comp->CopyLayers(active_composition_.get()); > + if (ret) > + return ret; > + copy_comp->CopyCompPlanes(active_composition_.get()); > + } else { > + return -EINVAL; > + } if (!CountdownExpired()) return -EINVAL; ret = copy_comp->CopyLayers(active_composition_.get()); if (ret) return ret; copy_comp->CopyCompPlanes(active_composition_.get()); > + lock.Unlock(); Just hold the lock through WritebackComposite(), and consider creating a ApplyFrameLocked() to hold it through ApplyFrame(). You should be able to reduce the number of times you have to check CountdownExpired() (or remove it) significantly by just consistently locking things. > + ret = > + WritebackComposite(copy_comp.get(), writeback_comp.get(), writeback_conn); > + if (ret) { > + ALOGE("Failed to prepare writebackScene"); > + return ret; > + } > + > + ApplyFrame(std::move(writeback_comp), 0, true); > + return 0; > +} > + > int DrmDisplayCompositor::FlattenScene() { > - return -EINVAL; > + DrmConnector *writeback_conn = > + drm_->resource_manager()->AvailableWritebackConnector(display_); > + if (!active_composition_ || !writeback_conn) > + return -EINVAL; > + std::vector<DrmCompositionPlane> &src_planes = > + active_composition_->composition_planes(); > + 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; > + > + if (writeback_conn->display() == display_) { > + return FlattenSynchronously(writeback_conn); > + } > + > + return 0; > } > > int DrmDisplayCompositor::SquashAll() { > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h > index 26201b9..4cc4a5e 100644 > --- a/drmdisplaycompositor.h > +++ b/drmdisplaycompositor.h > @@ -125,7 +125,9 @@ 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 = NULL, > + DrmConnector *writeback_conn = NULL); > int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst); > int ApplyDpms(DrmDisplayComposition *display_comp); > int DisablePlanes(DrmDisplayComposition *display_comp); > @@ -134,7 +136,10 @@ class DrmDisplayCompositor { > void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition, > int status, bool writeback = false); > int FlattenScene(); > + int FlattenSynchronously(DrmConnector *writeback_conn); Can we come up with a better name than Synchonrous/Asynchronous? Or at the very least comment on what that means? > > + int WritebackComposite(DrmDisplayComposition *src, DrmDisplayComposition *dst, > + DrmConnector *writeback_conn); > bool CountdownExpired() const; > > std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode); > -- > 2.7.4 >
On Tue, Apr 17, 2018 at 01:47:46PM -0400, Sean Paul wrote: > On Wed, Apr 11, 2018 at 04:22:28PM +0100, Alexandru Gheorghe wrote: > > Flatten scene on the same CRTC as the one driving the display. > > The active composition is played back to the display with a buffer > > attached to the writeback connector. > > Then we build a composition that has only one plane enabled and that > > uses the result of the writeback as the input. > > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > > --- > > drmdisplaycompositor.cpp | 203 +++++++++++++++++++++++++++++++++++++++++++++-- > > drmdisplaycompositor.h | 7 +- > > 2 files changed, 204 insertions(+), 6 deletions(-) > > > > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp > > index e535e8a..cb670e6 100644 > > --- a/drmdisplaycompositor.cpp > > +++ b/drmdisplaycompositor.cpp > > @@ -36,6 +36,7 @@ > > #include "drmplane.h" > > #include "drmresources.h" > > #include "glworker.h" > > +static const uint32_t kWaitWritebackFence = 100; // ms > > > > namespace android { > > > > @@ -523,7 +524,9 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) { > > } > > > > int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > - bool test_only) { > > + bool test_only, > > + DrmDisplayComposition *writeback_comp, > > + DrmConnector *writeback_conn) { > > ATRACE_CALL(); > > > > int ret = 0; > > @@ -532,6 +535,7 @@ 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; > > > > DrmConnector *connector = drm_->GetConnectorForDisplay(display_); > > if (!connector) { > > @@ -550,9 +554,37 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > return -ENOMEM; > > } > > > > + if (writeback_comp != NULL) { > > + if (writeback_conn == NULL) > > + return -EINVAL; > > + if (writeback_conn->writeback_fb_id().id() == 0 || > > + writeback_conn->writeback_out_fence().id() == 0) { > > + ALOGE("Writeback properties don't exit"); > > + return -EINVAL; > > + } > > + if (writeback_comp->layers().size() != 1) { > > + ALOGE("Invalid number of layers for writeback composition"); > > + 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); > > Upcasting int to u64 isn't a great idea, please go the other way (as we do with > out_fences). Genuinely curious about this, why does it make a difference I'm upcasting the address not the int itself. More, the kernel does this s32 __user *fence_ptr = u64_to_user_ptr(val); To be completly fair it should have been int32_t. > > > + if (ret < 0) { > > + ALOGE("Failed to add writeback_out_fence"); > > + return ret; > > + } > > + } > > This would be more readable if it was split off into a function. > > > if (crtc->out_fence_ptr_property().id() != 0) { > > - ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(), > > - (uint64_t) &out_fences[crtc->pipe()]); > > + ret = drmModeAtomicAddProperty(pset, crtc->id(), > > + crtc->out_fence_ptr_property().id(), > > + (uint64_t)&out_fences[crtc->pipe()]); > > if (ret < 0) { > > ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret); > > drmModeAtomicFree(pset); > > @@ -580,6 +612,15 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > } > > } > > > > + if (writeback_conn != NULL) { > > + ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), > > + writeback_conn->crtc_id_property().id(), > > + crtc->id()); > > + if (ret < 0) { > > + ALOGE("Failed to attach writeback"); > > + } > > + } > > Can you do this above with the rest of the writeback properties? > > > + > > for (DrmCompositionPlane &comp_plane : comp_planes) { > > DrmPlane *plane = comp_plane.plane(); > > DrmCrtc *crtc = comp_plane.crtc(); > > @@ -729,8 +770,18 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > > > if (!ret) { > > uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET; > > - if (test_only) > > + if (test_only) { > > flags |= DRM_MODE_ATOMIC_TEST_ONLY; > > + } else { > > + if (writeback_comp != NULL) { > > + if (!CountdownExpired() && active_composition_) { > > Given that we're holding the lock throughout this function, can't you just > abort at the start? Yes, we could/should. > > > + ALOGE("Writeback composition not needed, abort commit"); > > + drmModeAtomicFree(pset); > > + return -EINVAL; > > + }; > > + flags |= DRM_MODE_ATOMIC_NONBLOCK; > > I'm guessing this is the cause of the active_composition race you're fixing > earlier in the series? Could you please pull this out and squash it into that > patch as a "Use non-blocking commits" standalone? It might be useful for > testing, and this is something that's substantial enough to warrant the > additional visibility of its own patch. Not really, that race could happen even without non blocking. The current implementation is: 1. Commit 2. lock. 3. Set active_composition 4. Unlock. With two threads there is no guarantee active_composition will contain what it's been comitted. > > > + } > > + } > > > > ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_); > > if (ret) { > > @@ -769,6 +820,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > if (crtc->out_fence_ptr_property().id()) { > > display_comp->set_out_fence((int) out_fences[crtc->pipe()]); > > } > > + if (writeback_fence >= 0) { > > + if (writeback_comp->layers().size() != 1) { > > + ALOGE("Invalid numbers of layer for writeback_comp"); > > + return -EINVAL; > > + } > > You already test this above? > > > + writeback_comp->layers()[0].acquire_fence.Set(writeback_fence); > > + } > > > > return ret; > > } > > @@ -837,6 +895,8 @@ void DrmDisplayCompositor::ApplyFrame( > > if (active_composition_) > > active_composition_->SignalCompositionDone(); > > active_composition_.swap(composition); > > + flatten_countdown_ = FLATTEN_COUNTDOWN_INIT; > > + vsync_worker_.VSyncControl(!writeback); > > } > > } > > > > @@ -913,8 +973,141 @@ int DrmDisplayCompositor::ApplyComposition( > > return ret; > > } > > > > +int DrmDisplayCompositor::WritebackComposite(DrmDisplayComposition *src, > > + DrmDisplayComposition *dst, > > + DrmConnector *writeback_conn) { > > + int ret = 0; > > + if (src == NULL || dst == NULL) > > + return -EINVAL; > > + std::vector<DrmCompositionPlane> &src_planes = src->composition_planes(); > > + 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 (!squashed_comp.plane() && > > + comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY) > > + squashed_comp.set_plane(comp_plane.plane()); > > + else > > + dst->AddPlaneDisable(comp_plane.plane()); > > + } > > + > > + DrmFramebuffer *writeback_fb = NULL; > > + AutoLock lock(&lock_, __FUNCTION__); > > + if ((ret = lock.Lock())) > > Same comments regarding assignment in a conditional. > > > + return ret; > > + writeback_fb = &framebuffers_[framebuffer_index_]; > > + framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS; > > + ret = PrepareFramebuffer(*writeback_fb, dst, mode_.mode.h_display(), > > + mode_.mode.v_display()); > > + if (ret) { > > + ALOGE("Failed to prepare destination buffer"); > > + return ret; > > + } > > + lock.Unlock(); > > + ret = CommitFrame(src, true, dst, writeback_conn); > > + if (ret) { > > + ALOGE("Atomic check failed"); > > + return ret; > > + } > > + if ((ret = lock.Lock())) > > All of these locks and unlocks are going to cause races, as mentioned below, > please re-evaluate. > > > + return ret; > > + if (!CountdownExpired() && active_composition_) { > > + ALOGE("Writeback composition not needed abort"); > > + return -EINVAL; > > + } > > + ret = CommitFrame(src, false, dst, writeback_conn); > > + lock.Unlock(); > > + if (ret || dst->layers().size() != 1) { > > + ALOGE("Failed to flatten scene using writeback"); > > + return -EINVAL; > > + } > > + squashed_comp.source_layers().push_back(0); > > + ret = > > + sync_wait(dst->layers()[0].acquire_fence.Release(), kWaitWritebackFence); > > line break > > > + if (ret) { > > + ALOGE("Failed to wait on writeback fence"); > > + return ret; > > + } > > + 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; > > + } > > + return 0; > > +} > > + > > +int DrmDisplayCompositor::FlattenSynchronously(DrmConnector *writeback_conn) { > > + if (writeback_conn->display() != display_) { > > + ALOGE("Cannot flatten synchronously on different display"); > > + return -EINVAL; > > + } > > You check this right before calling, so this isn't needed. > > > + ALOGI("FlattenSynchronously using the same display"); > > I think you should downgrade this log level. > > > + int ret = 0; > > + /* Flattened composition with only one layer that is built > > + * using the writeback connector > > + */ > > + std::unique_ptr<DrmDisplayComposition> writeback_comp = > > + CreateInitializedComposition(); > > + /* Copy of the active_composition_, we need a copy because > > + * if we use the active composition we have to hold the lock > > + * for the entire sequence of flattening. > > + */ > > + std::unique_ptr<DrmDisplayComposition> copy_comp = > > + CreateInitializedComposition(); > > + > > + if (!copy_comp || !writeback_comp) > > + return -EINVAL; > > + AutoLock lock(&lock_, __FUNCTION__); > > + if ((ret = lock.Lock())) > > Assignments in if statements make me nervous. Since you don't need the > declaration above, just do: > > int ret = lock.Lock(); > if (ret) > > > + return ret; > > + if (CountdownExpired()) { > > + ret = copy_comp->CopyLayers(active_composition_.get()); > > + if (ret) > > + return ret; > > + copy_comp->CopyCompPlanes(active_composition_.get()); > > + } else { > > + return -EINVAL; > > + } > > if (!CountdownExpired()) > return -EINVAL; > > ret = copy_comp->CopyLayers(active_composition_.get()); > if (ret) > return ret; > copy_comp->CopyCompPlanes(active_composition_.get()); > > > > + lock.Unlock(); > > Just hold the lock through WritebackComposite(), and consider creating a > ApplyFrameLocked() to hold it through ApplyFrame(). You should be able to reduce > the number of times you have to check CountdownExpired() (or remove it) > significantly by just consistently locking things. > > > + ret = > > + WritebackComposite(copy_comp.get(), writeback_comp.get(), writeback_conn); > > + if (ret) { > > + ALOGE("Failed to prepare writebackScene"); > > + return ret; > > + } > > + > > + ApplyFrame(std::move(writeback_comp), 0, true); > > + return 0; > > +} > > + > > int DrmDisplayCompositor::FlattenScene() { > > - return -EINVAL; > > + DrmConnector *writeback_conn = > > + drm_->resource_manager()->AvailableWritebackConnector(display_); > > + if (!active_composition_ || !writeback_conn) > > + return -EINVAL; > > + std::vector<DrmCompositionPlane> &src_planes = > > + active_composition_->composition_planes(); > > + 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; > > + > > + if (writeback_conn->display() == display_) { > > + return FlattenSynchronously(writeback_conn); > > + } > > + > > + return 0; > > } > > > > int DrmDisplayCompositor::SquashAll() { > > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h > > index 26201b9..4cc4a5e 100644 > > --- a/drmdisplaycompositor.h > > +++ b/drmdisplaycompositor.h > > @@ -125,7 +125,9 @@ 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 = NULL, > > + DrmConnector *writeback_conn = NULL); > > int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst); > > int ApplyDpms(DrmDisplayComposition *display_comp); > > int DisablePlanes(DrmDisplayComposition *display_comp); > > @@ -134,7 +136,10 @@ class DrmDisplayCompositor { > > void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition, > > int status, bool writeback = false); > > int FlattenScene(); > > + int FlattenSynchronously(DrmConnector *writeback_conn); > > Can we come up with a better name than Synchonrous/Asynchronous? Or at the very > least comment on what that means? > > > > > + int WritebackComposite(DrmDisplayComposition *src, DrmDisplayComposition *dst, > > + DrmConnector *writeback_conn); > > bool CountdownExpired() const; > > > > std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode); > > -- > > 2.7.4 > > > > -- > Sean Paul, Software Engineer, Google / Chromium OS
On Wed, Apr 18, 2018 at 12:14:03PM +0100, Alexandru-Cosmin Gheorghe wrote: > On Tue, Apr 17, 2018 at 01:47:46PM -0400, Sean Paul wrote: > > On Wed, Apr 11, 2018 at 04:22:28PM +0100, Alexandru Gheorghe wrote: > > > Flatten scene on the same CRTC as the one driving the display. > > > The active composition is played back to the display with a buffer > > > attached to the writeback connector. > > > Then we build a composition that has only one plane enabled and that > > > uses the result of the writeback as the input. > > > > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > > > --- > > > drmdisplaycompositor.cpp | 203 +++++++++++++++++++++++++++++++++++++++++++++-- > > > drmdisplaycompositor.h | 7 +- > > > 2 files changed, 204 insertions(+), 6 deletions(-) > > > > > > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp > > > index e535e8a..cb670e6 100644 > > > --- a/drmdisplaycompositor.cpp > > > +++ b/drmdisplaycompositor.cpp > > > @@ -36,6 +36,7 @@ > > > #include "drmplane.h" > > > #include "drmresources.h" > > > #include "glworker.h" > > > +static const uint32_t kWaitWritebackFence = 100; // ms > > > > > > namespace android { > > > > > > @@ -523,7 +524,9 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) { > > > } > > > > > > int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > > - bool test_only) { > > > + bool test_only, > > > + DrmDisplayComposition *writeback_comp, > > > + DrmConnector *writeback_conn) { > > > ATRACE_CALL(); > > > > > > int ret = 0; > > > @@ -532,6 +535,7 @@ 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; > > > > > > DrmConnector *connector = drm_->GetConnectorForDisplay(display_); > > > if (!connector) { > > > @@ -550,9 +554,37 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > > return -ENOMEM; > > > } > > > > > > + if (writeback_comp != NULL) { > > > + if (writeback_conn == NULL) > > > + return -EINVAL; > > > + if (writeback_conn->writeback_fb_id().id() == 0 || > > > + writeback_conn->writeback_out_fence().id() == 0) { > > > + ALOGE("Writeback properties don't exit"); > > > + return -EINVAL; > > > + } > > > + if (writeback_comp->layers().size() != 1) { > > > + ALOGE("Invalid number of layers for writeback composition"); > > > + 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); > > > > Upcasting int to u64 isn't a great idea, please go the other way (as we do with > > out_fences). > > Genuinely curious about this, why does it make a difference I'm > upcasting the address not the int itself. Right, for some reason I had inserted a * inside the parens with my head. Please ignore :) > More, the kernel does this s32 __user *fence_ptr = u64_to_user_ptr(val); > To be completly fair it should have been int32_t. > > > > > > > + if (ret < 0) { > > > + ALOGE("Failed to add writeback_out_fence"); > > > + return ret; > > > + } > > > + } > > > > This would be more readable if it was split off into a function. > > > > > if (crtc->out_fence_ptr_property().id() != 0) { > > > - ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(), > > > - (uint64_t) &out_fences[crtc->pipe()]); > > > + ret = drmModeAtomicAddProperty(pset, crtc->id(), > > > + crtc->out_fence_ptr_property().id(), > > > + (uint64_t)&out_fences[crtc->pipe()]); > > > if (ret < 0) { > > > ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret); > > > drmModeAtomicFree(pset); > > > @@ -580,6 +612,15 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > > } > > > } > > > > > > + if (writeback_conn != NULL) { > > > + ret = drmModeAtomicAddProperty(pset, writeback_conn->id(), > > > + writeback_conn->crtc_id_property().id(), > > > + crtc->id()); > > > + if (ret < 0) { > > > + ALOGE("Failed to attach writeback"); > > > + } > > > + } > > > > Can you do this above with the rest of the writeback properties? > > > > > + > > > for (DrmCompositionPlane &comp_plane : comp_planes) { > > > DrmPlane *plane = comp_plane.plane(); > > > DrmCrtc *crtc = comp_plane.crtc(); > > > @@ -729,8 +770,18 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > > > > > if (!ret) { > > > uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET; > > > - if (test_only) > > > + if (test_only) { > > > flags |= DRM_MODE_ATOMIC_TEST_ONLY; > > > + } else { > > > + if (writeback_comp != NULL) { > > > + if (!CountdownExpired() && active_composition_) { > > > > Given that we're holding the lock throughout this function, can't you just > > abort at the start? > > Yes, we could/should. > > > > > > + ALOGE("Writeback composition not needed, abort commit"); > > > + drmModeAtomicFree(pset); > > > + return -EINVAL; > > > + }; > > > + flags |= DRM_MODE_ATOMIC_NONBLOCK; > > > > I'm guessing this is the cause of the active_composition race you're fixing > > earlier in the series? Could you please pull this out and squash it into that > > patch as a "Use non-blocking commits" standalone? It might be useful for > > testing, and this is something that's substantial enough to warrant the > > additional visibility of its own patch. > > Not really, that race could happen even without non blocking. The > current implementation is: > 1. Commit > 2. lock. > 3. Set active_composition > 4. Unlock. > > With two threads there is no guarantee active_composition will contain > what it's been comitted. > Makes sense, thanks for laying it out. Sean > > > > > > + } > > > + } > > > > > > ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_); > > > if (ret) { > > > @@ -769,6 +820,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, > > > if (crtc->out_fence_ptr_property().id()) { > > > display_comp->set_out_fence((int) out_fences[crtc->pipe()]); > > > } > > > + if (writeback_fence >= 0) { > > > + if (writeback_comp->layers().size() != 1) { > > > + ALOGE("Invalid numbers of layer for writeback_comp"); > > > + return -EINVAL; > > > + } > > > > You already test this above? > > > > > + writeback_comp->layers()[0].acquire_fence.Set(writeback_fence); > > > + } > > > > > > return ret; > > > } > > > @@ -837,6 +895,8 @@ void DrmDisplayCompositor::ApplyFrame( > > > if (active_composition_) > > > active_composition_->SignalCompositionDone(); > > > active_composition_.swap(composition); > > > + flatten_countdown_ = FLATTEN_COUNTDOWN_INIT; > > > + vsync_worker_.VSyncControl(!writeback); > > > } > > > } > > > > > > @@ -913,8 +973,141 @@ int DrmDisplayCompositor::ApplyComposition( > > > return ret; > > > } > > > > > > +int DrmDisplayCompositor::WritebackComposite(DrmDisplayComposition *src, > > > + DrmDisplayComposition *dst, > > > + DrmConnector *writeback_conn) { > > > + int ret = 0; > > > + if (src == NULL || dst == NULL) > > > + return -EINVAL; > > > + std::vector<DrmCompositionPlane> &src_planes = src->composition_planes(); > > > + 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 (!squashed_comp.plane() && > > > + comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY) > > > + squashed_comp.set_plane(comp_plane.plane()); > > > + else > > > + dst->AddPlaneDisable(comp_plane.plane()); > > > + } > > > + > > > + DrmFramebuffer *writeback_fb = NULL; > > > + AutoLock lock(&lock_, __FUNCTION__); > > > + if ((ret = lock.Lock())) > > > > Same comments regarding assignment in a conditional. > > > > > + return ret; > > > + writeback_fb = &framebuffers_[framebuffer_index_]; > > > + framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS; > > > + ret = PrepareFramebuffer(*writeback_fb, dst, mode_.mode.h_display(), > > > + mode_.mode.v_display()); > > > + if (ret) { > > > + ALOGE("Failed to prepare destination buffer"); > > > + return ret; > > > + } > > > + lock.Unlock(); > > > + ret = CommitFrame(src, true, dst, writeback_conn); > > > + if (ret) { > > > + ALOGE("Atomic check failed"); > > > + return ret; > > > + } > > > + if ((ret = lock.Lock())) > > > > All of these locks and unlocks are going to cause races, as mentioned below, > > please re-evaluate. > > > > > + return ret; > > > + if (!CountdownExpired() && active_composition_) { > > > + ALOGE("Writeback composition not needed abort"); > > > + return -EINVAL; > > > + } > > > + ret = CommitFrame(src, false, dst, writeback_conn); > > > + lock.Unlock(); > > > + if (ret || dst->layers().size() != 1) { > > > + ALOGE("Failed to flatten scene using writeback"); > > > + return -EINVAL; > > > + } > > > + squashed_comp.source_layers().push_back(0); > > > + ret = > > > + sync_wait(dst->layers()[0].acquire_fence.Release(), kWaitWritebackFence); > > > > line break > > > > > + if (ret) { > > > + ALOGE("Failed to wait on writeback fence"); > > > + return ret; > > > + } > > > + 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; > > > + } > > > + return 0; > > > +} > > > + > > > +int DrmDisplayCompositor::FlattenSynchronously(DrmConnector *writeback_conn) { > > > + if (writeback_conn->display() != display_) { > > > + ALOGE("Cannot flatten synchronously on different display"); > > > + return -EINVAL; > > > + } > > > > You check this right before calling, so this isn't needed. > > > > > + ALOGI("FlattenSynchronously using the same display"); > > > > I think you should downgrade this log level. > > > > > + int ret = 0; > > > + /* Flattened composition with only one layer that is built > > > + * using the writeback connector > > > + */ > > > + std::unique_ptr<DrmDisplayComposition> writeback_comp = > > > + CreateInitializedComposition(); > > > + /* Copy of the active_composition_, we need a copy because > > > + * if we use the active composition we have to hold the lock > > > + * for the entire sequence of flattening. > > > + */ > > > + std::unique_ptr<DrmDisplayComposition> copy_comp = > > > + CreateInitializedComposition(); > > > + > > > + if (!copy_comp || !writeback_comp) > > > + return -EINVAL; > > > + AutoLock lock(&lock_, __FUNCTION__); > > > + if ((ret = lock.Lock())) > > > > Assignments in if statements make me nervous. Since you don't need the > > declaration above, just do: > > > > int ret = lock.Lock(); > > if (ret) > > > > > + return ret; > > > + if (CountdownExpired()) { > > > + ret = copy_comp->CopyLayers(active_composition_.get()); > > > + if (ret) > > > + return ret; > > > + copy_comp->CopyCompPlanes(active_composition_.get()); > > > + } else { > > > + return -EINVAL; > > > + } > > > > if (!CountdownExpired()) > > return -EINVAL; > > > > ret = copy_comp->CopyLayers(active_composition_.get()); > > if (ret) > > return ret; > > copy_comp->CopyCompPlanes(active_composition_.get()); > > > > > > > + lock.Unlock(); > > > > Just hold the lock through WritebackComposite(), and consider creating a > > ApplyFrameLocked() to hold it through ApplyFrame(). You should be able to reduce > > the number of times you have to check CountdownExpired() (or remove it) > > significantly by just consistently locking things. > > > > > + ret = > > > + WritebackComposite(copy_comp.get(), writeback_comp.get(), writeback_conn); > > > + if (ret) { > > > + ALOGE("Failed to prepare writebackScene"); > > > + return ret; > > > + } > > > + > > > + ApplyFrame(std::move(writeback_comp), 0, true); > > > + return 0; > > > +} > > > + > > > int DrmDisplayCompositor::FlattenScene() { > > > - return -EINVAL; > > > + DrmConnector *writeback_conn = > > > + drm_->resource_manager()->AvailableWritebackConnector(display_); > > > + if (!active_composition_ || !writeback_conn) > > > + return -EINVAL; > > > + std::vector<DrmCompositionPlane> &src_planes = > > > + active_composition_->composition_planes(); > > > + 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; > > > + > > > + if (writeback_conn->display() == display_) { > > > + return FlattenSynchronously(writeback_conn); > > > + } > > > + > > > + return 0; > > > } > > > > > > int DrmDisplayCompositor::SquashAll() { > > > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h > > > index 26201b9..4cc4a5e 100644 > > > --- a/drmdisplaycompositor.h > > > +++ b/drmdisplaycompositor.h > > > @@ -125,7 +125,9 @@ 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 = NULL, > > > + DrmConnector *writeback_conn = NULL); > > > int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst); > > > int ApplyDpms(DrmDisplayComposition *display_comp); > > > int DisablePlanes(DrmDisplayComposition *display_comp); > > > @@ -134,7 +136,10 @@ class DrmDisplayCompositor { > > > void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition, > > > int status, bool writeback = false); > > > int FlattenScene(); > > > + int FlattenSynchronously(DrmConnector *writeback_conn); > > > > Can we come up with a better name than Synchonrous/Asynchronous? Or at the very > > least comment on what that means? > > > > > > > > + int WritebackComposite(DrmDisplayComposition *src, DrmDisplayComposition *dst, > > > + DrmConnector *writeback_conn); > > > bool CountdownExpired() const; > > > > > > std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode); > > > -- > > > 2.7.4 > > > > > > > -- > > Sean Paul, Software Engineer, Google / Chromium OS > > -- > Cheers, > Alex G
diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp index e535e8a..cb670e6 100644 --- a/drmdisplaycompositor.cpp +++ b/drmdisplaycompositor.cpp @@ -36,6 +36,7 @@ #include "drmplane.h" #include "drmresources.h" #include "glworker.h" +static const uint32_t kWaitWritebackFence = 100; // ms namespace android { @@ -523,7 +524,9 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) { } int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, - bool test_only) { + bool test_only, + DrmDisplayComposition *writeback_comp, + DrmConnector *writeback_conn) { ATRACE_CALL(); int ret = 0; @@ -532,6 +535,7 @@ 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; DrmConnector *connector = drm_->GetConnectorForDisplay(display_); if (!connector) { @@ -550,9 +554,37 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, return -ENOMEM; } + if (writeback_comp != NULL) { + if (writeback_conn == NULL) + return -EINVAL; + if (writeback_conn->writeback_fb_id().id() == 0 || + writeback_conn->writeback_out_fence().id() == 0) { + ALOGE("Writeback properties don't exit"); + return -EINVAL; + } + if (writeback_comp->layers().size() != 1) { + ALOGE("Invalid number of layers for writeback composition"); + 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(), - (uint64_t) &out_fences[crtc->pipe()]); + ret = drmModeAtomicAddProperty(pset, crtc->id(), + crtc->out_fence_ptr_property().id(), + (uint64_t)&out_fences[crtc->pipe()]); if (ret < 0) { ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret); drmModeAtomicFree(pset); @@ -580,6 +612,15 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, } } + if (writeback_conn != NULL) { + 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) { DrmPlane *plane = comp_plane.plane(); DrmCrtc *crtc = comp_plane.crtc(); @@ -729,8 +770,18 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, if (!ret) { uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET; - if (test_only) + if (test_only) { flags |= DRM_MODE_ATOMIC_TEST_ONLY; + } else { + if (writeback_comp != NULL) { + if (!CountdownExpired() && active_composition_) { + ALOGE("Writeback composition not needed, abort commit"); + drmModeAtomicFree(pset); + return -EINVAL; + }; + flags |= DRM_MODE_ATOMIC_NONBLOCK; + } + } ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_); if (ret) { @@ -769,6 +820,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, if (crtc->out_fence_ptr_property().id()) { display_comp->set_out_fence((int) out_fences[crtc->pipe()]); } + if (writeback_fence >= 0) { + if (writeback_comp->layers().size() != 1) { + ALOGE("Invalid numbers of layer for writeback_comp"); + return -EINVAL; + } + writeback_comp->layers()[0].acquire_fence.Set(writeback_fence); + } return ret; } @@ -837,6 +895,8 @@ void DrmDisplayCompositor::ApplyFrame( if (active_composition_) active_composition_->SignalCompositionDone(); active_composition_.swap(composition); + flatten_countdown_ = FLATTEN_COUNTDOWN_INIT; + vsync_worker_.VSyncControl(!writeback); } } @@ -913,8 +973,141 @@ int DrmDisplayCompositor::ApplyComposition( return ret; } +int DrmDisplayCompositor::WritebackComposite(DrmDisplayComposition *src, + DrmDisplayComposition *dst, + DrmConnector *writeback_conn) { + int ret = 0; + if (src == NULL || dst == NULL) + return -EINVAL; + std::vector<DrmCompositionPlane> &src_planes = src->composition_planes(); + 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 (!squashed_comp.plane() && + comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY) + squashed_comp.set_plane(comp_plane.plane()); + else + dst->AddPlaneDisable(comp_plane.plane()); + } + + DrmFramebuffer *writeback_fb = NULL; + AutoLock lock(&lock_, __FUNCTION__); + if ((ret = lock.Lock())) + return ret; + writeback_fb = &framebuffers_[framebuffer_index_]; + framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS; + ret = PrepareFramebuffer(*writeback_fb, dst, mode_.mode.h_display(), + mode_.mode.v_display()); + if (ret) { + ALOGE("Failed to prepare destination buffer"); + return ret; + } + lock.Unlock(); + ret = CommitFrame(src, true, dst, writeback_conn); + if (ret) { + ALOGE("Atomic check failed"); + return ret; + } + if ((ret = lock.Lock())) + return ret; + if (!CountdownExpired() && active_composition_) { + ALOGE("Writeback composition not needed abort"); + return -EINVAL; + } + ret = CommitFrame(src, false, dst, writeback_conn); + lock.Unlock(); + if (ret || dst->layers().size() != 1) { + ALOGE("Failed to flatten scene using writeback"); + return -EINVAL; + } + squashed_comp.source_layers().push_back(0); + ret = + sync_wait(dst->layers()[0].acquire_fence.Release(), kWaitWritebackFence); + if (ret) { + ALOGE("Failed to wait on writeback fence"); + return ret; + } + 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; + } + return 0; +} + +int DrmDisplayCompositor::FlattenSynchronously(DrmConnector *writeback_conn) { + if (writeback_conn->display() != display_) { + ALOGE("Cannot flatten synchronously on different display"); + return -EINVAL; + } + ALOGI("FlattenSynchronously using the same display"); + int ret = 0; + /* Flattened composition with only one layer that is built + * using the writeback connector + */ + std::unique_ptr<DrmDisplayComposition> writeback_comp = + CreateInitializedComposition(); + /* Copy of the active_composition_, we need a copy because + * if we use the active composition we have to hold the lock + * for the entire sequence of flattening. + */ + std::unique_ptr<DrmDisplayComposition> copy_comp = + CreateInitializedComposition(); + + if (!copy_comp || !writeback_comp) + return -EINVAL; + AutoLock lock(&lock_, __FUNCTION__); + if ((ret = lock.Lock())) + return ret; + if (CountdownExpired()) { + ret = copy_comp->CopyLayers(active_composition_.get()); + if (ret) + return ret; + copy_comp->CopyCompPlanes(active_composition_.get()); + } else { + return -EINVAL; + } + lock.Unlock(); + ret = + WritebackComposite(copy_comp.get(), writeback_comp.get(), writeback_conn); + if (ret) { + ALOGE("Failed to prepare writebackScene"); + return ret; + } + + ApplyFrame(std::move(writeback_comp), 0, true); + return 0; +} + int DrmDisplayCompositor::FlattenScene() { - return -EINVAL; + DrmConnector *writeback_conn = + drm_->resource_manager()->AvailableWritebackConnector(display_); + if (!active_composition_ || !writeback_conn) + return -EINVAL; + std::vector<DrmCompositionPlane> &src_planes = + active_composition_->composition_planes(); + 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; + + if (writeback_conn->display() == display_) { + return FlattenSynchronously(writeback_conn); + } + + return 0; } int DrmDisplayCompositor::SquashAll() { diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h index 26201b9..4cc4a5e 100644 --- a/drmdisplaycompositor.h +++ b/drmdisplaycompositor.h @@ -125,7 +125,9 @@ 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 = NULL, + DrmConnector *writeback_conn = NULL); int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst); int ApplyDpms(DrmDisplayComposition *display_comp); int DisablePlanes(DrmDisplayComposition *display_comp); @@ -134,7 +136,10 @@ class DrmDisplayCompositor { void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition, int status, bool writeback = false); int FlattenScene(); + int FlattenSynchronously(DrmConnector *writeback_conn); + int WritebackComposite(DrmDisplayComposition *src, DrmDisplayComposition *dst, + DrmConnector *writeback_conn); bool CountdownExpired() const; std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
Flatten scene on the same CRTC as the one driving the display. The active composition is played back to the display with a buffer attached to the writeback connector. Then we build a composition that has only one plane enabled and that uses the result of the writeback as the input. Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> --- drmdisplaycompositor.cpp | 203 +++++++++++++++++++++++++++++++++++++++++++++-- drmdisplaycompositor.h | 7 +- 2 files changed, 204 insertions(+), 6 deletions(-)