Re: [Intel-xe] [Intel-gfx] [PATCH v2 05/27] drm/i915/xe2lpd: Add fake PCH

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 07, 2023 at 05:57:19PM -0700, Matt Roper wrote:
> On Thu, Sep 07, 2023 at 03:43:59PM -0500, Lucas De Marchi wrote:
> > On Thu, Sep 07, 2023 at 10:04:42AM -0700, Matt Roper wrote:
> > > On Thu, Sep 07, 2023 at 08:37:35AM -0700, Lucas De Marchi wrote:
> > > > From: Gustavo Sousa <gustavo.sousa@intel.com>
> > > > 
> > > > Xe2_LPD has sourth display on the same SOC. As such, define a new fake
> > > 
> > > s/sourth/south/
> > > 
> > > You might also want to drop the word "same" from the description here
> > > since NDE and SDE are technically on different dies in this case (NDE is
> > > on the compute die, whereas SDE is on the SoC die).  To be 100% accurate
> > > we'd want to identify SDE behavior via the PICA's GMD_ID (since PICA
> > > also lives on the SoC die for this platform).  But since we've just been
> > 
> > I'd not re-architect this based on where the PICA lives as it seems very
> > easy to change in future.... tying the SDE behavior to the PICA behavior
> > because they are on the same die, doesn't seem very future proof.
> 
> The point is that tying it to any one thing for every platform is
> incorrect; figuring out a) which die is relevant to SDE behavior and b)
> how to fingerprint the variant and stepping of that die is very platform
> specific.  Art specifically suggested using the PICA ID in cases where
> the PICA lives on the die that we need to fingerprint but the NDE does
> not.  But again, that's not a silver bullet that can be used on every
> single platform.  Nor is using the ISA bus ID like we've done for a long
> time.  Nor is using the display version.  Nor is using just the PCI ID.
> There's no single answer here, which is why we need a major rethink of
> our strategy at some point in the future.  But that overhaul can wait
> for a future series; I just want to make sure that the commit messages
> here aren't causing further confusion.
> 
> > 
> > Here the real reason for the change is that from the SW perspective they
> > are under the same PCI device and there's no reason to look for a
> > different one. Maybe rewording it a "Xe2_LPD has south display on the
> > same PCI device" would be simpler?
> 
> No, that would be even less correct; PCI device isn't really related to
> any of this.  Obviously at the register level, everything our driver
> cares about (NDE, SDE, GT, etc.) is accessed through the same PCI device
> (e.g., 00:02.0 on an igpu).  Under the hood the various pieces of that
> PCI device (NDE, SDE, render GT, media GT, etc.) might be located
> together on a single chip, or may be spread across different dies.  When
> spread across different dies, those dies can be mixed-and-matched in
> various ways (and it seems like hardware design is trending toward more
> flexibility in mix-and-match).  
> 
> The register interface to the SDE (i.e., which registers exist and what
> bitfields they have inside) hasn't had any meaningful changes in a long
> time.  And if it does change in the future, the _interface_ changes are
> probably more tied to the display IP version than to anything else.
> However there's some important SDE handling that the driver needs to do
> that may vary based on the identity of the specific die that's
> responsible for doing SDE I/O on a given platform.  I.e., there may be
> I/O-related defects+workarounds that require special SDE programming
> when a certain die variant and/or stepping is present.  There can also
> be differences in how lanes are physically wired up, resulting in pin
> mapping changes.  In these cases we need to be able to fingerprint the
> identity of the specific die handling the I/O (which might be a compute
> die, an SoC die, and IOE die, a PCH die, etc.) and make our decisions
> accordingly.  If the SDE I/O happens on the same die as the north
> display functionality, then using the display version might be an
> effective way to fingerprint.  If the SDE I/O happens on a different die
> from the NDE, but on the same die the PICA lives on, the display
> architects suggested using the PICA ID in that case.  If neither of
> those cases are true, then we may need to look at PCI IDs or something.
> 
> In the past, the PCH was often where the SDE I/O responsibility was so
> we needed a way to identify exactly which PCH variant was present.  The
> "PCH ID" that we try to match on during driver startup is entirely
> unrelated to the SDE; it's just a random bus that we know was always
> part of every PCH and always present in the same predictable PCI slot,
> so it's handy for identification purposes.  The fact that we're still
> looking at the ISA bus on MTL today is 100% wrong because most (maybe
> all?) MTL platforms don't even have a PCH (so that ISA bus might be on a
> different die that we really don't care about at all).  For MTL I
> believe the NDE and the SDE's I/O are both on the same SoC die, so we
> should really just be making our decisions based on IP version and/or
> graphics device ID.

