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

[Sean Paul <seanpaul@chromium.org>]

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.

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel