| Message ID | 20230907153757.2249452-6-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable Lunar Lake display | expand |
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 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 >
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. 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? 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
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. 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
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 this contradicts what you said above that "To be 100% accurate we'd want to identify SDE behavior via the PICA's GMD_ID". That is not true because if what you are using to fingerprint SDE's behavior change from platform to platform, then you can't decide anymore on what to use to fingerprint it. At that point we'd better request a SDE id to be added. >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 which only happens to be on the same die where PICA is. *On LNL*. >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. I think that's a different read of what was discussed. My read was more in the lines of "if you really need that, you can use the PICA ID". With no guarantee of this being future-proof. That's why we decided it made no sense to change now. > >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 I agree here: MTL shouldn't be using that just like LNL is not. >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. 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). That's where we disagree... you seem to prefer it more fine grained while I'm perfectly happy with keeping it simpler and coarse until the day we need it. For LNL we have Panel Control, GMBUS DDC, HPD, SDE interrupts, GPSB, TCSS, PHYs and PICA on the SoC die, but those IO interfaces could very well be on a separate die I think we have to agree to disagree here and move on. What about the following? drm/i915/xe2lpd: Add fake PCH Xe2_LPD doesn't have south display engine on a PCH, it's actually on the SoC die (while north display engine is on compute die). As such it makes no sense to go through the PCI devices looking for an ISA bridge. For the places we currently use a PCH check, it's enough for now to just check the north display version. Use that to define a fake PCH to be used across the driver. Lucas De Marchi
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
On Fri, Sep 08, 2023 at 08:39:48AM +0300, Ville Syrjälä wrote: >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. maybe some future platform > >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 it was originally done for the discrete cards, I think DG1, and got extended to the next ones. Differently than BXT/GLK it doesn't work at all to try finding the ISA bridge as that would end up matching the wrong one. Lucas De Marchi >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
On Fri, Sep 08, 2023 at 12:51:09AM -0500, Lucas De Marchi wrote: > On Fri, Sep 08, 2023 at 08:39:48AM +0300, Ville Syrjälä wrote: > >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. > > maybe some future platform > > > > >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 > > it was originally done for the discrete cards, I think DG1, and got > extended to the next ones. Differently than BXT/GLK it doesn't work > at all to try finding the ISA bridge as that would end up matching the > wrong one. BXT/GLK don't look for the ISA bridge either. Well, they do, but they won't find a matching one and thus we're left with PCH_NONE.
On Fri, Sep 08, 2023 at 08:56:53AM +0300, Ville Syrjälä wrote: > On Fri, Sep 08, 2023 at 12:51:09AM -0500, Lucas De Marchi wrote: > > On Fri, Sep 08, 2023 at 08:39:48AM +0300, Ville Syrjälä wrote: > > >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. > > > > maybe some future platform > > > > > > > >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 > > > > it was originally done for the discrete cards, I think DG1, and got > > extended to the next ones. Differently than BXT/GLK it doesn't work > > at all to try finding the ISA bridge as that would end up matching the > > wrong one. > > BXT/GLK don't look for the ISA bridge either. Well, they do, but > they won't find a matching one and thus we're left with PCH_NONE. I guess we can also blame bspec for this mess a bit since for BXT/GLK it actually documents the SDE registers in the north display section, whereas everything else that has SDE registers documents them in the south display section.
On Fri, Sep 08, 2023 at 09:03:40AM +0300, Ville Syrjälä wrote: >On Fri, Sep 08, 2023 at 08:56:53AM +0300, Ville Syrjälä wrote: >> On Fri, Sep 08, 2023 at 12:51:09AM -0500, Lucas De Marchi wrote: >> > On Fri, Sep 08, 2023 at 08:39:48AM +0300, Ville Syrjälä wrote: >> > >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. >> > >> > maybe some future platform >> > >> > > >> > >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 >> > >> > it was originally done for the discrete cards, I think DG1, and got >> > extended to the next ones. Differently than BXT/GLK it doesn't work >> > at all to try finding the ISA bridge as that would end up matching the >> > wrong one. >> >> BXT/GLK don't look for the ISA bridge either. Well, they do, but >> they won't find a matching one and thus we're left with PCH_NONE. > >I guess we can also blame bspec for this mess a bit since for >BXT/GLK it actually documents the SDE registers in the north >display section, whereas everything else that has SDE registers >documents them in the south display section. yes, in the bxt/glk it looks for it, finds nothing, and we handle the PCH_NONE case throughout the code. When this was introduced for DG1 it was basically "oh, we can't follow the same pattern because we will find something and it will be the wrong one". For LNL we are going the same fake pch route introduced by the discrete cards because SDE and PCH have nothing to do with each other and we can't of course leave it as PCH_NONE as that would actually go through the BXT/GLK path, that is not what we want. Lucas De Marchi > >-- >Ville Syrjälä >Intel
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)