> -----Original Message----- > From: Matt Roper <matthew.d.roper@xxxxxxxxx> > Sent: 31 December 2020 05:31 > To: Surendrakumar Upadhyay, TejaskumarX > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Pandey, Hariom > <hariom.pandey@xxxxxxxxx> > Subject: Re: [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@xxxxxxxxx> Cc : Ville Syrjälä > > <ville.syrjala@xxxxxxxxxxxxxxx> > > Signed-off-by: Tejas Upadhyay > > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation > (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
