On Thu, Dec 31, 2020 at 12:48:06AM -0800, Surendrakumar Upadhyay, TejaskumarX wrote: > > > > -----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. 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 <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; 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 > > > 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 -- 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
