diff mbox series

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

Message ID 20180912005607.29522-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 Sept. 12, 2018, 12:56 a.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.

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 | 12 ++++++------
 2 files changed, 7 insertions(+), 6 deletions(-)

Comments

Rodrigo Vivi Sept. 12, 2018, 7:09 p.m. UTC | #1
On Tue, Sep 11, 2018 at 05:56:01PM -0700, Manasi Navare wrote:
> On Icelake, a separate power well PG2 is created for
> VDSC engine used for eDP/MIPI DSI.

bikeshed: To be more precise the PG2 wasn't *created*
here, but it was reserved only for VDSC on eDP/MIPI use
and everything else that was previously on PG2 on
previous platforms got moved to PG3...

> This patch adds a new
> display power domain for Power well 2.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>

But patches matches my understanding of the spec,
so:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.h    |  1 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 12 ++++++------
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
>  
>  	POWER_DOMAIN_NUM,
>  };
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 480dadb1047b..146e2d6cf954 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_EDP_MIPI:
> +		return "VDSC_EDP_MIPI";
>  	default:
>  		MISSING_CASE(domain);
>  		return "?";
> @@ -1966,18 +1968,16 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_AUDIO) |			\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  	/*
> -	 * - transcoder WD
> -	 * - KVMR (HW control)
> + 	 * - eDP/MIPI DSI VDSC
>  	 */
>  #define ICL_PW_2_POWER_DOMAINS (			\
> -	ICL_PW_3_POWER_DOMAINS |			\
> -	BIT_ULL(POWER_DOMAIN_INIT))
> +	BIT_ULL(POWER_DOMAIN_VDSC_EDP_MIPI))
>  	/*
> -	 * - eDP/DSI VDSC
> +	 * - transcoder WD
>  	 * - KVMR (HW control)
>  	 */
>  #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> -	ICL_PW_2_POWER_DOMAINS |			\
> +	ICL_PW_3_POWER_DOMAINS |			\
>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
>  	BIT_ULL(POWER_DOMAIN_INIT))
> -- 
> 2.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Imre Deak Sept. 14, 2018, 10:55 a.m. UTC | #2
On Tue, Sep 11, 2018 at 05:56:01PM -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.
> 
> 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 | 12 ++++++------
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,

This is better named VDSC_PIPE_A. The other pipes have also VDSC
functionality which could be on separate power wells in the future.

>  
>  	POWER_DOMAIN_NUM,
>  };
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 480dadb1047b..146e2d6cf954 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_EDP_MIPI:
> +		return "VDSC_EDP_MIPI";
>  	default:
>  		MISSING_CASE(domain);
>  		return "?";
> @@ -1966,18 +1968,16 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_AUDIO) |			\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  	/*
> -	 * - transcoder WD
> -	 * - KVMR (HW control)
> + 	 * - eDP/MIPI DSI VDSC

We're not changing anything in the PW3 domains list, so why changing
the above?

>  	 */
>  #define ICL_PW_2_POWER_DOMAINS (			\
> -	ICL_PW_3_POWER_DOMAINS |			\
> -	BIT_ULL(POWER_DOMAIN_INIT))

The above is bogus, both the PW3 domains and the INIT domain should
stay included in the list of PW2 domains.

> +	BIT_ULL(POWER_DOMAIN_VDSC_EDP_MIPI))
>  	/*
> -	 * - eDP/DSI VDSC
> +	 * - transcoder WD

Why adding here the transcoder WD?

>  	 * - KVMR (HW control)
>  	 */
>  #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> -	ICL_PW_2_POWER_DOMAINS |			\
> +	ICL_PW_3_POWER_DOMAINS |			\

The PW2 domain list should stay included in the DC_OFF domain list (and
so no need to add the PW3 domain list either).

>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
>  	BIT_ULL(POWER_DOMAIN_INIT))
> -- 
> 2.18.0
>
Navare, Manasi Sept. 18, 2018, 7:04 p.m. UTC | #3
Thanks Imre for your review comments. Please find the comments below:

On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > 
> > 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 | 12 ++++++------
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> 
> This is better named VDSC_PIPE_A. The other pipes have also VDSC
> functionality which could be on separate power wells in the future.
>

Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI DSI on Pipe A
will use this VDSC power well.
I will change this in the next revision.
 
