On Tue, 2017-12-05 at 15:59 +0200, Ville Syrjälä wrote:
> On Tue, Dec 05, 2017 at 12:15:39PM +0200, Mika Kahola wrote:
> >
> > crtc_mask is defined explicitly defined for a certain number of
> > pipes per
> > platform. Let's generalize this in a way that crtc_mask dependens
> > only on
> > the number of pipes defined in device info.
> >
> > Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_crt.c | 9 ++++++---
> > drivers/gpu/drm/i915/intel_ddi.c | 5 ++++-
> > drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++----
> > drivers/gpu/drm/i915/intel_hdmi.c | 4 +++-
> > drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++-----
> > drivers/gpu/drm/i915/intel_sdvo.c | 5 ++++-
> > drivers/gpu/drm/i915/intel_tv.c | 6 +++++-
> > 7 files changed, 37 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c
> > b/drivers/gpu/drm/i915/intel_crt.c
> > index 9f31aea..34f65b5 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -903,6 +903,7 @@ void intel_crt_init(struct drm_i915_private
> > *dev_priv)
> > struct intel_connector *intel_connector;
> > i915_reg_t adpa_reg;
> > u32 adpa;
> > + enum pipe pipe;
> >
> > if (HAS_PCH_SPLIT(dev_priv))
> > adpa_reg = PCH_ADPA;
> > @@ -950,10 +951,12 @@ void intel_crt_init(struct drm_i915_private
> > *dev_priv)
> >
> > crt->base.type = INTEL_OUTPUT_ANALOG;
> > crt->base.cloneable = (1 << INTEL_OUTPUT_DVO) | (1 <<
> > INTEL_OUTPUT_HDMI);
> > - if (IS_I830(dev_priv))
> > + if (IS_I830(dev_priv)) {
> > crt->base.crtc_mask = (1 << 0);
> > - else
> > - crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 <<
> > 2);
> > + } else {
> > + for_each_pipe(dev_priv, pipe)
> > + crt->base.crtc_mask |= (1 << pipe);
> > + }
> The only places you have to touch are DDI and MST. None of the other
> encoder types are relevant for new platforms at all.
That's true. For these newer platforms I wouldn't have needed to touch
other encoder types but I decided to go that road due to consistency. I
may drop these for the next iteration of the patch.
>
> Looks like you actually missed MST in this patch, and it looks like
> the
> code there is just wrong even now. It should really just set
> 'crtc_mask = BIT(pipe)' since the fake mst encoders are pipe
> specific.
Ouch, I completely missed the MST part.
>
> In fact I think what we should do is have a small function that
> filters
> out the non-existent pipes from the crtc_mask when populating
> encoder->possible_crtcs. And I wouldn't be opposed to
> s/1<<0/BIT(PIPE_A)/
> etc. everywhere we populate crtc_mask (maybe even
> s/crtc_mask/pipe_mask/
> to make it clear what we're talking about).
I'll test this small helper function and see what the outcome looks
like.
Thanks for the review!
>
> And maybe actually get rid of crtc_mask entirely and just populate
> possible_crtcs directly (with the help of aforementioned filtering
> function).
>
> Possibly some igt would be nice to confirm that possibly_crtcs etc.
> don't advertize invalid crtc indices.
>
--
Mika Kahola - Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
