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

[Manasi Navare]

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 at intel.com>
> > Cc: Imre Deak <imre.deak at intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare at 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
> >