[Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH support

Thu Dec 31 08:48:06 UTC 2020

> -----Original Message-----
> From: Matt Roper <matthew.d.roper at intel.com>
> Sent: 31 December 2020 05:31
> To: Surendrakumar Upadhyay, TejaskumarX
> <tejaskumarx.surendrakumar.upadhyay at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Pandey, Hariom
> <hariom.pandey at intel.com>
> Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH support
> 
> On Mon, Dec 28, 2020 at 11:42:35AM +0530, Tejas Upadhyay wrote:
> > We have TGP PCH support for Tigerlake and Rocketlake. Similarly now
> > TGP PCH can be used with Cometlake CPU.
> 
> Based on the 'compatibility' section of bspec 49181, I think the TGP PCH can
> technically be compatible with any gen9bc platform, not just CML.
> Although it seems unlikely that anyone is going to go back and create new
> products with a SKL+TGP pairing or something at this point, it's still probably
> best to write this patch based on GEN9_BC rather than CML.
>

Tejas : This patch is generated to support DELL's requirement where they are using CML CPU + TGP PCH. But I agree if we want to change CML with GEN9_BC. Please have a look at https://gitlab.freedesktop.org/drm/intel/-/issues/2742 and this patch has been verified by DELL as working for all of their platforms with CML CPU + TGP PCH (Off course it worked after I gave local workaround of Lee Shawn's patch https://patchwork.freedesktop.org/patch/401301/?series=83154&rev=5). Also this patch + https://patchwork.freedesktop.org/patch/401301/?series=83154&rev=5 (Lee Shawn's patch reviewed by you) + Adding IS_COMETLAKE check to Lee Shawn's patch needs to be merged by Jan 4th to complete upstreaming for CML CPU + TGP PCH. DELL is having critical requirement to finish upstreaming by Jan 4th.

> >
> > Changes since V1 :
> > 	- Matched HPD Pin mapping for PORT C and PORT D of CML CPU.
> >
> > Cc : Matt Roper <matthew.d.roper at intel.com> Cc : Ville Syrjälä
> > <ville.syrjala at linux.intel.com>
> > Signed-off-by: Tejas Upadhyay
> > <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c     | 7 +++++--
> >  drivers/gpu/drm/i915/display/intel_display.c | 5 +++++
> >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 3 ++-
> >  3 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 17eaa56c5a99..181d60a5e145 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -5301,7 +5301,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_COMETLAKE(dev_priv) && port >= PORT_C)
> > +		return HPD_PORT_TC1 + port + 1 - PORT_TC1;
> > +	else if (port >= PORT_TC1)
> >  		return HPD_PORT_TC1 + port - PORT_TC1;
> >  	else
> >  		return HPD_PORT_A + port - PORT_A;
> > @@ -5455,7 +5457,8 @@ void intel_ddi_init(struct drm_i915_private
> > *dev_priv, enum port port)
> >
> >  	if (IS_DG1(dev_priv))
> >  		encoder->hpd_pin = dg1_hpd_pin(dev_priv, port);
> > -	else if (IS_ROCKETLAKE(dev_priv))
> > +	else if (IS_ROCKETLAKE(dev_priv) || (IS_COMETLAKE(dev_priv) &&
> > +					     HAS_PCH_TGP(dev_priv)))
> >  		encoder->hpd_pin = rkl_hpd_pin(dev_priv, port);
> >  	else if (INTEL_GEN(dev_priv) >= 12)
> 
> I'd suggest leaving the RKL condition alone since nothing here has anything to
> do with RKL.  Instead change the gen12+ condition to
> HAS_PCH_TGP() and update the TGP-specific handler to do the port mapping
> described on bspec 49181.
> 
Tejas : Ok.

> Plus I don't think what you have here would map the ports correctly anyway.
> gen9 PORT_C/PORT_D would map to HPD_PORT_C/HPD_PORT_TC1 with the
> logic here, whereas the bspec says they should map to
> HPD_PORT_TC1/HPD_PORT_TC2.
>
Tejas : This have been taken care in tgl_hpd_pin() API with right HPD pin mapping and its tested working on RVP as well as by DELL. 

> >  		encoder->hpd_pin = tgl_hpd_pin(dev_priv, port); diff --git
> > a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index f2c48e5cdb43..47014471658f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -16163,6 +16163,11 @@ static void intel_setup_outputs(struct
> drm_i915_private *dev_priv)
> >  			intel_ddi_init(dev_priv, PORT_F);
> >
> >  		icl_dsi_init(dev_priv);
> > +	} else if (IS_COMETLAKE(dev_priv) && HAS_PCH_TGP(dev_priv)) {
> > +		intel_ddi_init(dev_priv, PORT_A);
> > +		intel_ddi_init(dev_priv, PORT_B);
> > +		intel_ddi_init(dev_priv, PORT_C);
> > +		intel_ddi_init(dev_priv, PORT_D);
> 
> As noted before, this relates to gen9bc in general, not just CML.
> 
Tejas : I will add GEN9BC check here with TGP specific handle.

> Is the only reason for this block because TGP's instance of SFUSE_STRAP
> doesn't have output presence bits anymore?  If you want, you could keep
> using the existing gen9bc block for consistency, but make the SFUSE_STRAP
> checks themselves conditional on a platform that has the presence bits.  E.g.,
>
Tejas : I am not much familiar with STRAP, can I go ahead with above approach GEN9BC && TGP PCH check?

>     /* ICP+ no longer has port presence bits */
>     found = INTEL_PCH_TYPE(dev_priv) >= PCH_ICP ?
>         ~0 : intel_de_read(dev_priv, SFUSE_STRAP);
> 
> >  	} else if (IS_GEN9_LP(dev_priv)) {
> >  		/*
> >  		 * FIXME: Broxton doesn't support port detection via the diff -
> -git
> > a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index c5959590562b..540c9d54b595 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -3174,7 +3174,8 @@ static u8 intel_hdmi_ddc_pin(struct
> > intel_encoder *encoder)
> >
> >  	if (INTEL_PCH_TYPE(dev_priv) >= PCH_DG1)
> >  		ddc_pin = dg1_port_to_ddc_pin(dev_priv, port);
> > -	else if (IS_ROCKETLAKE(dev_priv))
> > +	else if (IS_ROCKETLAKE(dev_priv) || (IS_COMETLAKE(dev_priv) &&
> > +					     HAS_PCH_TGP(dev_priv)))
> >  		ddc_pin = rkl_port_to_ddc_pin(dev_priv, port);
> 
> As above, none of the changes in this patch have any relation to RKL, so it
> doesn't make sense to update the RKL condition.  Instead just add the gen9bc
> port mapping logic to icl_port_to_ddc_pin().
> 
Tejas : Ok.
> Plus, it looks like what you have here right now will make the same mapping
> mistake for C and D that the HPD logic did.
> 
Tejas : It is doing proper pin mapping. Its tested by us on RVP and by DELL.
> 
> Matt
> 
> >  	else if (HAS_PCH_MCC(dev_priv))
> >  		ddc_pin = mcc_port_to_ddc_pin(dev_priv, port);
> > --
> > 2.28.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795