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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx