diff mbox

[69/89] drm/i915/skl: Adding power domains for AUX controllers

Message ID 1409830075-11139-70-git-send-email-damien.lespiau@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lespiau, Damien Sept. 4, 2014, 11:27 a.m. UTC 
From: Satheeshakrishna M <satheeshakrishna.m@intel.com>

Adding new power doamins for AUX controllers

v2: Added new power domains in power_domain_str per Imre's comment

v3: Added AUX power domains to older platforms

v4: Rebase on top of POWER_DOMAIN_PLLS.

Signed-off-by: Satheeshakrishna M <satheeshakrishna.m@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> (v3)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  8 ++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  4 ++++
 drivers/gpu/drm/i915/intel_pm.c     | 12 ++++++++++++
 3 files changed, 24 insertions(+)

Comments

Imre Deak Sept. 16, 2014, 12:35 p.m. UTC | #1 
On Thu, 2014-09-04 at 12:27 +0100, Damien Lespiau wrote:
> From: Satheeshakrishna M <satheeshakrishna.m@intel.com>
> 
> Adding new power doamins for AUX controllers
> 
> v2: Added new power domains in power_domain_str per Imre's comment
> 
> v3: Added AUX power domains to older platforms
> 
> v4: Rebase on top of POWER_DOMAIN_PLLS.
> 
> Signed-off-by: Satheeshakrishna M <satheeshakrishna.m@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> (v3)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---

This needs to be rebased on the recent CHV changes, adding the AUX
domains to the CMN and TX wells similarly to the VLV mappings. With that
this looks ok:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  drivers/gpu/drm/i915/i915_debugfs.c |  8 ++++++++
>  drivers/gpu/drm/i915/i915_drv.h     |  4 ++++
>  drivers/gpu/drm/i915/intel_pm.c     | 12 ++++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 88a4643..02cb310 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2273,6 +2273,14 @@ static const char *power_domain_str(enum intel_display_power_domain domain)
>  		return "AUDIO";
>  	case POWER_DOMAIN_PLLS:
>  		return "PLLS";
> +	case POWER_DOMAIN_AUX_A:
> +		return "AUX_A";
> +	case POWER_DOMAIN_AUX_B:
> +		return "AUX_B";
> +	case POWER_DOMAIN_AUX_C:
> +		return "AUX_C";
> +	case POWER_DOMAIN_AUX_D:
> +		return "AUX_D";
>  	case POWER_DOMAIN_INIT:
>  		return "INIT";
>  	default:
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a6e14db..91ea2b7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -138,6 +138,10 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_VGA,
>  	POWER_DOMAIN_AUDIO,
>  	POWER_DOMAIN_PLLS,
> +	POWER_DOMAIN_AUX_A,
> +	POWER_DOMAIN_AUX_B,
> +	POWER_DOMAIN_AUX_C,
> +	POWER_DOMAIN_AUX_D,
>  	POWER_DOMAIN_INIT,
>  
>  	POWER_DOMAIN_NUM,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 74a8519..ec849db 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7664,6 +7664,10 @@ EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
>  	BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) |		\
>  	BIT(POWER_DOMAIN_PORT_CRT) |			\
>  	BIT(POWER_DOMAIN_PLLS) |			\
> +	BIT(POWER_DOMAIN_AUX_A) |			\
> +	BIT(POWER_DOMAIN_AUX_B) |			\
> +	BIT(POWER_DOMAIN_AUX_C) |			\
> +	BIT(POWER_DOMAIN_AUX_D) |			\
>  	BIT(POWER_DOMAIN_INIT))
>  #define HSW_DISPLAY_POWER_DOMAINS (				\
>  	(POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) |	\
> @@ -7685,24 +7689,32 @@ EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
>  	BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) |	\
>  	BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) |	\
>  	BIT(POWER_DOMAIN_PORT_CRT) |		\
> +	BIT(POWER_DOMAIN_AUX_A) |		\
> +	BIT(POWER_DOMAIN_AUX_B) |		\
> +	BIT(POWER_DOMAIN_AUX_C) |		\
> +	BIT(POWER_DOMAIN_AUX_D) |		\
>  	BIT(POWER_DOMAIN_INIT))
>  
>  #define VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS (	\
>  	BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) |	\
>  	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
> +	BIT(POWER_DOMAIN_AUX_B) |		\
>  	BIT(POWER_DOMAIN_INIT))
>  
>  #define VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS (	\
>  	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
> +	BIT(POWER_DOMAIN_AUX_B) |		\
>  	BIT(POWER_DOMAIN_INIT))
>  
>  #define VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS (	\
>  	BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) |	\
>  	BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) |	\
> +	BIT(POWER_DOMAIN_AUX_C) |		\
>  	BIT(POWER_DOMAIN_INIT))
>  
>  #define VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS (	\
>  	BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) |	\
> +	BIT(POWER_DOMAIN_AUX_C) |		\
>  	BIT(POWER_DOMAIN_INIT))
>  
>  #define CHV_PIPE_A_POWER_DOMAINS (	\
Lespiau, Damien Sept. 18, 2014, 1:56 p.m. UTC | #2 
Hi Imre,

I actually had some question there as well:

On Tue, Sep 16, 2014 at 03:35:15PM +0300, Imre Deak wrote:
> > @@ -7685,24 +7689,32 @@ EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
> >  	BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) |	\
> >  	BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) |	\
> >  	BIT(POWER_DOMAIN_PORT_CRT) |		\
> > +	BIT(POWER_DOMAIN_AUX_A) |		\
> > +	BIT(POWER_DOMAIN_AUX_B) |		\
> > +	BIT(POWER_DOMAIN_AUX_C) |		\
> > +	BIT(POWER_DOMAIN_AUX_D) |		\
> >  	BIT(POWER_DOMAIN_INIT))

That's the VLV_DPIO_CMN_BC_POWER_DOMAINS, shouldn't we move AUX_A out of
this define?

Where does the port A lanes/aux should be on VLV and CHV, I don't see a
good fit nor where the POWER_DOMAIN_PORT_DDI_A_X_LANES is today

> >  #define VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS (	\
> >  	BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) |	\
> >  	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
> > +	BIT(POWER_DOMAIN_AUX_B) |		\
> >  	BIT(POWER_DOMAIN_INIT))
> >  
> >  #define VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS (	\
> >  	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
> > +	BIT(POWER_DOMAIN_AUX_B) |		\
> >  	BIT(POWER_DOMAIN_INIT))

Wouldn't it better to leave the AUX domain in the LANES_01 power domain,
otherwise we'll power up the full 4 lanes for aux transactions?

Thanks,
Imre Deak Sept. 18, 2014, 2:23 p.m. UTC | #3 
On Thu, 2014-09-18 at 14:56 +0100, Damien Lespiau wrote:
> Hi Imre,
> 
> I actually had some question there as well:
> 
> On Tue, Sep 16, 2014 at 03:35:15PM +0300, Imre Deak wrote:
> > > @@ -7685,24 +7689,32 @@ EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
> > >  	BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) |	\
> > >  	BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) |	\
> > >  	BIT(POWER_DOMAIN_PORT_CRT) |		\
> > > +	BIT(POWER_DOMAIN_AUX_A) |		\
> > > +	BIT(POWER_DOMAIN_AUX_B) |		\
> > > +	BIT(POWER_DOMAIN_AUX_C) |		\
> > > +	BIT(POWER_DOMAIN_AUX_D) |		\
> > >  	BIT(POWER_DOMAIN_INIT))
> 
> That's the VLV_DPIO_CMN_BC_POWER_DOMAINS, shouldn't we move AUX_A out of
> this define?

Yes I haven't noticed this. Port A and D don't exist on VLV so we don't
need to add them here. It wouldn't be a problem to add them, since we
never ask for these domains on VLV, but I agree that it's confusing to
add them here.

> Where does the port A lanes/aux should be on VLV and CHV, I don't see a
> good fit nor where the POWER_DOMAIN_PORT_DDI_A_X_LANES is today

POWER_DOMAIN_PORT_DDI_A_X_LANES and POWER_DOMAIN_PORT_DDI_D_X_LANES is
part of VLV_DISPLAY_POWER_DOMAINS, but again they are only a place
holder there. The display power well is needed for all domains, so I
defined the mapping simply accordingly.

> > >  #define VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS (	\
> > >  	BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) |	\
> > >  	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
> > > +	BIT(POWER_DOMAIN_AUX_B) |		\
> > >  	BIT(POWER_DOMAIN_INIT))
> > >  
> > >  #define VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS (	\
> > >  	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
> > > +	BIT(POWER_DOMAIN_AUX_B) |		\
> > >  	BIT(POWER_DOMAIN_INIT))
> 
> Wouldn't it better to leave the AUX domain in the LANES_01 power domain,
> otherwise we'll power up the full 4 lanes for aux transactions?

With this patch I thought that we'd keep things as-is for all platforms
except SKL. I imagined that we'd get only the AUX power domains for AUX
functionality (in patch 72/89) and not take the port power domains for
full port functionality. That would provide the extra power saving on
SKL and would keep the other platforms unchanged.

You are right that we probably don't need the TX power wells for AUX
functionality on VLV, but I haven't checked this yet, so this would need
to be done separately as a follow-up.

--Imre
Ville Syrjala Sept. 18, 2014, 2:29 p.m. UTC | #4 
On Thu, Sep 18, 2014 at 05:23:14PM +0300, Imre Deak wrote:
> On Thu, 2014-09-18 at 14:56 +0100, Damien Lespiau wrote:
 
> > > >  #define VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS (	\
> > > >  	BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) |	\
> > > >  	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
> > > > +	BIT(POWER_DOMAIN_AUX_B) |		\
> > > >  	BIT(POWER_DOMAIN_INIT))
> > > >  
> > > >  #define VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS (	\
> > > >  	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
> > > > +	BIT(POWER_DOMAIN_AUX_B) |		\
> > > >  	BIT(POWER_DOMAIN_INIT))
> > 
> > Wouldn't it better to leave the AUX domain in the LANES_01 power domain,
> > otherwise we'll power up the full 4 lanes for aux transactions?
> 
> With this patch I thought that we'd keep things as-is for all platforms
> except SKL. I imagined that we'd get only the AUX power domains for AUX
> functionality (in patch 72/89) and not take the port power domains for
> full port functionality. That would provide the extra power saving on
> SKL and would keep the other platforms unchanged.
> 
> You are right that we probably don't need the TX power wells for AUX
> functionality on VLV, but I haven't checked this yet, so this would need
> to be done separately as a follow-up.

And we always have the nasty CMN vs. TX ordering issue on VLV. So unless
someone adds the logic to bring the wells down and back up again when
we need to power up TX wells when CMN is already up, we just have to
keep TX wells powered whenever CMN is powered.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 88a4643..02cb310 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2273,6 +2273,14 @@  static const char *power_domain_str(enum intel_display_power_domain domain)
 		return "AUDIO";
 	case POWER_DOMAIN_PLLS:
 		return "PLLS";
+	case POWER_DOMAIN_AUX_A:
+		return "AUX_A";
+	case POWER_DOMAIN_AUX_B:
+		return "AUX_B";
+	case POWER_DOMAIN_AUX_C:
+		return "AUX_C";
+	case POWER_DOMAIN_AUX_D:
+		return "AUX_D";
 	case POWER_DOMAIN_INIT:
 		return "INIT";
 	default:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a6e14db..91ea2b7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -138,6 +138,10 @@  enum intel_display_power_domain {
 	POWER_DOMAIN_VGA,
 	POWER_DOMAIN_AUDIO,
 	POWER_DOMAIN_PLLS,
+	POWER_DOMAIN_AUX_A,
+	POWER_DOMAIN_AUX_B,
+	POWER_DOMAIN_AUX_C,
+	POWER_DOMAIN_AUX_D,
 	POWER_DOMAIN_INIT,
 
 	POWER_DOMAIN_NUM,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 74a8519..ec849db 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7664,6 +7664,10 @@  EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
 	BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) |		\
 	BIT(POWER_DOMAIN_PORT_CRT) |			\
 	BIT(POWER_DOMAIN_PLLS) |			\
+	BIT(POWER_DOMAIN_AUX_A) |			\
+	BIT(POWER_DOMAIN_AUX_B) |			\
+	BIT(POWER_DOMAIN_AUX_C) |			\
+	BIT(POWER_DOMAIN_AUX_D) |			\
 	BIT(POWER_DOMAIN_INIT))
 #define HSW_DISPLAY_POWER_DOMAINS (				\
 	(POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) |	\
@@ -7685,24 +7689,32 @@  EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
 	BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) |	\
 	BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) |	\
 	BIT(POWER_DOMAIN_PORT_CRT) |		\
+	BIT(POWER_DOMAIN_AUX_A) |		\
+	BIT(POWER_DOMAIN_AUX_B) |		\
+	BIT(POWER_DOMAIN_AUX_C) |		\
+	BIT(POWER_DOMAIN_AUX_D) |		\
 	BIT(POWER_DOMAIN_INIT))
 
 #define VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS (	\
 	BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) |	\
 	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
+	BIT(POWER_DOMAIN_AUX_B) |		\
 	BIT(POWER_DOMAIN_INIT))
 
 #define VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS (	\
 	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
+	BIT(POWER_DOMAIN_AUX_B) |		\
 	BIT(POWER_DOMAIN_INIT))
 
 #define VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS (	\
 	BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) |	\
 	BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) |	\
+	BIT(POWER_DOMAIN_AUX_C) |		\
 	BIT(POWER_DOMAIN_INIT))
 
 #define VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS (	\
 	BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) |	\
+	BIT(POWER_DOMAIN_AUX_C) |		\
 	BIT(POWER_DOMAIN_INIT))
 
 #define CHV_PIPE_A_POWER_DOMAINS (	\