Re: [Intel-gfx] [PATCH V4] drm/i915/gen9_bc : Add TGP PCH support

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org, hariom.pandey@intel.com
Subject: Re: [Intel-gfx] [PATCH V4] drm/i915/gen9_bc : Add TGP PCH support
Date: Tue, 19 Jan 2021 18:13:14 +0200	[thread overview]
Message-ID: <YAcFGumUwuonhUcP@intel.com> (raw)
In-Reply-To: <20210119155247.GA3970@intel.com>

On Tue, Jan 19, 2021 at 10:52:47AM -0500, Rodrigo Vivi wrote:
> On Mon, Jan 11, 2021 at 05:30:00PM -0800, Matt Roper wrote:
> > On Mon, Jan 11, 2021 at 07:21:55PM -0500, Rodrigo Vivi wrote:
> > > On Fri, Jan 08, 2021 at 05:39:22PM +0530, Tejas Upadhyay wrote:
> > > > We have TGP PCH support for Tigerlake and Rocketlake. Similarly
> > > > now TGP PCH can be used with Cometlake CPU.
> > > > 
> > > > Changes since V3 :
> > > > 	- Rebased to top drm-tip commit
> > > > 	- dev_priv replaced with i915 for new API
> > > > 	- Enable default Port B,C,D detection for TGP && GEN9_BC
> > > > Changes since V2 :
> > > >         - IS_COMETLAKE replaced with IS_GEN9_BC
> > > >         - VBT ddc pin remapping added
> > > >         - Added dedicated HPD pin and DDC pin handling API
> > > > Changes since V1 :
> > > >         - Matched HPD Pin mapping for PORT C and PORT D of CML CPU.
> > > > 
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_bios.c    |  9 +++++++++
> > > >  drivers/gpu/drm/i915/display/intel_ddi.c     |  7 +++++--
> > > >  drivers/gpu/drm/i915/display/intel_display.c |  9 ++++++++-
> > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 20 ++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_pch.c             |  3 ++-
> > > >  5 files changed, 44 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > > > index 987cf509337f..730b7f45e5d4 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > > > @@ -1630,6 +1630,12 @@ static const u8 rkl_pch_tgp_ddc_pin_map[] = {
> > > >  	[RKL_DDC_BUS_DDI_E] = GMBUS_PIN_10_TC2_ICP,
> > > >  };
> > > >  
> > > > +static const u8 gen9bc_tgp_ddc_pin_map[] = {
> > > > +	[DDC_BUS_DDI_B] = GMBUS_PIN_2_BXT,
> > > > +	[DDC_BUS_DDI_C] = GMBUS_PIN_9_TC1_ICP,
> > > > +	[DDC_BUS_DDI_D] = GMBUS_PIN_10_TC2_ICP,
> > > > +};
> > > 
> > > Could you please point out the spec you are using here?
> > > 
> > > VBT's spec at BSpec - at Block 2
> > > I can see the TGP table is same as ICP.
> > > 
> > > So I'm kind of confused now.
> > 
> > It's a weird place to document it, but bspec 49181 has a compatibility
> > section that describes how to map the TGP pins when paired with a gen9bc
> > CPU.
> 
> Really weird place, but it makes some sense now.
> We should file a BSpec bug and request this information to be consolidated
> with VBT one.
> 
> > 
> > > 
> > > > +
> > > >  static u8 map_ddc_pin(struct drm_i915_private *dev_priv, u8 vbt_pin)
> > > >  {
> > > >  	const u8 *ddc_pin_map;
> > > > @@ -1640,6 +1646,9 @@ static u8 map_ddc_pin(struct drm_i915_private *dev_priv, u8 vbt_pin)
> > > >  	} else if (IS_ROCKETLAKE(dev_priv) && INTEL_PCH_TYPE(dev_priv) == PCH_TGP) {
> > > >  		ddc_pin_map = rkl_pch_tgp_ddc_pin_map;
> > > >  		n_entries = ARRAY_SIZE(rkl_pch_tgp_ddc_pin_map);
> > > > +	} else if (HAS_PCH_TGP(dev_priv) && IS_GEN9_BC(dev_priv)) {
> > > > +		ddc_pin_map = gen9bc_tgp_ddc_pin_map;
> > > > +		n_entries = ARRAY_SIZE(gen9bc_tgp_ddc_pin_map);
> > > >  	} else if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) {
> > > >  		ddc_pin_map = icp_ddc_pin_map;
> > > >  		n_entries = ARRAY_SIZE(icp_ddc_pin_map);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > index 3df6913369bc..13f1268e2cff 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > @@ -5337,7 +5337,9 @@ static enum hpd_pin dg1_hpd_pin(struct drm_i915_private *dev_priv,
> > > >  static enum hpd_pin tgl_hpd_pin(struct drm_i915_private *dev_priv,
> > > >  				enum port port)
> > > >  {
> > > > -	if (port >= PORT_TC1)
> > > > +	if (IS_GEN9_BC(dev_priv) && port >= PORT_C)
> > > 
> > > gen9 in tgl function?!
> > > please, no!
> > 
> > We should probably rename this function to tgp since it ultimately gets
> > called on every possible TGP platform (TGL+TGP, RKL+TGP, gen9+TGP).  If
> > it weren't for RKL+CMP, I'd say that all these functions should just be
> > named after the PCH, but I guess the TC ports on RKL+CMP break the
> > pattern.
> 
> okay, so we need at least this.

static enum hpd_pin skl_hpd_pin(struct drm_i915_private *dev_priv,
				enum port port)
{
	if (HAS_PCH_TGP(dev_priv))
		return icl_hpd_pin(dev_priv, port);

	return HPD_PORT_A + port - PORT_A;
}

is what I have sitting in a local branch. Just never upstreamed it since
I wasn't sure the rkl+tgp was going to be a real thing.

Also this patch should be split into several independent parts.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx