diff mbox series

[v5,19/28] drm/i915/dsc: Add a power domain for VDSC on eDP/MIPI DSI

Message ID 20181005232306.31133-20-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show
Series Display Stream Compression enabling on eDP/DP | expand

Commit Message

Navare, Manasi Oct. 5, 2018, 11:22 p.m. UTC
On Icelake, a separate power well PG2 is created for
VDSC engine used for eDP/MIPI DSI. This patch adds a new
display power domain for Power well 2.

v2:
* Fix the power well mismatch CI error (Ville)
* Rename as VDSC_PIPE_A (Imre)
* Fix a whitespace (Anusha)
* Fix Comments (Imre)

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_display.h    | 1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

Navare, Manasi Oct. 16, 2018, 1:22 a.m. UTC | #1
Hi Imre/Ville,

This patch adds the power domain as per our discussion and feedback
on previous patch set.

Could you please take a look at this?

Manasi

On Fri, Oct 05, 2018 at 04:22:57PM -0700, Manasi Navare wrote:
> On Icelake, a separate power well PG2 is created for
> VDSC engine used for eDP/MIPI DSI. This patch adds a new
> display power domain for Power well 2.
> 
> v2:
> * Fix the power well mismatch CI error (Ville)
> * Rename as VDSC_PIPE_A (Imre)
> * Fix a whitespace (Anusha)
> * Fix Comments (Imre)
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.h    | 1 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index 9eaba1bccae8..4c513169960c 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -256,6 +256,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_MODESET,
>  	POWER_DOMAIN_GT_IRQ,
>  	POWER_DOMAIN_INIT,
> +	POWER_DOMAIN_VDSC_PIPE_A,
>  
>  	POWER_DOMAIN_NUM,
>  };
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 3cf8533e0834..3ed0a3a1015a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -146,6 +146,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  		return "MODESET";
>  	case POWER_DOMAIN_GT_IRQ:
>  		return "GT_IRQ";
> +	case POWER_DOMAIN_VDSC_PIPE_A:
> +		return "VDSC_PIPE_A";
>  	default:
>  		MISSING_CASE(domain);
>  		return "?";
> @@ -1971,9 +1973,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	 */
>  #define ICL_PW_2_POWER_DOMAINS (			\
>  	ICL_PW_3_POWER_DOMAINS |			\
> +	BIT_ULL(POWER_DOMAIN_VDSC_PIPE_A) |		\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  	/*
> -	 * - eDP/DSI VDSC
>  	 * - KVMR (HW control)
>  	 */
>  #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> -- 
> 2.18.0
>
Ville Syrjala Oct. 16, 2018, 7:01 p.m. UTC | #2
On Fri, Oct 05, 2018 at 04:22:57PM -0700, Manasi Navare wrote:
> On Icelake, a separate power well PG2 is created for
> VDSC engine used for eDP/MIPI DSI. This patch adds a new
> display power domain for Power well 2.
> 
> v2:
> * Fix the power well mismatch CI error (Ville)
> * Rename as VDSC_PIPE_A (Imre)
> * Fix a whitespace (Anusha)
> * Fix Comments (Imre)
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.h    | 1 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index 9eaba1bccae8..4c513169960c 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -256,6 +256,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_MODESET,
>  	POWER_DOMAIN_GT_IRQ,
>  	POWER_DOMAIN_INIT,
> +	POWER_DOMAIN_VDSC_PIPE_A,

I'd probably put it next to the other pipe related power domains.
So maybe after POWER_DOMAIN_PIPE_C_PANEL_FITTER.

And to match the current naming pattern it should be called
POWER_DOMAIN_PIPE_A_VDSC.

>  
>  	POWER_DOMAIN_NUM,
>  };
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 3cf8533e0834..3ed0a3a1015a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -146,6 +146,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  		return "MODESET";
>  	case POWER_DOMAIN_GT_IRQ:
>  		return "GT_IRQ";
> +	case POWER_DOMAIN_VDSC_PIPE_A:
> +		return "VDSC_PIPE_A";
>  	default:
>  		MISSING_CASE(domain);
>  		return "?";
> @@ -1971,9 +1973,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	 */
>  #define ICL_PW_2_POWER_DOMAINS (			\
>  	ICL_PW_3_POWER_DOMAINS |			\
> +	BIT_ULL(POWER_DOMAIN_VDSC_PIPE_A) |		\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  	/*
> -	 * - eDP/DSI VDSC
>  	 * - KVMR (HW control)
>  	 */
>  #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> -- 
> 2.18.0
Ville Syrjala Oct. 16, 2018, 7:19 p.m. UTC | #3
On Tue, Oct 16, 2018 at 10:01:11PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 05, 2018 at 04:22:57PM -0700, Manasi Navare wrote:
> > On Icelake, a separate power well PG2 is created for
> > VDSC engine used for eDP/MIPI DSI. This patch adds a new
> > display power domain for Power well 2.
> > 
> > v2:
> > * Fix the power well mismatch CI error (Ville)
> > * Rename as VDSC_PIPE_A (Imre)
> > * Fix a whitespace (Anusha)
> > * Fix Comments (Imre)
> > 
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.h    | 1 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 9eaba1bccae8..4c513169960c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -256,6 +256,7 @@ enum intel_display_power_domain {
> >  	POWER_DOMAIN_MODESET,
> >  	POWER_DOMAIN_GT_IRQ,
> >  	POWER_DOMAIN_INIT,
> > +	POWER_DOMAIN_VDSC_PIPE_A,
> 
> I'd probably put it next to the other pipe related power domains.
> So maybe after POWER_DOMAIN_PIPE_C_PANEL_FITTER.
> 
> And to match the current naming pattern it should be called
> POWER_DOMAIN_PIPE_A_VDSC.

Hmm. We could also give it an alias TRANSCODER_EDP_VDSC. Making
it an alias would avoid wasting yet another bit, but would make
the code easier to understand as we wouldn't have to add comments
explaining why we use a PIPE_A_VDSC power domain based on the
usage of the EDP transcoder.

The only slightly confusing piece would be
intel_display_power_domain_str() as that would have to reuturn
a string with both names perhaps?

> 
> >  
> >  	POWER_DOMAIN_NUM,
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 3cf8533e0834..3ed0a3a1015a 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -146,6 +146,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  		return "MODESET";
> >  	case POWER_DOMAIN_GT_IRQ:
> >  		return "GT_IRQ";
> > +	case POWER_DOMAIN_VDSC_PIPE_A:
> > +		return "VDSC_PIPE_A";
> >  	default:
> >  		MISSING_CASE(domain);
> >  		return "?";
> > @@ -1971,9 +1973,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	 */
> >  #define ICL_PW_2_POWER_DOMAINS (			\
> >  	ICL_PW_3_POWER_DOMAINS |			\
> > +	BIT_ULL(POWER_DOMAIN_VDSC_PIPE_A) |		\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  	/*
> > -	 * - eDP/DSI VDSC
> >  	 * - KVMR (HW control)
> >  	 */
> >  #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> > -- 
> > 2.18.0
> 
> -- 
> Ville Syrjälä
> Intel
Navare, Manasi Oct. 16, 2018, 7:19 p.m. UTC | #4
Thanks for your review comments.

On Tue, Oct 16, 2018 at 10:01:11PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 05, 2018 at 04:22:57PM -0700, Manasi Navare wrote:
> > On Icelake, a separate power well PG2 is created for
> > VDSC engine used for eDP/MIPI DSI. This patch adds a new
> > display power domain for Power well 2.
> > 
> > v2:
> > * Fix the power well mismatch CI error (Ville)
> > * Rename as VDSC_PIPE_A (Imre)
> > * Fix a whitespace (Anusha)
> > * Fix Comments (Imre)
> > 
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.h    | 1 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 9eaba1bccae8..4c513169960c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -256,6 +256,7 @@ enum intel_display_power_domain {
> >  	POWER_DOMAIN_MODESET,
> >  	POWER_DOMAIN_GT_IRQ,
> >  	POWER_DOMAIN_INIT,
> > +	POWER_DOMAIN_VDSC_PIPE_A,
> 
> I'd probably put it next to the other pipe related power domains.
> So maybe after POWER_DOMAIN_PIPE_C_PANEL_FITTER.
> 
> And to match the current naming pattern it should be called
> POWER_DOMAIN_PIPE_A_VDSC.

Okay will make these changes in the next rev.
With these changes, can I consider your r-b?

Regards
Manasi

> 
> >  
> >  	POWER_DOMAIN_NUM,
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 3cf8533e0834..3ed0a3a1015a 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -146,6 +146,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  		return "MODESET";
> >  	case POWER_DOMAIN_GT_IRQ:
> >  		return "GT_IRQ";
> > +	case POWER_DOMAIN_VDSC_PIPE_A:
> > +		return "VDSC_PIPE_A";
> >  	default:
> >  		MISSING_CASE(domain);
> >  		return "?";
> > @@ -1971,9 +1973,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	 */
> >  #define ICL_PW_2_POWER_DOMAINS (			\
> >  	ICL_PW_3_POWER_DOMAINS |			\
> > +	BIT_ULL(POWER_DOMAIN_VDSC_PIPE_A) |		\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  	/*
> > -	 * - eDP/DSI VDSC
> >  	 * - KVMR (HW control)
> >  	 */
> >  #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> > -- 
> > 2.18.0
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjala Oct. 16, 2018, 7:33 p.m. UTC | #5
On Tue, Oct 16, 2018 at 12:19:24PM -0700, Manasi Navare wrote:
> Thanks for your review comments.
> 
> On Tue, Oct 16, 2018 at 10:01:11PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 05, 2018 at 04:22:57PM -0700, Manasi Navare wrote:
> > > On Icelake, a separate power well PG2 is created for
> > > VDSC engine used for eDP/MIPI DSI. This patch adds a new
> > > display power domain for Power well 2.
> > > 
> > > v2:
> > > * Fix the power well mismatch CI error (Ville)
> > > * Rename as VDSC_PIPE_A (Imre)
> > > * Fix a whitespace (Anusha)
> > > * Fix Comments (Imre)
> > > 
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.h    | 1 +
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index 9eaba1bccae8..4c513169960c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -256,6 +256,7 @@ enum intel_display_power_domain {
> > >  	POWER_DOMAIN_MODESET,
> > >  	POWER_DOMAIN_GT_IRQ,
> > >  	POWER_DOMAIN_INIT,
> > > +	POWER_DOMAIN_VDSC_PIPE_A,
> > 
> > I'd probably put it next to the other pipe related power domains.
> > So maybe after POWER_DOMAIN_PIPE_C_PANEL_FITTER.
> > 
> > And to match the current naming pattern it should be called
> > POWER_DOMAIN_PIPE_A_VDSC.
> 
> Okay will make these changes in the next rev.
> With these changes, can I consider your r-b?

The slight rename+move I'd like to see. I'll leave it up to you
whether to pursue the aliasing TRANSCODER_EDP_VDSC idea.

> 
> Regards
> Manasi
> 
> > 
> > >  
> > >  	POWER_DOMAIN_NUM,
> > >  };
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 3cf8533e0834..3ed0a3a1015a 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -146,6 +146,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > >  		return "MODESET";
> > >  	case POWER_DOMAIN_GT_IRQ:
> > >  		return "GT_IRQ";
> > > +	case POWER_DOMAIN_VDSC_PIPE_A:
> > > +		return "VDSC_PIPE_A";
> > >  	default:
> > >  		MISSING_CASE(domain);
> > >  		return "?";
> > > @@ -1971,9 +1973,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > >  	 */
> > >  #define ICL_PW_2_POWER_DOMAINS (			\
> > >  	ICL_PW_3_POWER_DOMAINS |			\
> > > +	BIT_ULL(POWER_DOMAIN_VDSC_PIPE_A) |		\
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  	/*
> > > -	 * - eDP/DSI VDSC
> > >  	 * - KVMR (HW control)
> > >  	 */
> > >  #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> > > -- 
> > > 2.18.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel
Navare, Manasi Oct. 16, 2018, 7:42 p.m. UTC | #6
On Tue, Oct 16, 2018 at 10:19:06PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 16, 2018 at 10:01:11PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 05, 2018 at 04:22:57PM -0700, Manasi Navare wrote:
> > > On Icelake, a separate power well PG2 is created for
> > > VDSC engine used for eDP/MIPI DSI. This patch adds a new
> > > display power domain for Power well 2.
> > > 
> > > v2:
> > > * Fix the power well mismatch CI error (Ville)
> > > * Rename as VDSC_PIPE_A (Imre)
> > > * Fix a whitespace (Anusha)
> > > * Fix Comments (Imre)
> > > 
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.h    | 1 +
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index 9eaba1bccae8..4c513169960c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -256,6 +256,7 @@ enum intel_display_power_domain {
> > >  	POWER_DOMAIN_MODESET,
> > >  	POWER_DOMAIN_GT_IRQ,
> > >  	POWER_DOMAIN_INIT,
> > > +	POWER_DOMAIN_VDSC_PIPE_A,
> > 
> > I'd probably put it next to the other pipe related power domains.
> > So maybe after POWER_DOMAIN_PIPE_C_PANEL_FITTER.
> > 
> > And to match the current naming pattern it should be called
> > POWER_DOMAIN_PIPE_A_VDSC.
> 
> Hmm. We could also give it an alias TRANSCODER_EDP_VDSC. Making
> it an alias would avoid wasting yet another bit, but would make
> the code easier to understand as we wouldn't have to add comments
> explaining why we use a PIPE_A_VDSC power domain based on the
> usage of the EDP transcoder.
>

So you are suggesting adding an alias TRANSCODER_EDP_VDSC for POWER_DOMAIN_PIPE_A_VDSC?
But how does it avoid wasting another bit, since we would still have POWER_DOMAIN_PIPE_A_VDSC as a field
in enum power domains right?

Manasi
 
> The only slightly confusing piece would be
> intel_display_power_domain_str() as that would have to reuturn
> a string with both names perhaps?
> 
> > 
> > >  
> > >  	POWER_DOMAIN_NUM,
> > >  };
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 3cf8533e0834..3ed0a3a1015a 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -146,6 +146,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > >  		return "MODESET";
> > >  	case POWER_DOMAIN_GT_IRQ:
> > >  		return "GT_IRQ";
> > > +	case POWER_DOMAIN_VDSC_PIPE_A:
> > > +		return "VDSC_PIPE_A";
> > >  	default:
> > >  		MISSING_CASE(domain);
> > >  		return "?";
> > > @@ -1971,9 +1973,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > >  	 */
> > >  #define ICL_PW_2_POWER_DOMAINS (			\
> > >  	ICL_PW_3_POWER_DOMAINS |			\
> > > +	BIT_ULL(POWER_DOMAIN_VDSC_PIPE_A) |		\
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  	/*
> > > -	 * - eDP/DSI VDSC
> > >  	 * - KVMR (HW control)
> > >  	 */
> > >  #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> > > -- 
> > > 2.18.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjala Oct. 16, 2018, 7:45 p.m. UTC | #7
On Tue, Oct 16, 2018 at 12:42:05PM -0700, Manasi Navare wrote:
> On Tue, Oct 16, 2018 at 10:19:06PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 16, 2018 at 10:01:11PM +0300, Ville Syrjälä wrote:
> > > On Fri, Oct 05, 2018 at 04:22:57PM -0700, Manasi Navare wrote:
> > > > On Icelake, a separate power well PG2 is created for
> > > > VDSC engine used for eDP/MIPI DSI. This patch adds a new
> > > > display power domain for Power well 2.
> > > > 
> > > > v2:
> > > > * Fix the power well mismatch CI error (Ville)
> > > > * Rename as VDSC_PIPE_A (Imre)
> > > > * Fix a whitespace (Anusha)
> > > > * Fix Comments (Imre)
> > > > 
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.h    | 1 +
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
> > > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > index 9eaba1bccae8..4c513169960c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > @@ -256,6 +256,7 @@ enum intel_display_power_domain {
> > > >  	POWER_DOMAIN_MODESET,
> > > >  	POWER_DOMAIN_GT_IRQ,
> > > >  	POWER_DOMAIN_INIT,
> > > > +	POWER_DOMAIN_VDSC_PIPE_A,
> > > 
> > > I'd probably put it next to the other pipe related power domains.
> > > So maybe after POWER_DOMAIN_PIPE_C_PANEL_FITTER.
> > > 
> > > And to match the current naming pattern it should be called
> > > POWER_DOMAIN_PIPE_A_VDSC.
> > 
> > Hmm. We could also give it an alias TRANSCODER_EDP_VDSC. Making
> > it an alias would avoid wasting yet another bit, but would make
> > the code easier to understand as we wouldn't have to add comments
> > explaining why we use a PIPE_A_VDSC power domain based on the
> > usage of the EDP transcoder.
> >
> 
> So you are suggesting adding an alias TRANSCODER_EDP_VDSC for POWER_DOMAIN_PIPE_A_VDSC?
> But how does it avoid wasting another bit, since we would still have POWER_DOMAIN_PIPE_A_VDSC as a field
> in enum power domains right?

enum ... {
	...
	POWER_DOMAIN_PIPE_A_VDSC,
	POWER_DOMAIN_TRANSCODER_EDP_VDSC = POWER_DOMAIN_PIPE_A_VDSC,
	...
};
Navare, Manasi Oct. 16, 2018, 9:04 p.m. UTC | #8
On Tue, Oct 16, 2018 at 10:45:55PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 16, 2018 at 12:42:05PM -0700, Manasi Navare wrote:
> > On Tue, Oct 16, 2018 at 10:19:06PM +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 16, 2018 at 10:01:11PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Oct 05, 2018 at 04:22:57PM -0700, Manasi Navare wrote:
> > > > > On Icelake, a separate power well PG2 is created for
> > > > > VDSC engine used for eDP/MIPI DSI. This patch adds a new
> > > > > display power domain for Power well 2.
> > > > > 
> > > > > v2:
> > > > > * Fix the power well mismatch CI error (Ville)
> > > > > * Rename as VDSC_PIPE_A (Imre)
> > > > > * Fix a whitespace (Anusha)
> > > > > * Fix Comments (Imre)
> > > > > 
> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.h    | 1 +
> > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
> > > > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > index 9eaba1bccae8..4c513169960c 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > > @@ -256,6 +256,7 @@ enum intel_display_power_domain {
> > > > >  	POWER_DOMAIN_MODESET,
> > > > >  	POWER_DOMAIN_GT_IRQ,
> > > > >  	POWER_DOMAIN_INIT,
> > > > > +	POWER_DOMAIN_VDSC_PIPE_A,
> > > > 
> > > > I'd probably put it next to the other pipe related power domains.
> > > > So maybe after POWER_DOMAIN_PIPE_C_PANEL_FITTER.
> > > > 
> > > > And to match the current naming pattern it should be called
> > > > POWER_DOMAIN_PIPE_A_VDSC.
> > > 
> > > Hmm. We could also give it an alias TRANSCODER_EDP_VDSC. Making
> > > it an alias would avoid wasting yet another bit, but would make
> > > the code easier to understand as we wouldn't have to add comments
> > > explaining why we use a PIPE_A_VDSC power domain based on the
> > > usage of the EDP transcoder.
> > >
> > 
> > So you are suggesting adding an alias TRANSCODER_EDP_VDSC for POWER_DOMAIN_PIPE_A_VDSC?
> > But how does it avoid wasting another bit, since we would still have POWER_DOMAIN_PIPE_A_VDSC as a field
> > in enum power domains right?
> 
> enum ... {
> 	...
> 	POWER_DOMAIN_PIPE_A_VDSC,
> 	POWER_DOMAIN_TRANSCODER_EDP_VDSC = POWER_DOMAIN_PIPE_A_VDSC,
> 	...
> };

Why keep the POWER_DOMAIN_PIPE_A_VDSC name at all? Just for using it for *something*..?

Manasi

> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Navare, Manasi Oct. 16, 2018, 9:21 p.m. UTC | #9
On Tue, Oct 16, 2018 at 02:04:21PM -0700, Manasi Navare wrote:
> On Tue, Oct 16, 2018 at 10:45:55PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 16, 2018 at 12:42:05PM -0700, Manasi Navare wrote:
> > > On Tue, Oct 16, 2018 at 10:19:06PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Oct 16, 2018 at 10:01:11PM +0300, Ville Syrjälä wrote:
> > > > > On Fri, Oct 05, 2018 at 04:22:57PM -0700, Manasi Navare wrote:
> > > > > > On Icelake, a separate power well PG2 is created for
> > > > > > VDSC engine used for eDP/MIPI DSI. This patch adds a new
> > > > > > display power domain for Power well 2.
> > > > > > 
> > > > > > v2:
> > > > > > * Fix the power well mismatch CI error (Ville)
> > > > > > * Rename as VDSC_PIPE_A (Imre)
> > > > > > * Fix a whitespace (Anusha)
> > > > > > * Fix Comments (Imre)
> > > > > > 
> > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_display.h    | 1 +
> > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
> > > > > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > index 9eaba1bccae8..4c513169960c 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > > > @@ -256,6 +256,7 @@ enum intel_display_power_domain {
> > > > > >  	POWER_DOMAIN_MODESET,
> > > > > >  	POWER_DOMAIN_GT_IRQ,
> > > > > >  	POWER_DOMAIN_INIT,
> > > > > > +	POWER_DOMAIN_VDSC_PIPE_A,
> > > > > 
> > > > > I'd probably put it next to the other pipe related power domains.
> > > > > So maybe after POWER_DOMAIN_PIPE_C_PANEL_FITTER.
> > > > > 
> > > > > And to match the current naming pattern it should be called
> > > > > POWER_DOMAIN_PIPE_A_VDSC.
> > > > 
> > > > Hmm. We could also give it an alias TRANSCODER_EDP_VDSC. Making
> > > > it an alias would avoid wasting yet another bit, but would make
> > > > the code easier to understand as we wouldn't have to add comments
> > > > explaining why we use a PIPE_A_VDSC power domain based on the
> > > > usage of the EDP transcoder.
> > > >
> > > 
> > > So you are suggesting adding an alias TRANSCODER_EDP_VDSC for POWER_DOMAIN_PIPE_A_VDSC?
> > > But how does it avoid wasting another bit, since we would still have POWER_DOMAIN_PIPE_A_VDSC as a field
> > > in enum power domains right?
> > 
> > enum ... {
> > 	...
> > 	POWER_DOMAIN_PIPE_A_VDSC,
> > 	POWER_DOMAIN_TRANSCODER_EDP_VDSC = POWER_DOMAIN_PIPE_A_VDSC,
> > 	...
> > };
> 
> Why keep the POWER_DOMAIN_PIPE_A_VDSC name at all? Just for using it for *something*..?
> 
> Manasi
>

Ok final consensus is to name it as POWER_DOMAIN_TRANSCODER_EDP_VDSC as per the IRC discusion.

Manasi
 
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index 9eaba1bccae8..4c513169960c 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -256,6 +256,7 @@  enum intel_display_power_domain {
 	POWER_DOMAIN_MODESET,
 	POWER_DOMAIN_GT_IRQ,
 	POWER_DOMAIN_INIT,
+	POWER_DOMAIN_VDSC_PIPE_A,
 
 	POWER_DOMAIN_NUM,
 };
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 3cf8533e0834..3ed0a3a1015a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -146,6 +146,8 @@  intel_display_power_domain_str(enum intel_display_power_domain domain)
 		return "MODESET";
 	case POWER_DOMAIN_GT_IRQ:
 		return "GT_IRQ";
+	case POWER_DOMAIN_VDSC_PIPE_A:
+		return "VDSC_PIPE_A";
 	default:
 		MISSING_CASE(domain);
 		return "?";
@@ -1971,9 +1973,9 @@  void intel_display_power_put(struct drm_i915_private *dev_priv,
 	 */
 #define ICL_PW_2_POWER_DOMAINS (			\
 	ICL_PW_3_POWER_DOMAINS |			\
+	BIT_ULL(POWER_DOMAIN_VDSC_PIPE_A) |		\
 	BIT_ULL(POWER_DOMAIN_INIT))
 	/*
-	 * - eDP/DSI VDSC
 	 * - KVMR (HW control)
 	 */
 #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\