Re: [PATCH 1/2] drm/i915: prepare for multiple power wells

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

 



2013/10/22 Imre Deak <imre.deak@xxxxxxxxx>:
> In the future we'll need to support multiple power wells, so prepare for
> that here. Create a new power domains struct which contains all
> power domain/well specific fields. Since we'll have one lock protecting
> all power wells, move power_well->lock to the new struct too.
>
> No functional change.
>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 11 ++++++---
>  drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++----------------
>  2 files changed, 42 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 80957ca..7315095 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -911,12 +911,18 @@ struct intel_ilk_power_mgmt {
>  /* Power well structure for haswell */
>  struct i915_power_well {
>         struct drm_device *device;

Can't we also move "device" to i915_power_domains?


> -       struct mutex lock;
>         /* power well enable/disable usage count */
>         int count;
>         int i915_request;
>  };
>
> +#define I915_MAX_POWER_WELLS 1

How about this?

enum intel_power_wells {
        HASWELL_POWER_WELL = 0, /* Or any other more creative name,
since I guess we'll have an equivalent power well on other gens */
        I915_MAX_POWER_WELLS,
};

And then you switch all the code below that uses index zero for the
actual enum name.

Maybe "NON_LPSP_POWER_WELL = 0" since the docs describe LPSP (Low
Power Single Pipe) as a feature that is enabled when the power well is
disabled. Feel free to invent better names :)


> +
> +struct i915_power_domains {
> +       struct mutex lock;
> +       struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
> +};
> +
>  struct i915_dri1_state {
>         unsigned allow_batchbuffer : 1;
>         u32 __iomem *gfx_hws_cpu_addr;
> @@ -1412,8 +1418,7 @@ typedef struct drm_i915_private {
>          * mchdev_lock in intel_pm.c */
>         struct intel_ilk_power_mgmt ips;
>
> -       /* Haswell power well */
> -       struct i915_power_well power_well;
> +       struct i915_power_domains power_domains;
>
>         struct i915_psr psr;
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3e140ab..09ca6f9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5625,7 +5625,7 @@ void intel_display_power_get(struct drm_device *dev,
>                              enum intel_display_power_domain domain)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct i915_power_well *power_well = &dev_priv->power_well;
> +       struct i915_power_domains *power_domains;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
> @@ -5633,16 +5633,18 @@ void intel_display_power_get(struct drm_device *dev,
>         if (is_always_on_power_domain(dev, domain))
>                 return;
>
> -       mutex_lock(&power_well->lock);
> -       __intel_power_well_get(power_well);
> -       mutex_unlock(&power_well->lock);
> +       power_domains = &dev_priv->power_domains;
> +
> +       mutex_lock(&power_domains->lock);
> +       __intel_power_well_get(&power_domains->power_wells[0]);
> +       mutex_unlock(&power_domains->lock);
>  }
>
>  void intel_display_power_put(struct drm_device *dev,
>                              enum intel_display_power_domain domain)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct i915_power_well *power_well = &dev_priv->power_well;
> +       struct i915_power_domains *power_domains;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
> @@ -5650,12 +5652,14 @@ void intel_display_power_put(struct drm_device *dev,
>         if (is_always_on_power_domain(dev, domain))
>                 return;
>
> -       mutex_lock(&power_well->lock);
> -       __intel_power_well_put(power_well);
> -       mutex_unlock(&power_well->lock);
> +       power_domains = &dev_priv->power_domains;
> +
> +       mutex_lock(&power_domains->lock);
> +       __intel_power_well_put(&power_domains->power_wells[0]);
> +       mutex_unlock(&power_domains->lock);
>  }
>
> -static struct i915_power_well *hsw_pwr;
> +static struct i915_power_domains *hsw_pwr;
>
>  /* Display audio driver power well request */
>  void i915_request_power_well(void)
> @@ -5664,7 +5668,7 @@ void i915_request_power_well(void)
>                 return;
>
>         mutex_lock(&hsw_pwr->lock);
> -       __intel_power_well_get(hsw_pwr);
> +       __intel_power_well_get(&hsw_pwr->power_wells[0]);
>         mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_request_power_well);
> @@ -5676,7 +5680,7 @@ void i915_release_power_well(void)
>                 return;
>
>         mutex_lock(&hsw_pwr->lock);
> -       __intel_power_well_put(hsw_pwr);
> +       __intel_power_well_put(&hsw_pwr->power_wells[0]);
>         mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
> @@ -5684,12 +5688,15 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
>  int i915_init_power_well(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +       struct i915_power_well *power_well;
>
> -       hsw_pwr = &dev_priv->power_well;
> +       mutex_init(&power_domains->lock);
> +       hsw_pwr = power_domains;
>
> -       hsw_pwr->device = dev;
> -       mutex_init(&hsw_pwr->lock);
> -       hsw_pwr->count = 0;
> +       power_well = &power_domains->power_wells[0];
> +       power_well->device = dev;
> +       power_well->count = 0;

How about this?

for (i = 0; i < I915_MAX_POWER_WELLS; i++) {
        power_well = &power_domains->power_wells[0];
        power_well->device = dev; /* If you don't move this to the
power_domains struct. */
        power_well->count = 0;
}

Then we'll always be safe :)


With or without my 3 bikesheds (although I would really like to see them):
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>

By the way, do you also plan to change some functions so they take
struct i915_power_well (or an index representing the power well
number) as a parameter? E.g., intel_set_power_well.


>
>         return 0;
>  }
> @@ -5702,7 +5709,8 @@ void i915_remove_power_well(struct drm_device *dev)
>  void intel_set_power_well(struct drm_device *dev, bool enable)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct i915_power_well *power_well = &dev_priv->power_well;
> +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +       struct i915_power_well *power_well;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
> @@ -5710,8 +5718,9 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>         if (!i915_disable_power_well && !enable)
>                 return;
>
> -       mutex_lock(&power_well->lock);
> +       mutex_lock(&power_domains->lock);
>
> +       power_well = &power_domains->power_wells[0];
>         /*
>          * This function will only ever contribute one
>          * to the power well reference count. i915_request
> @@ -5729,20 +5738,24 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>                 __intel_power_well_put(power_well);
>
>   out:
> -       mutex_unlock(&power_well->lock);
> +       mutex_unlock(&power_domains->lock);
>  }
>
>  static void intel_resume_power_well(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct i915_power_well *power_well = &dev_priv->power_well;
> +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +       struct i915_power_well *power_well;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
>
> -       mutex_lock(&power_well->lock);
> +       mutex_lock(&power_domains->lock);
> +
> +       power_well = &power_domains->power_wells[0];
>         __intel_set_power_well(dev, power_well->count > 0);
> -       mutex_unlock(&power_well->lock);
> +
> +       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