diff mbox

[hwc,v2,12/18] drm_hwcomposer: Add utility function to create an initialized composition

Message ID 1523460149-1740-13-git-send-email-alexandru-cosmin.gheorghe@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alexandru-Cosmin Gheorghe April 11, 2018, 3:22 p.m. UTC 
There is a lot of boilerplate for creating an initialized
drmdisplaycomposition. This patch gathers that in a separate method.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmdisplaycompositor.cpp | 23 +++++++++++++++++++++++
 drmdisplaycompositor.h   |  2 ++
 2 files changed, 25 insertions(+)

Comments

Sean Paul April 17, 2018, 4:37 p.m. UTC | #1 
On Wed, Apr 11, 2018 at 04:22:23PM +0100, Alexandru Gheorghe wrote:
> There is a lot of boilerplate for creating an initialized
> drmdisplaycomposition. This patch gathers that in a separate method.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmdisplaycompositor.cpp | 23 +++++++++++++++++++++++
>  drmdisplaycompositor.h   |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index e556e86..6e5be24 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -221,6 +221,7 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
>      ALOGE("Failed to initialize drm compositor lock %d\n", ret);
>      return ret;
>    }
> +  planner_ = Planner::CreateInstance(drm);

What's this?

>  
>    initialized_ = true;
>    return 0;
> @@ -231,6 +232,28 @@ std::unique_ptr<DrmDisplayComposition> DrmDisplayCompositor::CreateComposition()
>    return std::unique_ptr<DrmDisplayComposition>(new DrmDisplayComposition());
>  }
>  
> +std::unique_ptr<DrmDisplayComposition>
> +DrmDisplayCompositor::CreateInitializedComposition() const {
> +  DrmCrtc *crtc = drm_->GetCrtcForDisplay(display_);
> +  if (!crtc) {
> +    ALOGE("Failed to find crtc for display = %d", display_);
> +    return std::unique_ptr<DrmDisplayComposition>();
> +  }
> +  std::unique_ptr<DrmDisplayComposition> comp = CreateComposition();
> +  std::shared_ptr<Importer> importer =
> +      drm_->resource_manager()->GetImporter(display_);
> +  if (!importer) {
> +    ALOGE("Failed to find resources for display = %d", display_);
> +    return std::unique_ptr<DrmDisplayComposition>();
> +  }
> +  int ret = comp->Init(drm_, crtc, importer.get(), planner_.get(), 0);
> +  if (ret) {
> +    ALOGE("Failed to init composition for display = %d", display_);
> +    return std::unique_ptr<DrmDisplayComposition>();
> +  }
> +  return comp;
> +}
> +

This seems sufficiently small that you can squash it into the patch that uses
it. The same can be said for some of the other "Add function to do X" which
don't use the function.

>  std::tuple<uint32_t, uint32_t, int>
>  DrmDisplayCompositor::GetActiveModeResolution() {
>    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> index f1965fb..ccaffb4 100644
> --- a/drmdisplaycompositor.h
> +++ b/drmdisplaycompositor.h
> @@ -87,6 +87,7 @@ class DrmDisplayCompositor {
>    int Init(DrmResources *drm, int display);
>  
>    std::unique_ptr<DrmDisplayComposition> CreateComposition() const;
> +  std::unique_ptr<DrmDisplayComposition> CreateInitializedComposition() const;
>    int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition);
>    int Composite();
>    int SquashAll();
> @@ -155,6 +156,7 @@ class DrmDisplayCompositor {
>    // we need to reset them on every Dump() call.
>    mutable uint64_t dump_frames_composited_;
>    mutable uint64_t dump_last_timestamp_ns_;
> +  std::unique_ptr<Planner> planner_;
>  };
>  }
>  
> -- 
> 2.7.4
>
Alexandru-Cosmin Gheorghe April 18, 2018, 10:29 a.m. UTC | #2 
On Tue, Apr 17, 2018 at 12:37:15PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:23PM +0100, Alexandru Gheorghe wrote:
> > There is a lot of boilerplate for creating an initialized
> > drmdisplaycomposition. This patch gathers that in a separate method.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drmdisplaycompositor.cpp | 23 +++++++++++++++++++++++
> >  drmdisplaycompositor.h   |  2 ++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> > index e556e86..6e5be24 100644
> > --- a/drmdisplaycompositor.cpp
> > +++ b/drmdisplaycompositor.cpp
> > @@ -221,6 +221,7 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
> >      ALOGE("Failed to initialize drm compositor lock %d\n", ret);
> >      return ret;
> >    }
> > +  planner_ = Planner::CreateInstance(drm);
> 
> What's this?

We need a planner for the function bellow, I could re-write this to be
an argument of CreateComposition if you think that helps.

> 
> >  
> >    initialized_ = true;
> >    return 0;
> > @@ -231,6 +232,28 @@ std::unique_ptr<DrmDisplayComposition> DrmDisplayCompositor::CreateComposition()
> >    return std::unique_ptr<DrmDisplayComposition>(new DrmDisplayComposition());
> >  }
> >  
> > +std::unique_ptr<DrmDisplayComposition>
> > +DrmDisplayCompositor::CreateInitializedComposition() const {
> > +  DrmCrtc *crtc = drm_->GetCrtcForDisplay(display_);
> > +  if (!crtc) {
> > +    ALOGE("Failed to find crtc for display = %d", display_);
> > +    return std::unique_ptr<DrmDisplayComposition>();
> > +  }
> > +  std::unique_ptr<DrmDisplayComposition> comp = CreateComposition();
> > +  std::shared_ptr<Importer> importer =
> > +      drm_->resource_manager()->GetImporter(display_);
> > +  if (!importer) {
> > +    ALOGE("Failed to find resources for display = %d", display_);
> > +    return std::unique_ptr<DrmDisplayComposition>();
> > +  }
> > +  int ret = comp->Init(drm_, crtc, importer.get(), planner_.get(), 0);
> > +  if (ret) {
> > +    ALOGE("Failed to init composition for display = %d", display_);
> > +    return std::unique_ptr<DrmDisplayComposition>();
> > +  }
> > +  return comp;
> > +}
> > +
> 
> This seems sufficiently small that you can squash it into the patch that uses
> it. The same can be said for some of the other "Add function to do X" which
> don't use the function.
> 
> >  std::tuple<uint32_t, uint32_t, int>
> >  DrmDisplayCompositor::GetActiveModeResolution() {
> >    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
> > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> > index f1965fb..ccaffb4 100644
> > --- a/drmdisplaycompositor.h
> > +++ b/drmdisplaycompositor.h
> > @@ -87,6 +87,7 @@ class DrmDisplayCompositor {
> >    int Init(DrmResources *drm, int display);
> >  
> >    std::unique_ptr<DrmDisplayComposition> CreateComposition() const;
> > +  std::unique_ptr<DrmDisplayComposition> CreateInitializedComposition() const;
> >    int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition);
> >    int Composite();
> >    int SquashAll();
> > @@ -155,6 +156,7 @@ class DrmDisplayCompositor {
> >    // we need to reset them on every Dump() call.
> >    mutable uint64_t dump_frames_composited_;
> >    mutable uint64_t dump_last_timestamp_ns_;
> > +  std::unique_ptr<Planner> planner_;
> >  };
> >  }
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
diff mbox

Patch

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index e556e86..6e5be24 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -221,6 +221,7 @@  int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
     ALOGE("Failed to initialize drm compositor lock %d\n", ret);
     return ret;
   }
+  planner_ = Planner::CreateInstance(drm);
 
   initialized_ = true;
   return 0;
@@ -231,6 +232,28 @@  std::unique_ptr<DrmDisplayComposition> DrmDisplayCompositor::CreateComposition()
   return std::unique_ptr<DrmDisplayComposition>(new DrmDisplayComposition());
 }
 
+std::unique_ptr<DrmDisplayComposition>
+DrmDisplayCompositor::CreateInitializedComposition() const {
+  DrmCrtc *crtc = drm_->GetCrtcForDisplay(display_);
+  if (!crtc) {
+    ALOGE("Failed to find crtc for display = %d", display_);
+    return std::unique_ptr<DrmDisplayComposition>();
+  }
+  std::unique_ptr<DrmDisplayComposition> comp = CreateComposition();
+  std::shared_ptr<Importer> importer =
+      drm_->resource_manager()->GetImporter(display_);
+  if (!importer) {
+    ALOGE("Failed to find resources for display = %d", display_);
+    return std::unique_ptr<DrmDisplayComposition>();
+  }
+  int ret = comp->Init(drm_, crtc, importer.get(), planner_.get(), 0);
+  if (ret) {
+    ALOGE("Failed to init composition for display = %d", display_);
+    return std::unique_ptr<DrmDisplayComposition>();
+  }
+  return comp;
+}
+
 std::tuple<uint32_t, uint32_t, int>
 DrmDisplayCompositor::GetActiveModeResolution() {
   DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index f1965fb..ccaffb4 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -87,6 +87,7 @@  class DrmDisplayCompositor {
   int Init(DrmResources *drm, int display);
 
   std::unique_ptr<DrmDisplayComposition> CreateComposition() const;
+  std::unique_ptr<DrmDisplayComposition> CreateInitializedComposition() const;
   int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition);
   int Composite();
   int SquashAll();
@@ -155,6 +156,7 @@  class DrmDisplayCompositor {
   // we need to reset them on every Dump() call.
   mutable uint64_t dump_frames_composited_;
   mutable uint64_t dump_last_timestamp_ns_;
+  std::unique_ptr<Planner> planner_;
 };
 }