| Message ID | 1426240142-24538-6-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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? > + 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()? 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
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 >
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. */ + list_for_each_entry(connector, + &dev->mode_config.connector_list, + head) { + if (!WARN_ON(connector->state)) { + 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];
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(-)