[PATCH hwc v2 08/18] drm_hwcomposer: Parse and store possible_clones information

[Alexandru-Cosmin Gheorghe]

Hi Sean,

On Mon, Apr 16, 2018 at 04:19:14PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:19PM +0100, Alexandru Gheorghe wrote:
> > drmModeEncoder has a field called possible_clones. It's a bit mask
> > which tells if the encoder could be simultaneously connected, to the
> > same CRTC, with the encoders specified in the possible_clones mask.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe at arm.com>
> > ---
> >  drmencoder.cpp   | 8 ++++++++
> >  drmencoder.h     | 4 ++++
> >  drmresources.cpp | 9 ++++++++-
> >  3 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drmencoder.cpp b/drmencoder.cpp
> > index 1da7ec3..ff675f5 100644
> > --- a/drmencoder.cpp
> > +++ b/drmencoder.cpp
> > @@ -39,6 +39,14 @@ DrmCrtc *DrmEncoder::crtc() const {
> >    return crtc_;
> >  }
> >  
> > +bool DrmEncoder::can_clone(DrmEncoder *encoder) {
> > +  return possible_clones_.find(encoder) != possible_clones_.end();
> > +}
> 
> The find() call is probably enough to justify CamelCase for this function. FTR,
> I _hate_ this part of the style guide and wish I had just picked one or the
> other.
> 
> To improve readability, can you also change the name of "encoder" to
> "possible_clone" like below so it's super obvious what this does?
> 

Sure, will do.

> > +
> > +void DrmEncoder::add_possible_clone(DrmEncoder *possible_clone) {
> > +  possible_clones_[possible_clone] = true;
> > +}
> > +
> >  void DrmEncoder::set_crtc(DrmCrtc *crtc) {
> >    crtc_ = crtc;
> >    set_display(crtc->display());
> > diff --git a/drmencoder.h b/drmencoder.h
> > index 7e06691..5e7c010 100644
> > --- a/drmencoder.h
> > +++ b/drmencoder.h
> > @@ -21,6 +21,7 @@
> >  
> >  #include <stdint.h>
> >  #include <vector>
> > +#include <map>
> 
> Alphabetical

Sure, wil do.

> 
> >  #include <xf86drmMode.h>
> >  
> >  namespace android {
> > @@ -43,6 +44,8 @@ class DrmEncoder {
> >    const std::vector<DrmCrtc *> &possible_crtcs() const {
> >      return possible_crtcs_;
> >    }
> > +  bool can_clone(DrmEncoder *encoder);
> > +  void add_possible_clone(DrmEncoder *possible_clone);
> >  
> >   private:
> >    uint32_t id_;
> > @@ -50,6 +53,7 @@ class DrmEncoder {
> >    int display_;
> >  
> >    std::vector<DrmCrtc *> possible_crtcs_;
> > +  std::map<DrmEncoder *, bool> possible_clones_;
> >  };
> >  }
> >  
> > diff --git a/drmresources.cpp b/drmresources.cpp
> > index a5ddda0..39f50be 100644
> > --- a/drmresources.cpp
> > +++ b/drmresources.cpp
> > @@ -97,6 +97,7 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
> >      crtcs_.emplace_back(std::move(crtc));
> >    }
> >  
> > +  std::vector<int> possible_clones;
> >    for (int i = 0; !ret && i < res->count_encoders; ++i) {
> >      drmModeEncoderPtr e = drmModeGetEncoder(fd(), res->encoders[i]);
> >      if (!e) {
> > @@ -117,12 +118,18 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
> >  
> >      std::unique_ptr<DrmEncoder> enc(
> >          new DrmEncoder(e, current_crtc, possible_crtcs));
> > -
> > +    possible_clones.push_back(e->possible_clones);
> >      drmModeFreeEncoder(e);
> >  
> >      encoders_.emplace_back(std::move(enc));
> >    }
> >  
> > +  for (uint32_t i = 0; i < encoders_.size(); i++) {
> > +    for (uint32_t j = 0; j < encoders_.size(); j++)
> 
> for (auto &enc: encoders_) {
>   for (auto &clone: encoders_) {
> 
> Or something similarly C++'y, looping through indices is sooo last decade :-)

Oldie but goldie. 
I do need the index in order to check the possible clones mask.
I will try to find something more millennial/Generation z :).

> 
> 
> > +      if (possible_clones[i] & (1 << j))
> > +        encoders_[i]->add_possible_clone(encoders_[j].get());
> > +  }
> > +
> >    for (int i = 0; !ret && i < res->count_connectors; ++i) {
> >      drmModeConnectorPtr c = drmModeGetConnector(fd(), res->connectors[i]);
> >      if (!c) {
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Cheers,
Alex G