> >  
> >  	POWER_DOMAIN_NUM,
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 480dadb1047b..146e2d6cf954 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_EDP_MIPI:
> > +		return "VDSC_EDP_MIPI";
> >  	default:
> >  		MISSING_CASE(domain);
> >  		return "?";
> > @@ -1966,18 +1968,16 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	BIT_ULL(POWER_DOMAIN_AUDIO) |			\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  	/*
> > -	 * - transcoder WD
> > -	 * - KVMR (HW control)
> > + 	 * - eDP/MIPI DSI VDSC

> 
> We're not changing anything in the PW3 domains list, so why changing
> the above?

These comments are below the PW3 domains define and before the PW2 domains define.
So I thought they were for PW2 domains define. Is that not the case?

If its for PW3 then I can keep them as is and if its for PW2 then we should have
eDP/DSI VDSC , KVMR since KVMR will enable PW2 and PW3. But why have Transcoder WD comment
for PW2?

> 
> >  	 */
> >  #define ICL_PW_2_POWER_DOMAINS (			\
> > -	ICL_PW_3_POWER_DOMAINS |			\
> > -	BIT_ULL(POWER_DOMAIN_INIT))
> 
> The above is bogus, both the PW3 domains and the INIT domain should
> stay included in the list of PW2 domains.

Okay I will add the PW2 and INIT domains back

> 
> > +	BIT_ULL(POWER_DOMAIN_VDSC_EDP_MIPI))
> >  	/*
> > -	 * - eDP/DSI VDSC
> > +	 * - transcoder WD
> 
> Why adding here the transcoder WD?

This comment is above PW3 domains so PW3 domain has Transcoder WD as per Bspec
and no eDP/DSI VDSC on PW3, so removed that.
Am I misreading something here in terms of comments?

So the basic question is the comments are for the PW domains defined above the comment or
for the PW domain defines below the comment?

Manasi

> 
> >  	 * - KVMR (HW control)
> >  	 */
> >  #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> > -	ICL_PW_2_POWER_DOMAINS |			\
> > +	ICL_PW_3_POWER_DOMAINS |			\
> 
> The PW2 domain list should stay included in the DC_OFF domain list (and
> so no need to add the PW3 domain list either).
> 
> >  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> > -- 
> > 2.18.0
> >
Ville Syrjälä Sept. 18, 2018, 7:12 p.m. UTC | #4
On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> Thanks Imre for your review comments. Please find the comments below:
> 
> On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > 
> > > 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 | 12 ++++++------
> > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > 
> > This is better named VDSC_PIPE_A. The other pipes have also VDSC
> > functionality which could be on separate power wells in the future.
> >
> 
> Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI DSI on Pipe A
> will use this VDSC power well.
> I will change this in the next revision.

Isn't the VDSC in the transcoder for now though? And I guess it's
moving to the pipe later?

If we call it PIPE_A then it's going to a bit confusing when we
use it with pipe B or C. Needs at least clear comments in the code
why we're doing something that looks like nonsense of the first
glance.
Navare, Manasi Sept. 18, 2018, 7:31 p.m. UTC | #5
On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > Thanks Imre for your review comments. Please find the comments below:
> > 
> > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > 
> > > > 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 | 12 ++++++------
> > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > 
> > > This is better named VDSC_PIPE_A. The other pipes have also VDSC
> > > functionality which could be on separate power wells in the future.
> > >
> > 
> > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI DSI on Pipe A
> > will use this VDSC power well.
> > I will change this in the next revision.
> 
> Isn't the VDSC in the transcoder for now though? And I guess it's
> moving to the pipe later?

VDSC engine is attached to the eDP/DSI transcoders and this gets used
for eDP/DSI VDSC on Pipe A.
So we could call it VDSC_PIPE_A since VDSC on Pipe A for eDP/DSI
will use this power well. But may be we should add a comment that
this is only for eDP/DSI on Pipe A since ICL does not support
VDSC on DP on Pipe A

What do you think?

Manasi

> 
> If we call it PIPE_A then it's going to a bit confusing when we
> use it with pipe B or C. Needs at least clear comments in the code
> why we're doing something that looks like nonsense of the first
> glance.
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Sept. 18, 2018, 7:46 p.m. UTC | #6
On Tue, Sep 18, 2018 at 12:31:54PM -0700, Manasi Navare wrote:
> On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > Thanks Imre for your review comments. Please find the comments below:
> > > 
> > > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > > 
> > > > > 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 | 12 ++++++------
> > > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > > 
> > > > This is better named VDSC_PIPE_A. The other pipes have also VDSC
> > > > functionality which could be on separate power wells in the future.
> > > >
> > > 
> > > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI DSI on Pipe A
> > > will use this VDSC power well.
> > > I will change this in the next revision.
> > 
> > Isn't the VDSC in the transcoder for now though? And I guess it's
> > moving to the pipe later?
> 
> VDSC engine is attached to the eDP/DSI transcoders and this gets used
> for eDP/DSI VDSC on Pipe A.

And what happens when I want to use pipe B instead?

> So we could call it VDSC_PIPE_A since VDSC on Pipe A for eDP/DSI
> will use this power well. But may be we should add a comment that
> this is only for eDP/DSI on Pipe A since ICL does not support
> VDSC on DP on Pipe A
> 
> What do you think?
> 
> Manasi
> 
> > 
> > If we call it PIPE_A then it's going to a bit confusing when we
> > use it with pipe B or C. Needs at least clear comments in the code
> > why we're doing something that looks like nonsense of the first
> > glance.
> > 
> > -- 
> > Ville Syrjälä
> > Intel
Navare, Manasi Sept. 18, 2018, 9:10 p.m. UTC | #7
On Tue, Sep 18, 2018 at 10:46:46PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 18, 2018 at 12:31:54PM -0700, Manasi Navare wrote:
> > On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > > Thanks Imre for your review comments. Please find the comments below:
> > > > 
> > > > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > > > 
> > > > > > 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 | 12 ++++++------
> > > > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > > > 
> > > > > This is better named VDSC_PIPE_A. The other pipes have also VDSC
> > > > > functionality which could be on separate power wells in the future.
> > > > >
> > > > 
> > > > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI DSI on Pipe A
> > > > will use this VDSC power well.
> > > > I will change this in the next revision.
> > > 
> > > Isn't the VDSC in the transcoder for now though? And I guess it's
> > > moving to the pipe later?
> > 
> > VDSC engine is attached to the eDP/DSI transcoders and this gets used
> > for eDP/DSI VDSC on Pipe A.
> 
> And what happens when I want to use pipe B instead?

DP VDSC on Pipe B uses the VDSC engine on Pipe B. Same for Pipe C
Can we have a situation where eDP uses Pipe B? Because in case of VDSC
in case of eDP, it will always use the VDSC on transcoder eDP which will
require this power well.

Manasi

> 
> > So we could call it VDSC_PIPE_A since VDSC on Pipe A for eDP/DSI
> > will use this power well. But may be we should add a comment that
> > this is only for eDP/DSI on Pipe A since ICL does not support
> > VDSC on DP on Pipe A
> > 
> > What do you think?
> > 
> > Manasi
> > 
> > > 
> > > If we call it PIPE_A then it's going to a bit confusing when we
> > > use it with pipe B or C. Needs at least clear comments in the code
> > > why we're doing something that looks like nonsense of the first
> > > glance.
> > > 
> > > -- 
> > > 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 Syrjälä Sept. 19, 2018, 10:57 a.m. UTC | #8
On Tue, Sep 18, 2018 at 02:10:17PM -0700, Manasi Navare wrote:
> On Tue, Sep 18, 2018 at 10:46:46PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 18, 2018 at 12:31:54PM -0700, Manasi Navare wrote:
> > > On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > > > Thanks Imre for your review comments. Please find the comments below:
> > > > > 
> > > > > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > > > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > > > > 
> > > > > > > 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 | 12 ++++++------
> > > > > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > > > > 
> > > > > > This is better named VDSC_PIPE_A. The other pipes have also VDSC
> > > > > > functionality which could be on separate power wells in the future.
> > > > > >
> > > > > 
> > > > > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI DSI on Pipe A
> > > > > will use this VDSC power well.
> > > > > I will change this in the next revision.
> > > > 
> > > > Isn't the VDSC in the transcoder for now though? And I guess it's
> > > > moving to the pipe later?
> > > 
> > > VDSC engine is attached to the eDP/DSI transcoders and this gets used
> > > for eDP/DSI VDSC on Pipe A.
> > 
> > And what happens when I want to use pipe B instead?
> 
> DP VDSC on Pipe B uses the VDSC engine on Pipe B. Same for Pipe C

There are no VDSCs in pipe B or C. There are VDSCs in transcoder B
and C. But that's not the same thing at all. The mux is between the
pipe and transcoder.

> Can we have a situation where eDP uses Pipe B?

Sure. 'xrandr --output eDP --crtc 1', or whatever.

> Because in case of VDSC
> in case of eDP, it will always use the VDSC on transcoder eDP which will
> require this power well.
> 
> Manasi
> 
> > 
> > > So we could call it VDSC_PIPE_A since VDSC on Pipe A for eDP/DSI
> > > will use this power well. But may be we should add a comment that
> > > this is only for eDP/DSI on Pipe A since ICL does not support
> > > VDSC on DP on Pipe A
> > > 
> > > What do you think?
> > > 
> > > Manasi
> > > 
> > > > 
> > > > If we call it PIPE_A then it's going to a bit confusing when we
> > > > use it with pipe B or C. Needs at least clear comments in the code
> > > > why we're doing something that looks like nonsense of the first
> > > > glance.
> > > > 
> > > > -- 
> > > > 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
Navare, Manasi Sept. 21, 2018, 8:34 a.m. UTC | #9
On Wed, Sep 19, 2018 at 01:57:00PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 18, 2018 at 02:10:17PM -0700, Manasi Navare wrote:
> > On Tue, Sep 18, 2018 at 10:46:46PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 18, 2018 at 12:31:54PM -0700, Manasi Navare wrote:
> > > > On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> > > > > On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > > > > Thanks Imre for your review comments. Please find the comments below:
> > > > > > 
> > > > > > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > > > > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > > > > > 
> > > > > > > > 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 | 12 ++++++------
> > > > > > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > > > > > 
> > > > > > > This is better named VDSC_PIPE_A. The other pipes have also VDSC
> > > > > > > functionality which could be on separate power wells in the future.
> > > > > > >
> > > > > > 
> > > > > > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI DSI on Pipe A
> > > > > > will use this VDSC power well.
> > > > > > I will change this in the next revision.
> > > > > 
> > > > > Isn't the VDSC in the transcoder for now though? And I guess it's
> > > > > moving to the pipe later?
> > > > 
> > > > VDSC engine is attached to the eDP/DSI transcoders and this gets used
> > > > for eDP/DSI VDSC on Pipe A.
> > > 
> > > And what happens when I want to use pipe B instead?
> > 
> > DP VDSC on Pipe B uses the VDSC engine on Pipe B. Same for Pipe C
> 
> There are no VDSCs in pipe B or C. There are VDSCs in transcoder B
> and C. But that's not the same thing at all. The mux is between the
> pipe and transcoder.
>

As per the display overview for Gen 11, the VDSC engine is present on Pipe B And C.
But for Pipe A only, it uses VDSC engine attached to transcoder eDP/DSI.
 
> > Can we have a situation where eDP uses Pipe B?
> 
> Sure. 'xrandr --output eDP --crtc 1', or whatever.
>

I guess even in this case, if its eDP/DSI, it will still use the VDSC engine
on transcoder eDP so will need to enable this power well 2.

Manasi
 
> > Because in case of VDSC
> > in case of eDP, it will always use the VDSC on transcoder eDP which will
> > require this power well.
> > 
> > Manasi
> > 
> > > 
> > > > So we could call it VDSC_PIPE_A since VDSC on Pipe A for eDP/DSI
> > > > will use this power well. But may be we should add a comment that
> > > > this is only for eDP/DSI on Pipe A since ICL does not support
> > > > VDSC on DP on Pipe A
> > > > 
> > > > What do you think?
> > > > 
> > > > Manasi
> > > > 
> > > > > 
> > > > > If we call it PIPE_A then it's going to a bit confusing when we
> > > > > use it with pipe B or C. Needs at least clear comments in the code
> > > > > why we're doing something that looks like nonsense of the first
> > > > > glance.
> > > > > 
> > > > > -- 
> > > > > 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 Syrjälä
> Intel
Ville Syrjälä Sept. 21, 2018, 1:46 p.m. UTC | #10
On Fri, Sep 21, 2018 at 01:34:00AM -0700, Manasi Navare wrote:
> On Wed, Sep 19, 2018 at 01:57:00PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 18, 2018 at 02:10:17PM -0700, Manasi Navare wrote:
> > > On Tue, Sep 18, 2018 at 10:46:46PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 18, 2018 at 12:31:54PM -0700, Manasi Navare wrote:
> > > > > On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> > > > > > On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > > > > > Thanks Imre for your review comments. Please find the comments below:
> > > > > > > 
> > > > > > > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > > > > > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > > > > > > 
> > > > > > > > > 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 | 12 ++++++------
> > > > > > > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > > > > > > 
> > > > > > > > This is better named VDSC_PIPE_A. The other pipes have also VDSC
> > > > > > > > functionality which could be on separate power wells in the future.
> > > > > > > >
> > > > > > > 
> > > > > > > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI DSI on Pipe A
> > > > > > > will use this VDSC power well.
> > > > > > > I will change this in the next revision.
> > > > > > 
> > > > > > Isn't the VDSC in the transcoder for now though? And I guess it's
> > > > > > moving to the pipe later?
> > > > > 
> > > > > VDSC engine is attached to the eDP/DSI transcoders and this gets used
> > > > > for eDP/DSI VDSC on Pipe A.
> > > > 
> > > > And what happens when I want to use pipe B instead?
> > > 
> > > DP VDSC on Pipe B uses the VDSC engine on Pipe B. Same for Pipe C
> > 
> > There are no VDSCs in pipe B or C. There are VDSCs in transcoder B
> > and C. But that's not the same thing at all. The mux is between the
> > pipe and transcoder.
> >
> 
> As per the display overview for Gen 11, the VDSC engine is present on Pipe B And C.

On transcoder B and C, not pipe B and C.

> But for Pipe A only, it uses VDSC engine attached to transcoder eDP/DSI.
>  
> > > Can we have a situation where eDP uses Pipe B?
> > 
> > Sure. 'xrandr --output eDP --crtc 1', or whatever.
> >
> 
> I guess even in this case, if its eDP/DSI, it will still use the VDSC engine
> on transcoder eDP so will need to enable this power well 2.
> 
> Manasi
>  
> > > Because in case of VDSC
> > > in case of eDP, it will always use the VDSC on transcoder eDP which will
> > > require this power well.
> > > 
> > > Manasi
> > > 
> > > > 
> > > > > So we could call it VDSC_PIPE_A since VDSC on Pipe A for eDP/DSI
> > > > > will use this power well. But may be we should add a comment that
> > > > > this is only for eDP/DSI on Pipe A since ICL does not support
> > > > > VDSC on DP on Pipe A
> > > > > 
> > > > > What do you think?
> > > > > 
> > > > > Manasi
> > > > > 
> > > > > > 
> > > > > > If we call it PIPE_A then it's going to a bit confusing when we
> > > > > > use it with pipe B or C. Needs at least clear comments in the code
> > > > > > why we're doing something that looks like nonsense of the first
> > > > > > glance.
> > > > > > 
> > > > > > -- 
> > > > > > 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 Syrjälä
> > Intel
Imre Deak Oct. 1, 2018, 9:35 a.m. UTC | #11
On Fri, Sep 21, 2018 at 04:46:47PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 21, 2018 at 01:34:00AM -0700, Manasi Navare wrote:
> > On Wed, Sep 19, 2018 at 01:57:00PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 18, 2018 at 02:10:17PM -0700, Manasi Navare wrote:
> > > > On Tue, Sep 18, 2018 at 10:46:46PM +0300, Ville Syrjälä wrote:
> > > > > On Tue, Sep 18, 2018 at 12:31:54PM -0700, Manasi Navare wrote:
> > > > > > On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> > > > > > > On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > > > > > > Thanks Imre for your review comments. Please find the comments below:
> > > > > > > > 
> > > > > > > > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > > > > > > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > > > > > > > 
> > > > > > > > > > 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 | 12 ++++++------
> > > > > > > > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > > > > > > > 
> > > > > > > > > This is better named VDSC_PIPE_A. The other pipes have also VDSC
> > > > > > > > > functionality which could be on separate power wells in the future.
> > > > > > > > >
> > > > > > > > 
> > > > > > > > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI DSI on Pipe A
> > > > > > > > will use this VDSC power well.
> > > > > > > > I will change this in the next revision.
> > > > > > > 
> > > > > > > Isn't the VDSC in the transcoder for now though? And I guess it's
> > > > > > > moving to the pipe later?
> > > > > > 
> > > > > > VDSC engine is attached to the eDP/DSI transcoders and this gets used
> > > > > > for eDP/DSI VDSC on Pipe A.
> > > > > 
> > > > > And what happens when I want to use pipe B instead?
> > > > 
> > > > DP VDSC on Pipe B uses the VDSC engine on Pipe B. Same for Pipe C
> > > 
> > > There are no VDSCs in pipe B or C. There are VDSCs in transcoder B
> > > and C. But that's not the same thing at all. The mux is between the
> > > pipe and transcoder.
> > >
> > 
> > As per the display overview for Gen 11, the VDSC engine is present on Pipe B And C.
> 
> On transcoder B and C, not pipe B and C.

Yep, I was wrong, the original name POWER_DOMAIN_VDSC_EDP_MIPI is ok.

Up to GEN11 pipe B,C use their associated pipe compression
engines/joiner if routed to transcoder B,C but they use the separate
compression engine (w/o a joiner) if routed to the eDP/MIPI transcoder.

One unclear thing is that the BSpec DSS_CTL1/2 register descriptions
(used for the eDP/MIPI DSC) show that they are backed by PG1, not PG2 as
implied elsewhere in the spec and in this patch.

Art, is that incorrect, or the registers are backed by a different power
well (PG1) than the functionality itself (PG2)?

