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

Commit Message

Alexandru-Cosmin Gheorghe April 11, 2018, 3:22 p.m. UTC

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 <[e-mail]>
---
 drmdisplaycompositor.cpp | 40 +++++++++++++++++-----------------------
 drmdisplaycompositor.h   |  2 +-
 2 files changed, 18 insertions(+), 24 deletions(-)

Comments

Sean Paul April 17, 2018, 5:02 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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.

> +  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?

> +    }
> +  }
>  
>    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
>

Alexandru-Cosmin Gheorghe April 18, 2018, 10:43 a.m. UTC | #2

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 <[e-mail]>
> > ---
> >  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

Patch

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) {
+  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);
+    }
+  }
 
   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);