Re: [PATCH 04/19] drm/i915: move power domain macros to intel_pm.c

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5774 bytes --]

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
> 
> 
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx