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

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Sun, 2015-03-22 at 16:20 +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Conselvan De Oliveira, Ander
> > Sent: Thursday, March 19, 2015 11:46 PM
> > To: Konduru, Chandra
> > Cc: intel-gfx@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@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@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?

Same place, pipe_required_fdi_lanes() called from
ironlake_check_fdi_lanes().

Ander


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx