| Message ID | 1392674540-10915-5-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 18 Feb 2014 00:02:05 +0200 Imre Deak <imre.deak@intel.com> wrote: > These macros are used only locally, so move them to the .c file. > > Also since logically the init power domain should be part of all power > wells add it to the always-on power wells too for consistency. Since > always-on power wells have noop handlers, this doesn't change the > functionality. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 10 ---------- > drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++++++-- > 2 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 796f971..de0c0e0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -121,8 +121,6 @@ enum intel_display_power_domain { > POWER_DOMAIN_NUM, > }; > > -#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) > - > #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) > #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \ > ((pipe) + POWER_DOMAIN_PIPE_A_PANEL_FITTER) > @@ -130,14 +128,6 @@ enum intel_display_power_domain { > ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \ > (tran) + POWER_DOMAIN_TRANSCODER_A) > > -#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ > - BIT(POWER_DOMAIN_PIPE_A) | \ > - BIT(POWER_DOMAIN_TRANSCODER_EDP)) > -#define BDW_ALWAYS_ON_POWER_DOMAINS ( \ > - BIT(POWER_DOMAIN_PIPE_A) | \ > - BIT(POWER_DOMAIN_TRANSCODER_EDP) | \ > - BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER)) > - > enum hpd_pin { > HPD_NONE = 0, > HPD_PORT_A = HPD_NONE, /* PORT_A is internal */ > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index db48d55..9a608f1 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5384,6 +5384,23 @@ void i915_release_power_well(void) > } > EXPORT_SYMBOL_GPL(i915_release_power_well); > > +#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) > + > +#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ > + BIT(POWER_DOMAIN_PIPE_A) | \ > + BIT(POWER_DOMAIN_TRANSCODER_EDP) | \ > + BIT(POWER_DOMAIN_INIT)) > +#define HSW_DISPLAY_POWER_DOMAINS ( \ > + (POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) | \ > + BIT(POWER_DOMAIN_INIT)) > + > +#define BDW_ALWAYS_ON_POWER_DOMAINS ( \ > + HSW_ALWAYS_ON_POWER_DOMAINS | \ > + BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER)) > +#define BDW_DISPLAY_POWER_DOMAINS ( \ > + (POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS) | \ > + BIT(POWER_DOMAIN_INIT)) > + > static struct i915_power_well i9xx_always_on_power_well[] = { > { > .name = "always-on", > @@ -5400,7 +5417,7 @@ static struct i915_power_well hsw_power_wells[] = { > }, > { > .name = "display", > - .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS, > + .domains = HSW_DISPLAY_POWER_DOMAINS, > .is_enabled = hsw_power_well_enabled, > .set = hsw_set_power_well, > }, > @@ -5414,7 +5431,7 @@ static struct i915_power_well bdw_power_wells[] = { > }, > { > .name = "display", > - .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS, > + .domains = BDW_DISPLAY_POWER_DOMAINS, > .is_enabled = hsw_power_well_enabled, > .set = hsw_set_power_well, > }, Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
2014-02-20 16:21 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: > On Tue, 18 Feb 2014 00:02:05 +0200 > Imre Deak <imre.deak@intel.com> wrote: > >> These macros are used only locally, so move them to the .c file. >> >> Also since logically the init power domain should be part of all power >> wells add it to the always-on power wells too for consistency. Since >> always-on power wells have noop handlers, this doesn't change the >> functionality. This patch should be split in two: one moves to .c, the other changes the init power domain. But I'm a little confused. I always understood that the "power_on" domains were things that were always enabled on the hardware, so you couldn't really turn them "off", no matter how many domains you put. But the init power domain is different: when you get the init domain, a lot of stuff can get enabled, and when you put it, a lot of stuff can be disabled. By this logic, your patch would not make sense. So I guess we probably have two different definitions for what is a always_on power domain. I think we probably need to add some comments in the source code to clarify the definitions of things, to avoid further confusion :) >> >> Signed-off-by: Imre Deak <imre.deak@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 10 ---------- >> drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++++++-- >> 2 files changed, 19 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 796f971..de0c0e0 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -121,8 +121,6 @@ enum intel_display_power_domain { >> POWER_DOMAIN_NUM, >> }; >> >> -#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) >> - >> #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) >> #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \ >> ((pipe) + POWER_DOMAIN_PIPE_A_PANEL_FITTER) >> @@ -130,14 +128,6 @@ enum intel_display_power_domain { >> ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \ >> (tran) + POWER_DOMAIN_TRANSCODER_A) >> >> -#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ >> - BIT(POWER_DOMAIN_PIPE_A) | \ >> - BIT(POWER_DOMAIN_TRANSCODER_EDP)) >> -#define BDW_ALWAYS_ON_POWER_DOMAINS ( \ >> - BIT(POWER_DOMAIN_PIPE_A) | \ >> - BIT(POWER_DOMAIN_TRANSCODER_EDP) | \ >> - BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER)) >> - >> enum hpd_pin { >> HPD_NONE = 0, >> HPD_PORT_A = HPD_NONE, /* PORT_A is internal */ >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index db48d55..9a608f1 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -5384,6 +5384,23 @@ void i915_release_power_well(void) >> } >> EXPORT_SYMBOL_GPL(i915_release_power_well); >> >> +#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) >> + >> +#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ >> + BIT(POWER_DOMAIN_PIPE_A) | \ >> + BIT(POWER_DOMAIN_TRANSCODER_EDP) | \ >> + BIT(POWER_DOMAIN_INIT)) >> +#define HSW_DISPLAY_POWER_DOMAINS ( \ >> + (POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) | \ >> + BIT(POWER_DOMAIN_INIT)) >> + >> +#define BDW_ALWAYS_ON_POWER_DOMAINS ( \ >> + HSW_ALWAYS_ON_POWER_DOMAINS | \ >> + BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER)) >> +#define BDW_DISPLAY_POWER_DOMAINS ( \ >> + (POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS) | \ >> + BIT(POWER_DOMAIN_INIT)) >> + >> static struct i915_power_well i9xx_always_on_power_well[] = { >> { >> .name = "always-on", >> @@ -5400,7 +5417,7 @@ static struct i915_power_well hsw_power_wells[] = { >> }, >> { >> .name = "display", >> - .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS, >> + .domains = HSW_DISPLAY_POWER_DOMAINS, >> .is_enabled = hsw_power_well_enabled, >> .set = hsw_set_power_well, >> }, >> @@ -5414,7 +5431,7 @@ static struct i915_power_well bdw_power_wells[] = { >> }, >> { >> .name = "display", >> - .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS, >> + .domains = BDW_DISPLAY_POWER_DOMAINS, >> .is_enabled = hsw_power_well_enabled, >> .set = hsw_set_power_well, >> }, > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > -- > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2014-02-24 at 10:38 -0300, Paulo Zanoni wrote: > 2014-02-20 16:21 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: > > On Tue, 18 Feb 2014 00:02:05 +0200 > > Imre Deak <imre.deak@intel.com> wrote: > > > >> These macros are used only locally, so move them to the .c file. > >> > >> Also since logically the init power domain should be part of all power > >> wells add it to the always-on power wells too for consistency. Since > >> always-on power wells have noop handlers, this doesn't change the > >> functionality. > > This patch should be split in two: one moves to .c, the other changes > the init power domain. Ok. > But I'm a little confused. I always understood that the "power_on" > domains were things that were always enabled on the hardware, so you > couldn't really turn them "off", no matter how many domains you put. > But the init power domain is different: when you get the init domain, > a lot of stuff can get enabled, and when you put it, a lot of stuff > can be disabled. By this logic, your patch would not make sense. So I > guess we probably have two different definitions for what is a > always_on power domain. I think we probably need to add some comments > in the source code to clarify the definitions of things, to avoid > further confusion :) In general getting a power domain guarantees that you can access the relevant registers, so if you get POWER_DOMAIN_INIT it should enable all the HW resources necessary for this. For always-on power wells that won't involve turning on a power well, but it may involve enabling other resources. A good example is the HSW CRT port power domain, which is on the always-on power well, so we don't need to enable any power well for it, but we still need to disable runtime PM. --Imre > > >> > >> Signed-off-by: Imre Deak <imre.deak@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_drv.h | 10 ---------- > >> drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++++++-- > >> 2 files changed, 19 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index 796f971..de0c0e0 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -121,8 +121,6 @@ enum intel_display_power_domain { > >> POWER_DOMAIN_NUM, > >> }; > >> > >> -#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) > >> - > >> #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) > >> #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \ > >> ((pipe) + POWER_DOMAIN_PIPE_A_PANEL_FITTER) > >> @@ -130,14 +128,6 @@ enum intel_display_power_domain { > >> ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \ > >> (tran) + POWER_DOMAIN_TRANSCODER_A) > >> > >> -#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ > >> - BIT(POWER_DOMAIN_PIPE_A) | \ > >> - BIT(POWER_DOMAIN_TRANSCODER_EDP)) > >> -#define BDW_ALWAYS_ON_POWER_DOMAINS ( \ > >> - BIT(POWER_DOMAIN_PIPE_A) | \ > >> - BIT(POWER_DOMAIN_TRANSCODER_EDP) | \ > >> - BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER)) > >> - > >> enum hpd_pin { > >> HPD_NONE = 0, > >> HPD_PORT_A = HPD_NONE, /* PORT_A is internal */ > >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >> index db48d55..9a608f1 100644 > >> --- a/drivers/gpu/drm/i915/intel_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_pm.c > >> @@ -5384,6 +5384,23 @@ void i915_release_power_well(void) > >> } > >> EXPORT_SYMBOL_GPL(i915_release_power_well); > >> > >> +#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) > >> + > >> +#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ > >> + BIT(POWER_DOMAIN_PIPE_A) | \ > >> + BIT(POWER_DOMAIN_TRANSCODER_EDP) | \ > >> + BIT(POWER_DOMAIN_INIT)) > >> +#define HSW_DISPLAY_POWER_DOMAINS ( \ > >> + (POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) | \ > >> + BIT(POWER_DOMAIN_INIT)) > >> + > >> +#define BDW_ALWAYS_ON_POWER_DOMAINS ( \ > >> + HSW_ALWAYS_ON_POWER_DOMAINS | \ > >> + BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER)) > >> +#define BDW_DISPLAY_POWER_DOMAINS ( \ > >> + (POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS) | \ > >> + BIT(POWER_DOMAIN_INIT)) > >> + > >> static struct i915_power_well i9xx_always_on_power_well[] = { > >> { > >> .name = "always-on", > >> @@ -5400,7 +5417,7 @@ static struct i915_power_well hsw_power_wells[] = { > >> }, > >> { > >> .name = "display", > >> - .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS, > >> + .domains = HSW_DISPLAY_POWER_DOMAINS, > >> .is_enabled = hsw_power_well_enabled, > >> .set = hsw_set_power_well, > >> }, > >> @@ -5414,7 +5431,7 @@ static struct i915_power_well bdw_power_wells[] = { > >> }, > >> { > >> .name = "display", > >> - .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS, > >> + .domains = BDW_DISPLAY_POWER_DOMAINS, > >> .is_enabled = hsw_power_well_enabled, > >> .set = hsw_set_power_well, > >> }, > > > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > -- > > Jesse Barnes, Intel Open Source Technology Center > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 796f971..de0c0e0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -121,8 +121,6 @@ enum intel_display_power_domain { POWER_DOMAIN_NUM, }; -#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) - #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \ ((pipe) + POWER_DOMAIN_PIPE_A_PANEL_FITTER) @@ -130,14 +128,6 @@ enum intel_display_power_domain { ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \ (tran) + POWER_DOMAIN_TRANSCODER_A) -#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ - BIT(POWER_DOMAIN_PIPE_A) | \ - BIT(POWER_DOMAIN_TRANSCODER_EDP)) -#define BDW_ALWAYS_ON_POWER_DOMAINS ( \ - BIT(POWER_DOMAIN_PIPE_A) | \ - BIT(POWER_DOMAIN_TRANSCODER_EDP) | \ - BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER)) - enum hpd_pin { HPD_NONE = 0, HPD_PORT_A = HPD_NONE, /* PORT_A is internal */ diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index db48d55..9a608f1 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5384,6 +5384,23 @@ void i915_release_power_well(void) } EXPORT_SYMBOL_GPL(i915_release_power_well); +#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1) + +#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PIPE_A) | \ + BIT(POWER_DOMAIN_TRANSCODER_EDP) | \ + BIT(POWER_DOMAIN_INIT)) +#define HSW_DISPLAY_POWER_DOMAINS ( \ + (POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) | \ + BIT(POWER_DOMAIN_INIT)) + +#define BDW_ALWAYS_ON_POWER_DOMAINS ( \ + HSW_ALWAYS_ON_POWER_DOMAINS | \ + BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER)) +#define BDW_DISPLAY_POWER_DOMAINS ( \ + (POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS) | \ + BIT(POWER_DOMAIN_INIT)) + static struct i915_power_well i9xx_always_on_power_well[] = { { .name = "always-on", @@ -5400,7 +5417,7 @@ static struct i915_power_well hsw_power_wells[] = { }, { .name = "display", - .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS, + .domains = HSW_DISPLAY_POWER_DOMAINS, .is_enabled = hsw_power_well_enabled, .set = hsw_set_power_well, }, @@ -5414,7 +5431,7 @@ static struct i915_power_well bdw_power_wells[] = { }, { .name = "display", - .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS, + .domains = BDW_DISPLAY_POWER_DOMAINS, .is_enabled = hsw_power_well_enabled, .set = hsw_set_power_well, },
These macros are used only locally, so move them to the .c file. Also since logically the init power domain should be part of all power wells add it to the always-on power wells too for consistency. Since always-on power wells have noop handlers, this doesn't change the functionality. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 10 ---------- drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++++++-- 2 files changed, 19 insertions(+), 12 deletions(-)