Re: [PATCH hwc v2 07/18] drm_hwcomposer: Add display field to Drmencoder

[Alexandru-Cosmin Gheorghe]

Hi Sean,

On Mon, Apr 16, 2018 at 04:02:07PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:18PM +0100, Alexandru Gheorghe wrote:
> > In the current implementation TryEncoderForDisplay just looks
> > at the crtc linked to the display, if that's not assigned to
> > a display it means the encoder could be used, otherwise iterate
> > to the list of possible_crtcs and find one which is not used.
> > 
> > This logic works fine when you have just one encoder connected to a
> > crtc but with two or more, like is the case when we attach a writeback
> > connector, we need to know if we already assigned the encoder to a
> > display.
> > 
> > Signed-off-by: Alexandru Gheorghe 
> > ---
> >  drmencoder.cpp | 14 ++
> >  drmencoder.h   |  4 
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/drmencoder.cpp b/drmencoder.cpp
> > index 3d762f3..1da7ec3 100644
> > --- a/drmencoder.cpp
> > +++ b/drmencoder.cpp
> > @@ -27,6 +27,7 @@ DrmEncoder::DrmEncoder(drmModeEncoderPtr e, DrmCrtc 
> > *current_crtc,
> > const std::vector _crtcs)
> >  : id_(e->encoder_id),
> >crtc_(current_crtc),
> > +  display_(-1),
> >possible_crtcs_(possible_crtcs) {
> >  }
> >  
> > @@ -40,5 +41,18 @@ DrmCrtc *DrmEncoder::crtc() const {
> >  
> >  void DrmEncoder::set_crtc(DrmCrtc *crtc) {
> >crtc_ = crtc;
> > +  set_display(crtc->display());
> > +}
> > +
> > +int DrmEncoder::display() const {
> > +  return display_;
> > +}
> > +
> > +void DrmEncoder::set_display(int display) {
> > +  display_ = display;
> > +}
> 
> Instead of adding this, just call set_crtc() in TryEncoderForDisplay() for the
> already-bound case. That way we only have one entry point for this.

Fair enough, I will remove set_display.

> 
> > +
> > +bool DrmEncoder::can_bind(int display) const {
> > +  return display_ == -1 || display_ == display;
> >  }
> >  }
> > diff --git a/drmencoder.h b/drmencoder.h
> > index 58ccbfb..7e06691 100644
> > --- a/drmencoder.h
> > +++ b/drmencoder.h
> > @@ -36,6 +36,9 @@ class DrmEncoder {
> >  
> >DrmCrtc *crtc() const;
> >void set_crtc(DrmCrtc *crtc);
> > +  bool can_bind(int display) const;
> > +  void set_display(int display);
> > +  int display() const;
> >  
> >const std::vector _crtcs() const {
> >  return possible_crtcs_;
> > @@ -44,6 +47,7 @@ class DrmEncoder {
> >   private:
> >uint32_t id_;
> >DrmCrtc *crtc_;
> > +  int display_;
> >  
> >std::vector possible_crtcs_;
> >  };
> > -- 
> > 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

Re: [PATCH hwc v2 07/18] drm_hwcomposer: Add display field to Drmencoder

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:18PM +0100, Alexandru Gheorghe wrote:
> In the current implementation TryEncoderForDisplay just looks
> at the crtc linked to the display, if that's not assigned to
> a display it means the encoder could be used, otherwise iterate
> to the list of possible_crtcs and find one which is not used.
> 
> This logic works fine when you have just one encoder connected to a
> crtc but with two or more, like is the case when we attach a writeback
> connector, we need to know if we already assigned the encoder to a
> display.
> 
> Signed-off-by: Alexandru Gheorghe 
> ---
>  drmencoder.cpp | 14 ++
>  drmencoder.h   |  4 
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drmencoder.cpp b/drmencoder.cpp
> index 3d762f3..1da7ec3 100644
> --- a/drmencoder.cpp
> +++ b/drmencoder.cpp
> @@ -27,6 +27,7 @@ DrmEncoder::DrmEncoder(drmModeEncoderPtr e, DrmCrtc 
> *current_crtc,
> const std::vector _crtcs)
>  : id_(e->encoder_id),
>crtc_(current_crtc),
> +  display_(-1),
>possible_crtcs_(possible_crtcs) {
>  }
>  
> @@ -40,5 +41,18 @@ DrmCrtc *DrmEncoder::crtc() const {
>  
>  void DrmEncoder::set_crtc(DrmCrtc *crtc) {
>crtc_ = crtc;
> +  set_display(crtc->display());
> +}
> +
> +int DrmEncoder::display() const {
> +  return display_;
> +}
> +
> +void DrmEncoder::set_display(int display) {
> +  display_ = display;
> +}

Instead of adding this, just call set_crtc() in TryEncoderForDisplay() for the
already-bound case. That way we only have one entry point for this.

> +
> +bool DrmEncoder::can_bind(int display) const {
> +  return display_ == -1 || display_ == display;
>  }
>  }
> diff --git a/drmencoder.h b/drmencoder.h
> index 58ccbfb..7e06691 100644
> --- a/drmencoder.h
> +++ b/drmencoder.h
> @@ -36,6 +36,9 @@ class DrmEncoder {
>  
>DrmCrtc *crtc() const;
>void set_crtc(DrmCrtc *crtc);
> +  bool can_bind(int display) const;
> +  void set_display(int display);
> +  int display() const;
>  
>const std::vector _crtcs() const {
>  return possible_crtcs_;
> @@ -44,6 +47,7 @@ class DrmEncoder {
>   private:
>uint32_t id_;
>DrmCrtc *crtc_;
> +  int display_;
>  
>std::vector possible_crtcs_;
>  };
> -- 
> 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