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

Thu Jan 7 04:12:07 UTC 2021

Hello,

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

Thanks,
Tejas

> -----Original Message-----
> From: Lyude Paul <lyude at redhat.com>
> Sent: 06 January 2021 23:45
> To: Surendrakumar Upadhyay, TejaskumarX
> <tejaskumarx.surendrakumar.upadhyay at intel.com>; Roper, Matthew D
> <matthew.d.roper at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Pandey, Hariom
> <hariom.pandey at intel.com>
> Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH support
> 
> On Mon, 2021-01-04 at 04:40 +0000, Surendrakumar Upadhyay, TejaskumarX
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Matt Roper <matthew.d.roper at intel.com>
> > > Sent: 01 January 2021 02:32
> > > To: Surendrakumar Upadhyay, TejaskumarX
> > > <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > > Cc: intel-gfx at lists.freedesktop.org; Pandey, Hariom
> > > <hariom.pandey at intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH
> > > support
> > >
> > > On Thu, Dec 31, 2020 at 12:48:06AM -0800, Surendrakumar Upadhyay,
> > > TejaskumarX wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Matt Roper <matthew.d.roper at intel.com>
> > > > > Sent: 31 December 2020 05:31
> > > > > To: Surendrakumar Upadhyay, TejaskumarX
> > > > > <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > > > > Cc: intel-gfx at lists.freedesktop.org; Pandey, Hariom
> > > > > <hariom.pandey at intel.com>
> > > > > Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/cml : Add TGP PCH
> > > > > support
> > > > >
> > > > > On Mon, Dec 28, 2020 at 11:42:35AM +0530, Tejas Upadhyay wrote:
> > > > > > We have TGP PCH support for Tigerlake and Rocketlake.
> > > > > > Similarly now TGP PCH can be used with Cometlake CPU.
> > > > >
> > > > > Based on the 'compatibility' section of bspec 49181, I think the
> > > > > TGP PCH can technically be compatible with any gen9bc platform,
> > > > > not just
> > > CML.
> > > > > Although it seems unlikely that anyone is going to go back and
> > > > > create new products with a SKL+TGP pairing or something at this
> > > > > point, it's still probably best to write this patch based on
> > > > > GEN9_BC rather
> > > than CML.
> > > > >
> > > >
> > > >
> > > > Tejas : This patch is generated to support DELL's requirement
> > > > where they
> > > are using CML CPU + TGP PCH. But I agree if we want to change CML
> > > with GEN9_BC. Please have a look at
> > > https://gitlab.freedesktop.org/drm/intel/-
> > > /issues/2742 and this patch has been verified by DELL as working for
> > > all of their platforms with CML CPU + TGP PCH (Off course it worked
> > > after I gave local workaround of Lee Shawn's patch
> > > https://patchwork.freedesktop.org/patch/401301/?series=83154&rev=5).
> > > Also this patch +
> > > https://patchwork.freedesktop.org/patch/401301/?series=83154&rev=5
> > > (Lee Shawn's patch reviewed by you) + Adding IS_COMETLAKE check to
> > > Lee Shawn's patch needs to be merged by Jan 4th to complete
> > > upstreaming for CML CPU + TGP PCH. DELL is having critical
> > > requirement to finish upstreaming by Jan 4th.
> > >
> > > 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 at intel.com> Cc : Ville Syrjälä
> > > > > > <ville.syrjala at linux.intel.com>
> > > > > > Signed-off-by: Tejas Upadhyay
> > > > > > <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_ddi.c     | 7 +++++--
> > > > > >  drivers/gpu/drm/i915/display/intel_display.c | 5 +++++
> > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 3 ++-
> > > > > >  3 files changed, 12 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > index 17eaa56c5a99..181d60a5e145 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > @@ -5301,7 +5301,9 @@ static enum hpd_pin dg1_hpd_pin(struct
> > > > > > drm_i915_private *dev_priv,  static enum hpd_pin
> > > > > > tgl_hpd_pin(struct
> > > > > drm_i915_private *dev_priv,
> > > > > >  enum port port)
> > > > > >  {
> > > > > > -if (port >= PORT_TC1)
> > > > > > +if (IS_COMETLAKE(dev_priv) && port >= PORT_C) return
> > > HPD_PORT_TC1
> > > > > > ++ port + 1 - PORT_TC1;
> > >
> > > 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 at lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > >
> > > > > --
> > > > > Matt Roper
> > > > > Graphics Software Engineer
> > > > > VTT-OSGC Platform Enablement
> > > > > Intel Corporation
> > > > > (916) 356-2795
> > >
> > > --
> > > Matt Roper
> > > Graphics Software Engineer
> > > VTT-OSGC Platform Enablement
> > > Intel Corporation
> > > (916) 356-2795
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat