Re: [PATCH] drm/i915: add intel_display_power_enabled_sw() for use in atomic ctx

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux