Re: [PATCH] drm/i915: Generalize definition for crtc mask

[Mika Kahola <mika.kahola@intel.com>]

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@intel.com>
> > ---
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx