2013/11/27 Imre Deak <imre.deak@xxxxxxxxx>:
> Atm we call intel_display_power_enabled() from
> i915_capture_error_state() in IRQ context and then take a mutex. To fix
> this add a new intel_display_power_enabled_sw() which returns the domain
> state based on software tracking as opposed to reading the actual HW
> state.
>
> Since we use domain_use_count for this without locking on the reader
> side make sure we increase the counter only after enabling all required
> power wells and decrease it before disabling any of these power wells.
>
> Regression introduced in
> commit 1b02383464b4a915627ef3b8fd0ad7f07168c54c
> Author: Imre Deak <imre.deak@xxxxxxxxx>
> Date: Tue Sep 24 16:17:09 2013 +0300
>
> drm/i915: support for multiple power wells
>
> Note that atm we depend on the value returned by
> intel_display_power_enabled_sw() in i915_capture_error_state() to avoid
> unclaimed register access reports. This was never guaranteed though,
> since another thread can disable the power concurrently. If this is a
> problem we need another explicit way to disable the reporting during
> error captures.
>
> v2:
> - remove barriers as the caller can't depend on the value
> returned from i915_capture_error_state_sw() anyway (Ville)
> - dump the state of pipe/transcoder power domain state (Daniel)
>
> Reported-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
Makes sense to me. Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 --
> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_pm.c | 24 ++++++++++++++++--------
> 4 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ed7a1fc..cc06b9f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -971,9 +971,7 @@ struct i915_power_domains {
> int power_well_count;
>
> struct mutex lock;
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
> int domain_use_count[POWER_DOMAIN_NUM];
> -#endif
> struct i915_power_well *power_wells;
> };
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e4dbae2..464ae66 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11334,6 +11334,7 @@ struct intel_display_error_state {
> } cursor[I915_MAX_PIPES];
>
> struct intel_pipe_error_state {
> + bool power_domain_on;
> u32 source;
> } pipe[I915_MAX_PIPES];
>
> @@ -11348,6 +11349,7 @@ struct intel_display_error_state {
> } plane[I915_MAX_PIPES];
>
> struct intel_transcoder_error_state {
> + bool power_domain_on;
> enum transcoder cpu_transcoder;
>
> u32 conf;
> @@ -11385,7 +11387,9 @@ intel_display_capture_error_state(struct drm_device *dev)
> error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
>
> for_each_pipe(i) {
> - if (!intel_display_power_enabled(dev, POWER_DOMAIN_PIPE(i)))
> + error->pipe[i].power_domain_on =
> + intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i));
> + if (!error->pipe[i].power_domain_on)
> continue;
>
> if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
> @@ -11421,8 +11425,9 @@ intel_display_capture_error_state(struct drm_device *dev)
> for (i = 0; i < error->num_transcoders; i++) {
> enum transcoder cpu_transcoder = transcoders[i];
>
> - if (!intel_display_power_enabled(dev,
> - POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
> + error->transcoder[i].power_domain_on =
> + intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i));
> + if (!error->transcoder[i].power_domain_on)
> continue;
>
> error->transcoder[i].cpu_transcoder = cpu_transcoder;
> @@ -11457,6 +11462,8 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
> error->power_well_driver);
> for_each_pipe(i) {
> err_printf(m, "Pipe [%d]:\n", i);
> + err_printf(m, " Power: %s\n",
> + error->pipe[i].power_domain_on ? "on" : "off");
> err_printf(m, " SRC: %08x\n", error->pipe[i].source);
>
> err_printf(m, "Plane [%d]:\n", i);
> @@ -11482,6 +11489,8 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
> for (i = 0; i < error->num_transcoders; i++) {
> err_printf(m, "CPU transcoder: %c\n",
> transcoder_name(error->transcoder[i].cpu_transcoder));
> + err_printf(m, " Power: %s\n",
> + error->transcoder[i].power_domain_on ? "on" : "off");
> err_printf(m, " CONF: %08x\n", error->transcoder[i].conf);
> err_printf(m, " HTOTAL: %08x\n", error->transcoder[i].htotal);
> err_printf(m, " HBLANK: %08x\n", error->transcoder[i].hblank);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0231281..5dea389 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -842,6 +842,8 @@ int intel_power_domains_init(struct drm_device *dev);
> void intel_power_domains_remove(struct drm_device *dev);
> bool intel_display_power_enabled(struct drm_device *dev,
> enum intel_display_power_domain domain);
> +bool intel_display_power_enabled_sw(struct drm_device *dev,
> + enum intel_display_power_domain domain);
> void intel_display_power_get(struct drm_device *dev,
> enum intel_display_power_domain domain);
> void intel_display_power_put(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7149f60..311199c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5635,6 +5635,17 @@ static bool hsw_power_well_enabled(struct drm_device *dev,
> (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
> }
>
> +bool intel_display_power_enabled_sw(struct drm_device *dev,
> + enum intel_display_power_domain domain)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct i915_power_domains *power_domains;
> +
> + power_domains = &dev_priv->power_domains;
> +
> + return power_domains->domain_use_count[domain];
> +}
> +
> bool intel_display_power_enabled(struct drm_device *dev,
> enum intel_display_power_domain domain)
> {
> @@ -5755,12 +5766,11 @@ void intel_display_power_get(struct drm_device *dev,
>
> mutex_lock(&power_domains->lock);
>
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
> - power_domains->domain_use_count[domain]++;
> -#endif
> for_each_power_well(i, power_well, BIT(domain), power_domains)
> __intel_power_well_get(dev, power_well);
>
> + power_domains->domain_use_count[domain]++;
> +
> mutex_unlock(&power_domains->lock);
> }
>
> @@ -5776,13 +5786,11 @@ void intel_display_power_put(struct drm_device *dev,
>
> mutex_lock(&power_domains->lock);
>
> - for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> - __intel_power_well_put(dev, power_well);
> -
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
> WARN_ON(!power_domains->domain_use_count[domain]);
> power_domains->domain_use_count[domain]--;
> -#endif
> +
> + for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> + __intel_power_well_put(dev, power_well);
>
> mutex_unlock(&power_domains->lock);
> }
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
