Re: [PATCH v3 1/1] drm/i915: Add modular FIA

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

 



On Thu, 2019-07-11 at 16:49 -0700, Lucas De Marchi wrote:
> On Thu, Jul 11, 2019 at 04:15:42PM -0700, Summers, Stuart wrote:
> > On Thu, 2019-07-11 at 13:58 -0700, Lucas De Marchi wrote:
> > > From: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > > 
> > > Some platforms may have Modular FIA. If Modular FIA is used in
> > > the
> > > SOC,
> > > then Display Driver will access the additional instances of
> > > FIA based on pre-assigned offset in GTTMADDR space.
> > > 
> > > Each Modular FIA instance has its own IOSF Sideband Port ID
> > > and it houses only 2 Type-C Port. In SOC that has more than
> > > two Type-C Ports, there are multiple instances of Modular FIA.
> > > Gunit will need to use different destination ID when it access
> > > different pair of Type-C Port.
> > > 
> > > The DFLEXDPSP register has Modular FIA bit starting on Tiger
> > > Lake.  If
> > > Modular FIA is used in the SOC, this register bit exists in all
> > > the
> > > instances of Modular FIA. IOM FW is required to program only the
> > > MF
> > > bit
> > > in first FIA instance that houses the Type-C Port 0 and Port 1,
> > > for
> > > Display Driver to read from.
> > > 
> > > v2 (Lucas):
> > >   - Move all accesses to FIA to be contained in intel_tc.c, along
> > > with
> > >     display_fia that is now called tc_phy_fia
> > >   - Save the fia instance number on intel_digital_port, so we
> > > don't
> > > have
> > >     to query if modular FIA is used on every access
> > > v3 (Lucas): Make function static
> > > v4 (Lucas): Move enum phy_fia to the header and use it in
> > >    intel_digital_port (suggested by Ville)
> > > 
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > Acked-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.h |  6 +++
> > >  drivers/gpu/drm/i915/display/intel_tc.c      | 43
> > > ++++++++++++++++
> > > ----
> > >  drivers/gpu/drm/i915/i915_reg.h              | 13 ++++--
> > >  drivers/gpu/drm/i915/intel_device_info.h     |  1 +
> > >  drivers/gpu/drm/i915/intel_drv.h             |  1 +
> > >  5 files changed, 52 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> > > b/drivers/gpu/drm/i915/display/intel_display.h
> > > index 8a4a57ef82a2..8b048976f7b4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > @@ -243,6 +243,12 @@ enum phy {
> > > 
> > >  #define phy_name(a) ((a) + 'A')
> > > 
> > > +enum phy_fia {
> > > +	FIA1,
> > > +	FIA2,
> > > +	FIA3,
> > > +};
> > > +
> > >  #define for_each_pipe(__dev_priv, __p) \
> > >  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes;
> > > (__p)++)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > > b/drivers/gpu/drm/i915/display/intel_tc.c
> > > index f44ee4bfe7c8..9400da4f7916 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > @@ -22,6 +22,24 @@ static const char *tc_port_mode_name(enum
> > > tc_port_mode mode)
> > >  	return names[mode];
> > >  }
> > > 
> > > +static bool has_modular_fia(struct drm_i915_private *i915)
> > > +{
> > > +	if (!INTEL_INFO(i915)->display.has_modular_fia)
> > > +		return false;
> > > +
> > > +	return intel_uncore_read(&i915->uncore,
> > > +				 PORT_TX_DFLEXDPSP(FIA1)) &
> > > MODULAR_FIA_MASK;
> > > +}
> > > +
> > > +static enum phy_fia tc_port_to_fia(struct drm_i915_private
> > > *i915,
> > > +				   enum tc_port tc_port)
> > > +{
> > > +	if (!has_modular_fia(i915))
> > > +		return FIA1;
> > > +
> > > +	return tc_port / 2;
> > 
> > I realize this is described in the commit message, but would be
> > nice to
> > have a brief comment describing why we need this conversion.
> > 
> > > +}
> > > +
> > >  u32 intel_tc_port_get_lane_mask(struct intel_digital_port
> > > *dig_port)
> > >  {
> > >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > > > base.base.dev);
> > > 
> > > @@ -29,7 +47,8 @@ u32 intel_tc_port_get_lane_mask(struct
> > > intel_digital_port *dig_port)
> > >  	struct intel_uncore *uncore = &i915->uncore;
> > >  	u32 lane_mask;
> > > 
> > > -	lane_mask = intel_uncore_read(uncore, PORT_TX_DFLEXDPSP);
> > > +	lane_mask = intel_uncore_read(uncore,
> > > +				      PORT_TX_DFLEXDPSP(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	WARN_ON(lane_mask == 0xffffffff);
> > > 
> > > @@ -78,7 +97,8 @@ void intel_tc_port_set_fia_lane_count(struct
> > > intel_digital_port *dig_port,
> > > 
> > >  	WARN_ON(lane_reversal && dig_port->tc_mode != TC_PORT_LEGACY);
> > > 
> > > -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPMLE1);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPMLE1(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	val &= ~DFLEXDPMLE1_DPMLETC_MASK(tc_port);
> > > 
> > >  	switch (required_lanes) {
> > > @@ -97,7 +117,8 @@ void intel_tc_port_set_fia_lane_count(struct
> > > intel_digital_port *dig_port,
> > >  		MISSING_CASE(required_lanes);
> > >  	}
> > > 
> > > -	intel_uncore_write(uncore, PORT_TX_DFLEXDPMLE1, val);
> > > +	intel_uncore_write(uncore,
> > > +			   PORT_TX_DFLEXDPMLE1(dig_port->tc_phy_fia),
> > > val);
> > >  }
> > > 
> > >  static void tc_port_fixup_legacy_flag(struct intel_digital_port
> > > *dig_port,
> > > @@ -129,7 +150,8 @@ static u32 tc_port_live_status_mask(struct
> > > intel_digital_port *dig_port)
> > >  	u32 mask = 0;
> > >  	u32 val;
> > > 
> > > -	val = intel_uncore_rea
> > > d(uncore, PORT_TX_DFLEXDPSP);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPSP(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	if (val == 0xffffffff) {
> > >  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, nothingo make it
> > > t
> > > connected\n",
> > > @@ -159,7 +181,8 @@ static bool icl_tc_phy_status_complete(struct
> > > intel_digital_port *dig_port)
> > >  	struct intel_uncore *uncore = &i915->uncore;
> > >  	u32 val;
> > > 
> > > -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPPMS);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPPMS(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	if (val == 0xffffffff) {
> > >  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assuming not
> > > complete\n",
> > >  			      dig_port->tc_port_name);
> > > @@ -177,7 +200,8 @@ static bool icl_tc_phy_set_safe_mode(struct
> > > intel_digital_port *dig_port,
> > >  	struct intel_uncore *uncore = &i915->uncore;
> > >  	u32 val;
> > > 
> > > -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPCSSS(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	if (val == 0xffffffff) {
> > >  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, can't set safe-
> > > mode to %s\n",
> > >  			      dig_port->tc_port_name,
> > > @@ -190,7 +214,8 @@ static bool icl_tc_phy_set_safe_mode(struct
> > > intel_digital_port *dig_port,
> > >  	if (!enable)
> > >  		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > > 
> > > -	intel_uncore_write(uncore, PORT_TX_DFLEXDPCSSS, val);
> > > +	intel_uncore_write(uncore,
> > > +			   PORT_TX_DFLEXDPCSSS(dig_port->tc_phy_fia),
> > > val);
> > > 
> > >  	if (enable && wait_for(!icl_tc_phy_status_complete(dig_port),
> > > 10))
> > >  		DRM_DEBUG_KMS("Port %s: PHY complete clear timed
> > > out\n",
> > > @@ -206,7 +231,8 @@ static bool icl_tc_phy_is_in_safe_mode(struct
> > > intel_digital_port *dig_port)
> > >  	struct intel_uncore *uncore = &i915->uncore;
> > >  	u32 val;
> > > 
> > > -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPCSSS(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	if (val == 0xffffffff) {
> > >  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assume safe
> > > mode\n",
> > >  			      dig_port->tc_port_name);
> > > @@ -503,4 +529,5 @@ void intel_tc_port_init(struct
> > > intel_digital_port
> > > *dig_port, bool is_legacy)
> > >  	mutex_init(&dig_port->tc_lock);
> > >  	dig_port->tc_legacy_port = is_legacy;
> > >  	dig_port->tc_link_refcount = 0;
> > > +	dig_port->tc_phy_fia = tc_port_to_fia(i915, tc_port);
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 95b9ca1fda2e..d0510022013c 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -2203,9 +2203,13 @@ enum i915_power_well_id {
> > >  #define   DW6_OLDO_DYN_PWR_DOWN_EN	(1 << 28)
> > > 
> > >  #define FIA1_BASE			0x163000
> > > +#define FIA2_BASE			0x16E000
> > > +#define FIA3_BASE			0x16F000
> > > +#define _FIA(fia)			_PICK((fia), FIA1_BASE,
> > > FIA2_BASE, FIA3_BASE)
> > > +#define _MMIO_FIA(fia, off)		_MMIO(_FIA(fia) +
> > > (off))
> > > 
> > >  /* ICL PHY DFLEX registers */
> > > -#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE +
> > > 0x008C0)
> > > +#define PORT_TX_DFLEXDPMLE1(fia)	_MMIO_FIA((fia),  0x008C0)
> > >  #define   DFLEXDPMLE1_DPMLETC_MASK(tc_port)	(0xf << (4 *
> > > (tc_port)))
> > >  #define   DFLEXDPMLE1_DPMLETC_ML0(tc_port)	(1 << (4 *
> > > (tc_port)))
> > >  #define   DFLEXDPMLE1_DPMLETC_ML1_0(tc_port)	(3 << (4 *
> > > (tc_port)))
> > > @@ -11484,17 +11488,18 @@ enum skl_power_gate {
> > >  						_ICL_DSC1_RC_BUF_THRESH
> > > _1_UDW_PB, \
> > >  						_ICL_DSC1_RC_BUF_THRESH
> > > _1_UDW_PC)
> > > 
> > > -#define PORT_TX_DFLEXDPSP			_MMIO(FIA1_BASE
> > > +
> > > 0x008A0)
> > > +#define PORT_TX_DFLEXDPSP(fia)			_MMIO_FIA((fia)
> > > , 0x008A0)
> > > +#define   MODULAR_FIA_MASK			(1 << 4)
> > >  #define   TC_LIVE_STATE_TBT(tc_port)		(1 <<
> > > ((tc_port) * 8 +
> > > 6))
> > >  #define   TC_LIVE_STATE_TC(tc_port)		(1 <<
> > > ((tc_port) * 8 +
> > > 5))
> > >  #define   DP_LANE_ASSIGNMENT_SHIFT(tc_port)	((tc_port) * 8)
> > >  #define   DP_LANE_ASSIGNMENT_MASK(tc_port)	(0xf <<
> > > ((tc_port) *
> > > 8))
> > >  #define   DP_LANE_ASSIGNMENT(tc_port, x)	((x) << ((tc_port) *
> > > 8))
> > > 
> > > -#define PORT_TX_DFLEXDPPMS				_MMIO(F
> > > IA1_BASE
> > > + 0x00890)
> > > +#define PORT_TX_DFLEXDPPMS(fia)			_MMIO_FIA((fia)
> > > , 0x00890)
> > >  #define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)		(1 <<
> > > (tc_port))
> > > 
> > > -#define PORT_TX_DFLEXDPCSSS			_MMIO(FIA1_BASE
> > > +
> > > 0x00894)
> > > +#define PORT_TX_DFLEXDPCSSS(fia)		_MMIO_FIA((fia),
> > > 0x00894)
> > >  #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 <<
> > > (tc_port))
> > > 
> > >  #endif /* _I915_REG_H_ */
> > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> > > b/drivers/gpu/drm/i915/intel_device_info.h
> > > index ddafc819bf30..e9dc86ed517b 100644
> > > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > > @@ -136,6 +136,7 @@ enum intel_ppgtt_type {
> > >  	func(has_gmch); \
> > >  	func(has_hotplug); \
> > >  	func(has_ipc); \
> > > +	func(has_modular_fia); \
> > 
> > If we have a register to tell us whether the platform supports
> > this,
> > why do we need this feature flag?
> 
> because the platform may not have the register bit that tells if the
> we
> have modular FIA, as is the case with Ice Lake. The bit is reserved
> there. This is actually "may_have_modular_fia" rather than
> "has_modular_fia", but the name is pretty bad to make it that way.

Ah, right, makes sense.

I would still like a comment above if possible, but not a blocker:
Reviewed-by: Stuart Summers <stuart.summers@xxxxxxxxx>

Thanks,
Stuart

> 
> Lucas De Marchi
> 
> > 
> > Thanks,
> > Stuart
> > 
> > >  	func(has_overlay); \
> > >  	func(has_psr); \
> > >  	func(overlay_needs_physical); \
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 770f9f6aad84..e8ecbd55476e 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1245,6 +1245,7 @@ struct intel_digital_port {
> > >  	bool tc_legacy_port:1;
> > >  	char tc_port_name[8];
> > >  	enum tc_port_mode tc_mode;
> > > +	enum phy_fia tc_phy_fia;
> > > 
> > >  	void (*write_infoframe)(struct intel_encoder *encoder,
> > >  				const struct intel_crtc_state
> > > *crtc_state,
> 
> 

Attachment: smime.p7s
Description: S/MIME cryptographic signature

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

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

  Powered by Linux