--Imre
Imre Deak Oct. 1, 2018, 9:45 a.m. UTC | #12
On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > [...]
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 480dadb1047b..146e2d6cf954 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_EDP_MIPI:
> > > +		return "VDSC_EDP_MIPI";
> > >  	default:
> > >  		MISSING_CASE(domain);
> > >  		return "?";
> > > @@ -1966,18 +1968,16 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > >  	BIT_ULL(POWER_DOMAIN_AUDIO) |			\
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  	/*
> > > -	 * - transcoder WD
> > > -	 * - KVMR (HW control)
> > > + 	 * - eDP/MIPI DSI VDSC
> 
> > 
> > We're not changing anything in the PW3 domains list, so why changing
> > the above?
> 
> These comments are below the PW3 domains define and before the PW2
> domains define.  So I thought they were for PW2 domains define. Is
> that not the case?
> 
> If its for PW3 then I can keep them as is and if its for PW2 then we
> should have eDP/DSI VDSC , KVMR since KVMR will enable PW2 and PW3.

Yes, the above comments are for PW3.

--Imre
Runyan, Arthur J Oct. 1, 2018, 6:32 p.m. UTC | #13
The power domains printed inside the register description are out of date.  
The bspec text page on power wells has a note about that and it describes what functions are in each domain.

> -----Original Message-----
> From: Deak, Imre
> Sent: Monday, 1 October, 2018 2:36 AM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Runyan, Arthur J
> <arthur.j.runyan@intel.com>
> Cc: Navare, Manasi D <manasi.d.navare@intel.com>; intel-
> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v4 19/25] drm/i915/dsc: Add a power domain
> for VDSC on eDP/MIPI DSI
> 
> On Fri, Sep 21, 2018 at 04:46:47PM +0300, Ville Syrjälä wrote:
> > On Fri, Sep 21, 2018 at 01:34:00AM -0700, Manasi Navare wrote:
> > > On Wed, Sep 19, 2018 at 01:57:00PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 18, 2018 at 02:10:17PM -0700, Manasi Navare wrote:
> > > > > On Tue, Sep 18, 2018 at 10:46:46PM +0300, Ville Syrjälä wrote:
> > > > > > On Tue, Sep 18, 2018 at 12:31:54PM -0700, Manasi Navare wrote:
> > > > > > > On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> > > > > > > > On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > > > > > > > Thanks Imre for your review comments. Please find the
> comments below:
> > > > > > > > >
> > > > > > > > > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > > > > > > > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > > > > > > > >
> > > > > > > > > > > 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 | 12 ++++++---
> ---
> > > > > > > > > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h
> b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > > > > > > > >
> > > > > > > > > > This is better named VDSC_PIPE_A. The other pipes have
> also VDSC
> > > > > > > > > > functionality which could be on separate power wells in the
> future.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI
> DSI on Pipe A
> > > > > > > > > will use this VDSC power well.
> > > > > > > > > I will change this in the next revision.
> > > > > > > >
> > > > > > > > Isn't the VDSC in the transcoder for now though? And I guess it's
> > > > > > > > moving to the pipe later?
> > > > > > >
> > > > > > > VDSC engine is attached to the eDP/DSI transcoders and this gets
> used
> > > > > > > for eDP/DSI VDSC on Pipe A.
> > > > > >
> > > > > > And what happens when I want to use pipe B instead?
> > > > >
> > > > > DP VDSC on Pipe B uses the VDSC engine on Pipe B. Same for Pipe C
> > > >
> > > > There are no VDSCs in pipe B or C. There are VDSCs in transcoder B
> > > > and C. But that's not the same thing at all. The mux is between the
> > > > pipe and transcoder.
> > > >
> > >
> > > As per the display overview for Gen 11, the VDSC engine is present on
> Pipe B And C.
> >
> > On transcoder B and C, not pipe B and C.
> 
> Yep, I was wrong, the original name POWER_DOMAIN_VDSC_EDP_MIPI is
> ok.
> 
> Up to GEN11 pipe B,C use their associated pipe compression
> engines/joiner if routed to transcoder B,C but they use the separate
> compression engine (w/o a joiner) if routed to the eDP/MIPI transcoder.
> 
> One unclear thing is that the BSpec DSS_CTL1/2 register descriptions
> (used for the eDP/MIPI DSC) show that they are backed by PG1, not PG2 as
> implied elsewhere in the spec and in this patch.
> 
> Art, is that incorrect, or the registers are backed by a different power
> well (PG1) than the functionality itself (PG2)?
> 
> --Imre
Imre Deak Oct. 2, 2018, 11:45 a.m. UTC | #14
Thanks, found the note now. So all the EDP/MIPI VDSC regs and
functionality are in PG2.

On Mon, Oct 01, 2018 at 09:32:48PM +0300, Runyan, Arthur J wrote:
> The power domains printed inside the register description are out of date.  
> The bspec text page on power wells has a note about that and it describes what functions are in each domain.
> 
> > -----Original Message-----
> > From: Deak, Imre
> > Sent: Monday, 1 October, 2018 2:36 AM
> > To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Runyan, Arthur J
> > <arthur.j.runyan@intel.com>
> > Cc: Navare, Manasi D <manasi.d.navare@intel.com>; intel-
> > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi, Rodrigo
> > <rodrigo.vivi@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH v4 19/25] drm/i915/dsc: Add a power domain
> > for VDSC on eDP/MIPI DSI
> > 
> > On Fri, Sep 21, 2018 at 04:46:47PM +0300, Ville Syrjälä wrote:
> > > On Fri, Sep 21, 2018 at 01:34:00AM -0700, Manasi Navare wrote:
> > > > On Wed, Sep 19, 2018 at 01:57:00PM +0300, Ville Syrjälä wrote:
> > > > > On Tue, Sep 18, 2018 at 02:10:17PM -0700, Manasi Navare wrote:
> > > > > > On Tue, Sep 18, 2018 at 10:46:46PM +0300, Ville Syrjälä wrote:
> > > > > > > On Tue, Sep 18, 2018 at 12:31:54PM -0700, Manasi Navare wrote:
> > > > > > > > On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> > > > > > > > > On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > > > > > > > > Thanks Imre for your review comments. Please find the
> > comments below:
> > > > > > > > > >
> > > > > > > > > > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > > > > > > > > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > > > > > > > > >
> > > > > > > > > > > > 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 | 12 ++++++---
> > ---
> > > > > > > > > > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h
> > b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > > > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > > > > > > > > >
> > > > > > > > > > > This is better named VDSC_PIPE_A. The other pipes have
> > also VDSC
> > > > > > > > > > > functionality which could be on separate power wells in the
> > future.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI
> > DSI on Pipe A
> > > > > > > > > > will use this VDSC power well.
> > > > > > > > > > I will change this in the next revision.
> > > > > > > > >
> > > > > > > > > Isn't the VDSC in the transcoder for now though? And I guess it's
> > > > > > > > > moving to the pipe later?
> > > > > > > >
> > > > > > > > VDSC engine is attached to the eDP/DSI transcoders and this gets
> > used
> > > > > > > > for eDP/DSI VDSC on Pipe A.
> > > > > > >
> > > > > > > And what happens when I want to use pipe B instead?
> > > > > >
> > > > > > DP VDSC on Pipe B uses the VDSC engine on Pipe B. Same for Pipe C
> > > > >
> > > > > There are no VDSCs in pipe B or C. There are VDSCs in transcoder B
> > > > > and C. But that's not the same thing at all. The mux is between the
> > > > > pipe and transcoder.
> > > > >
> > > >
> > > > As per the display overview for Gen 11, the VDSC engine is present on
> > Pipe B And C.
> > >
> > > On transcoder B and C, not pipe B and C.
> > 
> > Yep, I was wrong, the original name POWER_DOMAIN_VDSC_EDP_MIPI is
> > ok.
> > 
> > Up to GEN11 pipe B,C use their associated pipe compression
> > engines/joiner if routed to transcoder B,C but they use the separate
> > compression engine (w/o a joiner) if routed to the eDP/MIPI transcoder.
> > 
> > One unclear thing is that the BSpec DSS_CTL1/2 register descriptions
> > (used for the eDP/MIPI DSC) show that they are backed by PG1, not PG2 as
> > implied elsewhere in the spec and in this patch.
> > 
> > Art, is that incorrect, or the registers are backed by a different power
> > well (PG1) than the functionality itself (PG2)?
> > 
> > --Imre
Navare, Manasi Oct. 2, 2018, 6:20 p.m. UTC | #15
On Tue, Oct 02, 2018 at 02:45:23PM +0300, Imre Deak wrote:
> Thanks, found the note now. So all the EDP/MIPI VDSC regs and
> functionality are in PG2.

Yes so if cpu transcoder is eDP then we need to enable the PG2 power well

> 
> On Mon, Oct 01, 2018 at 09:32:48PM +0300, Runyan, Arthur J wrote:
> > The power domains printed inside the register description are out of date.  
> > The bspec text page on power wells has a note about that and it describes what functions are in each domain.
> > 
> > > -----Original Message-----
> > > From: Deak, Imre
> > > Sent: Monday, 1 October, 2018 2:36 AM
> > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Runyan, Arthur J
> > > <arthur.j.runyan@intel.com>
> > > Cc: Navare, Manasi D <manasi.d.navare@intel.com>; intel-
> > > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi, Rodrigo
> > > <rodrigo.vivi@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH v4 19/25] drm/i915/dsc: Add a power domain
> > > for VDSC on eDP/MIPI DSI
> > > 
> > > On Fri, Sep 21, 2018 at 04:46:47PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Sep 21, 2018 at 01:34:00AM -0700, Manasi Navare wrote:
> > > > > On Wed, Sep 19, 2018 at 01:57:00PM +0300, Ville Syrjälä wrote:
> > > > > > On Tue, Sep 18, 2018 at 02:10:17PM -0700, Manasi Navare wrote:
> > > > > > > On Tue, Sep 18, 2018 at 10:46:46PM +0300, Ville Syrjälä wrote:
> > > > > > > > On Tue, Sep 18, 2018 at 12:31:54PM -0700, Manasi Navare wrote:
> > > > > > > > > On Tue, Sep 18, 2018 at 10:12:24PM +0300, Ville Syrjälä wrote:
> > > > > > > > > > On Tue, Sep 18, 2018 at 12:04:35PM -0700, Manasi Navare wrote:
> > > > > > > > > > > Thanks Imre for your review comments. Please find the
> > > comments below:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Sep 14, 2018 at 01:55:00PM +0300, Imre Deak wrote:
> > > > > > > > > > > > On Tue, Sep 11, 2018 at 05:56:01PM -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.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 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 | 12 ++++++---
> > > ---
> > > > > > > > > > > > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h
> > > b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > > > > > > index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
> > > > > > > > > > > >
> > > > > > > > > > > > This is better named VDSC_PIPE_A. The other pipes have
> > > also VDSC
> > > > > > > > > > > > functionality which could be on separate power wells in the
> > > future.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Yea naming it as VDSC_PIPE_A makes sense since eDP/MIPI
> > > DSI on Pipe A
> > > > > > > > > > > will use this VDSC power well.
> > > > > > > > > > > I will change this in the next revision.
> > > > > > > > > >
> > > > > > > > > > Isn't the VDSC in the transcoder for now though? And I guess it's
> > > > > > > > > > moving to the pipe later?
> > > > > > > > >
> > > > > > > > > VDSC engine is attached to the eDP/DSI transcoders and this gets
> > > used
> > > > > > > > > for eDP/DSI VDSC on Pipe A.
> > > > > > > >
> > > > > > > > And what happens when I want to use pipe B instead?
> > > > > > >
> > > > > > > DP VDSC on Pipe B uses the VDSC engine on Pipe B. Same for Pipe C
> > > > > >
> > > > > > There are no VDSCs in pipe B or C. There are VDSCs in transcoder B
> > > > > > and C. But that's not the same thing at all. The mux is between the
> > > > > > pipe and transcoder.
> > > > > >
> > > > >
> > > > > As per the display overview for Gen 11, the VDSC engine is present on
> > > Pipe B And C.
> > > >
> > > > On transcoder B and C, not pipe B and C.
> > > 
> > > Yep, I was wrong, the original name POWER_DOMAIN_VDSC_EDP_MIPI is
> > > ok.

Actually as per the discussion with Ville, in order to maintain the power well
naming conventions naming it as POWER_DOMAIN_VDSC_PIPE_A is better since compression on pipe A
will currently only be for eDP/MIPI. We do not support compression on Pipe A for external display port.
This should be documented properly.

Also the way to add this as suggested by Ville was similar to intel_ddi_main_link_aux_domain()
So add a similar helper function that will check the cpu transcoder type and if it is eDP, then
return POWER_DOMAIN_VDSC_PIPE_A

Does this sound good? If we have consensus on this I will spin the patch with this change.

Manasi

> > > 
> > > Up to GEN11 pipe B,C use their associated pipe compression
> > > engines/joiner if routed to transcoder B,C but they use the separate
> > > compression engine (w/o a joiner) if routed to the eDP/MIPI transcoder.
> > > 
> > > One unclear thing is that the BSpec DSS_CTL1/2 register descriptions
> > > (used for the eDP/MIPI DSC) show that they are backed by PG1, not PG2 as
> > > implied elsewhere in the spec and in this patch.
> > > 
> > > Art, is that incorrect, or the registers are backed by a different power
> > > well (PG1) than the functionality itself (PG2)?
> > > 
> > > --Imre
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index 3fe52788b4cf..bef71d27cdfe 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_EDP_MIPI,
 
 	POWER_DOMAIN_NUM,
 };
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 480dadb1047b..146e2d6cf954 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_EDP_MIPI:
+		return "VDSC_EDP_MIPI";
 	default:
 		MISSING_CASE(domain);
 		return "?";
@@ -1966,18 +1968,16 @@  void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_AUDIO) |			\
 	BIT_ULL(POWER_DOMAIN_INIT))
 	/*
-	 * - transcoder WD
-	 * - KVMR (HW control)
+ 	 * - eDP/MIPI DSI VDSC
 	 */
 #define ICL_PW_2_POWER_DOMAINS (			\
-	ICL_PW_3_POWER_DOMAINS |			\
-	BIT_ULL(POWER_DOMAIN_INIT))
+	BIT_ULL(POWER_DOMAIN_VDSC_EDP_MIPI))
 	/*
-	 * - eDP/DSI VDSC
+	 * - transcoder WD
 	 * - KVMR (HW control)
 	 */
 #define ICL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
-	ICL_PW_2_POWER_DOMAINS |			\
+	ICL_PW_3_POWER_DOMAINS |			\
 	BIT_ULL(POWER_DOMAIN_MODESET) |			\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
 	BIT_ULL(POWER_DOMAIN_INIT))