Re: [PATCH] drm/i915: consolidate and tighten encoder cloning checks

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 30, 2013 at 03:04:25PM +0200, Daniel Vetter wrote:
> Only lvds/tv did actually check for cloning or not, but many more
> places should.
> 
> Notices because my ivb tried to enable both cpu edp and vga on the
> first crtc - the resulting confusion between has_pch_encoder,
> has_dp_encoder but not actually being a pch dp encoder resulting in
> hilarity (hitting a BUG).
> 
> We _really_ need an igt to random-walk our modeset space more
> exhaustively.
> 
> I haven't figured out yet why exactly we see this configuration
> request from the setcrtc ioctl, but I can reliably reproduce it by
> unplugging a bunch of connectors and restarting X.
> 
> Strangely restarting X again fixes things ...
> 
> v2: Kill intel_encoder_check_is_cloned, spotted by Paulo.
> 
> v3: Drop the now unused pipe param.
> 
> v4: Kill the stray printk Chris spotted.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Patch merged to dinq (imo a bit late for -fixes) with Chris' r-b.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c | 64 ++++++++++++++----------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_lvds.c    |  3 --
>  drivers/gpu/drm/i915/intel_tv.c      |  3 --
>  4 files changed, 24 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 14bb844..ba33a82 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5843,27 +5843,9 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int pipe = intel_crtc->pipe;
>  	int plane = intel_crtc->plane;
> -	int num_connectors = 0;
> -	bool is_cpu_edp = false;
> -	struct intel_encoder *encoder;
>  	int ret;
>  
> -	for_each_encoder_on_crtc(dev, crtc, encoder) {
> -		switch (encoder->type) {
> -		case INTEL_OUTPUT_EDP:
> -			if (enc_to_dig_port(&encoder->base)->port == PORT_A)
> -				is_cpu_edp = true;
> -			break;
> -		}
> -
> -		num_connectors++;
> -	}
> -
> -	WARN(num_connectors != 1, "%d connectors attached to pipe %c\n",
> -	     num_connectors, pipe_name(pipe));
> -
>  	if (!intel_ddi_pll_mode_set(crtc))
>  		return -EINVAL;
>  
> @@ -7523,28 +7505,6 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = {
>  	.load_lut = intel_crtc_load_lut,
>  };
>  
> -bool intel_encoder_check_is_cloned(struct intel_encoder *encoder)
> -{
> -	struct intel_encoder *other_encoder;
> -	struct drm_crtc *crtc = &encoder->new_crtc->base;
> -
> -	if (WARN_ON(!crtc))
> -		return false;
> -
> -	list_for_each_entry(other_encoder,
> -			    &crtc->dev->mode_config.encoder_list,
> -			    base.head) {
> -
> -		if (&other_encoder->new_crtc->base != crtc ||
> -		    encoder == other_encoder)
> -			continue;
> -		else
> -			return true;
> -	}
> -
> -	return false;
> -}
> -
>  static bool intel_encoder_crtc_ok(struct drm_encoder *encoder,
>  				  struct drm_crtc *crtc)
>  {
> @@ -7713,6 +7673,25 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  		      pipe_config->pch_pfit.size);
>  }
>  
> +static bool check_encoder_cloning(struct drm_crtc *crtc)
> +{
> +	int num_encoders = 0;
> +	bool uncloneable_encoders = false;
> +	struct intel_encoder *encoder;
> +
> +	list_for_each_entry(encoder, &crtc->dev->mode_config.encoder_list,
> +			    base.head) {
> +		if (&encoder->new_crtc->base != crtc)
> +			continue;
> +
> +		num_encoders++;
> +		if (!encoder->cloneable)
> +			uncloneable_encoders = true;
> +	}
> +
> +	return !(num_encoders > 1 && uncloneable_encoders);
> +}
> +
>  static struct intel_crtc_config *
>  intel_modeset_pipe_config(struct drm_crtc *crtc,
>  			  struct drm_framebuffer *fb,
> @@ -7725,6 +7704,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	int plane_bpp, ret = -EINVAL;
>  	bool retry = true;
>  
> +	if (!check_encoder_cloning(crtc)) {
> +		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
>  	if (!pipe_config)
>  		return ERR_PTR(-ENOMEM);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3db4b14..a844a05 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -627,7 +627,6 @@ extern void intel_crtc_load_lut(struct drm_crtc *crtc);
>  extern void intel_crtc_update_dpms(struct drm_crtc *crtc);
>  extern void intel_encoder_destroy(struct drm_encoder *encoder);
>  extern void intel_encoder_dpms(struct intel_encoder *encoder, int mode);
> -extern bool intel_encoder_check_is_cloned(struct intel_encoder *encoder);
>  extern void intel_connector_dpms(struct drm_connector *, int mode);
>  extern bool intel_connector_get_hw_state(struct intel_connector *connector);
>  extern void intel_modeset_check_state(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 6554860..10c3d56 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -264,9 +264,6 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  		return false;
>  	}
>  
> -	if (intel_encoder_check_is_cloned(&lvds_encoder->base))
> -		return false;
> -
>  	if ((I915_READ(lvds_encoder->reg) & LVDS_A3_POWER_MASK) ==
>  	    LVDS_A3_POWER_UP)
>  		lvds_bpp = 8*3;
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 7d11a5a..39debd8 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -914,9 +914,6 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>  	if (!tv_mode)
>  		return false;
>  
> -	if (intel_encoder_check_is_cloned(&intel_tv->base))
> -		return false;
> -
>  	pipe_config->adjusted_mode.clock = tv_mode->clock;
>  	DRM_DEBUG_KMS("forcing bpc to 8 for TV\n");
>  	pipe_config->pipe_bpp = 8*3;
> -- 
> 1.7.11.7
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch