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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 05, 2017 at 03:59:35PM +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.
> 
> 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.
> 
> 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).
> 
> 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.

+1 on a generic (i.e. DRIVER_ANY) igt that validates the various
possible_* masks. Lots of drivers e.g. don't set anything, hooray.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx