Re: [PATCH v3 16/29] drm/i915/xe2lpd: Add display power well

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Sep 12, 2023 at 11:30:50AM -0500, Lucas De Marchi wrote:
> On Tue, Sep 12, 2023 at 09:04:17AM -0700, Matt Roper wrote:
> > On Mon, Sep 11, 2023 at 09:48:24PM -0700, Lucas De Marchi wrote:
> > > From: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@xxxxxxxxx>
> > > 
> > > Add Display Power Well for LNL platform. It's mostly the same as MTL
> > 
> > It might be better to say "Xe2_LPD" and "Xe_LPD+" instead of LNL/MTL?
> 
> ok
> 
> > 
> > > platform so reuse the code. PGPICA1 contains type-C capable port slices
> > > which requires the well to power powered up, so add new power well
> > > definition for it.
> > 
> > Maybe add a sentence noting that the dc_off fake powerwell will be added
> > in a follow-up patch so that people don't think it was overlooked here?
> 
> ok
> 
> > 
> > > 
> > > BSpec: 68886
> > > Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@xxxxxxxxx>
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > ---
> > >  .../i915/display/intel_display_power_map.c    | 36 ++++++++++++++-
> > >  .../i915/display/intel_display_power_well.c   | 44 +++++++++++++++++++
> > >  .../i915/display/intel_display_power_well.h   |  1 +
> > >  .../gpu/drm/i915/display/intel_dp_aux_regs.h  |  5 +++
> > >  4 files changed, 85 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_map.c b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> > > index 0f1b93d139ca..31c11586ede5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_map.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> > > @@ -1536,6 +1536,38 @@ static const struct i915_power_well_desc_list xelpdp_power_wells[] = {
> > >  	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
> > >  };
> > > 
> > > +I915_DECL_PW_DOMAINS(xe2lpd_pwdoms_pica_tc,
> > > +		     POWER_DOMAIN_PORT_DDI_LANES_TC1,
> > > +		     POWER_DOMAIN_PORT_DDI_LANES_TC2,
> > > +		     POWER_DOMAIN_PORT_DDI_LANES_TC3,
> > > +		     POWER_DOMAIN_PORT_DDI_LANES_TC4,
> > > +		     POWER_DOMAIN_AUX_USBC1,
> > > +		     POWER_DOMAIN_AUX_USBC2,
> > > +		     POWER_DOMAIN_AUX_USBC3,
> > > +		     POWER_DOMAIN_AUX_USBC4,
> > > +		     POWER_DOMAIN_AUX_TBT1,
> > > +		     POWER_DOMAIN_AUX_TBT2,
> > > +		     POWER_DOMAIN_AUX_TBT3,
> > > +		     POWER_DOMAIN_AUX_TBT4,
> > > +		     POWER_DOMAIN_INIT);
> > > +
> > > +static const struct i915_power_well_desc xe2lpd_power_wells_pica[] = {
> > > +	{
> > > +		.instances = &I915_PW_INSTANCES(I915_PW("PICA_TC",
> > > +							&xe2lpd_pwdoms_pica_tc,
> > > +							.id = DISP_PW_ID_NONE),
> > > +					       ),
> > > +		.ops = &xe2lpd_pica_power_well_ops,
> > > +	},
> > > +};
> > > +
> > > +static const struct i915_power_well_desc_list xe2lpd_power_wells[] = {
> > > +	I915_PW_DESCRIPTORS(i9xx_power_wells_always_on),
> > > +	I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
> > > +	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
> > > +	I915_PW_DESCRIPTORS(xe2lpd_power_wells_pica),
> > > +};
> > > +
> > >  static void init_power_well_domains(const struct i915_power_well_instance *inst,
> > >  				    struct i915_power_well *power_well)
> > >  {
> > > @@ -1643,7 +1675,9 @@ int intel_display_power_map_init(struct i915_power_domains *power_domains)
> > >  		return 0;
> > >  	}
> > > 
> > > -	if (DISPLAY_VER(i915) >= 14)
> > > +	if (DISPLAY_VER(i915) >= 20)
> > > +		return set_power_wells(power_domains, xe2lpd_power_wells);
> > > +	else if (DISPLAY_VER(i915) >= 14)
> > >  		return set_power_wells(power_domains, xelpdp_power_wells);
> > >  	else if (IS_DG2(i915))
> > >  		return set_power_wells(power_domains, xehpd_power_wells);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > index ca0714eba17a..adfe9901bde4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > @@ -1833,6 +1833,43 @@ static bool xelpdp_aux_power_well_enabled(struct drm_i915_private *dev_priv,
> > >  		XELPDP_DP_AUX_CH_CTL_POWER_STATUS;
> > >  }
> > > 
> > > +static void xe2lpd_pica_power_well_enable(struct drm_i915_private *dev_priv,
> > > +					  struct i915_power_well *power_well)
> > > +{
> > > +	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
> > > +		     XE2LPD_PICA_CTL_POWER_REQUEST,
> > > +		     XE2LPD_PICA_CTL_POWER_REQUEST);
> > 
> > Do we need rmw here?  Bit 31 is the only writable bit in the register
> > (bit 30 is RO and can't be clobbered), so a simple write should suffice?
> > Ditto on the disable below.
> > 
> > > +
> > > +	if (intel_de_wait_for_set(dev_priv, XE2LPD_PICA_PW_CTL,
> > > +				  XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
> > > +		drm_dbg_kms(&dev_priv->drm, "pica power well enable timeout\n");
> > > +
> > > +		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when enabled");
> > > +	}
> > > +}
> > > +
> > > +static void xe2lpd_pica_power_well_disable(struct drm_i915_private *dev_priv,
> > > +					   struct i915_power_well *power_well)
> > > +{
> > > +	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
> > > +		     XE2LPD_PICA_CTL_POWER_REQUEST,
> > > +		     0);
> > > +
> > > +	if (intel_de_wait_for_clear(dev_priv, XE2LPD_PICA_PW_CTL,
> > > +				    XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
> > > +		drm_dbg_kms(&dev_priv->drm, "pica power well disable timeout\n");
> > > +
> > > +		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when disabled");
> > > +	}
> > > +}
> > > +
> > > +static bool xe2lpd_pica_power_well_enabled(struct drm_i915_private *dev_priv,
> > > +					   struct i915_power_well *power_well)
> > > +{
> > > +	return intel_de_read(dev_priv, XE2LPD_PICA_PW_CTL) &
> > > +		XE2LPD_PICA_CTL_POWER_STATUS;
> > > +}
> > > +
> > >  const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
> > >  	.sync_hw = i9xx_power_well_sync_hw_noop,
> > >  	.enable = i9xx_always_on_power_well_noop,
> > > @@ -1952,3 +1989,10 @@ const struct i915_power_well_ops xelpdp_aux_power_well_ops = {
> > >  	.disable = xelpdp_aux_power_well_disable,
> > >  	.is_enabled = xelpdp_aux_power_well_enabled,
> > >  };
> > > +
> > > +const struct i915_power_well_ops xe2lpd_pica_power_well_ops = {
> > > +	.sync_hw = i9xx_power_well_sync_hw_noop,
> > > +	.enable = xe2lpd_pica_power_well_enable,
> > > +	.disable = xe2lpd_pica_power_well_disable,
> > > +	.is_enabled = xe2lpd_pica_power_well_enabled,
> > > +};
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.h b/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > index a8736588314d..9357a9a73c06 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > @@ -176,5 +176,6 @@ extern const struct i915_power_well_ops icl_aux_power_well_ops;
> > >  extern const struct i915_power_well_ops icl_ddi_power_well_ops;
> > >  extern const struct i915_power_well_ops tgl_tc_cold_off_ops;
> > >  extern const struct i915_power_well_ops xelpdp_aux_power_well_ops;
> > > +extern const struct i915_power_well_ops xe2lpd_pica_power_well_ops;
> > > 
> > >  #endif
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> > > index b4e96baae25a..c869f5661530 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> > > @@ -86,4 +86,9 @@
> > >  		 _XELPDP_DP_AUX_CH_DATA(__xe2lpd_aux_ch_idx(aux_ch), i) :	\
> > >  		 _XELPDP_DP_AUX_CH_DATA(aux_ch, i))
> > > 
> > > +/* PICA Power Well Control */
> > > +#define XE2LPD_PICA_PW_CTL			_MMIO(0x16fe04)
> > 
> > Is this the right header for this?  It doesn't seem directly related to
> > DP AUX.
> 
> In Xe2_LPD, those AUX registers are also
> 
> I had the same question myself while rebasing this patch as this is
> not related to DP. However the register itself seems to have the same
> functionality as the other registers above with power request/status.
> And if you look at bspec 69009, all of them are under pica for Xe2_LPD,
> too.
> 
> I'm not sure what was the criteria for the split of this DP header.
> It's clearer for things like gt, engine, snps phy, etc, but this one
> seems to have been more or less arbitrary.
> 
> Any suggestion for a better place? A new display/intel_pica_regs.h
> maybe? That may not be as future proof in the cases register move from
> one place to the other like happened in Xe2_LPD: see bspec 69009.  All
> the PORT_BUF_CTL*, PORT_HOTPLUG_CTL, etc should then be in this header,
> which doesn't match previous platforms.

I kind of like display/intel_pica_regs.h.  As you noted, a bunch of
stuff that's in intel_cx0_phy_regs.h today should really probably be in
a PICA header instead because those aren't actually PHY registers (the
real CX0 PHY registers are the things that can only be accessed over the
message bus like PHY_C10_VDR_CMN and such).

I'm not too concerned about where this lands in the short term though;
even i915_reg.h would be fine as a short-term dumping ground if
necessary.  Don't let the comment here block the LNL display work; we
can always do a separate follow-up series to clarify the placement of
some of our registers for MTL+LNL.


Matt

> 
> If instead of matching HW we try to match where it's used in SW, then
> maybe a intel_display_power_well_regs.h, but that too doesn't match
> previous platforms.
> 
> +Jani
> 
> Lucas De Marchi
> 
> > 
> > 
> > Matt
> > 
> > > +#define   XE2LPD_PICA_CTL_POWER_REQUEST		REG_BIT(31)
> > > +#define   XE2LPD_PICA_CTL_POWER_STATUS		REG_BIT(30)
> > > +
> > >  #endif /* __INTEL_DP_AUX_REGS_H__ */
> > > --
> > > 2.40.1
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux