Re: [PATCH hwc v2 14/18] drm_hwcomposer: Fix race in ApplyFrame

[Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com>]

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@arm.com>
> > ---
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel