On Tue, Apr 17, 2018 at 01:02:18PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:25PM +0100, Alexandru Gheorghe wrote:
> > ApplyFrame holds the lock just when it swaps the value of
> > active_composition_, in a multithread context we could end up in a
> > situation where something is shown on the screen, but something else
> > is set in active_composition_. Fix it by holding the lock during
> > CommitFrame.
> >
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@xxxxxxx>
> > ---
> > drmdisplaycompositor.cpp | 40 +++++++++++++++++-----------------------
> > drmdisplaycompositor.h | 2 +-
> > 2 files changed, 18 insertions(+), 24 deletions(-)
> >
> > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> > index afd3b05..576539b 100644
> > --- a/drmdisplaycompositor.cpp
> > +++ b/drmdisplaycompositor.cpp
> > @@ -791,11 +791,6 @@ std::tuple<int, uint32_t> DrmDisplayCompositor::CreateModeBlob(
> > }
> >
> > void DrmDisplayCompositor::ClearDisplay() {
> > - AutoLock lock(&lock_, "compositor");
> > - int ret = lock.Lock();
> > - if (ret)
> > - return;
> > -
> > if (!active_composition_)
> > return;
> >
> > @@ -808,11 +803,25 @@ void DrmDisplayCompositor::ClearDisplay() {
> > }
> >
> > void DrmDisplayCompositor::ApplyFrame(
> > - std::unique_ptr<DrmDisplayComposition> composition, int status) {
> > + std::unique_ptr<DrmDisplayComposition> composition, int status,
> > + bool writeback) {
>
> The writeback argument addition seems unrelated to this change.
Agree.
>
> > + AutoLock lock(&lock_, __FUNCTION__);
> > + if (lock.Lock())
> > + return;
> > int ret = status;
> > -
> > - if (!ret)
> > + if (!ret) {
> > + if (writeback && !CountdownExpired()) {
> > + ALOGE("Abort playing back scene");
> > + return;
> > + }
> > ret = CommitFrame(composition.get(), false);
> > + if (!ret) {
> > + ++dump_frames_composited_;
> > + if (active_composition_)
> > + active_composition_->SignalCompositionDone();
> > + active_composition_.swap(composition);
>
> Why move this stuff?
Because both CommitFrame and swap need the lock, the code in between
don't need that.
>
> > + }
> > + }
> >
> > if (ret) {
> > ALOGE("Composite failed for display %d", display_);
> > @@ -821,21 +830,6 @@ void DrmDisplayCompositor::ApplyFrame(
> > ClearDisplay();
> > return;
> > }
> > - ++dump_frames_composited_;
> > -
> > - if (active_composition_)
> > - active_composition_->SignalCompositionDone();
> > -
> > - ret = pthread_mutex_lock(&lock_);
> > - if (ret)
> > - ALOGE("Failed to acquire lock for active_composition swap");
> > -
> > - active_composition_.swap(composition);
> > -
> > - if (!ret)
> > - ret = pthread_mutex_unlock(&lock_);
> > - if (ret)
> > - ALOGE("Failed to release lock for active_composition swap");
> > }
> >
> > int DrmDisplayCompositor::ApplyComposition(
> > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> > index 0f8daad..b35ef70 100644
> > --- a/drmdisplaycompositor.h
> > +++ b/drmdisplaycompositor.h
> > @@ -127,7 +127,7 @@ class DrmDisplayCompositor {
> >
> > void ClearDisplay();
> > void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
> > - int status);
> > + int status, bool writeback = false);
> >
> > std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
> >
> > --
> > 2.7.4
> >
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
--
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
