| 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 |
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
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 >
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 > >
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.
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
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
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
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
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
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
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
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
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
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
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 --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))
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(-)