[Intel-gfx] [PATCH 4/5] drm/i915: take power well refs when needed
Paulo Zanoni
przanoni at gmail.com
Tue Oct 15 21:54:00 CEST 2013
2013/10/14 Jesse Barnes <jbarnes at virtuousgeek.org>:
> When accessing the display regs for hw state readout or cross check, we
> need to make sure the power well is enabled so we can read valid
> register state.
On the current code (HSW) we already check for the power wells in the
HW state readout code: if the power well is disabled, then transcoders
A/B/C are disabled. The current code takes care to not touch registers
on a disabled power well.
>
> Likewise, in an actual mode set, we need to take a ref on the
> appropriate power well so that the mode set succeeds. From then on, the
> power well ref will be tracked by the CRTC enable/disable code.
>
> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 2 ++
> drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 313a8c9..91c3e6c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1367,6 +1367,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
> i915_disable_vga_mem(dev);
> intel_display_power_put(dev, POWER_DOMAIN_VGA);
>
> + intel_display_power_put(dev, POWER_DOMAIN_PIPE(0));
> +
Why is this here? It certainly deserves a comment in the code.
> /* Only enable hotplug handling once the fbdev is fully set up. */
> dev_priv->enable_hotplug_processing = true;
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6e4729b..62ee110 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3841,6 +3841,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> if (intel_crtc->active)
> return;
>
> + intel_display_power_get(dev, POWER_DOMAIN_PIPE(pipe));
> +
> intel_crtc->active = true;
>
> for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -3975,6 +3977,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
> intel_update_watermarks(crtc);
>
> intel_update_fbc(dev);
> +
> + if (IS_VALLEYVIEW(dev))
> + intel_display_power_put(dev, POWER_DOMAIN_PIPE(pipe));
So now HSW uses global_resources while VLV uses crtc enable/disable. I
really think both platforms should try to do the same thing.
By the way, at least on Haswell, if we do an equivalent change we'll
have problems, because when you disable the power well at
crtc_disable, then everything you did at crtc_mode_set will be undone,
and it won't be redone at crtc_enable. When you reenable the power
well, the registers go back to their default values, not the values
that were previously there. Did you check if VLV behaves the same?
> }
>
> static void i9xx_crtc_off(struct drm_crtc *crtc)
> @@ -4134,6 +4139,11 @@ static void intel_connector_check_state(struct intel_connector *connector)
> * consider. */
> void intel_connector_dpms(struct drm_connector *connector, int mode)
> {
> + struct intel_crtc *intel_crtc = to_intel_crtc(connector->encoder->crtc);
> + enum intel_display_power_domain domain;
> +
> + domain = POWER_DOMAIN_PIPE(intel_crtc->pipe);
> +
> /* All the simple cases only support two dpms states. */
> if (mode != DRM_MODE_DPMS_ON)
> mode = DRM_MODE_DPMS_OFF;
> @@ -4141,6 +4151,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
> if (mode == connector->dpms)
> return;
>
> + intel_display_power_get(connector->dev, domain);
> connector->dpms = mode;
>
> /* Only need to change hw state when actually enabled */
> @@ -4148,6 +4159,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
> intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
>
> intel_modeset_check_state(connector->dev);
> + intel_display_power_put(connector->dev, domain);
> }
>
> /* Simple connector->get_hw_state implementation for encoders that support only
> @@ -9192,6 +9204,15 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> intel_crtc_disable(&intel_crtc->base);
>
> + /*
> + * We take a ref here so the mode set will hit live hw. Once
> + * we call the CRTC enable, we can drop our ref since it'll get
> + * tracked there from then on.
> + */
> + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
> + intel_display_power_get(dev,
> + POWER_DOMAIN_PIPE(intel_crtc->pipe));
> +
> for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> if (intel_crtc->base.enabled)
> dev_priv->display.crtc_disable(&intel_crtc->base);
> @@ -9247,6 +9268,10 @@ done:
> }
>
> out:
> + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
> + intel_display_power_put(dev,
> + POWER_DOMAIN_PIPE(intel_crtc->pipe));
> +
> kfree(pipe_config);
> kfree(saved_mode);
> return ret;
> @@ -10692,6 +10717,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>
> crtc->base.enabled = crtc->active;
>
> + if (crtc->active)
> + intel_display_power_get(dev,
> + POWER_DOMAIN_PIPE(crtc->pipe));
> +
What about the panel fitter power domains? Sometimes the panel fitter
is the thing that makes you require a power well, even though you're
on a pipe that doesn't need it.
And on Haswell you also have to take into account
TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first
doesn't need the power well but the second needs it.
> +
> DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
> crtc->base.base.id,
> crtc->active ? "enabled" : "disabled");
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list