On Mon, 2021-01-04 at 04:40 +0000, Surendrakumar Upadhyay, TejaskumarX wrote: > > > > -----Original Message----- > > From: Matt Roper <matthew.d.roper@xxxxxxxxx> > > Sent: 01 January 2021 02:32 > > 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 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. > > 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 <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; > 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 > > > > > 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 > -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
