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@xxxxxxxxx> wrote:
> > On Thu, Jul 6, 2017 at 7:40 AM, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxx>
> >
> >> };
> >>
> >> #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@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
