[Intel-gfx] [PATCH 16/19] drm/i915: Check lane sharing between pipes B & C using atomic state

[Konduru, Chandra]

> -----Original Message-----
> From: Conselvan De Oliveira, Ander
> Sent: Thursday, March 19, 2015 11:46 PM
> To: Konduru, Chandra
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 16/19] drm/i915: Check lane sharing between pipes B & C
> using atomic state
> 
> On Thu, 2015-03-19 at 20:58 +0000, Konduru, Chandra wrote:
> >
> > > -----Original Message-----
> > > From: Conselvan De Oliveira, Ander
> > > Sent: Friday, March 13, 2015 2:49 AM
> > > To: intel-gfx at lists.freedesktop.org
> > > Cc: Konduru, Chandra; Conselvan De Oliveira, Ander
> > > Subject: [PATCH 16/19] drm/i915: Check lane sharing between pipes B
> > > & C using atomic state
> > >
> > > Makes that code atomic ready.
> > >
> > > Signed-off-by: Ander Conselvan de Oliveira
> > > <ander.conselvan.de.oliveira at 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 e720a48..8c97186 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5537,13 +5537,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;
> > > +	}
> > >
> > > -	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;
> > > @@ -5552,6 +5559,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) {
> > > @@ -5579,7 +5588,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;
> > > @@ -5591,7 +5600,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;
> > >  		}
> > > @@ -5601,15 +5610,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;
> > > +}
> > > +
> > >  #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);
> >
> > In this scenario, crtc_states are created for both pipe B & C as an
> > operation on one can affect the other. I may be missing something
> > here, but where is the other crtc_state being used: compute/check flow
> > and/or commit flow?
> 
> The function pipe_required_fdi_lanes() above is changed in this patch to use the
> crtc_state in the drm atomic state to determined FDI lane availability. At this
> point we don't yet allow changes to multiple pipes, so the mode set is rejected if
> the pipe that is not being mode set uses too many lanes.

If request requires too many lanes then mode set is rejected, that is fine.
But my query is when the request requires lanes that can be supported.
In that case where the other crtc_state is being used?

> 
> Ander
> 
> >
> > > +		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
> >