On Mon, 2014-02-24 at 10:38 -0300, Paulo Zanoni wrote:
> 2014-02-20 16:21 GMT-03:00 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>:
> > On Tue, 18 Feb 2014 00:02:05 +0200
> > Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxxx>
> >
> > --
> > Jesse Barnes, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
