| Message ID | 5564304A.8090808@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 26, 2015 at 10:35:22AM +0200, Maarten Lankhorst wrote: > Assume the callers lock everything with drm_modeset_lock_all. > > This change had to be done after converting suspend/resume to > use atomic_state so the atomic state is preserved, otherwise > all transitional state is erased. > > Now all callers of .crtc_enable and .crtc_disable go through > atomic modeset! :-D > > Changes since v1: > - Only check for crtc_state->active in valleyview_modeset_global_pipes. > - Only check for crtc_state->active in modeset_update_crtc_power_domains. > Changes since v2: > - Rework on top of the changed patch order. > Changes since v3: > - Rename intel_crtc_toggle in description to *_control > - Change return value to int. > - Do not add plane state, should be done implicitly already. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++-------------- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 2 files changed, 33 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e9680eb85bd0..32bab28432f7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5973,38 +5973,49 @@ void intel_display_suspend(struct drm_device *dev) > } > > /* Master function to enable/disable CRTC and corresponding power wells */ > -void intel_crtc_control(struct drm_crtc *crtc, bool enable) > +int intel_crtc_control(struct drm_crtc *crtc, bool enable) We change the return value to 'int' here, but we never check the error code anywhere. Maybe we should hold this off for another patch and then also propagate the return value back up the call chain? It seems like at least some of the connector-specific DPMS functions should recognize the failure of intel_crtc_update_dpms() before trying to continue with other operations. > { > struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_mode_config *config = &dev->mode_config; > + struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - enum intel_display_power_domain domain; > - unsigned long domains; > + struct intel_crtc_state *pipe_config; > + struct drm_atomic_state *state; > + int ret; > > if (enable == intel_crtc->active) > - return; > + return 0; > > if (enable && !crtc->state->enable) > - return; > + return 0; > > - crtc->state->active = enable; > - if (enable) { > - domains = get_crtc_power_domains(crtc); > - for_each_power_domain(domain, domains) > - intel_display_power_get(dev_priv, domain); > - intel_crtc->enabled_power_domains = domains; > + /* this function should be called with drm_modeset_lock_all for now */ > + if (WARN_ON(!ctx)) > + return -EIO; EIO seems like an unusual choice here (but I'm not really sure what the best option would be). Matt > + lockdep_assert_held(&ctx->ww_ctx); > > - dev_priv->display.crtc_enable(crtc); > - intel_crtc_enable_planes(crtc); > - } else { > - intel_crtc_disable_planes(crtc); > - dev_priv->display.crtc_disable(crtc); > + state = drm_atomic_state_alloc(dev); > + if (WARN_ON(!state)) > + return -ENOMEM; > > - domains = intel_crtc->enabled_power_domains; > - for_each_power_domain(domain, domains) > - intel_display_power_put(dev_priv, domain); > - intel_crtc->enabled_power_domains = 0; > + state->acquire_ctx = ctx; > + state->allow_modeset = true; > + > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > + if (IS_ERR(pipe_config)) { > + ret = PTR_ERR(pipe_config); > + goto err; > } > + pipe_config->base.active = enable; > + > + ret = intel_set_mode(state); > + if (!ret) > + return ret; > + > +err: > + DRM_ERROR("Updating crtc active failed with %i\n", ret); > + drm_atomic_state_free(state); > + return ret; > } > > /** > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 60a29ff65f1f..c113f187090f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -989,7 +989,7 @@ void intel_mark_busy(struct drm_device *dev); > void intel_mark_idle(struct drm_device *dev); > void intel_crtc_restore_mode(struct drm_crtc *crtc); > void intel_display_suspend(struct drm_device *dev); > -void intel_crtc_control(struct drm_crtc *crtc, bool enable); > +int intel_crtc_control(struct drm_crtc *crtc, bool enable); > void intel_crtc_update_dpms(struct drm_crtc *crtc); > void intel_encoder_destroy(struct drm_encoder *encoder); > int intel_connector_init(struct intel_connector *); > -- > 2.1.0 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 29-05-15 om 02:57 schreef Matt Roper: > On Tue, May 26, 2015 at 10:35:22AM +0200, Maarten Lankhorst wrote: >> Assume the callers lock everything with drm_modeset_lock_all. >> >> This change had to be done after converting suspend/resume to >> use atomic_state so the atomic state is preserved, otherwise >> all transitional state is erased. >> >> Now all callers of .crtc_enable and .crtc_disable go through >> atomic modeset! :-D >> >> Changes since v1: >> - Only check for crtc_state->active in valleyview_modeset_global_pipes. >> - Only check for crtc_state->active in modeset_update_crtc_power_domains. >> Changes since v2: >> - Rework on top of the changed patch order. >> Changes since v3: >> - Rename intel_crtc_toggle in description to *_control >> - Change return value to int. >> - Do not add plane state, should be done implicitly already. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++-------------- >> drivers/gpu/drm/i915/intel_drv.h | 2 +- >> 2 files changed, 33 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index e9680eb85bd0..32bab28432f7 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -5973,38 +5973,49 @@ void intel_display_suspend(struct drm_device *dev) >> } >> >> /* Master function to enable/disable CRTC and corresponding power wells */ >> -void intel_crtc_control(struct drm_crtc *crtc, bool enable) >> +int intel_crtc_control(struct drm_crtc *crtc, bool enable) > We change the return value to 'int' here, but we never check the error > code anywhere. Maybe we should hold this off for another patch and then > also propagate the return value back up the call chain? It seems like > at least some of the connector-specific DPMS functions should recognize > the failure of intel_crtc_update_dpms() before trying to continue with > other operations. Yeah, but I wasn't sure which ones, and handling is a lot harder. So I wanted to mark this as 'int' to warn future users that this function may fail so they can check. >> { >> struct drm_device *dev = crtc->dev; >> - struct drm_i915_private *dev_priv = dev->dev_private; >> + struct drm_mode_config *config = &dev->mode_config; >> + struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - enum intel_display_power_domain domain; >> - unsigned long domains; >> + struct intel_crtc_state *pipe_config; >> + struct drm_atomic_state *state; >> + int ret; >> >> if (enable == intel_crtc->active) >> - return; >> + return 0; >> >> if (enable && !crtc->state->enable) >> - return; >> + return 0; >> >> - crtc->state->active = enable; >> - if (enable) { >> - domains = get_crtc_power_domains(crtc); >> - for_each_power_domain(domain, domains) >> - intel_display_power_get(dev_priv, domain); >> - intel_crtc->enabled_power_domains = domains; >> + /* this function should be called with drm_modeset_lock_all for now */ >> + if (WARN_ON(!ctx)) >> + return -EIO; > EIO seems like an unusual choice here (but I'm not really sure what the > best option would be). No idea, I just wanted to make sure that it was recognized as programming error. -EINVAL Is used everywhere in the atomic path, -ENODEV didn't seem appropriate. >> + lockdep_assert_held(&ctx->ww_ctx); >> >> - dev_priv->display.crtc_enable(crtc); >> - intel_crtc_enable_planes(crtc); >> - } else { >> - intel_crtc_disable_planes(crtc); >> - dev_priv->display.crtc_disable(crtc); >> + state = drm_atomic_state_alloc(dev); >> + if (WARN_ON(!state)) >> + return -ENOMEM; >> >> - domains = intel_crtc->enabled_power_domains; >> - for_each_power_domain(domain, domains) >> - intel_display_power_put(dev_priv, domain); >> - intel_crtc->enabled_power_domains = 0; >> + state->acquire_ctx = ctx; >> + state->allow_modeset = true; >> + >> + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); >> + if (IS_ERR(pipe_config)) { >> + ret = PTR_ERR(pipe_config); >> + goto err; >> } >> + pipe_config->base.active = enable; >> + >> + ret = intel_set_mode(state); >> + if (!ret) >> + return ret; >> + >> +err: >> + DRM_ERROR("Updating crtc active failed with %i\n", ret); >> + drm_atomic_state_free(state); >> + return ret; >> } >> >> /** >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 60a29ff65f1f..c113f187090f 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -989,7 +989,7 @@ void intel_mark_busy(struct drm_device *dev); >> void intel_mark_idle(struct drm_device *dev); >> void intel_crtc_restore_mode(struct drm_crtc *crtc); >> void intel_display_suspend(struct drm_device *dev); >> -void intel_crtc_control(struct drm_crtc *crtc, bool enable); >> +int intel_crtc_control(struct drm_crtc *crtc, bool enable); >> void intel_crtc_update_dpms(struct drm_crtc *crtc); >> void intel_encoder_destroy(struct drm_encoder *encoder); >> int intel_connector_init(struct intel_connector *); >> -- >> 2.1.0 >> >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e9680eb85bd0..32bab28432f7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5973,38 +5973,49 @@ void intel_display_suspend(struct drm_device *dev) } /* Master function to enable/disable CRTC and corresponding power wells */ -void intel_crtc_control(struct drm_crtc *crtc, bool enable) +int intel_crtc_control(struct drm_crtc *crtc, bool enable) { struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_mode_config *config = &dev->mode_config; + struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - enum intel_display_power_domain domain; - unsigned long domains; + struct intel_crtc_state *pipe_config; + struct drm_atomic_state *state; + int ret; if (enable == intel_crtc->active) - return; + return 0; if (enable && !crtc->state->enable) - return; + return 0; - crtc->state->active = enable; - if (enable) { - domains = get_crtc_power_domains(crtc); - for_each_power_domain(domain, domains) - intel_display_power_get(dev_priv, domain); - intel_crtc->enabled_power_domains = domains; + /* this function should be called with drm_modeset_lock_all for now */ + if (WARN_ON(!ctx)) + return -EIO; + lockdep_assert_held(&ctx->ww_ctx); - dev_priv->display.crtc_enable(crtc); - intel_crtc_enable_planes(crtc); - } else { - intel_crtc_disable_planes(crtc); - dev_priv->display.crtc_disable(crtc); + state = drm_atomic_state_alloc(dev); + if (WARN_ON(!state)) + return -ENOMEM; - domains = intel_crtc->enabled_power_domains; - for_each_power_domain(domain, domains) - intel_display_power_put(dev_priv, domain); - intel_crtc->enabled_power_domains = 0; + state->acquire_ctx = ctx; + state->allow_modeset = true; + + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); + if (IS_ERR(pipe_config)) { + ret = PTR_ERR(pipe_config); + goto err; } + pipe_config->base.active = enable; + + ret = intel_set_mode(state); + if (!ret) + return ret; + +err: + DRM_ERROR("Updating crtc active failed with %i\n", ret); + drm_atomic_state_free(state); + return ret; } /** diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 60a29ff65f1f..c113f187090f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -989,7 +989,7 @@ void intel_mark_busy(struct drm_device *dev); void intel_mark_idle(struct drm_device *dev); void intel_crtc_restore_mode(struct drm_crtc *crtc); void intel_display_suspend(struct drm_device *dev); -void intel_crtc_control(struct drm_crtc *crtc, bool enable); +int intel_crtc_control(struct drm_crtc *crtc, bool enable); void intel_crtc_update_dpms(struct drm_crtc *crtc); void intel_encoder_destroy(struct drm_encoder *encoder); int intel_connector_init(struct intel_connector *);
Assume the callers lock everything with drm_modeset_lock_all. This change had to be done after converting suspend/resume to use atomic_state so the atomic state is preserved, otherwise all transitional state is erased. Now all callers of .crtc_enable and .crtc_disable go through atomic modeset! :-D Changes since v1: - Only check for crtc_state->active in valleyview_modeset_global_pipes. - Only check for crtc_state->active in modeset_update_crtc_power_domains. Changes since v2: - Rework on top of the changed patch order. Changes since v3: - Rename intel_crtc_toggle in description to *_control - Change return value to int. - Do not add plane state, should be done implicitly already. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++-------------- drivers/gpu/drm/i915/intel_drv.h | 2 +- 2 files changed, 33 insertions(+), 22 deletions(-)