I think ideally SDE would have its own IP version/etc. we could
use to identify it.

I'm not really sure why we even started down this "fake PCH" route
since we never added that for BXT/GLK either, and they managed just
fine without it despite keeping a bunch of the logic in the SDE
register range (instead of moving it back to the NDE range).

> If I remember correctly, LNL moved the NDE display
> to the compute die, but left the PICA on the SoC die.  So assuming the
> SoC die is still where the I/O happens (I don't have the platform docs
> open at the moment), the PICA ID could potentially be used to
> fingerprint the die for the purposes of die-specific workarounds.  It
> might even vary between different SKUs of LNL, MTL, etc. so we really
> need to dig into the platform specs to figure out the right course of
> action (the graphics bspec doesn't cover that high-level platform
> layout).
> 
> 
> Matt
> 
> > 
> > Lucas De Marchi
> > 
> > > able to get by so far with just matching PICA behavior on the display
> > > version rather than on its own version, we can just use display version
> > > for this as well, at least for now.  We may need to revisit this all
> > > down the road once we have platforms with more possible combinations of
> > > these components.  Of course we really need to rework the SDE handling
> > > in general (and break its assumption that SDE behavior is tied to PCH on
> > > modern platforms), but that's work for a future patch series.
> > > 
> > > I was originally wondering if we could just reuse PCH_MTP here, but it
> > > looks like there's one place where we setup HPD interrupts that needs
> > > different handling.  So this should be good enough for now, and we can
> > > revisit the whole SDE design separately down the road.
> > > 
> > > With the minor commit message fix above,
> > > 
> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > 
> > > 
> > > > PCH entry for it.
> > > > 
> > > > v2: Match on display IP version rather than on platform (Matt Roper)
> > > > 
> > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/soc/intel_pch.c | 5 ++++-
> > > >  drivers/gpu/drm/i915/soc/intel_pch.h | 2 ++
> > > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/soc/intel_pch.c b/drivers/gpu/drm/i915/soc/intel_pch.c
> > > > index dfffdfa50b97..240beafb38ed 100644
> > > > --- a/drivers/gpu/drm/i915/soc/intel_pch.c
> > > > +++ b/drivers/gpu/drm/i915/soc/intel_pch.c
> > > > @@ -222,7 +222,10 @@ void intel_detect_pch(struct drm_i915_private *dev_priv)
> > > >  	 * South display engine on the same PCI device: just assign the fake
> > > >  	 * PCH.
> > > >  	 */
> > > > -	if (IS_DG2(dev_priv)) {
> > > > +	if (DISPLAY_VER(dev_priv) >= 20) {
> > > > +		dev_priv->pch_type = PCH_LNL;
> > > > +		return;
> > > > +	} else if (IS_DG2(dev_priv)) {
> > > >  		dev_priv->pch_type = PCH_DG2;
> > > >  		return;
> > > >  	} else if (IS_DG1(dev_priv)) {
> > > > diff --git a/drivers/gpu/drm/i915/soc/intel_pch.h b/drivers/gpu/drm/i915/soc/intel_pch.h
> > > > index 32aff5a70d04..1b03ea60a7a8 100644
> > > > --- a/drivers/gpu/drm/i915/soc/intel_pch.h
> > > > +++ b/drivers/gpu/drm/i915/soc/intel_pch.h
> > > > @@ -30,6 +30,7 @@ enum intel_pch {
> > > >  	/* Fake PCHs, functionality handled on the same PCI dev */
> > > >  	PCH_DG1 = 1024,
> > > >  	PCH_DG2,
> > > > +	PCH_LNL,
> > > >  };
> > > > 
> > > >  #define INTEL_PCH_DEVICE_ID_MASK		0xff80
> > > > @@ -66,6 +67,7 @@ enum intel_pch {
> > > > 
> > > >  #define INTEL_PCH_TYPE(dev_priv)		((dev_priv)->pch_type)
> > > >  #define INTEL_PCH_ID(dev_priv)			((dev_priv)->pch_id)
> > > > +#define HAS_PCH_LNL(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_LNL)
> > > >  #define HAS_PCH_MTP(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_MTP)
> > > >  #define HAS_PCH_DG2(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_DG2)
> > > >  #define HAS_PCH_ADP(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_ADP)
> > > > --
> > > > 2.40.1
> > > > 
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > Linux GPU Platform Enablement
> > > Intel Corporation
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation

-- 
Ville Syrjälä
Intel