Re: [PATCH 07/19] drm/i915: Copy the staged connector config to the legacy atomic state

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

On Thu, 2015-03-19 at 00:38 +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 07/19] drm/i915: Copy the staged connector config to the
> > legacy atomic state
> > 
> > With this in place, we can start converting pieces of the modeset code to look at
> > the connector atomic state instead of the staged config.
> > 
> > v2: Handle the load detect staged config changes too. (Ander)
> >     Remove unnecessary blank line. (Daniel)
> > 
> > Signed-off-by: Ander Conselvan de Oliveira
> > <ander.conselvan.de.oliveira@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 52
> > +++++++++++++++++++++++++++++++-----
> >  1 file changed, 45 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index abdbd0c..6465f6d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8805,6 +8805,7 @@ bool intel_get_load_detect_pipe(struct
> > drm_connector *connector,
> >  	struct drm_framebuffer *fb;
> >  	struct drm_mode_config *config = &dev->mode_config;
> >  	struct drm_atomic_state *state = NULL;
> > +	struct drm_connector_state *connector_state;
> >  	int ret, i = -1;
> > 
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", @@
> > -8892,6 +8893,15 @@ retry:
> > 
> >  	state->acquire_ctx = ctx;
> > 
> > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > +	if (IS_ERR(connector_state)) {
> > +		ret = PTR_ERR(connector_state);
> > +		goto fail;
> > +	}
> > +
> > +	connector_state->crtc = crtc;
> > +	connector_state->best_encoder = &intel_encoder->base;
> > +
> >  	if (!mode)
> >  		mode = &load_detect_mode;
> > 
> > @@ -8957,6 +8967,7 @@ void intel_release_load_detect_pipe(struct
> > drm_connector *connector,
> >  	struct drm_crtc *crtc = encoder->crtc;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct drm_atomic_state *state;
> > +	struct drm_connector_state *connector_state;
> > 
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> >  		      connector->base.id, connector->name, @@ -8964,17
> > +8975,23 @@ void intel_release_load_detect_pipe(struct drm_connector
> > *connector,
> > 
> >  	if (old->load_detect_temp) {
> >  		state = drm_atomic_state_alloc(dev);
> > -		if (!state) {
> > -			DRM_DEBUG_KMS("can't release load detect pipe\n");
> > -			return;
> > -		}
> > +		if (!state)
> > +			goto fail;
> 
> Is the above deletion of lines from code added by another patch in this series or
> from a different series? May be you can squash them into one.

That was added in patch 3, the one that adds the drm_atomic_state
parameter to intel_set_mode(). I chose to do it that way in order to not
complicate that patch unnecessarily. It wasn't possible to convert each
different call to intel_set_mode() separately, since that would create a
state of breakage that prevents bisecting.

So the approach I found most reasonable was to just add the state
parameter in patch number 3, and leave the proper population of the
atomic state to a separate patch. The main idea was to simplify the
review process.

> 
> > 
> >  		state->acquire_ctx = ctx;
> > 
> > +		connector_state = drm_atomic_get_connector_state(state,
> > connector);
> > +		if (IS_ERR(connector_state))
> > +			goto fail;
> > +
> >  		to_intel_connector(connector)->new_encoder = NULL;
> >  		intel_encoder->new_crtc = NULL;
> >  		intel_crtc->new_enabled = false;
> >  		intel_crtc->new_config = NULL;
> > +
> > +		connector_state->best_encoder = NULL;
> > +		connector_state->crtc = NULL;
> > +
> >  		intel_set_mode(crtc, NULL, 0, 0, NULL, state);
> > 
> >  		drm_atomic_state_free(state);
> > @@ -8990,6 +9007,11 @@ void intel_release_load_detect_pipe(struct
> > drm_connector *connector,
> >  	/* Switch crtc and encoder back off if necessary */
> >  	if (old->dpms_mode != DRM_MODE_DPMS_ON)
> >  		connector->funcs->dpms(connector, old->dpms_mode);
> > +
> > +	return;
> > +fail:
> > +	DRM_DEBUG_KMS("Couldn't release load detect pipe.\n");
> > +	drm_atomic_state_free(state);
> >  }
> 
> Learning while reviewing connector/encoder side of handling.
> But I think someone also should look at the encoder/connector side or 
> atomic handling.

I think Daniel already did a high level review of this. Perhaps he could
do it again for v2 of this patch.

Ander

> > 
> >  static int i9xx_pll_refclk(struct drm_device *dev, @@ -11646,9 +11668,11 @@
> > intel_set_config_compute_mode_changes(struct drm_mode_set *set,  static int
> > intel_modeset_stage_output_state(struct drm_device *dev,
> >  				 struct drm_mode_set *set,
> > -				 struct intel_set_config *config)
> > +				 struct intel_set_config *config,
> > +				 struct drm_atomic_state *state)
> >  {
> >  	struct intel_connector *connector;
> > +	struct drm_connector_state *connector_state;
> >  	struct intel_encoder *encoder;
> >  	struct intel_crtc *crtc;
> >  	int ro;
> > @@ -11712,6 +11736,14 @@ intel_modeset_stage_output_state(struct
> > drm_device *dev,
> >  		}
> >  		connector->new_encoder->new_crtc = to_intel_crtc(new_crtc);
> > 
> > +		connector_state =
> > +			drm_atomic_get_connector_state(state, &connector-
> > >base);
> > +		if (IS_ERR(connector_state))
> > +			return PTR_ERR(connector_state);
> > +
> > +		connector_state->crtc = new_crtc;
> > +		connector_state->best_encoder = &connector->new_encoder-
> > >base;
> > +
> >  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n",
> >  			connector->base.base.id,
> >  			connector->base.name,
> > @@ -11744,9 +11776,15 @@ intel_modeset_stage_output_state(struct
> > drm_device *dev,
> >  	}
> >  	/* Now we've also updated encoder->new_crtc for all encoders. */
> >  	for_each_intel_connector(dev, connector) {
> > -		if (connector->new_encoder)
> > +		connector_state =
> > +			drm_atomic_get_connector_state(state, &connector-
> > >base);
> > +
> > +		if (connector->new_encoder) {
> >  			if (connector->new_encoder != connector->encoder)
> >  				connector->encoder = connector-
> > >new_encoder;
> > +		} else {
> > +			connector_state->crtc = NULL;
> > +		}
> >  	}
> >  	for_each_intel_crtc(dev, crtc) {
> >  		crtc->new_enabled = false;
> > @@ -11855,7 +11893,7 @@ static int intel_crtc_set_config(struct
> > drm_mode_set *set)
> > 
> >  	state->acquire_ctx = dev->mode_config.acquire_ctx;
> > 
> > -	ret = intel_modeset_stage_output_state(dev, set, config);
> > +	ret = intel_modeset_stage_output_state(dev, set, config, state);
> >  	if (ret)
> >  		goto fail;
> > 
> > --
> > 2.1.0
> 


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