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

Tue Oct 2 18:20:05 UTC 2018

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 at linux.intel.com>; Runyan, Arthur J
> > > <arthur.j.runyan at intel.com>
> > > Cc: Navare, Manasi D <manasi.d.navare at intel.com>; intel-
> > > gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Vivi, Rodrigo
> > > <rodrigo.vivi at 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 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.
> > > > > > > > > >
> > > > > > > > > > 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