Re: [PATCH 16/20] drm/i915: Check lane sharing between pipes B & C using atomic state

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Mar 20, 2015 at 04:18:16PM +0200, Ander Conselvan de Oliveira wrote:
> Makes that code atomic ready.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 49 ++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 19e8928..abfb7a9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5536,13 +5536,20 @@ bool intel_connector_get_hw_state(struct intel_connector *connector)
>  	return encoder->get_hw_state(encoder, &pipe);
>  }
>  
> -static int pipe_required_fdi_lanes(struct drm_device *dev, enum pipe pipe)
> +static int pipe_required_fdi_lanes(struct drm_atomic_state *state,
> +				   enum pipe pipe)
>  {
>  	struct intel_crtc *crtc =
> -		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> +		to_intel_crtc(intel_get_crtc_for_pipe(state->dev, pipe));
> +	struct intel_crtc_state *crtc_state;
> +
> +	crtc_state = intel_atomic_get_crtc_state(state, crtc);
> +	if (WARN_ON(IS_ERR(crtc_state))) {
> +		/* Cause modeset to fail due to excess lanes. */
> +		return 5;
> +	}

Why that? If you can't get at the state it simply might be due to ww-mutex
deadlock. The correct way to handle that is to pass the error all down the
callchain. Atm this isn't a problem since we always grab all locks, but
without this error passing the code isn't strictly atomic ready yet.


>  
> -	if (crtc->base.state->enable &&
> -	    crtc->config->has_pch_encoder)
> +	if (crtc_state->base.enable && crtc_state->has_pch_encoder)
>  		return crtc->config->fdi_lanes;
>  
>  	return 0;
> @@ -5551,6 +5558,8 @@ static int pipe_required_fdi_lanes(struct drm_device *dev, enum pipe pipe)
>  static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
>  				     struct intel_crtc_state *pipe_config)
>  {
> +	struct drm_atomic_state *state = pipe_config->base.state;
> +
>  	DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n",
>  		      pipe_name(pipe), pipe_config->fdi_lanes);
>  	if (pipe_config->fdi_lanes > 4) {
> @@ -5578,7 +5587,7 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
>  		return true;
>  	case PIPE_B:
>  		if (pipe_config->fdi_lanes > 2 &&
> -		    pipe_required_fdi_lanes(dev, PIPE_C) > 0) {
> +		    pipe_required_fdi_lanes(state, PIPE_C) > 0) {
>  			DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %c: %i lanes\n",
>  				      pipe_name(pipe), pipe_config->fdi_lanes);
>  			return false;
> @@ -5590,7 +5599,7 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
>  				      pipe_name(pipe), pipe_config->fdi_lanes);
>  			return false;
>  		}
> -		if (pipe_required_fdi_lanes(dev, PIPE_B) > 2) {
> +		if (pipe_required_fdi_lanes(state, PIPE_B) > 2) {
>  			DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n");
>  			return false;
>  		}
> @@ -5600,15 +5609,41 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
>  	}
>  }
>  
> +static int add_pipe_b_c_to_state(struct drm_atomic_state *state)
> +{
> +	struct intel_crtc *pipe_B =
> +		to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_B));
> +	struct intel_crtc *pipe_C =
> +		to_intel_crtc(intel_get_crtc_for_pipe(state->dev, PIPE_C));
> +	struct intel_crtc_state *crtc_state;
> +
> +	crtc_state = intel_atomic_get_crtc_state(state, pipe_B);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	crtc_state = intel_atomic_get_crtc_state(state, pipe_C);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	return 0;
> +}

Ah maybe I should read on with the code first ;-)

Imo for atomic check functions it's better to grab additional state
objects late and wire the error code throughs. Two benefits for that
approach over yours here where you grab needed states upfront:
- You only acquire locks when really needed - e.g. on ivb we only need the
  other states for pipes B&C.
- There's no split between where you acquire a state object and where you
  really need it, hence no need for WARN_ON and friends to make the code
  robust.

The downside is ofc that you need to thread the error code through
callchains. But you also need to thread the -EINVAL back, so ime from
writing atomic helpers it's in practice not more work.

I'll punt on this patch for now until we've discussed this a bit.
-Daniel

> +
>  #define RETRY 1
>  static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
>  				       struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> -	int lane, link_bw, fdi_dotclock;
> +	int lane, link_bw, fdi_dotclock, ret;
>  	bool setup_ok, needs_recompute = false;
>  
> +	if (IS_IVYBRIDGE(dev) &&
> +	    (intel_crtc->pipe == PIPE_B || intel_crtc->pipe == PIPE_C)) {
> +		ret = add_pipe_b_c_to_state(pipe_config->base.state);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  retry:
>  	/* FDI is a binary signal running at ~2.7GHz, encoding
>  	 * each output octet as 10 bits. The actual frequency
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx