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

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

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

-- 
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel