Re: [PATCH 05/19] drm/i915: Update dummy connector atomic state with current config

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

On Thu, 2015-03-19 at 20:55 +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 05/19] drm/i915: Update dummy connector atomic state with
> > current config
> > 
> > Keep that state updated so that we can write code that depends on it on the
> > follow up patches.
> > 
> > v2: Fix BUG() due to stale connector_state->crtc value. (Chandra)
> > Signed-off-by: Ander Conselvan de Oliveira
> > <ander.conselvan.de.oliveira@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++++++++++++----
> > ----
> >  1 file changed, 32 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 62b9021..abdbd0c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10114,6 +10114,27 @@ static void
> > intel_modeset_update_staged_output_state(struct drm_device *dev)
> >  	}
> >  }
> > 
> > +/* Transitional helper to copy current connector/encoder state to
> > + * connector->state. This is needed so that code that is partially
> > + * converted to atomic does the right thing.
> > + */
> > +static void intel_modeset_update_connector_atomic_state(struct
> > +drm_device *dev) {
> > +	struct intel_connector *connector;
> > +
> > +	for_each_intel_connector(dev, connector) {
> > +		if (connector->base.encoder) {
> > +			connector->base.state->best_encoder =
> > +				connector->base.encoder;
> > +			connector->base.state->crtc =
> > +				connector->base.encoder->crtc;
> > +		} else {
> > +			connector->base.state->best_encoder = NULL;
> > +			connector->base.state->crtc = NULL;
> > +		}
> > +	}
> > +}
> > +
> >  /**
> >   * intel_modeset_commit_output_state
> >   *
> > @@ -10137,6 +10158,8 @@ static void
> > intel_modeset_commit_output_state(struct drm_device *dev)
> >  		crtc->base.state->enable = crtc->new_enabled;
> >  		crtc->base.enabled = crtc->new_enabled;
> >  	}
> > +
> > +	intel_modeset_update_connector_atomic_state(dev);
> >  }
> > 
> >  static void
> > @@ -12876,15 +12899,13 @@ static void intel_setup_outputs(struct
> > drm_device *dev)
> >  	 * be removed since we'll be setting up real connector state, which
> >  	 * will contain Intel-specific properties.
> >  	 */
> > -	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> > -		list_for_each_entry(connector,
> > -				    &dev->mode_config.connector_list,
> > -				    head) {
> > -			if (!WARN_ON(connector->state)) {
> > -				connector->state =
> > -					kzalloc(sizeof(*connector->state),
> > -						GFP_KERNEL);
> > -			}
> > +	/* FIXME: need to update the comment above. */
> 
> Can you fix the comment instead of adding another FIXME to fix it later?

Oh, I added that FIXME as a note to myself and then let it slip through.
I'll fix and resend.

> 
> > +	list_for_each_entry(connector,
> > +			    &dev->mode_config.connector_list,
> > +			    head) {
> > +		if (!WARN_ON(connector->state)) {
> 
> In intel_modeset_stage_output_state() there is a call to setup
> connector state:
> 	connector_state = drm_atomic_get_connector_state(state, &connector->base);
> Is that call is covering modeset via crtc_set_config() but not during init flow, so 
> doing it here?  
> If that is the case, we probably know that connector->state
> is never being set, so why have WARN_ON()?

The purpose of that WARN_ON is to remind us to remove this whole loop
when we implement connector_states with connector properties. Matt added
this here to avoid a NULL pointer dereference when using nuclear page
flip, but it should go away and be part of connector initialization.

Ander


> As lower level compute code is already 
> > +			connector->state = kzalloc(sizeof(*connector->state),
> > +						   GFP_KERNEL);
> >  		}
> >  	}
> > 
> > @@ -13937,6 +13958,8 @@ void intel_modeset_setup_hw_state(struct
> > drm_device *dev,
> >  				       "[setup_hw_state]");
> >  	}
> > 
> > +	intel_modeset_update_connector_atomic_state(dev);
> > +
> >  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> >  		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
> > 
> > --
> > 2.1.0
> 


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