[hwc,v2,6/6] drm_hwcomposer: Add out-fence support
diff mbox

Message ID 20170927115841.29134-7-robert.foss@collabora.com
State New
Headers show

Commit Message

Robert Foss Sept. 27, 2017, 11:58 a.m. UTC
Add support for out-fences through the OUT_FENCE_PTR property.
Out-fences signal when their associated buffer may be read by a device.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---

Changes since v1:
  Sergi Granell
    - Set atomic property to be out_fences[crtc->pipe()] not out_fences[0]

 drmdisplaycomposition.h  |  9 +++++++++
 drmdisplaycompositor.cpp | 16 ++++++++++++++++
 drmhwctwo.cpp            |  9 ++-------
 3 files changed, 27 insertions(+), 7 deletions(-)

Comments

Sean Paul Sept. 27, 2017, 7:11 p.m. UTC | #1
On Wed, Sep 27, 2017 at 7:58 AM, Robert Foss <robert.foss@collabora.com> wrote:
> Add support for out-fences through the OUT_FENCE_PTR property.
> Out-fences signal when their associated buffer may be read by a device.
>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>
> Changes since v1:
>   Sergi Granell
>     - Set atomic property to be out_fences[crtc->pipe()] not out_fences[0]
>
>  drmdisplaycomposition.h  |  9 +++++++++
>  drmdisplaycompositor.cpp | 16 ++++++++++++++++
>  drmhwctwo.cpp            |  9 ++-------
>  3 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drmdisplaycomposition.h b/drmdisplaycomposition.h
> index b165adc..0586d58 100644
> --- a/drmdisplaycomposition.h
> +++ b/drmdisplaycomposition.h
> @@ -189,6 +189,14 @@ class DrmDisplayComposition {
>      return planner_;
>    }
>
> +  int take_out_fence() {
> +    return out_fence_.Release();
> +  }
> +
> +  void set_out_fence(int out_fence) {
> +    out_fence_.Set(dup(out_fence));

Why dup if you're just going to close the original? I think the helper
functions actually hurt you here. It would be easier to understand
what was going on if you just manipulated out_fence_ directly in
CommitFrame (then you wouldn't need the dup/close).

> +  }
> +
>    void Dump(std::ostringstream *out) const;
>
>   private:
> @@ -215,6 +223,7 @@ class DrmDisplayComposition {
>    int timeline_current_ = 0;
>    int timeline_squash_done_ = 0;
>    int timeline_pre_comp_done_ = 0;
> +  UniqueFd out_fence_ = -1;
>
>    bool geometry_changed_;
>    std::vector<DrmHwcLayer> layers_;
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index 71c0451..a1427d3 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -492,6 +492,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>        display_comp->composition_planes();
>    std::vector<DrmCompositionRegion> &pre_comp_regions =
>        display_comp->pre_comp_regions();
> +  uint64_t out_fences[drm_->GetCrtcCount()];

Huh. I didn't know you could do this.

>
>    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
>    if (!connector) {
> @@ -510,6 +511,16 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>      return -ENOMEM;
>    }
>
> +  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()]);
> +    if (ret < 0) {
> +      ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret);
> +      drmModeAtomicFree(pset);
> +      return ret;
> +    }
> +  }
> +
>    if (mode_.needs_modeset) {
>      ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->mode_property().id(),
>                                     mode_.blob_id) < 0 ||
> @@ -708,6 +719,11 @@ out:
>      mode_.needs_modeset = false;
>    }
>
> +  if (crtc->out_fence_ptr_property().id()) {
> +    display_comp->set_out_fence((int) out_fences[crtc->pipe()]);
> +    close((int) out_fences[crtc->pipe()]);
> +  }
> +
>    return ret;
>  }
>
> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> index 762ee8c..89399bf 100644
> --- a/drmhwctwo.cpp
> +++ b/drmhwctwo.cpp
> @@ -557,19 +557,14 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>      i = overlay_planes.erase(i);
>    }
>
> +  AddFenceToRetireFence(composition->take_out_fence());
> +
>    ret = compositor_.ApplyComposition(std::move(composition));
>    if (ret) {
>      ALOGE("Failed to apply the frame composition ret=%d", ret);
>      return HWC2::Error::BadParameter;
>    }
>
> -  // Now that the release fences have been generated by the compositor, make
> -  // sure they're managed properly
> -  for (std::pair<const uint32_t, DrmHwcTwo::HwcLayer *> &l : z_map) {
> -    l.second->manage_release_fence();
> -    AddFenceToRetireFence(l.second->release_fence());
> -  }
> -
>    // The retire fence returned here is for the last frame, so return it and
>    // promote the next retire fence
>    *retire_fence = retire_fence_.Release();
> --
> 2.11.0
>
Robert Foss Sept. 28, 2017, 4:21 p.m. UTC | #2
On Wed, 2017-09-27 at 15:11 -0400, Sean Paul wrote:
> On Wed, Sep 27, 2017 at 7:58 AM, Robert Foss <robert.foss@collabora.c
> om> wrote:
> > Add support for out-fences through the OUT_FENCE_PTR property.
> > Out-fences signal when their associated buffer may be read by a
> > device.
> > 
> > Signed-off-by: Robert Foss <robert.foss@collabora.com>
> > ---
> > 
> > Changes since v1:
> >   Sergi Granell
> >     - Set atomic property to be out_fences[crtc->pipe()] not
> > out_fences[0]
> > 
> >  drmdisplaycomposition.h  |  9 +++++++++
> >  drmdisplaycompositor.cpp | 16 ++++++++++++++++
> >  drmhwctwo.cpp            |  9 ++-------
> >  3 files changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drmdisplaycomposition.h b/drmdisplaycomposition.h
> > index b165adc..0586d58 100644
> > --- a/drmdisplaycomposition.h
> > +++ b/drmdisplaycomposition.h
> > @@ -189,6 +189,14 @@ class DrmDisplayComposition {
> >      return planner_;
> >    }
> > 
> > +  int take_out_fence() {
> > +    return out_fence_.Release();
> > +  }
> > +
> > +  void set_out_fence(int out_fence) {
> > +    out_fence_.Set(dup(out_fence));
> 
> Why dup if you're just going to close the original? I think the
> helper
> functions actually hurt you here. It would be easier to understand
> what was going on if you just manipulated out_fence_ directly in
> CommitFrame (then you wouldn't need the dup/close).

Yeah, that makes a lot of sense, and is a lot easier to read too.

> 
> > +  }
> > +
> >    void Dump(std::ostringstream *out) const;
> > 
> >   private:
> > @@ -215,6 +223,7 @@ class DrmDisplayComposition {
> >    int timeline_current_ = 0;
> >    int timeline_squash_done_ = 0;
> >    int timeline_pre_comp_done_ = 0;
> > +  UniqueFd out_fence_ = -1;
> > 
> >    bool geometry_changed_;
> >    std::vector<DrmHwcLayer> layers_;
> > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> > index 71c0451..a1427d3 100644
> > --- a/drmdisplaycompositor.cpp
> > +++ b/drmdisplaycompositor.cpp
> > @@ -492,6 +492,7 @@ int
> > DrmDisplayCompositor::CommitFrame(DrmDisplayComposition
> > *display_comp,
> >        display_comp->composition_planes();
> >    std::vector<DrmCompositionRegion> &pre_comp_regions =
> >        display_comp->pre_comp_regions();
> > +  uint64_t out_fences[drm_->GetCrtcCount()];
> 
> Huh. I didn't know you could do this.

C99 and variable length arrays man.
Progress at a glacial pace is still happening in C-land.


Rob.

Patch
diff mbox

diff --git a/drmdisplaycomposition.h b/drmdisplaycomposition.h
index b165adc..0586d58 100644
--- a/drmdisplaycomposition.h
+++ b/drmdisplaycomposition.h
@@ -189,6 +189,14 @@  class DrmDisplayComposition {
     return planner_;
   }
 
+  int take_out_fence() {
+    return out_fence_.Release();
+  }
+
+  void set_out_fence(int out_fence) {
+    out_fence_.Set(dup(out_fence));
+  }
+
   void Dump(std::ostringstream *out) const;
 
  private:
@@ -215,6 +223,7 @@  class DrmDisplayComposition {
   int timeline_current_ = 0;
   int timeline_squash_done_ = 0;
   int timeline_pre_comp_done_ = 0;
+  UniqueFd out_fence_ = -1;
 
   bool geometry_changed_;
   std::vector<DrmHwcLayer> layers_;
diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index 71c0451..a1427d3 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -492,6 +492,7 @@  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
       display_comp->composition_planes();
   std::vector<DrmCompositionRegion> &pre_comp_regions =
       display_comp->pre_comp_regions();
+  uint64_t out_fences[drm_->GetCrtcCount()];
 
   DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
   if (!connector) {
@@ -510,6 +511,16 @@  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
     return -ENOMEM;
   }
 
+  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()]);
+    if (ret < 0) {
+      ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret);
+      drmModeAtomicFree(pset);
+      return ret;
+    }
+  }
+
   if (mode_.needs_modeset) {
     ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->mode_property().id(),
                                    mode_.blob_id) < 0 ||
@@ -708,6 +719,11 @@  out:
     mode_.needs_modeset = false;
   }
 
+  if (crtc->out_fence_ptr_property().id()) {
+    display_comp->set_out_fence((int) out_fences[crtc->pipe()]);
+    close((int) out_fences[crtc->pipe()]);
+  }
+
   return ret;
 }
 
diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
index 762ee8c..89399bf 100644
--- a/drmhwctwo.cpp
+++ b/drmhwctwo.cpp
@@ -557,19 +557,14 @@  HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
     i = overlay_planes.erase(i);
   }
 
+  AddFenceToRetireFence(composition->take_out_fence());
+
   ret = compositor_.ApplyComposition(std::move(composition));
   if (ret) {
     ALOGE("Failed to apply the frame composition ret=%d", ret);
     return HWC2::Error::BadParameter;
   }
 
-  // Now that the release fences have been generated by the compositor, make
-  // sure they're managed properly
-  for (std::pair<const uint32_t, DrmHwcTwo::HwcLayer *> &l : z_map) {
-    l.second->manage_release_fence();
-    AddFenceToRetireFence(l.second->release_fence());
-  }
-
   // The retire fence returned here is for the last frame, so return it and
   // promote the next retire fence
   *retire_fence = retire_fence_.Release();