Re: [PATCH 09/10] drm/i915/cnl: Enable DDI-F on Cannonlake.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jan 26, 2018 at 09:39:42PM +0000, Pandiyan, Dhinakaran wrote:
> 
> On Thu, 2018-01-25 at 14:03 -0800, Rodrigo Vivi wrote:
> > Now let's finish the Port-F support by adding the
> > proper port F detection, irq and power well support.
> > 
> > v2: Rebase
> > v3: Use BIT_ULL
> > v4: Cover missed case on ddi init.
> > v5: Update commit message.
> > v6: Rebase on top of display headers rework.
> > v7: Squash power-well handling related to DDI F to this
> >     patch to avoid warns as pointed out by DK.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
> >  drivers/gpu/drm/i915/intel_ddi.c        |  4 ++++
> >  drivers/gpu/drm/i915/intel_display.c    |  6 +++++-
> >  drivers/gpu/drm/i915/intel_display.h    |  2 ++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 19 ++++++++++++++++---
> >  5 files changed, 29 insertions(+), 4 deletions(-)
> 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 30fa2041a45f..c4042e342f50 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -157,11 +157,13 @@ enum intel_display_power_domain {
> >  	POWER_DOMAIN_PORT_DDI_C_LANES,
> >  	POWER_DOMAIN_PORT_DDI_D_LANES,
> >  	POWER_DOMAIN_PORT_DDI_E_LANES,
> > +	POWER_DOMAIN_PORT_DDI_F_LANES,
> 
> What well does this need? {B,C,D}_LANES all enable/disable power well 2
> from what I can tell. I don't see a
> BIT_ULL(POWER_DOMAIN_PORT_DDI_F_LANES) in this patch.

indeed. They need to be added along with other *DDI_{B,C,D}_LANES on
CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS.

> 
> >  	POWER_DOMAIN_PORT_DDI_A_IO,
> >  	POWER_DOMAIN_PORT_DDI_B_IO,
> >  	POWER_DOMAIN_PORT_DDI_C_IO,
> >  	POWER_DOMAIN_PORT_DDI_D_IO,
> >  	POWER_DOMAIN_PORT_DDI_E_IO,
> > +	POWER_DOMAIN_PORT_DDI_F_IO,
> >  	POWER_DOMAIN_PORT_DSI,
> >  	POWER_DOMAIN_PORT_CRT,
> >  	POWER_DOMAIN_PORT_OTHER,
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 3526b563b8ec..30e50ea16960 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -94,6 +94,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  		return "PORT_DDI_D_LANES";
> >  	case POWER_DOMAIN_PORT_DDI_E_LANES:
> >  		return "PORT_DDI_E_LANES";
> > +	case POWER_DOMAIN_PORT_DDI_F_LANES:
> > +		return "PORT_DDI_F_LANES";
> >  	case POWER_DOMAIN_PORT_DDI_A_IO:
> >  		return "PORT_DDI_A_IO";
> >  	case POWER_DOMAIN_PORT_DDI_B_IO:
> > @@ -104,6 +106,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  		return "PORT_DDI_D_IO";
> >  	case POWER_DOMAIN_PORT_DDI_E_IO:
> >  		return "PORT_DDI_E_IO";
> > +	case POWER_DOMAIN_PORT_DDI_F_IO:
> > +		return "PORT_DDI_F_IO";
> >  	case POWER_DOMAIN_PORT_DSI:
> >  		return "PORT_DSI";
> >  	case POWER_DOMAIN_PORT_CRT:
> > @@ -1860,6 +1864,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  #define CNL_DISPLAY_AUX_F_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_F) |			\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> > +#define CNL_DISPLAY_DDI_F_IO_POWER_DOMAINS (		\
> > +	BIT_ULL(POWER_DOMAIN_PORT_DDI_F_IO) |		\
> > +	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define CNL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> >  	CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> >  	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
> > @@ -2411,6 +2418,12 @@ static struct i915_power_well cnl_power_wells[] = {
> >  		.id = SKL_DISP_PW_DDI_D,
> >  	},
> >  	{
> > +		.name = "DDI F IO power well",
> > +		.domains = CNL_DISPLAY_DDI_F_IO_POWER_DOMAINS,
> > +		.ops = &hsw_power_well_ops,
> > +		.id = CNL_DISP_PW_DDI_F,
> > +	},
> 
> Again same question about the enabling order, can this be enabled after
> power well2? 

Ok, now I see your question from another angle.

I hope it works or the proposed solution for power well won't work at all.

I can't recall anywhere on the spec where this order should matter. Imre?

If the order matters somehow we will need to bring the old patches back to
add the full definition of wells. So I hope we don't need that.

> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx