Re: [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere

Daniel Vetter
2015-05-04T13:42:01Z

On Wed, Apr 22, 2015 at 01:24:20PM +0200, maarten.lankhorst@linux.intel.com wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

A bit more explanation in the commit message here would be useful, i.e.
what changed that we need this now.
-Daniel
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++--
> drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++-----------------------
> drivers/gpu/drm/i915/intel_drv.h | 1 -
> 3 files changed, 39 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 60e86f331313..91c9a4fc8b6a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3589,12 +3589,18 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
> */
> if (crtc->config->cpu_transcoder == TRANSCODER_EDP &&
> !crtc->config->pch_pfit.enabled) {
> + bool active = crtc->active;
> +
> + if (active)
> + intel_crtc_control(&crtc->base, false);
> +
> crtc->config->pch_pfit.force_thru = true;
>
> intel_display_power_get(dev_priv,
> POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
>
> - intel_crtc_reset(&crtc->base);
> + if (active)
> + intel_crtc_control(&crtc->base, true);
> }
> drm_modeset_unlock_all(dev);
> }
> @@ -3613,12 +3619,18 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
> * routing.
> */
> if (crtc->config->pch_pfit.force_thru) {
> - crtc->config->pch_pfit.force_thru = false;
> + bool active = crtc->active;
>
> - intel_crtc_reset(&crtc->base);
> + if (active)
> + intel_crtc_control(&crtc->base, false);
> +
> + crtc->config->pch_pfit.force_thru = false;
>
> intel_display_power_put(dev_priv,
> POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
> +
> + if (active)
> + intel_crtc_control(&crtc->base, true);
> }
> drm_modeset_unlock_all(dev);
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 438d8e213748..2ffacb4c3a12 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3101,22 +3101,8 @@ static void intel_update_primary_planes(struct drm_device *dev)
> }
> }
>
> -void intel_crtc_reset(struct intel_crtc *crtc)
> -{
> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -
> - if (!crtc->active)
> - return;
> -
> - intel_crtc_disable_planes(&crtc->base);
> - dev_priv->display.crtc_disable(&crtc->base);
> - dev_priv->display.crtc_enable(&crtc->base);
> - intel_crtc_enable_planes(&crtc->base);
> -}
> -
> void intel_prepare_reset(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *crtc;
>
> /* no reset support for gen2 */
> @@ -3128,18 +3114,12 @@ void intel_prepare_reset(struct drm_device *dev)
> return;
>
> drm_modeset_lock_all(dev);
> -
> /*
> * Disabling the crtcs gracefully seems nicer. Also the
> * g33 docs say we should at least disable all the planes.
> */
> - for_each_intel_crtc(dev, crtc) {
> - if (!crtc->active)
> - continue;
> -
> - intel_crtc_disable_planes(&crtc->base);
> - dev_priv->display.crtc_disable(&crtc->base);
> - }
> + for_each_intel_crtc(dev, crtc)
> + intel_crtc_control(&crtc->base, false);
> }
>
> void intel_finish_reset(struct drm_device *dev)
> @@ -5739,26 +5719,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
> enum intel_display_power_domain domain;
> unsigned long domains;
>
> + if (enable == intel_crtc->active)
> + return;
> +
> + if (enable && !crtc->state->enable)
> + return;
> +
> + crtc->state->active = enable;
> if (enable) {
> - if (!intel_crtc->active) {
> - 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;
> -
> - dev_priv->display.crtc_enable(crtc);
> - intel_crtc_enable_planes(crtc);
> - }
> + 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;
> +
> + dev_priv->display.crtc_enable(crtc);
> + intel_crtc_enable_planes(crtc);
> } else {
> - if (intel_crtc->active) {
> - intel_crtc_disable_planes(crtc);
> - dev_priv->display.crtc_disable(crtc);
> -
> - 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;
> - }
> + intel_crtc_disable_planes(crtc);
> + dev_priv->display.crtc_disable(crtc);
> +
> + 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;
> }
> }
>
> @@ -5775,8 +5758,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
> enable |= intel_encoder->connectors_active;
>
> intel_crtc_control(crtc, enable);
> -
> - crtc->state->active = enable;
> }
>
> void intel_encoder_destroy(struct drm_encoder *encoder)
> @@ -14087,8 +14068,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> plane = crtc->plane;
> to_intel_plane_state(crtc->base.primary->state)->visible = true;
> crtc->plane = !plane;
> - intel_crtc_disable_planes(&crtc->base);
> - dev_priv->display.crtc_disable(&crtc->base);
> + intel_crtc_control(&crtc->base, false);
> crtc->plane = plane;
>
> /* ... and break all links. */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fb89f5f3498c..9668b17d7e0e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -987,7 +987,6 @@ 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_crtc_control(struct drm_crtc *crtc, bool enable);
> -void intel_crtc_reset(struct intel_crtc *crtc);
> 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

--
Daniel Vetter
Software Engineer, Intel Corporation