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@xxxxxxx>
> > > ---
> > > 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
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
