[V2] drm/i915/cml : Add TGP PCH support

Commit Message

Tejas Upadhyay Dec. 28, 2020, 6:12 a.m. UTC

We have TGP PCH support for Tigerlake and Rocketlake. Similarly
now TGP PCH can be used with Cometlake CPU.

Changes since V1 :
	- Matched HPD Pin mapping for PORT C and PORT D of CML CPU.

Cc : Matt Roper <[e-mail]>
Cc : Ville Syrjälä <[e-mail]> 
Signed-off-by: Tejas Upadhyay <[e-mail]>
---
 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(-)

Comments

Tejas Upadhyay Dec. 28, 2020, 6:26 a.m. UTC | #1

Hi,

Please note that https://patchwork.freedesktop.org/patch/398145/?series=83154 this patch is required for HDMI to work on CML + TGP PCH combo.

Thanks,
Tejas

> -----Original Message-----
> From: Intel-gfx <[e-mail]> On Behalf Of Tejas
> Upadhyay
> Sent: 28 December 2020 11:43
> To: [e-mail]; Pandey, Hariom
> <[e-mail]>
> Subject: [Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH support
> 
> We have TGP PCH support for Tigerlake and Rocketlake. Similarly now TGP
> PCH can be used with Cometlake CPU.
> 
> Changes since V1 :
> 	- Matched HPD Pin mapping for PORT C and PORT D of CML CPU.
> 
> Cc : Matt Roper <[e-mail]> Cc : Ville Syrjälä
> <[e-mail]>
> Signed-off-by: Tejas Upadhyay
> <[e-mail]>
> ---
>  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)
>  		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);
>  	} 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);
>  	else if (HAS_PCH_MCC(dev_priv))
>  		ddc_pin = mcc_port_to_ddc_pin(dev_priv, port);
> --
> 2.28.0
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Matt Roper Dec. 31, 2020, 12:01 a.m. UTC | #2

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.

> 
> Changes since V1 :
> 	- Matched HPD Pin mapping for PORT C and PORT D of CML CPU.
> 
> Cc : Matt Roper <[e-mail]>
> Cc : Ville Syrjälä <[e-mail]> 
> Signed-off-by: Tejas Upadhyay <[e-mail]>
> ---
>  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.

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.

>  		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.

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.,

    /* 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().

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.


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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Tejas Upadhyay Dec. 31, 2020, 8:48 a.m. UTC | #3

> -----Original Message-----
> From: Matt Roper <[e-mail]>
> Sent: 31 December 2020 05:31
> To: Surendrakumar Upadhyay, TejaskumarX
> <[e-mail]>
> Cc: [e-mail]; Pandey, Hariom
> <[e-mail]>
> 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 <[e-mail]> Cc : Ville Syrjälä
> > <[e-mail]>
> > Signed-off-by: Tejas Upadhyay
> > <[e-mail]>
> > ---
> >  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
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> [phone]

Matt Roper Dec. 31, 2020, 9:01 p.m. UTC | #4

On Thu, Dec 31, 2020 at 12:48:06AM -0800, Surendrakumar Upadhyay, TejaskumarX wrote:
> 
> 
> > -----Original Message-----
> > From: Matt Roper <[e-mail]>
> > Sent: 31 December 2020 05:31
> > To: Surendrakumar Upadhyay, TejaskumarX
> > <[e-mail]>
> > Cc: [e-mail]; Pandey, Hariom
> > <[e-mail]>
> > 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.

The changes from Shawn are to make RKL (a gen12 platform) work with the
older gen9-style CMP PCH.  What you're doing here is making a gen9
platform work with a newer gen12-style TGP PCH.  Although those are
converses of each other, I don't think the driver changes should depend
on each other.  Shawn's series shouldn't be necessary for your work or
vice versa.  I'm not sure when Shawn plans to merge his series; I had
some further changes suggested, so he might be working on those before
merging his work.

I'm not sure what leads to the Jan 4th date, but assuming "finish
upstreaming" means that you want the patch to land in a final release
kernel by that date, there's pretty much no way that would be possible
at this point.  Getting patches like this reviewed and applied to an
Intel tree is only the first step along the maintainer chain that
eventually leads to a release from Linus or a stable kernel maintainer.
Plus when a customer says they want something upstream, one of the most
important things for them is that the patch has been fully reviewed and
tested and has a relatively high chance of being correct.  We can't rush
patches in to meet deadlines if we think they're only going to work in
certain situations and cause problems for other ones.

One other thing that I don't see addressed anywhere in this patch is
that the driver doesn't consider gen9 + TGP to be a valid combination
and will throw a warning in intel_pch_type() if detected.

> 
> > >
> > > Changes since V1 :
> > > - Matched HPD Pin mapping for PORT C and PORT D of CML CPU.
> > >
> > > Cc : Matt Roper <[e-mail]> Cc : Ville Syrjälä
> > > <[e-mail]>
> > > Signed-off-by: Tejas Upadhyay
> > > <[e-mail]>
> > > ---
> > >  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;

Why is the offset written as "port + 1 - PORT_TC1?"  This platform
doesn't have TC ports as inputs, so it's completely unintuitive how
"+ 1 - PORT_TC1" would relate to PORT_C unless you go lookup the enum
values (plus the unexpected "+1" part is really easy to overlook as I
did the first time I looked through this patch).

This should just be written with a more straightforward offset as:

        return HPD_PORT_TC1 + port - PORT_C;

> > > +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?

The output initialization is already a bit of a mess (and we plan to
overhaul the design soon), so adding special case conditions like this
just makes it more complicated and harder to follow.  I'm asking what
led you to create a new block rather than just letting the existing
block continue to be used.  I can see where TGP's lack of strap bits
could be a problem (since reading those bits as 0 would incorrectly lead
you to believe that the output didn't exist), but if that's the only
thing you were trying to avoid, then it's probably simpler to just
update the place we read the fuse value.  Were there other reasons you
also decided to create a new block?


> 
> >     /* 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.

Are you sure this was fully tested and you didn't forget any of the
outputs?  The first thing the function does is

        WARN_ON(port == PORT_C);

which means that you should have immediately started seeing warnings on
any CML platforms with an HDMI output on DDI-C (which is a valid setup).
What we should be warning on is PORT_A since gen9 platforms like CML
can't handle HDMI on port A.


Matt

> >
> > 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
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > [phone]

Tejas Upadhyay Jan. 4, 2021, 4:40 a.m. UTC | #5

> -----Original Message-----
> From: Matt Roper <[e-mail]>
> Sent: 01 January 2021 02:32
> To: Surendrakumar Upadhyay, TejaskumarX
> <[e-mail]>
> Cc: [e-mail]; Pandey, Hariom
> <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH support
> 
> On Thu, Dec 31, 2020 at 12:48:06AM -0800, Surendrakumar Upadhyay,
> TejaskumarX wrote:
> >
> >
> > > -----Original Message-----
> > > From: Matt Roper <[e-mail]>
> > > Sent: 31 December 2020 05:31
> > > To: Surendrakumar Upadhyay, TejaskumarX
> > > <[e-mail]>
> > > Cc: [e-mail]; Pandey, Hariom
> > > <[e-mail]>
> > > 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.
> 
> The changes from Shawn are to make RKL (a gen12 platform) work with the
> older gen9-style CMP PCH.  What you're doing here is making a gen9
> platform work with a newer gen12-style TGP PCH.  Although those are
> converses of each other, I don't think the driver changes should depend on
> each other.  Shawn's series shouldn't be necessary for your work or vice
> versa.  I'm not sure when Shawn plans to merge his series; I had some
> further changes suggested, so he might be working on those before merging
> his work.

Tejas : Just to bring your attention here that RKL RVP ddc_pins are not mapped correctly with TGP PCH currently, specially when it comes to remapping of VBT values to platform. I think Shawn's patch is addressing that as well. I have tested RKL RVP we don't have right ddc pin mapping currently thus there was problem in detection of ports. Please check https://jira.devtools.intel.com/browse/VLK-16850 (ignore "port C" wording in VLK, its port TC1). Please suggest if I need to create patch for VBT remap for CML/GEN9BC  on top of Shawn's patch or I can send as part of this patchset?

> 
> I'm not sure what leads to the Jan 4th date, but assuming "finish
> upstreaming" means that you want the patch to land in a final release kernel
> by that date, there's pretty much no way that would be possible at this point.
> Getting patches like this reviewed and applied to an Intel tree is only the first
> step along the maintainer chain that eventually leads to a release from Linus
> or a stable kernel maintainer.
> Plus when a customer says they want something upstream, one of the most
> important things for them is that the patch has been fully reviewed and
> tested and has a relatively high chance of being correct.  We can't rush
> patches in to meet deadlines if we think they're only going to work in certain
> situations and cause problems for other ones.
> 
> One other thing that I don't see addressed anywhere in this patch is that the
> driver doesn't consider gen9 + TGP to be a valid combination and will throw a
> warning in intel_pch_type() if detected.

Tejas : Yes I am planning to add GEN9 + TGP as valid combo, so it does not throw warning. And about the deadline, we will follow our process for sure.

> 
> >
> > > >
> > > > Changes since V1 :
> > > > - Matched HPD Pin mapping for PORT C and PORT D of CML CPU.
> > > >
> > > > Cc : Matt Roper <[e-mail]> Cc : Ville Syrjälä
> > > > <[e-mail]>
> > > > Signed-off-by: Tejas Upadhyay
> > > > <[e-mail]>
> > > > ---
> > > >  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;
> 
> Why is the offset written as "port + 1 - PORT_TC1?"  This platform doesn't
> have TC ports as inputs, so it's completely unintuitive how "+ 1 - PORT_TC1"
> would relate to PORT_C unless you go lookup the enum values (plus the
> unexpected "+1" part is really easy to overlook as I did the first time I looked
> through this patch).
> 
> This should just be written with a more straightforward offset as:
> 
>         return HPD_PORT_TC1 + port - PORT_C;
Tejas : Ok.
> 
> > > > +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?
> 
> The output initialization is already a bit of a mess (and we plan to overhaul
> the design soon), so adding special case conditions like this just makes it
> more complicated and harder to follow.  I'm asking what led you to create a
> new block rather than just letting the existing block continue to be used.  I
> can see where TGP's lack of strap bits could be a problem (since reading
> those bits as 0 would incorrectly lead you to believe that the output didn't
> exist), but if that's the only thing you were trying to avoid, then it's probably
> simpler to just update the place we read the fuse value.  Were there other
> reasons you also decided to create a new block?

Tejas : As I told I am not much clear with STRAP logic, but the observation was that only port A output was getting initialized by default. Okay I will avoid fuse checks for GEN9 BC && TGP PCH case and use existing blocks.
> 
> 
> >
> > >     /* 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.
> 
> Are you sure this was fully tested and you didn't forget any of the outputs?
> The first thing the function does is
> 
>         WARN_ON(port == PORT_C);
> 
> which means that you should have immediately started seeing warnings on
> any CML platforms with an HDMI output on DDI-C (which is a valid setup).
> What we should be warning on is PORT_A since gen9 platforms like CML
> can't handle HDMI on port A.
Tejas : Warns are coming, but they will not affect result. However I am planning to remove/update warns in the next patch versions. Also yes we have tested with 4 (A,B,C,D) outputs. I know the fact that CML has E output as well but I have not added it, should I add that also?. So far RKL (UDIMM) RVP + TGP (with all ports), RKL (SODIMM) RVP + TGP (with all ports), CML (UDIMM) RVP + TGP (with A,B,C,D ports), CML (SODIMM) RVP + TGP (with A,B,C,D ports), AIO(Canonical/Wostron, CML CPU(RKL RVP) + TGP PCH), Caribou(Dell board, CML CPU(RKL RVP) + TGP PCH), Seal(Dell board, CML CPU(RKL RVP) + TGP PCH), Proghron(Canonical/Wostron, CML CPU(RKL RVP) + TGP PCH) boards are tested and successfully able to enumerate all the ports (including HDMI on every board).
> 
> 
> Matt
> 
> > >
> > > 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
> > > > [e-mail]
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Matt Roper
> > > Graphics Software Engineer
> > > VTT-OSGC Platform Enablement
> > > Intel Corporation
> > > [phone]
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> [phone]

Lyude Paul Jan. 6, 2021, 6:14 p.m. UTC | #6

On Mon, 2021-01-04 at 04:40 +0000, Surendrakumar Upadhyay, TejaskumarX wrote:
> 
> 
> > -----Original Message-----
> > From: Matt Roper <[e-mail]>
> > Sent: 01 January 2021 02:32
> > To: Surendrakumar Upadhyay, TejaskumarX
> > <[e-mail]>
> > Cc: [e-mail]; Pandey, Hariom
> > <[e-mail]>
> > Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH support
> > 
> > On Thu, Dec 31, 2020 at 12:48:06AM -0800, Surendrakumar Upadhyay,
> > TejaskumarX wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Matt Roper <[e-mail]>
> > > > Sent: 31 December 2020 05:31
> > > > To: Surendrakumar Upadhyay, TejaskumarX
> > > > <[e-mail]>
> > > > Cc: [e-mail]; Pandey, Hariom
> > > > <[e-mail]>
> > > > 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.
> > 
> > The changes from Shawn are to make RKL (a gen12 platform) work with the
> > older gen9-style CMP PCH.  What you're doing here is making a gen9
> > platform work with a newer gen12-style TGP PCH.  Although those are
> > converses of each other, I don't think the driver changes should depend on
> > each other.  Shawn's series shouldn't be necessary for your work or vice
> > versa.  I'm not sure when Shawn plans to merge his series; I had some
> > further changes suggested, so he might be working on those before merging
> > his work.
> 
> Tejas : Just to bring your attention here that RKL RVP ddc_pins are not
> mapped correctly with TGP PCH currently, specially when it comes to
> remapping of VBT values to platform. I think Shawn's patch is addressing
> that as well. I have tested RKL RVP we don't have right ddc pin mapping
> currently thus there was problem in detection of ports. Please check 
> https://jira.devtools.intel.com/browse/VLK-16850 (ignore "port C" wording in
> VLK, its port TC1). Please suggest if I need to create patch for VBT remap
> for CML/GEN9BC  on top of Shawn's patch or I can send as part of this
> patchset?

Just an FYI the DDC patch for this already had the reviews needed, so I went
ahead and pushed it upstream yesterday. So, just rebase your patch against
upstream and it should be OK.

> 
> > 
> > I'm not sure what leads to the Jan 4th date, but assuming "finish
> > upstreaming" means that you want the patch to land in a final release
> > kernel
> > by that date, there's pretty much no way that would be possible at this
> > point.
> > Getting patches like this reviewed and applied to an Intel tree is only
> > the first
> > step along the maintainer chain that eventually leads to a release from
> > Linus
> > or a stable kernel maintainer.
> > Plus when a customer says they want something upstream, one of the most
> > important things for them is that the patch has been fully reviewed and
> > tested and has a relatively high chance of being correct.  We can't rush
> > patches in to meet deadlines if we think they're only going to work in
> > certain
> > situations and cause problems for other ones.

Just an FYI, customers like Dell (FWIW-they're the reason I'm looking at these
patches right now) are dealing with situations where there are upstream
requirements for the code that they use. So, just having a "mostly correct
patch" isn't really enough, having this development happen upstream first
should always be the priority anyway.

> > 
> > One other thing that I don't see addressed anywhere in this patch is that
> > the
> > driver doesn't consider gen9 + TGP to be a valid combination and will
> > throw a
> > warning in intel_pch_type() if detected.
> 
> Tejas : Yes I am planning to add GEN9 + TGP as valid combo, so it does not
> throw warning. And about the deadline, we will follow our process for sure.

When do you think this patch will be ready btw?

> 
> > 
> > > 
> > > > > 
> > > > > Changes since V1 :
> > > > > - Matched HPD Pin mapping for PORT C and PORT D of CML CPU.
> > > > > 
> > > > > Cc : Matt Roper <[e-mail]> Cc : Ville Syrjälä
> > > > > <[e-mail]>
> > > > > Signed-off-by: Tejas Upadhyay
> > > > > <[e-mail]>
> > > > > ---
> > > > >  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;
> > 
> > Why is the offset written as "port + 1 - PORT_TC1?"  This platform doesn't
> > have TC ports as inputs, so it's completely unintuitive how "+ 1 -
> > PORT_TC1"
> > would relate to PORT_C unless you go lookup the enum values (plus the
> > unexpected "+1" part is really easy to overlook as I did the first time I
> > looked
> > through this patch).
> > 
> > This should just be written with a more straightforward offset as:
> > 
> >         return HPD_PORT_TC1 + port - PORT_C;
> Tejas : Ok.
> > 
> > > > > +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?
> > 
> > The output initialization is already a bit of a mess (and we plan to
> > overhaul
> > the design soon), so adding special case conditions like this just makes
> > it
> > more complicated and harder to follow.  I'm asking what led you to create
> > a
> > new block rather than just letting the existing block continue to be
> > used.  I
> > can see where TGP's lack of strap bits could be a problem (since reading
> > those bits as 0 would incorrectly lead you to believe that the output
> > didn't
> > exist), but if that's the only thing you were trying to avoid, then it's
> > probably
> > simpler to just update the place we read the fuse value.  Were there other
> > reasons you also decided to create a new block?
> 
> Tejas : As I told I am not much clear with STRAP logic, but the observation
> was that only port A output was getting initialized by default. Okay I will
> avoid fuse checks for GEN9 BC && TGP PCH case and use existing blocks.
> > 
> > 
> > > 
> > > >     /* 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.
> > 
> > Are you sure this was fully tested and you didn't forget any of the
> > outputs?
> > The first thing the function does is
> > 
> >         WARN_ON(port == PORT_C);
> > 
> > which means that you should have immediately started seeing warnings on
> > any CML platforms with an HDMI output on DDI-C (which is a valid setup).
> > What we should be warning on is PORT_A since gen9 platforms like CML
> > can't handle HDMI on port A.
> Tejas : Warns are coming, but they will not affect result. However I am
> planning to remove/update warns in the next patch versions. Also yes we have
> tested with 4 (A,B,C,D) outputs. I know the fact that CML has E output as
> well but I have not added it, should I add that also?. So far RKL (UDIMM)
> RVP + TGP (with all ports), RKL (SODIMM) RVP + TGP (with all ports), CML
> (UDIMM) RVP + TGP (with A,B,C,D ports), CML (SODIMM) RVP + TGP (with A,B,C,D
> ports), AIO(Canonical/Wostron, CML CPU(RKL RVP) + TGP PCH), Caribou(Dell
> board, CML CPU(RKL RVP) + TGP PCH), Seal(Dell board, CML CPU(RKL RVP) + TGP
> PCH), Proghron(Canonical/Wostron, CML CPU(RKL RVP) + TGP PCH) boards are
> tested and successfully able to enumerate all the ports (including HDMI on
> every board).
> > 
> > 
> > Matt
> > 
> > > > 
> > > > 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
> > > > > [e-mail]
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > --
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > VTT-OSGC Platform Enablement
> > > > Intel Corporation
> > > > [phone]
> > 
> > --
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > [phone]
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Tejas Upadhyay Jan. 7, 2021, 4:12 a.m. UTC | #7

Hello,

I already have new patchset upstream for review, please review https://patchwork.freedesktop.org/patch/412072/ .

Thanks,
Tejas

> -----Original Message-----
> From: Lyude Paul <[e-mail]>
> Sent: 06 January 2021 23:45
> To: Surendrakumar Upadhyay, TejaskumarX
> <[e-mail]>; Roper, Matthew D
> <[e-mail]>
> Cc: [e-mail]; Pandey, Hariom
> <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH support
> 
> On Mon, 2021-01-04 at 04:40 +0000, Surendrakumar Upadhyay, TejaskumarX
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Matt Roper <[e-mail]>
> > > Sent: 01 January 2021 02:32
> > > To: Surendrakumar Upadhyay, TejaskumarX
> > > <[e-mail]>
> > > Cc: [e-mail]; Pandey, Hariom
> > > <[e-mail]>
> > > Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH
> > > support
> > >
> > > On Thu, Dec 31, 2020 at 12:48:06AM -0800, Surendrakumar Upadhyay,
> > > TejaskumarX wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Matt Roper <[e-mail]>
> > > > > Sent: 31 December 2020 05:31
> > > > > To: Surendrakumar Upadhyay, TejaskumarX
> > > > > <[e-mail]>
> > > > > Cc: [e-mail]; Pandey, Hariom
> > > > > <[e-mail]>
> > > > > 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.
> > >
> > > The changes from Shawn are to make RKL (a gen12 platform) work with
> > > the older gen9-style CMP PCH.  What you're doing here is making a
> > > gen9 platform work with a newer gen12-style TGP PCH.  Although those
> > > are converses of each other, I don't think the driver changes should
> > > depend on each other.  Shawn's series shouldn't be necessary for
> > > your work or vice versa.  I'm not sure when Shawn plans to merge his
> > > series; I had some further changes suggested, so he might be working
> > > on those before merging his work.
> >
> > Tejas : Just to bring your attention here that RKL RVP ddc_pins are
> > not mapped correctly with TGP PCH currently, specially when it comes
> > to remapping of VBT values to platform. I think Shawn's patch is
> > addressing that as well. I have tested RKL RVP we don't have right ddc
> > pin mapping currently thus there was problem in detection of ports.
> > Please check
> > https://jira.devtools.intel.com/browse/VLK-16850 (ignore "port C"
> > wording in VLK, its port TC1). Please suggest if I need to create
> > patch for VBT remap for CML/GEN9BC  on top of Shawn's patch or I can
> > send as part of this patchset?
> 
> Just an FYI the DDC patch for this already had the reviews needed, so I went
> ahead and pushed it upstream yesterday. So, just rebase your patch against
> upstream and it should be OK.
> 
> >
> > >
> > > I'm not sure what leads to the Jan 4th date, but assuming "finish
> > > upstreaming" means that you want the patch to land in a final
> > > release kernel by that date, there's pretty much no way that would
> > > be possible at this point.
> > > Getting patches like this reviewed and applied to an Intel tree is
> > > only the first step along the maintainer chain that eventually leads
> > > to a release from Linus or a stable kernel maintainer.
> > > Plus when a customer says they want something upstream, one of the
> > > most important things for them is that the patch has been fully
> > > reviewed and tested and has a relatively high chance of being
> > > correct.  We can't rush patches in to meet deadlines if we think
> > > they're only going to work in certain situations and cause problems
> > > for other ones.
> 
> Just an FYI, customers like Dell (FWIW-they're the reason I'm looking at these
> patches right now) are dealing with situations where there are upstream
> requirements for the code that they use. So, just having a "mostly correct
> patch" isn't really enough, having this development happen upstream first
> should always be the priority anyway.
> 
> > >
> > > One other thing that I don't see addressed anywhere in this patch is
> > > that the driver doesn't consider gen9 + TGP to be a valid
> > > combination and will throw a warning in intel_pch_type() if
> > > detected.
> >
> > Tejas : Yes I am planning to add GEN9 + TGP as valid combo, so it does
> > not throw warning. And about the deadline, we will follow our process for
> sure.
> 
> When do you think this patch will be ready btw?
> 
> >
> > >
> > > >
> > > > > >
> > > > > > Changes since V1 :
> > > > > > - Matched HPD Pin mapping for PORT C and PORT D of CML CPU.
> > > > > >
> > > > > > Cc : Matt Roper <[e-mail]> Cc : Ville Syrjälä
> > > > > > <[e-mail]>
> > > > > > Signed-off-by: Tejas Upadhyay
> > > > > > <[e-mail]>
> > > > > > ---
> > > > > >  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;
> > >
> > > Why is the offset written as "port + 1 - PORT_TC1?"  This platform
> > > doesn't have TC ports as inputs, so it's completely unintuitive how
> > > "+ 1 - PORT_TC1"
> > > would relate to PORT_C unless you go lookup the enum values (plus
> > > the unexpected "+1" part is really easy to overlook as I did the
> > > first time I looked through this patch).
> > >
> > > This should just be written with a more straightforward offset as:
> > >
> > >         return HPD_PORT_TC1 + port - PORT_C;
> > Tejas : Ok.
> > >
> > > > > > +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?
> > >
> > > The output initialization is already a bit of a mess (and we plan to
> > > overhaul the design soon), so adding special case conditions like
> > > this just makes it more complicated and harder to follow.  I'm
> > > asking what led you to create a new block rather than just letting
> > > the existing block continue to be used.  I can see where TGP's lack
> > > of strap bits could be a problem (since reading those bits as 0
> > > would incorrectly lead you to believe that the output didn't exist),
> > > but if that's the only thing you were trying to avoid, then it's
> > > probably simpler to just update the place we read the fuse value.
> > > Were there other reasons you also decided to create a new block?
> >
> > Tejas : As I told I am not much clear with STRAP logic, but the
> > observation was that only port A output was getting initialized by
> > default. Okay I will avoid fuse checks for GEN9 BC && TGP PCH case and use
> existing blocks.
> > >
> > >
> > > >
> > > > >     /* 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.
> > >
> > > Are you sure this was fully tested and you didn't forget any of the
> > > outputs?
> > > The first thing the function does is
> > >
> > >         WARN_ON(port == PORT_C);
> > >
> > > which means that you should have immediately started seeing warnings
> > > on any CML platforms with an HDMI output on DDI-C (which is a valid
> setup).
> > > What we should be warning on is PORT_A since gen9 platforms like CML
> > > can't handle HDMI on port A.
> > Tejas : Warns are coming, but they will not affect result. However I
> > am planning to remove/update warns in the next patch versions. Also
> > yes we have tested with 4 (A,B,C,D) outputs. I know the fact that CML
> > has E output as well but I have not added it, should I add that also?.
> > So far RKL (UDIMM) RVP + TGP (with all ports), RKL (SODIMM) RVP + TGP
> > (with all ports), CML
> > (UDIMM) RVP + TGP (with A,B,C,D ports), CML (SODIMM) RVP + TGP (with
> > A,B,C,D ports), AIO(Canonical/Wostron, CML CPU(RKL RVP) + TGP PCH),
> > Caribou(Dell board, CML CPU(RKL RVP) + TGP PCH), Seal(Dell board, CML
> > CPU(RKL RVP) + TGP PCH), Proghron(Canonical/Wostron, CML CPU(RKL
> RVP)
> > + TGP PCH) boards are tested and successfully able to enumerate all
> > the ports (including HDMI on every board).
> > >
> > >
> > > Matt
> > >
> > > > >
> > > > > 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
> > > > > > [e-mail]
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > >
> > > > > --
> > > > > Matt Roper
> > > > > Graphics Software Engineer
> > > > > VTT-OSGC Platform Enablement
> > > > > Intel Corporation
> > > > > [phone]
> > >
> > > --
> > > Matt Roper
> > > Graphics Software Engineer
> > > VTT-OSGC Platform Enablement
> > > Intel Corporation
> > > [phone]
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat

Patch

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)
 		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);
 	} 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);
 	else if (HAS_PCH_MCC(dev_priv))
 		ddc_pin = mcc_port_to_ddc_pin(dev_priv, port);