From: Paulo Zanoni <paulo.r.zanoni@intel.com>
To: Imre Deak <imre.deak@intel.com>, intel-gfx@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@intel.com>
Subject: Re: [PATCH 04/10] drm/i915: Constify power well descriptors
Date: Wed, 01 Aug 2018 14:39:31 -0700 [thread overview]
Message-ID: <1533159571.2571.11.camel@intel.com> (raw)
In-Reply-To: <20180720141504.22832-5-imre.deak@intel.com>
Em Sex, 2018-07-20 às 17:14 +0300, Imre Deak escreveu:
> It makes sense to keep unchanging data const. Extract such fields
> from
> the i915_power_well struct into a new i915_power_well_desc struct
> that
> we initialize during compile time. For the rest of the dynamic
> fields allocate an array of i915_power_well objects in i915 dev_priv,
> and link to each of these objects their corresponding
> i915_power_well_desc object.
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Quite a few issues pointed by checkpatch for this patch, please take a
look at them.
More below:
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
> drivers/gpu/drm/i915/i915_drv.c | 8 +-
> drivers/gpu/drm/i915/i915_drv.h | 14 ++-
> drivers/gpu/drm/i915/intel_display.h | 4 +-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_hdcp.c | 6 +-
> drivers/gpu/drm/i915/intel_runtime_pm.c | 204 +++++++++++++++++++---
> ----------
> 7 files changed, 144 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index b3aefd623557..eb284cac8fda 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2833,10 +2833,10 @@ static int i915_power_domain_info(struct
> seq_file *m, void *unused)
> enum intel_display_power_domain power_domain;
>
> power_well = &power_domains->power_wells[i];
> - seq_printf(m, "%-25s %d\n", power_well->name,
> + seq_printf(m, "%-25s %d\n", power_well->desc->name,
> power_well->count);
>
> - for_each_power_domain(power_domain, power_well-
> >domains)
> + for_each_power_domain(power_domain, power_well-
> >desc->domains)
> seq_printf(m, " %-23s %d\n",
> intel_display_power_domain_str(powe
> r_domain),
> power_domains-
> >domain_use_count[power_domain]);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 3c984530fef9..5743db4500fb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -922,7 +922,9 @@ static int i915_driver_init_early(struct
> drm_i915_private *dev_priv,
> intel_uc_init_early(dev_priv);
> intel_pm_setup(dev_priv);
> intel_init_dpio(dev_priv);
> - intel_power_domains_init(dev_priv);
> + ret = intel_power_domains_init(dev_priv);
> + if (ret < 0)
> + goto err_uc;
> intel_irq_init(dev_priv);
> intel_hangcheck_init(dev_priv);
> intel_init_display_hooks(dev_priv);
> @@ -934,6 +936,9 @@ static int i915_driver_init_early(struct
> drm_i915_private *dev_priv,
>
> return 0;
>
> +err_uc:
> + intel_uc_cleanup_early(dev_priv);
Please leave the guc fixes for a different patch, regardless of how
innocent they look.
Everything else looks good!
Thanks,
Paulo
> + i915_gem_cleanup_early(dev_priv);
> err_workqueues:
> i915_workqueues_cleanup(dev_priv);
> err_engines:
> @@ -948,6 +953,7 @@ static int i915_driver_init_early(struct
> drm_i915_private *dev_priv,
> static void i915_driver_cleanup_early(struct drm_i915_private
> *dev_priv)
> {
> intel_irq_fini(dev_priv);
> + intel_power_domains_cleanup(dev_priv);
> intel_uc_cleanup_early(dev_priv);
> i915_gem_cleanup_early(dev_priv);
> i915_workqueues_cleanup(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 4fb937399440..3ae200a9e8f1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -862,13 +862,9 @@ struct i915_power_well_ops {
> };
>
> /* Power well structure for haswell */
> -struct i915_power_well {
> +struct i915_power_well_desc {
> const char *name;
> bool always_on;
> - /* power well enable/disable usage count */
> - int count;
> - /* cached hw enabled state */
> - bool hw_enabled;
> u64 domains;
> /* unique identifier for this power well */
> enum i915_power_well_id id;
> @@ -891,6 +887,14 @@ struct i915_power_well {
> const struct i915_power_well_ops *ops;
> };
>
> +struct i915_power_well {
> + const struct i915_power_well_desc *desc;
> + /* power well enable/disable usage count */
> + int count;
> + /* cached hw enabled state */
> + bool hw_enabled;
> +};
> +
> struct i915_power_domains {
> /*
> * Power wells needed for initialization at driver init and
> suspend
> diff --git a/drivers/gpu/drm/i915/intel_display.h
> b/drivers/gpu/drm/i915/intel_display.h
> index 9292001cdd14..a626282d590b 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -322,11 +322,11 @@ struct intel_link_m_n {
>
> #define for_each_power_domain_well(__dev_priv, __power_well,
> __domain_mask) \
> for_each_power_well(__dev_priv, __power_well)
> \
> - for_each_if((__power_well)->domains &
> (__domain_mask))
> + for_each_if((__power_well)->desc->domains &
> (__domain_mask))
>
> #define for_each_power_domain_well_rev(__dev_priv, __power_well,
> __domain_mask) \
> for_each_power_well_rev(__dev_priv, __power_well)
> \
> - for_each_if((__power_well)->domains &
> (__domain_mask))
> + for_each_if((__power_well)->desc->domains &
> (__domain_mask))
>
> #define for_each_new_intel_plane_in_state(__state, plane,
> new_plane_state, __i) \
> for ((__i) = 0; \
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 32be305c0e89..25f9b035cfe8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1943,6 +1943,7 @@ int intel_psr_wait_for_idle(const struct
> intel_crtc_state *new_crtc_state);
>
> /* intel_runtime_pm.c */
> int intel_power_domains_init(struct drm_i915_private *);
> +void intel_power_domains_cleanup(struct drm_i915_private *);
> void intel_power_domains_init_hw(struct drm_i915_private *dev_priv,
> bool resume);
> void intel_power_domains_fini_hw(struct drm_i915_private *);
> void intel_power_domains_suspend(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c
> b/drivers/gpu/drm/i915/intel_hdcp.c
> index 0cc6a861bcf8..26e48fc95543 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -57,9 +57,9 @@ static bool hdcp_key_loadable(struct
> drm_i915_private *dev_priv)
>
> /* PG1 (power well #1) needs to be enabled */
> for_each_power_well(dev_priv, power_well) {
> - if (power_well->id == id) {
> - enabled = power_well->ops-
> >is_enabled(dev_priv,
> - power_
> well);
> + if (power_well->desc->id == id) {
> + enabled = power_well->desc->ops-
> >is_enabled(dev_priv,
> +
> power_well);
> break;
> }
> }
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index f119cbe4f61d..8b3c241bee55 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -159,17 +159,17 @@ intel_display_power_domain_str(enum
> intel_display_power_domain domain)
> static void intel_power_well_enable(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - DRM_DEBUG_KMS("enabling %s\n", power_well->name);
> - power_well->ops->enable(dev_priv, power_well);
> + DRM_DEBUG_KMS("enabling %s\n", power_well->desc->name);
> + power_well->desc->ops->enable(dev_priv, power_well);
> power_well->hw_enabled = true;
> }
>
> static void intel_power_well_disable(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - DRM_DEBUG_KMS("disabling %s\n", power_well->name);
> + DRM_DEBUG_KMS("disabling %s\n", power_well->desc->name);
> power_well->hw_enabled = false;
> - power_well->ops->disable(dev_priv, power_well);
> + power_well->desc->ops->disable(dev_priv, power_well);
> }
>
> static void intel_power_well_get(struct drm_i915_private *dev_priv,
> @@ -183,7 +183,7 @@ static void intel_power_well_put(struct
> drm_i915_private *dev_priv,
> struct i915_power_well *power_well)
> {
> WARN(!power_well->count, "Use count on power well %s is
> already zero",
> - power_well->name);
> + power_well->desc->name);
>
> if (!--power_well->count)
> intel_power_well_disable(dev_priv, power_well);
> @@ -213,7 +213,7 @@ bool __intel_display_power_is_enabled(struct
> drm_i915_private *dev_priv,
> is_enabled = true;
>
> for_each_power_domain_well_rev(dev_priv, power_well,
> BIT_ULL(domain)) {
> - if (power_well->always_on)
> + if (power_well->desc->always_on)
> continue;
>
> if (!power_well->hw_enabled) {
> @@ -323,7 +323,7 @@ static void hsw_power_well_pre_disable(struct
> drm_i915_private *dev_priv,
> static void hsw_wait_for_power_well_enable(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - enum i915_power_well_id id = power_well->id;
> + enum i915_power_well_id id = power_well->desc->id;
>
> /* Timeout for PW1:10 us, AUX:not specified, other PWs:20
> us. */
> WARN_ON(intel_wait_for_register(dev_priv,
> @@ -350,7 +350,7 @@ static u32 hsw_power_well_requesters(struct
> drm_i915_private *dev_priv,
> static void hsw_wait_for_power_well_disable(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - enum i915_power_well_id id = power_well->id;
> + enum i915_power_well_id id = power_well->desc->id;
> bool disabled;
> u32 reqs;
>
> @@ -370,7 +370,7 @@ static void
> hsw_wait_for_power_well_disable(struct drm_i915_private *dev_priv,
> return;
>
> DRM_DEBUG_KMS("%s forced on (bios:%d driver:%d kvmr:%d
> debug:%d)\n",
> - power_well->name,
> + power_well->desc->name,
> !!(reqs & 1), !!(reqs & 2), !!(reqs & 4),
> !!(reqs & 8));
> }
>
> @@ -386,8 +386,8 @@ static void gen9_wait_for_power_well_fuses(struct
> drm_i915_private *dev_priv,
> static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - enum i915_power_well_id id = power_well->id;
> - bool wait_fuses = power_well->hsw.has_fuses;
> + enum i915_power_well_id id = power_well->desc->id;
> + bool wait_fuses = power_well->desc->hsw.has_fuses;
> enum skl_power_gate uninitialized_var(pg);
> u32 val;
>
> @@ -421,17 +421,19 @@ static void hsw_power_well_enable(struct
> drm_i915_private *dev_priv,
> if (wait_fuses)
> gen9_wait_for_power_well_fuses(dev_priv, pg);
>
> - hsw_power_well_post_enable(dev_priv, power_well-
> >hsw.irq_pipe_mask,
> - power_well->hsw.has_vga);
> + hsw_power_well_post_enable(dev_priv,
> + power_well->desc-
> >hsw.irq_pipe_mask,
> + power_well->desc->hsw.has_vga);
> }
>
> static void hsw_power_well_disable(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - enum i915_power_well_id id = power_well->id;
> + enum i915_power_well_id id = power_well->desc->id;
> u32 val;
>
> - hsw_power_well_pre_disable(dev_priv, power_well-
> >hsw.irq_pipe_mask);
> + hsw_power_well_pre_disable(dev_priv,
> + power_well->desc-
> >hsw.irq_pipe_mask);
>
> val = I915_READ(HSW_PWR_WELL_CTL_DRIVER(id));
> I915_WRITE(HSW_PWR_WELL_CTL_DRIVER(id),
> @@ -445,7 +447,7 @@ static void
> icl_combo_phy_aux_power_well_enable(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - enum i915_power_well_id id = power_well->id;
> + enum i915_power_well_id id = power_well->desc->id;
> enum port port = ICL_AUX_PW_TO_PORT(id);
> u32 val;
>
> @@ -462,7 +464,7 @@ static void
> icl_combo_phy_aux_power_well_disable(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - enum i915_power_well_id id = power_well->id;
> + enum i915_power_well_id id = power_well->desc->id;
> enum port port = ICL_AUX_PW_TO_PORT(id);
> u32 val;
>
> @@ -484,7 +486,7 @@ icl_combo_phy_aux_power_well_disable(struct
> drm_i915_private *dev_priv,
> static bool hsw_power_well_enabled(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - enum i915_power_well_id id = power_well->id;
> + enum i915_power_well_id id = power_well->desc->id;
> u32 mask = HSW_PWR_WELL_CTL_REQ(id) |
> HSW_PWR_WELL_CTL_STATE(id);
>
> return (I915_READ(HSW_PWR_WELL_CTL_DRIVER(id)) & mask) ==
> mask;
> @@ -723,7 +725,7 @@ static void skl_enable_dc6(struct
> drm_i915_private *dev_priv)
> static void hsw_power_well_sync_hw(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - enum i915_power_well_id id = power_well->id;
> + enum i915_power_well_id id = power_well->desc->id;
> u32 mask = HSW_PWR_WELL_CTL_REQ(id);
> u32 bios_req = I915_READ(HSW_PWR_WELL_CTL_BIOS(id));
>
> @@ -740,19 +742,19 @@ static void hsw_power_well_sync_hw(struct
> drm_i915_private *dev_priv,
> static void bxt_dpio_cmn_power_well_enable(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - bxt_ddi_phy_init(dev_priv, power_well->bxt.phy);
> + bxt_ddi_phy_init(dev_priv, power_well->desc->bxt.phy);
> }
>
> static void bxt_dpio_cmn_power_well_disable(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - bxt_ddi_phy_uninit(dev_priv, power_well->bxt.phy);
> + bxt_ddi_phy_uninit(dev_priv, power_well->desc->bxt.phy);
> }
>
> static bool bxt_dpio_cmn_power_well_enabled(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - return bxt_ddi_phy_is_enabled(dev_priv, power_well-
> >bxt.phy);
> + return bxt_ddi_phy_is_enabled(dev_priv, power_well->desc-
> >bxt.phy);
> }
>
> static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private
> *dev_priv)
> @@ -761,16 +763,17 @@ static void
> bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
>
> power_well = lookup_power_well(dev_priv, BXT_DPIO_CMN_A);
> if (power_well->count > 0)
> - bxt_ddi_phy_verify_state(dev_priv, power_well-
> >bxt.phy);
> + bxt_ddi_phy_verify_state(dev_priv, power_well->desc-
> >bxt.phy);
>
> power_well = lookup_power_well(dev_priv, BXT_DPIO_CMN_BC);
> if (power_well->count > 0)
> - bxt_ddi_phy_verify_state(dev_priv, power_well-
> >bxt.phy);
> + bxt_ddi_phy_verify_state(dev_priv, power_well->desc-
> >bxt.phy);
>
> if (IS_GEMINILAKE(dev_priv)) {
> power_well = lookup_power_well(dev_priv,
> GLK_DPIO_CMN_C);
> if (power_well->count > 0)
> - bxt_ddi_phy_verify_state(dev_priv,
> power_well->bxt.phy);
> + bxt_ddi_phy_verify_state(dev_priv,
> + power_well->desc-
> >bxt.phy);
> }
> }
>
> @@ -869,7 +872,7 @@ static void i830_pipes_power_well_sync_hw(struct
> drm_i915_private *dev_priv,
> static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> struct i915_power_well *power_well,
> bool enable)
> {
> - enum i915_power_well_id power_well_id = power_well->id;
> + enum i915_power_well_id power_well_id = power_well->desc-
> >id;
> u32 mask;
> u32 state;
> u32 ctrl;
> @@ -917,7 +920,7 @@ static void vlv_power_well_disable(struct
> drm_i915_private *dev_priv,
> static bool vlv_power_well_enabled(struct drm_i915_private
> *dev_priv,
> struct i915_power_well
> *power_well)
> {
> - enum i915_power_well_id power_well_id = power_well->id;
> + enum i915_power_well_id power_well_id = power_well->desc-
> >id;
> bool enabled = false;
> u32 mask;
> u32 state;
> @@ -1107,7 +1110,7 @@ lookup_power_well(struct drm_i915_private
> *dev_priv,
> struct i915_power_well *power_well;
>
> power_well = &power_domains->power_wells[i];
> - if (power_well->id == power_well_id)
> + if (power_well->desc->id == power_well_id)
> return power_well;
> }
>
> @@ -1146,7 +1149,7 @@ static void assert_chv_phy_status(struct
> drm_i915_private *dev_priv)
> PHY_STATUS_SPLINE_LDO(DPIO_PHY1
> , DPIO_CH0, 0) |
> PHY_STATUS_SPLINE_LDO(DPIO_PHY1
> , DPIO_CH0, 1));
>
> - if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> + if (cmn_bc->desc->ops->is_enabled(dev_priv, cmn_bc)) {
> phy_status |= PHY_POWERGOOD(DPIO_PHY0);
>
> /* this assumes override is only used to enable
> lanes */
> @@ -1187,7 +1190,7 @@ static void assert_chv_phy_status(struct
> drm_i915_private *dev_priv)
> phy_status |=
> PHY_STATUS_SPLINE_LDO(DPIO_PHY0, DPIO_CH1, 1);
> }
>
> - if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> + if (cmn_d->desc->ops->is_enabled(dev_priv, cmn_d)) {
> phy_status |= PHY_POWERGOOD(DPIO_PHY1);
>
> /* this assumes override is only used to enable
> lanes */
> @@ -1231,10 +1234,10 @@ static void
> chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> enum pipe pipe;
> uint32_t tmp;
>
> - WARN_ON_ONCE(power_well->id != PUNIT_POWER_WELL_DPIO_CMN_BC
> &&
> - power_well->id != PUNIT_POWER_WELL_DPIO_CMN_D);
> + WARN_ON_ONCE(power_well->desc->id !=
> PUNIT_POWER_WELL_DPIO_CMN_BC &&
> + power_well->desc->id !=
> PUNIT_POWER_WELL_DPIO_CMN_D);
>
> - if (power_well->id == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> + if (power_well->desc->id == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> pipe = PIPE_A;
> phy = DPIO_PHY0;
> } else {
> @@ -1262,7 +1265,7 @@ static void
> chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> DPIO_SUS_CLK_CONFIG_GATE_CLKREQ;
> vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW28, tmp);
>
> - if (power_well->id == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> + if (power_well->desc->id == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> tmp = vlv_dpio_read(dev_priv, pipe,
> _CHV_CMN_DW6_CH1);
> tmp |= DPIO_DYNPWRDOWNEN_CH1;
> vlv_dpio_write(dev_priv, pipe, _CHV_CMN_DW6_CH1,
> tmp);
> @@ -1293,10 +1296,10 @@ static void
> chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> {
> enum dpio_phy phy;
>
> - WARN_ON_ONCE(power_well->id != PUNIT_POWER_WELL_DPIO_CMN_BC
> &&
> - power_well->id != PUNIT_POWER_WELL_DPIO_CMN_D);
> + WARN_ON_ONCE(power_well->desc->id !=
> PUNIT_POWER_WELL_DPIO_CMN_BC &&
> + power_well->desc->id !=
> PUNIT_POWER_WELL_DPIO_CMN_D);
>
> - if (power_well->id == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> + if (power_well->desc->id == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> phy = DPIO_PHY0;
> assert_pll_disabled(dev_priv, PIPE_A);
> assert_pll_disabled(dev_priv, PIPE_B);
> @@ -2051,7 +2054,7 @@ static const struct i915_power_well_ops
> chv_dpio_cmn_power_well_ops = {
> .is_enabled = vlv_power_well_enabled,
> };
>
> -static struct i915_power_well i9xx_always_on_power_well[] = {
> +static const struct i915_power_well_desc i9xx_always_on_power_well[]
> = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2068,7 +2071,7 @@ static const struct i915_power_well_ops
> i830_pipes_power_well_ops = {
> .is_enabled = i830_pipes_power_well_enabled,
> };
>
> -static struct i915_power_well i830_power_wells[] = {
> +static const struct i915_power_well_desc i830_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2105,7 +2108,7 @@ static const struct i915_power_well_ops
> bxt_dpio_cmn_power_well_ops = {
> .is_enabled = bxt_dpio_cmn_power_well_enabled,
> };
>
> -static struct i915_power_well hsw_power_wells[] = {
> +static const struct i915_power_well_desc hsw_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2124,7 +2127,7 @@ static struct i915_power_well hsw_power_wells[]
> = {
> },
> };
>
> -static struct i915_power_well bdw_power_wells[] = {
> +static const struct i915_power_well_desc bdw_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2165,7 +2168,7 @@ static const struct i915_power_well_ops
> vlv_dpio_power_well_ops = {
> .is_enabled = vlv_power_well_enabled,
> };
>
> -static struct i915_power_well vlv_power_wells[] = {
> +static const struct i915_power_well_desc vlv_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2223,7 +2226,7 @@ static struct i915_power_well vlv_power_wells[]
> = {
> },
> };
>
> -static struct i915_power_well chv_power_wells[] = {
> +static const struct i915_power_well_desc chv_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2263,12 +2266,12 @@ bool
> intel_display_power_well_is_enabled(struct drm_i915_private
> *dev_priv,
> bool ret;
>
> power_well = lookup_power_well(dev_priv, power_well_id);
> - ret = power_well->ops->is_enabled(dev_priv, power_well);
> + ret = power_well->desc->ops->is_enabled(dev_priv,
> power_well);
>
> return ret;
> }
>
> -static struct i915_power_well skl_power_wells[] = {
> +static const struct i915_power_well_desc skl_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2336,7 +2339,7 @@ static struct i915_power_well skl_power_wells[]
> = {
> },
> };
>
> -static struct i915_power_well bxt_power_wells[] = {
> +static const struct i915_power_well_desc bxt_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2390,7 +2393,7 @@ static struct i915_power_well bxt_power_wells[]
> = {
> },
> };
>
> -static struct i915_power_well glk_power_wells[] = {
> +static const struct i915_power_well_desc glk_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2490,7 +2493,7 @@ static struct i915_power_well glk_power_wells[]
> = {
> },
> };
>
> -static struct i915_power_well cnl_power_wells[] = {
> +static const struct i915_power_well_desc cnl_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2594,7 +2597,7 @@ static const struct i915_power_well_ops
> icl_combo_phy_aux_power_well_ops = {
> .is_enabled = hsw_power_well_enabled,
> };
>
> -static struct i915_power_well icl_power_wells[] = {
> +static const struct i915_power_well_desc icl_power_wells[] = {
> {
> .name = "always-on",
> .always_on = 1,
> @@ -2813,7 +2816,7 @@ static void assert_power_well_ids_unique(struct
> drm_i915_private *dev_priv)
>
> power_well_ids = 0;
> for (i = 0; i < power_domains->power_well_count; i++) {
> - enum i915_power_well_id id = power_domains-
> >power_wells[i].id;
> + enum i915_power_well_id id = power_domains-
> >power_wells[i].desc->id;
>
> WARN_ON(id >= sizeof(power_well_ids) * 8);
> WARN_ON(power_well_ids & BIT_ULL(id));
> @@ -2821,10 +2824,28 @@ static void
> assert_power_well_ids_unique(struct drm_i915_private *dev_priv)
> }
> }
>
> -#define set_power_wells(power_domains, __power_wells) ({
> \
> - (power_domains)->power_wells = (__power_wells);
> \
> - (power_domains)->power_well_count =
> ARRAY_SIZE(__power_wells); \
> -})
> +static int __set_power_wells(struct i915_power_domains
> *power_domains,
> + const struct i915_power_well_desc
> *power_well_descs,
> + int power_well_count)
> +{
> + int i;
> +
> + power_domains->power_well_count = power_well_count;
> + power_domains->power_wells = kcalloc(power_well_count,
> + sizeof(*power_domains-
> >power_wells),
> + GFP_KERNEL);
> + if (!power_domains->power_wells)
> + return -ENOMEM;
> +
> + for (i = 0; i < power_well_count; i++)
> + power_domains->power_wells[i].desc =
> &power_well_descs[i];
> +
> + return 0;
> +}
> +
> +#define set_power_wells(power_domains, __power_well_descs) \
> + __set_power_wells(power_domains, __power_well_descs, \
> + ARRAY_SIZE(__power_well_descs))
>
> /**
> * intel_power_domains_init - initializes the power domain
> structures
> @@ -2836,6 +2857,7 @@ static void assert_power_well_ids_unique(struct
> drm_i915_private *dev_priv)
> int intel_power_domains_init(struct drm_i915_private *dev_priv)
> {
> struct i915_power_domains *power_domains = &dev_priv-
> >power_domains;
> + int err;
>
> i915_modparams.disable_power_well =
> sanitize_disable_power_well_option(dev_priv,
> @@ -2852,15 +2874,15 @@ int intel_power_domains_init(struct
> drm_i915_private *dev_priv)
> * the disabling order is reversed.
> */
> if (IS_ICELAKE(dev_priv)) {
> - set_power_wells(power_domains, icl_power_wells);
> + err = set_power_wells(power_domains,
> icl_power_wells);
> } else if (IS_HASWELL(dev_priv)) {
> - set_power_wells(power_domains, hsw_power_wells);
> + err = set_power_wells(power_domains,
> hsw_power_wells);
> } else if (IS_BROADWELL(dev_priv)) {
> - set_power_wells(power_domains, bdw_power_wells);
> + err = set_power_wells(power_domains,
> bdw_power_wells);
> } else if (IS_GEN9_BC(dev_priv)) {
> - set_power_wells(power_domains, skl_power_wells);
> + err = set_power_wells(power_domains,
> skl_power_wells);
> } else if (IS_CANNONLAKE(dev_priv)) {
> - set_power_wells(power_domains, cnl_power_wells);
> + err = set_power_wells(power_domains,
> cnl_power_wells);
>
> /*
> * DDI and Aux IO are getting enabled for all ports
> @@ -2872,22 +2894,34 @@ int intel_power_domains_init(struct
> drm_i915_private *dev_priv)
> power_domains->power_well_count -= 2;
>
> } else if (IS_BROXTON(dev_priv)) {
> - set_power_wells(power_domains, bxt_power_wells);
> + err = set_power_wells(power_domains,
> bxt_power_wells);
> } else if (IS_GEMINILAKE(dev_priv)) {
> - set_power_wells(power_domains, glk_power_wells);
> + err = set_power_wells(power_domains,
> glk_power_wells);
> } else if (IS_CHERRYVIEW(dev_priv)) {
> - set_power_wells(power_domains, chv_power_wells);
> + err = set_power_wells(power_domains,
> chv_power_wells);
> } else if (IS_VALLEYVIEW(dev_priv)) {
> - set_power_wells(power_domains, vlv_power_wells);
> + err = set_power_wells(power_domains,
> vlv_power_wells);
> } else if (IS_I830(dev_priv)) {
> - set_power_wells(power_domains, i830_power_wells);
> + err = set_power_wells(power_domains,
> i830_power_wells);
> } else {
> - set_power_wells(power_domains,
> i9xx_always_on_power_well);
> + err = set_power_wells(power_domains,
> i9xx_always_on_power_well);
> }
>
> - assert_power_well_ids_unique(dev_priv);
> + if (!err)
> + assert_power_well_ids_unique(dev_priv);
>
> - return 0;
> + return err;
> +}
> +
> +/**
> + * intel_power_domains_cleanup - clean up power domains resources
> + * @dev_priv: i915 device instance
> + *
> + * Release any resources acquired by intel_power_domains_init()
> + */
> +void intel_power_domains_cleanup(struct drm_i915_private *dev_priv)
> +{
> + kfree(dev_priv->power_domains.power_wells);
> }
>
> static void intel_power_domains_sync_hw(struct drm_i915_private
> *dev_priv)
> @@ -2897,9 +2931,9 @@ static void intel_power_domains_sync_hw(struct
> drm_i915_private *dev_priv)
>
> mutex_lock(&power_domains->lock);
> for_each_power_well(dev_priv, power_well) {
> - power_well->ops->sync_hw(dev_priv, power_well);
> - power_well->hw_enabled = power_well->ops-
> >is_enabled(dev_priv,
> -
> power_well);
> + power_well->desc->ops->sync_hw(dev_priv,
> power_well);
> + power_well->hw_enabled =
> + power_well->desc->ops->is_enabled(dev_priv,
> power_well);
> }
> mutex_unlock(&power_domains->lock);
> }
> @@ -3402,7 +3436,7 @@ static void chv_phy_control_init(struct
> drm_i915_private *dev_priv)
> * override and set the lane powerdown bits accding to the
> * current lane status.
> */
> - if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> + if (cmn_bc->desc->ops->is_enabled(dev_priv, cmn_bc)) {
> uint32_t status = I915_READ(DPLL(PIPE_A));
> unsigned int mask;
>
> @@ -3433,7 +3467,7 @@ static void chv_phy_control_init(struct
> drm_i915_private *dev_priv)
> dev_priv->chv_phy_assert[DPIO_PHY0] = true;
> }
>
> - if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> + if (cmn_d->desc->ops->is_enabled(dev_priv, cmn_d)) {
> uint32_t status = I915_READ(DPIO_PHY_STATUS);
> unsigned int mask;
>
> @@ -3469,15 +3503,15 @@ static void vlv_cmnlane_wa(struct
> drm_i915_private *dev_priv)
> lookup_power_well(dev_priv,
> PUNIT_POWER_WELL_DISP2D);
>
> /* If the display might be already active skip this */
> - if (cmn->ops->is_enabled(dev_priv, cmn) &&
> - disp2d->ops->is_enabled(dev_priv, disp2d) &&
> + if (cmn->desc->ops->is_enabled(dev_priv, cmn) &&
> + disp2d->desc->ops->is_enabled(dev_priv, disp2d) &&
> I915_READ(DPIO_CTL) & DPIO_CMNRST)
> return;
>
> DRM_DEBUG_KMS("toggling display PHY side reset\n");
>
> /* cmnlane needs DPLL registers */
> - disp2d->ops->enable(dev_priv, disp2d);
> + disp2d->desc->ops->enable(dev_priv, disp2d);
>
> /*
> * From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx:
> @@ -3486,7 +3520,7 @@ static void vlv_cmnlane_wa(struct
> drm_i915_private *dev_priv)
> * Simply ungating isn't enough to reset the PHY enough to
> get
> * ports and lanes running.
> */
> - cmn->ops->disable(dev_priv, cmn);
> + cmn->desc->ops->disable(dev_priv, cmn);
> }
>
> /**
> @@ -3602,9 +3636,9 @@ static void
> intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
> enum intel_display_power_domain domain;
>
> DRM_DEBUG_DRIVER("%-25s %d\n",
> - power_well->name, power_well-
> >count);
> + power_well->desc->name, power_well-
> >count);
>
> - for_each_power_domain(domain, power_well->domains)
> + for_each_power_domain(domain, power_well->desc-
> >domains)
> DRM_DEBUG_DRIVER(" %-23s %d\n",
> intel_display_power_domain_
> str(domain),
> power_domains-
> >domain_use_count[domain]);
> @@ -3640,22 +3674,24 @@ void intel_power_domains_verify_state(struct
> drm_i915_private *dev_priv)
> * and PW1 power wells) are under FW control, so
> ignore them,
> * since their state can change asynchronously.
> */
> - if (!power_well->domains)
> + if (!power_well->desc->domains)
> continue;
>
> - enabled = power_well->ops->is_enabled(dev_priv,
> power_well);
> - if ((power_well->count || power_well->always_on) !=
> enabled)
> + enabled = power_well->desc->ops-
> >is_enabled(dev_priv,
> + power_we
> ll);
> + if ((power_well->count || power_well->desc-
> >always_on) != enabled)
> DRM_ERROR("power well %s state mismatch
> (refcount %d/enabled %d)",
> - power_well->name, power_well-
> >count, enabled);
> + power_well->desc->name,
> + power_well->count, enabled);
>
> domains_count = 0;
> - for_each_power_domain(domain, power_well->domains)
> + for_each_power_domain(domain, power_well->desc-
> >domains)
> domains_count += power_domains-
> >domain_use_count[domain];
>
> if (power_well->count != domains_count) {
> DRM_ERROR("power well %s refcount/domain
> refcount mismatch "
> "(refcount %d/domains refcount
> %d)\n",
> - power_well->name, power_well-
> >count,
> + power_well->desc->name,
> power_well->count,
> domains_count);
> dump_domain_info = true;
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2018-08-01 21:39 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-20 14:14 [PATCH 00/10] drm/i915: Clean up power well descriptors Imre Deak
2018-07-20 14:14 ` [PATCH 01/10] drm/i915/icl: Fix power well anonymous union initializers Imre Deak
2018-07-30 6:07 ` Rodrigo Vivi
2018-08-01 21:01 ` Paulo Zanoni
2018-08-02 23:17 ` Lucas De Marchi
2018-08-03 0:24 ` Paulo Zanoni
2018-08-03 10:23 ` Imre Deak
2018-08-03 21:09 ` Lucas De Marchi
2018-07-20 14:14 ` [PATCH 02/10] drm/i915: Rename intel_power_domains_fini() to intel_power_domains_fini_hw() Imre Deak
2018-07-30 6:09 ` Rodrigo Vivi
2018-08-01 21:02 ` Paulo Zanoni
2018-07-20 14:14 ` [PATCH 03/10] drm/i915/vlv: Remove redundant power well ID asserts Imre Deak
2018-08-01 21:03 ` Paulo Zanoni
2018-07-20 14:14 ` [PATCH 04/10] drm/i915: Constify power well descriptors Imre Deak
2018-08-01 21:39 ` Paulo Zanoni [this message]
2018-08-02 12:03 ` Imre Deak
2018-08-02 23:04 ` Paulo Zanoni
2018-07-20 14:14 ` [PATCH 05/10] drm/i915/vlv: Use power well CTL IDX instead of ID Imre Deak
2018-08-01 22:15 ` Paulo Zanoni
2018-08-02 11:56 ` Imre Deak
2018-07-20 14:15 ` [PATCH 06/10] drm/i915/ddi: " Imre Deak
2018-08-02 20:40 ` Paulo Zanoni
2018-08-03 9:13 ` Imre Deak
2018-07-20 14:15 ` [PATCH 07/10] drm/i915: Remove redundant power well IDs Imre Deak
2018-08-02 21:26 ` Paulo Zanoni
2018-08-03 9:31 ` Imre Deak
2018-07-20 14:15 ` [PATCH 08/10] drm/i915: Make power well ID names more uniform Imre Deak
2018-08-02 21:32 ` Paulo Zanoni
2018-07-20 14:15 ` [PATCH 09/10] drm/i915: Use existing power well IDs where possible Imre Deak
2018-08-02 21:39 ` Paulo Zanoni
2018-08-03 9:34 ` Imre Deak
2018-07-20 14:15 ` [PATCH 10/10] drm/i915/icl: Add missing power gate enums Imre Deak
2018-08-02 21:46 ` Paulo Zanoni
2018-07-20 14:53 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Clean up power well descriptors Patchwork
2018-07-20 14:57 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-07-20 15:15 ` ✓ Fi.CI.BAT: success " Patchwork
2018-07-21 15:39 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1533159571.2571.11.camel@intel.com \
--to=paulo.r.zanoni@intel.com \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.