| Message ID | 1499352040-8819-3-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 6, 2017 at 7:40 AM, Imre Deak <imre.deak@intel.com> wrote: > Atm, the power well IDs are defined in separate platform specific enums, > which isn't ideal for the following reasons: > - the IDs are used by helpers like lookup_power_well() in a platform > independent way > - the always-on power well is used by multiple platforms and so needs > now separate IDs, although these IDs refer to the same thing I liked the always-on unifying... so much that I believe it deserved a separated patch. > > To make things more consistent use a single enum instead of the two > separate ones, listing the IDs per platform (or set of very similar > platforms like all GEN9/10). Replace the separate always-on power > well IDs with a single ID. > > While at it also add a note clarifying the distinction between regular > power wells that follow a common programming pattern and custom ones > that are programmed in some other way. The IDs for regular power wells > need to stay fixed, since they also define the request and state HW flag > positions in their corresponding power well control register(s). > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/i915_reg.h | 41 ++++++++++++++++++++++----------- > drivers/gpu/drm/i915/intel_runtime_pm.c | 29 ++++++++++++----------- > 3 files changed, 44 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 81cd21e..c9b98ed 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1382,7 +1382,7 @@ struct i915_power_well { > bool hw_enabled; > u64 domains; > /* unique identifier for this power well */ > - unsigned long id; > + enum i915_power_well_id id; > /* > * Arbitraty data associated with this power well. Platform and power > * well specific. > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 3f7beff..e4135bd 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1063,9 +1063,19 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > #define DP_SSS_RESET(pipe) _DP_SSS(0x2, (pipe)) > #define DP_SSS_PWR_GATE(pipe) _DP_SSS(0x3, (pipe)) > > -/* See the PUNIT HAS v0.8 for the below bits */ > -enum punit_power_well { > - /* These numbers are fixed and must match the position of the pw bits */ > +/** > + * i915_power_well_id: > + * > + * Platform specific IDs used to look up power wells and - except for custom > + * power wells - to define request/status register flag bit positions. As such > + * the set of IDs on a given platform must be unique and except for custom > + * power wells their value must stay fixed. > + */ > +enum i915_power_well_id { > + /* > + * VLV/CHV > + * - PUNIT_REG_PWRGT_CTRL, PUNIT_REG_PWRGT_STATUS (PUNIT HAS v0.8) > + */ > PUNIT_POWER_WELL_RENDER = 0, > PUNIT_POWER_WELL_MEDIA = 1, > PUNIT_POWER_WELL_DISP2D = 3, > @@ -1080,13 +1090,11 @@ enum punit_power_well { > /* - custom power well */ > CHV_DISP_PW_PIPE_A, /* 13 */ > > - /* Not actual bit groups. Used as IDs for lookup_power_well() */ > - PUNIT_POWER_WELL_ALWAYS_ON, > -}; > - > -enum skl_disp_power_wells { > - /* These numbers are fixed and must match the position of the pw bits */ > - SKL_DISP_PW_MISC_IO, > + /* > + * GEN9+ > + * - HSW_PWR_WELL_DRIVER > + */ > + SKL_DISP_PW_MISC_IO = 0, > SKL_DISP_PW_DDI_A_E, > GLK_DISP_PW_DDI_A = SKL_DISP_PW_DDI_A_E, > CNL_DISP_PW_DDI_A = SKL_DISP_PW_DDI_A_E, > @@ -1105,13 +1113,18 @@ enum skl_disp_power_wells { > SKL_DISP_PW_1 = 14, > SKL_DISP_PW_2, > > - /* Not actual bit groups. Used as IDs for lookup_power_well() */ > - SKL_DISP_PW_ALWAYS_ON, > + /* - custom power wells */ > SKL_DISP_PW_DC_OFF, > - > BXT_DPIO_CMN_A, > BXT_DPIO_CMN_BC, > - GLK_DPIO_CMN_C, > + GLK_DPIO_CMN_C, /* 19 */ > + > + /* > + * Multiple platforms. > + * Must start following the highest ID of any platform. > + * - custom power wells > + */ > + I915_DISP_PW_ALWAYS_ON = 20, What about just leaving I915_DISP_PW_ALWAYS_ON, I have the feeling that if that increases we will forget to update here.... but I will leave you to decide... so, with or without any split or change feel free to use: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > }; > > #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 5f5dee4..ad314c1 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -50,10 +50,11 @@ > */ > > bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > - int power_well_id); > + enum i915_power_well_id power_well_id); > > static struct i915_power_well * > -lookup_power_well(struct drm_i915_private *dev_priv, int power_well_id); > +lookup_power_well(struct drm_i915_private *dev_priv, > + enum i915_power_well_id power_well_id); > > const char * > intel_display_power_domain_str(enum intel_display_power_domain domain) > @@ -344,7 +345,7 @@ static void skl_power_well_pre_disable(struct drm_i915_private *dev_priv, > static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv, > struct i915_power_well *power_well) > { > - int id = power_well->id; > + enum i915_power_well_id id = power_well->id; > > /* Timeout for PW1:10 us, AUX:not specified, other PWs:20 us. */ > WARN_ON(intel_wait_for_register(dev_priv, > @@ -354,7 +355,8 @@ static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv, > 1)); > } > > -static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id) > +static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, > + enum i915_power_well_id id) > { > u32 req_mask = SKL_POWER_WELL_REQ(id); > u32 ret; > @@ -370,7 +372,7 @@ static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id) > static void gen9_wait_for_power_well_disable(struct drm_i915_private *dev_priv, > struct i915_power_well *power_well) > { > - int id = power_well->id; > + enum i915_power_well_id id = power_well->id; > bool disabled; > u32 reqs; > > @@ -837,7 +839,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > case CNL_DISP_PW_AUX_D: > break; > default: > - WARN(1, "Unknown power well %lu\n", power_well->id); > + WARN(1, "Unknown power well %u\n", power_well->id); > return; > } > > @@ -1089,7 +1091,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 punit_power_well power_well_id = power_well->id; > + enum i915_power_well_id power_well_id = power_well->id; > u32 mask; > u32 state; > u32 ctrl; > @@ -1137,7 +1139,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) > { > - int power_well_id = power_well->id; > + enum i915_power_well_id power_well_id = power_well->id; > bool enabled = false; > u32 mask; > u32 state; > @@ -1324,8 +1326,9 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, > > #define POWER_DOMAIN_MASK (GENMASK_ULL(POWER_DOMAIN_NUM - 1, 0)) > > -static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv, > - int power_well_id) > +static struct i915_power_well * > +lookup_power_well(struct drm_i915_private *dev_priv, > + enum i915_power_well_id power_well_id) > { > struct i915_power_domains *power_domains = &dev_priv->power_domains; > int i; > @@ -2117,7 +2120,7 @@ static struct i915_power_well vlv_power_wells[] = { > .always_on = 1, > .domains = POWER_DOMAIN_MASK, > .ops = &i9xx_always_on_power_well_ops, > - .id = PUNIT_POWER_WELL_ALWAYS_ON, > + .id = I915_DISP_PW_ALWAYS_ON, > }, > { > .name = "display", > @@ -2202,7 +2205,7 @@ static struct i915_power_well chv_power_wells[] = { > }; > > bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > - int power_well_id) > + enum i915_power_well_id power_well_id) > { > struct i915_power_well *power_well; > bool ret; > @@ -2219,7 +2222,7 @@ static struct i915_power_well skl_power_wells[] = { > .always_on = 1, > .domains = POWER_DOMAIN_MASK, > .ops = &i9xx_always_on_power_well_ops, > - .id = SKL_DISP_PW_ALWAYS_ON, > + .id = I915_DISP_PW_ALWAYS_ON, > }, > { > .name = "power well 1", > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jul 11, 2017 at 9:43 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote: > On Thu, Jul 6, 2017 at 7:40 AM, Imre Deak <imre.deak@intel.com> wrote: >> Atm, the power well IDs are defined in separate platform specific enums, >> which isn't ideal for the following reasons: >> - the IDs are used by helpers like lookup_power_well() in a platform >> independent way >> - the always-on power well is used by multiple platforms and so needs >> now separate IDs, although these IDs refer to the same thing > > I liked the always-on unifying... so much that I believe it deserved > a separated patch. > >> >> To make things more consistent use a single enum instead of the two >> separate ones, listing the IDs per platform (or set of very similar >> platforms like all GEN9/10). Replace the separate always-on power >> well IDs with a single ID. >> >> While at it also add a note clarifying the distinction between regular >> power wells that follow a common programming pattern and custom ones >> that are programmed in some other way. The IDs for regular power wells >> need to stay fixed, since they also define the request and state HW flag >> positions in their corresponding power well control register(s). >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Imre Deak <imre.deak@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 2 +- >> drivers/gpu/drm/i915/i915_reg.h | 41 ++++++++++++++++++++++----------- >> drivers/gpu/drm/i915/intel_runtime_pm.c | 29 ++++++++++++----------- >> 3 files changed, 44 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 81cd21e..c9b98ed 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1382,7 +1382,7 @@ struct i915_power_well { >> bool hw_enabled; >> u64 domains; >> /* unique identifier for this power well */ >> - unsigned long id; >> + enum i915_power_well_id id; >> /* >> * Arbitraty data associated with this power well. Platform and power >> * well specific. >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 3f7beff..e4135bd 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -1063,9 +1063,19 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) >> #define DP_SSS_RESET(pipe) _DP_SSS(0x2, (pipe)) >> #define DP_SSS_PWR_GATE(pipe) _DP_SSS(0x3, (pipe)) >> >> -/* See the PUNIT HAS v0.8 for the below bits */ >> -enum punit_power_well { >> - /* These numbers are fixed and must match the position of the pw bits */ >> +/** >> + * i915_power_well_id: >> + * >> + * Platform specific IDs used to look up power wells and - except for custom >> + * power wells - to define request/status register flag bit positions. As such >> + * the set of IDs on a given platform must be unique and except for custom >> + * power wells their value must stay fixed. Actually I have one request here. Could you please add a comment that state bit is id*2 and req is id*2+1? Before your series this definition was right below, but with your series applied it takes me a few extra steps to remember where it was defined to check that HSW/BDW id 15... >> + */ >> +enum i915_power_well_id { >> + /* >> + * VLV/CHV >> + * - PUNIT_REG_PWRGT_CTRL, PUNIT_REG_PWRGT_STATUS (PUNIT HAS v0.8) >> + */ >> PUNIT_POWER_WELL_RENDER = 0, >> PUNIT_POWER_WELL_MEDIA = 1, >> PUNIT_POWER_WELL_DISP2D = 3, >> @@ -1080,13 +1090,11 @@ enum punit_power_well { >> /* - custom power well */ >> CHV_DISP_PW_PIPE_A, /* 13 */ >> >> - /* Not actual bit groups. Used as IDs for lookup_power_well() */ >> - PUNIT_POWER_WELL_ALWAYS_ON, >> -}; >> - >> -enum skl_disp_power_wells { >> - /* These numbers are fixed and must match the position of the pw bits */ >> - SKL_DISP_PW_MISC_IO, >> + /* >> + * GEN9+ >> + * - HSW_PWR_WELL_DRIVER >> + */ >> + SKL_DISP_PW_MISC_IO = 0, >> SKL_DISP_PW_DDI_A_E, >> GLK_DISP_PW_DDI_A = SKL_DISP_PW_DDI_A_E, >> CNL_DISP_PW_DDI_A = SKL_DISP_PW_DDI_A_E, >> @@ -1105,13 +1113,18 @@ enum skl_disp_power_wells { >> SKL_DISP_PW_1 = 14, >> SKL_DISP_PW_2, >> >> - /* Not actual bit groups. Used as IDs for lookup_power_well() */ >> - SKL_DISP_PW_ALWAYS_ON, >> + /* - custom power wells */ >> SKL_DISP_PW_DC_OFF, >> - >> BXT_DPIO_CMN_A, >> BXT_DPIO_CMN_BC, >> - GLK_DPIO_CMN_C, >> + GLK_DPIO_CMN_C, /* 19 */ >> + >> + /* >> + * Multiple platforms. >> + * Must start following the highest ID of any platform. >> + * - custom power wells >> + */ >> + I915_DISP_PW_ALWAYS_ON = 20, > > What about just leaving > I915_DISP_PW_ALWAYS_ON, > > I have the feeling that if that increases we will forget to update here.... > > but I will leave you to decide... so, with or without any split or > change feel free to use: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> }; >> >> #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c >> index 5f5dee4..ad314c1 100644 >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c >> @@ -50,10 +50,11 @@ >> */ >> >> bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, >> - int power_well_id); >> + enum i915_power_well_id power_well_id); >> >> static struct i915_power_well * >> -lookup_power_well(struct drm_i915_private *dev_priv, int power_well_id); >> +lookup_power_well(struct drm_i915_private *dev_priv, >> + enum i915_power_well_id power_well_id); >> >> const char * >> intel_display_power_domain_str(enum intel_display_power_domain domain) >> @@ -344,7 +345,7 @@ static void skl_power_well_pre_disable(struct drm_i915_private *dev_priv, >> static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv, >> struct i915_power_well *power_well) >> { >> - int id = power_well->id; >> + enum i915_power_well_id id = power_well->id; >> >> /* Timeout for PW1:10 us, AUX:not specified, other PWs:20 us. */ >> WARN_ON(intel_wait_for_register(dev_priv, >> @@ -354,7 +355,8 @@ static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv, >> 1)); >> } >> >> -static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id) >> +static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, >> + enum i915_power_well_id id) >> { >> u32 req_mask = SKL_POWER_WELL_REQ(id); >> u32 ret; >> @@ -370,7 +372,7 @@ static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id) >> static void gen9_wait_for_power_well_disable(struct drm_i915_private *dev_priv, >> struct i915_power_well *power_well) >> { >> - int id = power_well->id; >> + enum i915_power_well_id id = power_well->id; >> bool disabled; >> u32 reqs; >> >> @@ -837,7 +839,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, >> case CNL_DISP_PW_AUX_D: >> break; >> default: >> - WARN(1, "Unknown power well %lu\n", power_well->id); >> + WARN(1, "Unknown power well %u\n", power_well->id); >> return; >> } >> >> @@ -1089,7 +1091,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 punit_power_well power_well_id = power_well->id; >> + enum i915_power_well_id power_well_id = power_well->id; >> u32 mask; >> u32 state; >> u32 ctrl; >> @@ -1137,7 +1139,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) >> { >> - int power_well_id = power_well->id; >> + enum i915_power_well_id power_well_id = power_well->id; >> bool enabled = false; >> u32 mask; >> u32 state; >> @@ -1324,8 +1326,9 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, >> >> #define POWER_DOMAIN_MASK (GENMASK_ULL(POWER_DOMAIN_NUM - 1, 0)) >> >> -static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv, >> - int power_well_id) >> +static struct i915_power_well * >> +lookup_power_well(struct drm_i915_private *dev_priv, >> + enum i915_power_well_id power_well_id) >> { >> struct i915_power_domains *power_domains = &dev_priv->power_domains; >> int i; >> @@ -2117,7 +2120,7 @@ static struct i915_power_well vlv_power_wells[] = { >> .always_on = 1, >> .domains = POWER_DOMAIN_MASK, >> .ops = &i9xx_always_on_power_well_ops, >> - .id = PUNIT_POWER_WELL_ALWAYS_ON, >> + .id = I915_DISP_PW_ALWAYS_ON, >> }, >> { >> .name = "display", >> @@ -2202,7 +2205,7 @@ static struct i915_power_well chv_power_wells[] = { >> }; >> >> bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, >> - int power_well_id) >> + enum i915_power_well_id power_well_id) >> { >> struct i915_power_well *power_well; >> bool ret; >> @@ -2219,7 +2222,7 @@ static struct i915_power_well skl_power_wells[] = { >> .always_on = 1, >> .domains = POWER_DOMAIN_MASK, >> .ops = &i9xx_always_on_power_well_ops, >> - .id = SKL_DISP_PW_ALWAYS_ON, >> + .id = I915_DISP_PW_ALWAYS_ON, >> }, >> { >> .name = "power well 1", >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br
On Tue, Jul 11, 2017 at 10:21:55AM -0700, Rodrigo Vivi wrote: > On Tue, Jul 11, 2017 at 9:43 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote: > > On Thu, Jul 6, 2017 at 7:40 AM, Imre Deak <imre.deak@intel.com> wrote: > >> Atm, the power well IDs are defined in separate platform specific enums, > >> which isn't ideal for the following reasons: > >> - the IDs are used by helpers like lookup_power_well() in a platform > >> independent way > >> - the always-on power well is used by multiple platforms and so needs > >> now separate IDs, although these IDs refer to the same thing > > > > I liked the always-on unifying... so much that I believe it deserved > > a separated patch. > > > >> > >> To make things more consistent use a single enum instead of the two > >> separate ones, listing the IDs per platform (or set of very similar > >> platforms like all GEN9/10). Replace the separate always-on power > >> well IDs with a single ID. > >> > >> While at it also add a note clarifying the distinction between regular > >> power wells that follow a common programming pattern and custom ones > >> that are programmed in some other way. The IDs for regular power wells > >> need to stay fixed, since they also define the request and state HW flag > >> positions in their corresponding power well control register(s). > >> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Signed-off-by: Imre Deak <imre.deak@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_drv.h | 2 +- > >> drivers/gpu/drm/i915/i915_reg.h | 41 ++++++++++++++++++++++----------- > >> drivers/gpu/drm/i915/intel_runtime_pm.c | 29 ++++++++++++----------- > >> 3 files changed, 44 insertions(+), 28 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index 81cd21e..c9b98ed 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -1382,7 +1382,7 @@ struct i915_power_well { > >> bool hw_enabled; > >> u64 domains; > >> /* unique identifier for this power well */ > >> - unsigned long id; > >> + enum i915_power_well_id id; > >> /* > >> * Arbitraty data associated with this power well. Platform and power > >> * well specific. > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index 3f7beff..e4135bd 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -1063,9 +1063,19 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > >> #define DP_SSS_RESET(pipe) _DP_SSS(0x2, (pipe)) > >> #define DP_SSS_PWR_GATE(pipe) _DP_SSS(0x3, (pipe)) > >> > >> -/* See the PUNIT HAS v0.8 for the below bits */ > >> -enum punit_power_well { > >> - /* These numbers are fixed and must match the position of the pw bits */ > >> +/** > >> + * i915_power_well_id: > >> + * > >> + * Platform specific IDs used to look up power wells and - except for custom > >> + * power wells - to define request/status register flag bit positions. As such > >> + * the set of IDs on a given platform must be unique and except for custom > >> + * power wells their value must stay fixed. > > Actually I have one request here. > > Could you please add a comment that state bit is id*2 and req is id*2+1? > > Before your series this definition was right below, but with your > series applied it takes me a few extra steps to remember where it was > defined to check that HSW/BDW id 15... That's different on VLV/CHV and HSW+. I added the actual registers below where the mapping is described, but can copy that info to here as well. > > >> + */ > >> +enum i915_power_well_id { > >> + /* > >> + * VLV/CHV > >> + * - PUNIT_REG_PWRGT_CTRL, PUNIT_REG_PWRGT_STATUS (PUNIT HAS v0.8) > >> + */ > >> PUNIT_POWER_WELL_RENDER = 0, > >> PUNIT_POWER_WELL_MEDIA = 1, > >> PUNIT_POWER_WELL_DISP2D = 3, > >> @@ -1080,13 +1090,11 @@ enum punit_power_well { > >> /* - custom power well */ > >> CHV_DISP_PW_PIPE_A, /* 13 */ > >> > >> - /* Not actual bit groups. Used as IDs for lookup_power_well() */ > >> - PUNIT_POWER_WELL_ALWAYS_ON, > >> -}; > >> - > >> -enum skl_disp_power_wells { > >> - /* These numbers are fixed and must match the position of the pw bits */ > >> - SKL_DISP_PW_MISC_IO, > >> + /* > >> + * GEN9+ > >> + * - HSW_PWR_WELL_DRIVER > >> + */ > >> + SKL_DISP_PW_MISC_IO = 0, > >> SKL_DISP_PW_DDI_A_E, > >> GLK_DISP_PW_DDI_A = SKL_DISP_PW_DDI_A_E, > >> CNL_DISP_PW_DDI_A = SKL_DISP_PW_DDI_A_E, > >> @@ -1105,13 +1113,18 @@ enum skl_disp_power_wells { > >> SKL_DISP_PW_1 = 14, > >> SKL_DISP_PW_2, > >> > >> - /* Not actual bit groups. Used as IDs for lookup_power_well() */ > >> - SKL_DISP_PW_ALWAYS_ON, > >> + /* - custom power wells */ > >> SKL_DISP_PW_DC_OFF, > >> - > >> BXT_DPIO_CMN_A, > >> BXT_DPIO_CMN_BC, > >> - GLK_DPIO_CMN_C, > >> + GLK_DPIO_CMN_C, /* 19 */ > >> + > >> + /* > >> + * Multiple platforms. > >> + * Must start following the highest ID of any platform. > >> + * - custom power wells > >> + */ > >> + I915_DISP_PW_ALWAYS_ON = 20, > > > > What about just leaving > > I915_DISP_PW_ALWAYS_ON, > > > > I have the feeling that if that increases we will forget to update here.... > > > > but I will leave you to decide... so, with or without any split or > > change feel free to use: > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > >> }; > >> > >> #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) > >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > >> index 5f5dee4..ad314c1 100644 > >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > >> @@ -50,10 +50,11 @@ > >> */ > >> > >> bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > >> - int power_well_id); > >> + enum i915_power_well_id power_well_id); > >> > >> static struct i915_power_well * > >> -lookup_power_well(struct drm_i915_private *dev_priv, int power_well_id); > >> +lookup_power_well(struct drm_i915_private *dev_priv, > >> + enum i915_power_well_id power_well_id); > >> > >> const char * > >> intel_display_power_domain_str(enum intel_display_power_domain domain) > >> @@ -344,7 +345,7 @@ static void skl_power_well_pre_disable(struct drm_i915_private *dev_priv, > >> static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv, > >> struct i915_power_well *power_well) > >> { > >> - int id = power_well->id; > >> + enum i915_power_well_id id = power_well->id; > >> > >> /* Timeout for PW1:10 us, AUX:not specified, other PWs:20 us. */ > >> WARN_ON(intel_wait_for_register(dev_priv, > >> @@ -354,7 +355,8 @@ static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv, > >> 1)); > >> } > >> > >> -static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id) > >> +static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, > >> + enum i915_power_well_id id) > >> { > >> u32 req_mask = SKL_POWER_WELL_REQ(id); > >> u32 ret; > >> @@ -370,7 +372,7 @@ static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id) > >> static void gen9_wait_for_power_well_disable(struct drm_i915_private *dev_priv, > >> struct i915_power_well *power_well) > >> { > >> - int id = power_well->id; > >> + enum i915_power_well_id id = power_well->id; > >> bool disabled; > >> u32 reqs; > >> > >> @@ -837,7 +839,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > >> case CNL_DISP_PW_AUX_D: > >> break; > >> default: > >> - WARN(1, "Unknown power well %lu\n", power_well->id); > >> + WARN(1, "Unknown power well %u\n", power_well->id); > >> return; > >> } > >> > >> @@ -1089,7 +1091,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 punit_power_well power_well_id = power_well->id; > >> + enum i915_power_well_id power_well_id = power_well->id; > >> u32 mask; > >> u32 state; > >> u32 ctrl; > >> @@ -1137,7 +1139,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) > >> { > >> - int power_well_id = power_well->id; > >> + enum i915_power_well_id power_well_id = power_well->id; > >> bool enabled = false; > >> u32 mask; > >> u32 state; > >> @@ -1324,8 +1326,9 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, > >> > >> #define POWER_DOMAIN_MASK (GENMASK_ULL(POWER_DOMAIN_NUM - 1, 0)) > >> > >> -static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv, > >> - int power_well_id) > >> +static struct i915_power_well * > >> +lookup_power_well(struct drm_i915_private *dev_priv, > >> + enum i915_power_well_id power_well_id) > >> { > >> struct i915_power_domains *power_domains = &dev_priv->power_domains; > >> int i; > >> @@ -2117,7 +2120,7 @@ static struct i915_power_well vlv_power_wells[] = { > >> .always_on = 1, > >> .domains = POWER_DOMAIN_MASK, > >> .ops = &i9xx_always_on_power_well_ops, > >> - .id = PUNIT_POWER_WELL_ALWAYS_ON, > >> + .id = I915_DISP_PW_ALWAYS_ON, > >> }, > >> { > >> .name = "display", > >> @@ -2202,7 +2205,7 @@ static struct i915_power_well chv_power_wells[] = { > >> }; > >> > >> bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > >> - int power_well_id) > >> + enum i915_power_well_id power_well_id) > >> { > >> struct i915_power_well *power_well; > >> bool ret; > >> @@ -2219,7 +2222,7 @@ static struct i915_power_well skl_power_wells[] = { > >> .always_on = 1, > >> .domains = POWER_DOMAIN_MASK, > >> .ops = &i9xx_always_on_power_well_ops, > >> - .id = SKL_DISP_PW_ALWAYS_ON, > >> + .id = I915_DISP_PW_ALWAYS_ON, > >> }, > >> { > >> .name = "power well 1", > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > Rodrigo Vivi > > Blog: http://blog.vivi.eng.br > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 81cd21e..c9b98ed 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1382,7 +1382,7 @@ struct i915_power_well { bool hw_enabled; u64 domains; /* unique identifier for this power well */ - unsigned long id; + enum i915_power_well_id id; /* * Arbitraty data associated with this power well. Platform and power * well specific. diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 3f7beff..e4135bd 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1063,9 +1063,19 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define DP_SSS_RESET(pipe) _DP_SSS(0x2, (pipe)) #define DP_SSS_PWR_GATE(pipe) _DP_SSS(0x3, (pipe)) -/* See the PUNIT HAS v0.8 for the below bits */ -enum punit_power_well { - /* These numbers are fixed and must match the position of the pw bits */ +/** + * i915_power_well_id: + * + * Platform specific IDs used to look up power wells and - except for custom + * power wells - to define request/status register flag bit positions. As such + * the set of IDs on a given platform must be unique and except for custom + * power wells their value must stay fixed. + */ +enum i915_power_well_id { + /* + * VLV/CHV + * - PUNIT_REG_PWRGT_CTRL, PUNIT_REG_PWRGT_STATUS (PUNIT HAS v0.8) + */ PUNIT_POWER_WELL_RENDER = 0, PUNIT_POWER_WELL_MEDIA = 1, PUNIT_POWER_WELL_DISP2D = 3, @@ -1080,13 +1090,11 @@ enum punit_power_well { /* - custom power well */ CHV_DISP_PW_PIPE_A, /* 13 */ - /* Not actual bit groups. Used as IDs for lookup_power_well() */ - PUNIT_POWER_WELL_ALWAYS_ON, -}; - -enum skl_disp_power_wells { - /* These numbers are fixed and must match the position of the pw bits */ - SKL_DISP_PW_MISC_IO, + /* + * GEN9+ + * - HSW_PWR_WELL_DRIVER + */ + SKL_DISP_PW_MISC_IO = 0, SKL_DISP_PW_DDI_A_E, GLK_DISP_PW_DDI_A = SKL_DISP_PW_DDI_A_E, CNL_DISP_PW_DDI_A = SKL_DISP_PW_DDI_A_E, @@ -1105,13 +1113,18 @@ enum skl_disp_power_wells { SKL_DISP_PW_1 = 14, SKL_DISP_PW_2, - /* Not actual bit groups. Used as IDs for lookup_power_well() */ - SKL_DISP_PW_ALWAYS_ON, + /* - custom power wells */ SKL_DISP_PW_DC_OFF, - BXT_DPIO_CMN_A, BXT_DPIO_CMN_BC, - GLK_DPIO_CMN_C, + GLK_DPIO_CMN_C, /* 19 */ + + /* + * Multiple platforms. + * Must start following the highest ID of any platform. + * - custom power wells + */ + I915_DISP_PW_ALWAYS_ON = 20, }; #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2)) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 5f5dee4..ad314c1 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -50,10 +50,11 @@ */ bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, - int power_well_id); + enum i915_power_well_id power_well_id); static struct i915_power_well * -lookup_power_well(struct drm_i915_private *dev_priv, int power_well_id); +lookup_power_well(struct drm_i915_private *dev_priv, + enum i915_power_well_id power_well_id); const char * intel_display_power_domain_str(enum intel_display_power_domain domain) @@ -344,7 +345,7 @@ static void skl_power_well_pre_disable(struct drm_i915_private *dev_priv, static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv, struct i915_power_well *power_well) { - int id = power_well->id; + enum i915_power_well_id id = power_well->id; /* Timeout for PW1:10 us, AUX:not specified, other PWs:20 us. */ WARN_ON(intel_wait_for_register(dev_priv, @@ -354,7 +355,8 @@ static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv, 1)); } -static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id) +static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, + enum i915_power_well_id id) { u32 req_mask = SKL_POWER_WELL_REQ(id); u32 ret; @@ -370,7 +372,7 @@ static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id) static void gen9_wait_for_power_well_disable(struct drm_i915_private *dev_priv, struct i915_power_well *power_well) { - int id = power_well->id; + enum i915_power_well_id id = power_well->id; bool disabled; u32 reqs; @@ -837,7 +839,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, case CNL_DISP_PW_AUX_D: break; default: - WARN(1, "Unknown power well %lu\n", power_well->id); + WARN(1, "Unknown power well %u\n", power_well->id); return; } @@ -1089,7 +1091,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 punit_power_well power_well_id = power_well->id; + enum i915_power_well_id power_well_id = power_well->id; u32 mask; u32 state; u32 ctrl; @@ -1137,7 +1139,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) { - int power_well_id = power_well->id; + enum i915_power_well_id power_well_id = power_well->id; bool enabled = false; u32 mask; u32 state; @@ -1324,8 +1326,9 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, #define POWER_DOMAIN_MASK (GENMASK_ULL(POWER_DOMAIN_NUM - 1, 0)) -static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv, - int power_well_id) +static struct i915_power_well * +lookup_power_well(struct drm_i915_private *dev_priv, + enum i915_power_well_id power_well_id) { struct i915_power_domains *power_domains = &dev_priv->power_domains; int i; @@ -2117,7 +2120,7 @@ static struct i915_power_well vlv_power_wells[] = { .always_on = 1, .domains = POWER_DOMAIN_MASK, .ops = &i9xx_always_on_power_well_ops, - .id = PUNIT_POWER_WELL_ALWAYS_ON, + .id = I915_DISP_PW_ALWAYS_ON, }, { .name = "display", @@ -2202,7 +2205,7 @@ static struct i915_power_well chv_power_wells[] = { }; bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, - int power_well_id) + enum i915_power_well_id power_well_id) { struct i915_power_well *power_well; bool ret; @@ -2219,7 +2222,7 @@ static struct i915_power_well skl_power_wells[] = { .always_on = 1, .domains = POWER_DOMAIN_MASK, .ops = &i9xx_always_on_power_well_ops, - .id = SKL_DISP_PW_ALWAYS_ON, + .id = I915_DISP_PW_ALWAYS_ON, }, { .name = "power well 1",
Atm, the power well IDs are defined in separate platform specific enums, which isn't ideal for the following reasons: - the IDs are used by helpers like lookup_power_well() in a platform independent way - the always-on power well is used by multiple platforms and so needs now separate IDs, although these IDs refer to the same thing To make things more consistent use a single enum instead of the two separate ones, listing the IDs per platform (or set of very similar platforms like all GEN9/10). Replace the separate always-on power well IDs with a single ID. While at it also add a note clarifying the distinction between regular power wells that follow a common programming pattern and custom ones that are programmed in some other way. The IDs for regular power wells need to stay fixed, since they also define the request and state HW flag positions in their corresponding power well control register(s). Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_reg.h | 41 ++++++++++++++++++++++----------- drivers/gpu/drm/i915/intel_runtime_pm.c | 29 ++++++++++++----------- 3 files changed, 44 insertions(+), 28 deletions(-)