| Message ID | bebbdad2decb22f3db29e6bc66746b4a05e1387b.1715086509.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915: PCI ID macro and subplatform changes | expand |
On Tue, May 07, 2024 at 03:56:48PM +0300, Jani Nikula wrote: > It's confusing for INTEL_CFL_IDS() to include all CML PCI IDs. Even if > we treat them the same in a lot of places, CML is a platform of its own, > and the lists of PCI IDs should not conflate them. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > arch/x86/kernel/early-quirks.c | 1 + > drivers/gpu/drm/i915/display/intel_display_device.c | 1 + > include/drm/i915_pciids.h | 12 +++++++----- > 3 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c > index 59f4aefc6bc1..2e2d15be4025 100644 > --- a/arch/x86/kernel/early-quirks.c > +++ b/arch/x86/kernel/early-quirks.c > @@ -547,6 +547,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = { > INTEL_BXT_IDS(&gen9_early_ops), > INTEL_KBL_IDS(&gen9_early_ops), > INTEL_CFL_IDS(&gen9_early_ops), > + INTEL_CML_IDS(&gen9_early_ops), > INTEL_GLK_IDS(&gen9_early_ops), > INTEL_CNL_IDS(&gen9_early_ops), > INTEL_ICL_11_IDS(&gen11_early_ops), > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c > index 56a2e17d7d9e..3aa7d1cdd228 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_device.c > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c > @@ -832,6 +832,7 @@ static const struct { > INTEL_GLK_IDS(&glk_display), > INTEL_KBL_IDS(&skl_display), > INTEL_CFL_IDS(&skl_display), > + INTEL_CML_IDS(&skl_display), > INTEL_ICL_11_IDS(&icl_display), > INTEL_EHL_IDS(&jsl_ehl_display), > INTEL_JSL_IDS(&jsl_ehl_display), > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h > index 85ce33ad6e26..5f52c504ffde 100644 > --- a/include/drm/i915_pciids.h > +++ b/include/drm/i915_pciids.h > @@ -472,6 +472,12 @@ > INTEL_VGA_DEVICE(0x9BCA, info), \ > INTEL_VGA_DEVICE(0x9BCC, info) > > +#define INTEL_CML_IDS(info) \ > + INTEL_CML_GT1_IDS(info), \ > + INTEL_CML_GT2_IDS(info), \ > + INTEL_CML_U_GT1_IDS(info), \ > + INTEL_CML_U_GT2_IDS(info) > + > #define INTEL_KBL_IDS(info) \ > INTEL_KBL_GT1_IDS(info), \ > INTEL_KBL_GT2_IDS(info), \ > @@ -535,11 +541,7 @@ > INTEL_WHL_U_GT1_IDS(info), \ > INTEL_WHL_U_GT2_IDS(info), \ > INTEL_WHL_U_GT3_IDS(info), \ > - INTEL_AML_CFL_GT2_IDS(info), \ > - INTEL_CML_GT1_IDS(info), \ > - INTEL_CML_GT2_IDS(info), \ > - INTEL_CML_U_GT1_IDS(info), \ > - INTEL_CML_U_GT2_IDS(info) > + INTEL_AML_CFL_GT2_IDS(info) Why only CML and not AML and WHL as well? > > /* CNL */ > #define INTEL_CNL_PORT_F_IDS(info) \ > -- > 2.39.2 >
On Tue, 07 May 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > On Tue, May 07, 2024 at 03:56:48PM +0300, Jani Nikula wrote: >> @@ -535,11 +541,7 @@ >> INTEL_WHL_U_GT1_IDS(info), \ >> INTEL_WHL_U_GT2_IDS(info), \ >> INTEL_WHL_U_GT3_IDS(info), \ >> - INTEL_AML_CFL_GT2_IDS(info), \ >> - INTEL_CML_GT1_IDS(info), \ >> - INTEL_CML_GT2_IDS(info), \ >> - INTEL_CML_U_GT1_IDS(info), \ >> - INTEL_CML_U_GT2_IDS(info) >> + INTEL_AML_CFL_GT2_IDS(info) > > Why only CML and not AML and WHL as well? Mainly because we don't have a separate enumeration in enum intel_platform for AML or WHL, while for CML we do. We don't even have subplatforms for AML or WHL. So we don't need to distinguish them. That said, we could also have a rule that anything with a name needs to have a PCI ID macro. Could lean either way. BR, Jani. > >> >> /* CNL */ >> #define INTEL_CNL_PORT_F_IDS(info) \ >> -- >> 2.39.2 >>
On Tue, May 07, 2024 at 09:47:16AM -0400, Rodrigo Vivi wrote: > On Tue, May 07, 2024 at 03:56:48PM +0300, Jani Nikula wrote: > > It's confusing for INTEL_CFL_IDS() to include all CML PCI IDs. Even if > > we treat them the same in a lot of places, CML is a platform of its own, > > and the lists of PCI IDs should not conflate them. > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: linux-pci@vger.kernel.org > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > arch/x86/kernel/early-quirks.c | 1 + > > drivers/gpu/drm/i915/display/intel_display_device.c | 1 + > > include/drm/i915_pciids.h | 12 +++++++----- > > 3 files changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c > > index 59f4aefc6bc1..2e2d15be4025 100644 > > --- a/arch/x86/kernel/early-quirks.c > > +++ b/arch/x86/kernel/early-quirks.c > > @@ -547,6 +547,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = { > > INTEL_BXT_IDS(&gen9_early_ops), > > INTEL_KBL_IDS(&gen9_early_ops), > > INTEL_CFL_IDS(&gen9_early_ops), > > + INTEL_CML_IDS(&gen9_early_ops), > > INTEL_GLK_IDS(&gen9_early_ops), > > INTEL_CNL_IDS(&gen9_early_ops), > > INTEL_ICL_11_IDS(&gen11_early_ops), > > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c > > index 56a2e17d7d9e..3aa7d1cdd228 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_device.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c > > @@ -832,6 +832,7 @@ static const struct { > > INTEL_GLK_IDS(&glk_display), > > INTEL_KBL_IDS(&skl_display), > > INTEL_CFL_IDS(&skl_display), > > + INTEL_CML_IDS(&skl_display), > > INTEL_ICL_11_IDS(&icl_display), > > INTEL_EHL_IDS(&jsl_ehl_display), > > INTEL_JSL_IDS(&jsl_ehl_display), > > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h > > index 85ce33ad6e26..5f52c504ffde 100644 > > --- a/include/drm/i915_pciids.h > > +++ b/include/drm/i915_pciids.h > > @@ -472,6 +472,12 @@ > > INTEL_VGA_DEVICE(0x9BCA, info), \ > > INTEL_VGA_DEVICE(0x9BCC, info) > > > > +#define INTEL_CML_IDS(info) \ > > + INTEL_CML_GT1_IDS(info), \ > > + INTEL_CML_GT2_IDS(info), \ > > + INTEL_CML_U_GT1_IDS(info), \ > > + INTEL_CML_U_GT2_IDS(info) > > + > > #define INTEL_KBL_IDS(info) \ > > INTEL_KBL_GT1_IDS(info), \ > > INTEL_KBL_GT2_IDS(info), \ > > @@ -535,11 +541,7 @@ > > INTEL_WHL_U_GT1_IDS(info), \ > > INTEL_WHL_U_GT2_IDS(info), \ > > INTEL_WHL_U_GT3_IDS(info), \ > > - INTEL_AML_CFL_GT2_IDS(info), \ > > - INTEL_CML_GT1_IDS(info), \ > > - INTEL_CML_GT2_IDS(info), \ > > - INTEL_CML_U_GT1_IDS(info), \ > > - INTEL_CML_U_GT2_IDS(info) > > + INTEL_AML_CFL_GT2_IDS(info) > > Why only CML and not AML and WHL as well? Why do we even have CML as a separate platform? The only difference I can see is is that we do allow_read_ctx_timestamp() for CML but not for CFL. Does that even make sense?
On Wed, 08 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, May 07, 2024 at 09:47:16AM -0400, Rodrigo Vivi wrote: >> On Tue, May 07, 2024 at 03:56:48PM +0300, Jani Nikula wrote: >> > It's confusing for INTEL_CFL_IDS() to include all CML PCI IDs. Even if >> > we treat them the same in a lot of places, CML is a platform of its own, >> > and the lists of PCI IDs should not conflate them. >> > >> > Cc: Bjorn Helgaas <bhelgaas@google.com> >> > Cc: linux-pci@vger.kernel.org >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> > --- >> > arch/x86/kernel/early-quirks.c | 1 + >> > drivers/gpu/drm/i915/display/intel_display_device.c | 1 + >> > include/drm/i915_pciids.h | 12 +++++++----- >> > 3 files changed, 9 insertions(+), 5 deletions(-) >> > >> > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c >> > index 59f4aefc6bc1..2e2d15be4025 100644 >> > --- a/arch/x86/kernel/early-quirks.c >> > +++ b/arch/x86/kernel/early-quirks.c >> > @@ -547,6 +547,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = { >> > INTEL_BXT_IDS(&gen9_early_ops), >> > INTEL_KBL_IDS(&gen9_early_ops), >> > INTEL_CFL_IDS(&gen9_early_ops), >> > + INTEL_CML_IDS(&gen9_early_ops), >> > INTEL_GLK_IDS(&gen9_early_ops), >> > INTEL_CNL_IDS(&gen9_early_ops), >> > INTEL_ICL_11_IDS(&gen11_early_ops), >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c >> > index 56a2e17d7d9e..3aa7d1cdd228 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display_device.c >> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c >> > @@ -832,6 +832,7 @@ static const struct { >> > INTEL_GLK_IDS(&glk_display), >> > INTEL_KBL_IDS(&skl_display), >> > INTEL_CFL_IDS(&skl_display), >> > + INTEL_CML_IDS(&skl_display), >> > INTEL_ICL_11_IDS(&icl_display), >> > INTEL_EHL_IDS(&jsl_ehl_display), >> > INTEL_JSL_IDS(&jsl_ehl_display), >> > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h >> > index 85ce33ad6e26..5f52c504ffde 100644 >> > --- a/include/drm/i915_pciids.h >> > +++ b/include/drm/i915_pciids.h >> > @@ -472,6 +472,12 @@ >> > INTEL_VGA_DEVICE(0x9BCA, info), \ >> > INTEL_VGA_DEVICE(0x9BCC, info) >> > >> > +#define INTEL_CML_IDS(info) \ >> > + INTEL_CML_GT1_IDS(info), \ >> > + INTEL_CML_GT2_IDS(info), \ >> > + INTEL_CML_U_GT1_IDS(info), \ >> > + INTEL_CML_U_GT2_IDS(info) >> > + >> > #define INTEL_KBL_IDS(info) \ >> > INTEL_KBL_GT1_IDS(info), \ >> > INTEL_KBL_GT2_IDS(info), \ >> > @@ -535,11 +541,7 @@ >> > INTEL_WHL_U_GT1_IDS(info), \ >> > INTEL_WHL_U_GT2_IDS(info), \ >> > INTEL_WHL_U_GT3_IDS(info), \ >> > - INTEL_AML_CFL_GT2_IDS(info), \ >> > - INTEL_CML_GT1_IDS(info), \ >> > - INTEL_CML_GT2_IDS(info), \ >> > - INTEL_CML_U_GT1_IDS(info), \ >> > - INTEL_CML_U_GT2_IDS(info) >> > + INTEL_AML_CFL_GT2_IDS(info) >> >> Why only CML and not AML and WHL as well? > > Why do we even have CML as a separate platform? The only difference > I can see is is that we do allow_read_ctx_timestamp() for CML but > not for CFL. Does that even make sense? git blame tells me: 5f4ae2704d59 ("drm/i915: Identify Cometlake platform") dbc7e72897a4 ("drm/i915/gt: Make the CTX_TIMESTAMP readable on !rcs") BR, Jani.
On Wed, May 08, 2024 at 02:45:10PM +0300, Jani Nikula wrote: > On Wed, 08 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Tue, May 07, 2024 at 09:47:16AM -0400, Rodrigo Vivi wrote: > >> On Tue, May 07, 2024 at 03:56:48PM +0300, Jani Nikula wrote: > >> > It's confusing for INTEL_CFL_IDS() to include all CML PCI IDs. Even if > >> > we treat them the same in a lot of places, CML is a platform of its own, > >> > and the lists of PCI IDs should not conflate them. > >> > > >> > Cc: Bjorn Helgaas <bhelgaas@google.com> > >> > Cc: linux-pci@vger.kernel.org > >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> > --- > >> > arch/x86/kernel/early-quirks.c | 1 + > >> > drivers/gpu/drm/i915/display/intel_display_device.c | 1 + > >> > include/drm/i915_pciids.h | 12 +++++++----- > >> > 3 files changed, 9 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c > >> > index 59f4aefc6bc1..2e2d15be4025 100644 > >> > --- a/arch/x86/kernel/early-quirks.c > >> > +++ b/arch/x86/kernel/early-quirks.c > >> > @@ -547,6 +547,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = { > >> > INTEL_BXT_IDS(&gen9_early_ops), > >> > INTEL_KBL_IDS(&gen9_early_ops), > >> > INTEL_CFL_IDS(&gen9_early_ops), > >> > + INTEL_CML_IDS(&gen9_early_ops), > >> > INTEL_GLK_IDS(&gen9_early_ops), > >> > INTEL_CNL_IDS(&gen9_early_ops), > >> > INTEL_ICL_11_IDS(&gen11_early_ops), > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c > >> > index 56a2e17d7d9e..3aa7d1cdd228 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_display_device.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c > >> > @@ -832,6 +832,7 @@ static const struct { > >> > INTEL_GLK_IDS(&glk_display), > >> > INTEL_KBL_IDS(&skl_display), > >> > INTEL_CFL_IDS(&skl_display), > >> > + INTEL_CML_IDS(&skl_display), > >> > INTEL_ICL_11_IDS(&icl_display), > >> > INTEL_EHL_IDS(&jsl_ehl_display), > >> > INTEL_JSL_IDS(&jsl_ehl_display), > >> > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h > >> > index 85ce33ad6e26..5f52c504ffde 100644 > >> > --- a/include/drm/i915_pciids.h > >> > +++ b/include/drm/i915_pciids.h > >> > @@ -472,6 +472,12 @@ > >> > INTEL_VGA_DEVICE(0x9BCA, info), \ > >> > INTEL_VGA_DEVICE(0x9BCC, info) > >> > > >> > +#define INTEL_CML_IDS(info) \ > >> > + INTEL_CML_GT1_IDS(info), \ > >> > + INTEL_CML_GT2_IDS(info), \ > >> > + INTEL_CML_U_GT1_IDS(info), \ > >> > + INTEL_CML_U_GT2_IDS(info) > >> > + > >> > #define INTEL_KBL_IDS(info) \ > >> > INTEL_KBL_GT1_IDS(info), \ > >> > INTEL_KBL_GT2_IDS(info), \ > >> > @@ -535,11 +541,7 @@ > >> > INTEL_WHL_U_GT1_IDS(info), \ > >> > INTEL_WHL_U_GT2_IDS(info), \ > >> > INTEL_WHL_U_GT3_IDS(info), \ > >> > - INTEL_AML_CFL_GT2_IDS(info), \ > >> > - INTEL_CML_GT1_IDS(info), \ > >> > - INTEL_CML_GT2_IDS(info), \ > >> > - INTEL_CML_U_GT1_IDS(info), \ > >> > - INTEL_CML_U_GT2_IDS(info) > >> > + INTEL_AML_CFL_GT2_IDS(info) > >> > >> Why only CML and not AML and WHL as well? > > > > Why do we even have CML as a separate platform? The only difference > > I can see is is that we do allow_read_ctx_timestamp() for CML but > > not for CFL. Does that even make sense? > > git blame tells me: > > 5f4ae2704d59 ("drm/i915: Identify Cometlake platform") > dbc7e72897a4 ("drm/i915/gt: Make the CTX_TIMESTAMP readable on !rcs") Right. That explains why we need it on CML+. But is there some reason we can't just do it on CFL as well, even if not strictly necessary? I would assume that setting FORCE_TO_NONPRIV on an already non-privileged register should be totally fine.
On Wed, May 08, 2024 at 11:33:43AM +0300, Jani Nikula wrote: > On Tue, 07 May 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > On Tue, May 07, 2024 at 03:56:48PM +0300, Jani Nikula wrote: > >> @@ -535,11 +541,7 @@ > >> INTEL_WHL_U_GT1_IDS(info), \ > >> INTEL_WHL_U_GT2_IDS(info), \ > >> INTEL_WHL_U_GT3_IDS(info), \ > >> - INTEL_AML_CFL_GT2_IDS(info), \ > >> - INTEL_CML_GT1_IDS(info), \ > >> - INTEL_CML_GT2_IDS(info), \ > >> - INTEL_CML_U_GT1_IDS(info), \ > >> - INTEL_CML_U_GT2_IDS(info) > >> + INTEL_AML_CFL_GT2_IDS(info) > > > > Why only CML and not AML and WHL as well? > > Mainly because we don't have a separate enumeration in enum > intel_platform for AML or WHL, while for CML we do. We don't even have > subplatforms for AML or WHL. So we don't need to distinguish them. > > That said, we could also have a rule that anything with a name needs to > have a PCI ID macro. Could lean either way. Fair enough. Let's go this way. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > BR, > Jani. > > > > >> > >> /* CNL */ > >> #define INTEL_CNL_PORT_F_IDS(info) \ > >> -- > >> 2.39.2 > >> > > -- > Jani Nikula, Intel
On Wed, 08 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, May 08, 2024 at 02:45:10PM +0300, Jani Nikula wrote: >> On Wed, 08 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > On Tue, May 07, 2024 at 09:47:16AM -0400, Rodrigo Vivi wrote: >> >> On Tue, May 07, 2024 at 03:56:48PM +0300, Jani Nikula wrote: >> >> > It's confusing for INTEL_CFL_IDS() to include all CML PCI IDs. Even if >> >> > we treat them the same in a lot of places, CML is a platform of its own, >> >> > and the lists of PCI IDs should not conflate them. [snip] >> >> Why only CML and not AML and WHL as well? >> > >> > Why do we even have CML as a separate platform? The only difference >> > I can see is is that we do allow_read_ctx_timestamp() for CML but >> > not for CFL. Does that even make sense? >> >> git blame tells me: >> >> 5f4ae2704d59 ("drm/i915: Identify Cometlake platform") >> dbc7e72897a4 ("drm/i915/gt: Make the CTX_TIMESTAMP readable on !rcs") > > Right. That explains why we need it on CML+. But is there some reason > we can't just do it on CFL as well, even if not strictly necessary? > I would assume that setting FORCE_TO_NONPRIV on an already > non-privileged register should be totally fine. I have absolutely no idea. I'm somewhat thinking "CML being a separate platform" is a separate problem from "CFL PCI ID macros including CML". I'm starting to think the PCI ID macros should be grouped by "does the platform have a name of its own", not by how those macros are actually used by the driver. Keeping them separate at the PCI ID macro level just reduces the pain in maintaining the PCI IDs, and lets us wiggle stuff around in the driver how we see fit. And that spins back to Rodrigo's question, "Why only CML and not AML and WHL as well?". Yeah, indeed. If we decide to stop treating CML as a separate platform in the *driver*, that's another matter. BR, Jani.
On Fri, May 10, 2024 at 01:24:12PM +0300, Jani Nikula wrote: > On Wed, 08 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Wed, May 08, 2024 at 02:45:10PM +0300, Jani Nikula wrote: > >> On Wed, 08 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > >> > On Tue, May 07, 2024 at 09:47:16AM -0400, Rodrigo Vivi wrote: > >> >> On Tue, May 07, 2024 at 03:56:48PM +0300, Jani Nikula wrote: > >> >> > It's confusing for INTEL_CFL_IDS() to include all CML PCI IDs. Even if > >> >> > we treat them the same in a lot of places, CML is a platform of its own, > >> >> > and the lists of PCI IDs should not conflate them. > > [snip] > > >> >> Why only CML and not AML and WHL as well? > >> > > >> > Why do we even have CML as a separate platform? The only difference > >> > I can see is is that we do allow_read_ctx_timestamp() for CML but > >> > not for CFL. Does that even make sense? > >> > >> git blame tells me: > >> > >> 5f4ae2704d59 ("drm/i915: Identify Cometlake platform") > >> dbc7e72897a4 ("drm/i915/gt: Make the CTX_TIMESTAMP readable on !rcs") > > > > Right. That explains why we need it on CML+. But is there some reason > > we can't just do it on CFL as well, even if not strictly necessary? > > I would assume that setting FORCE_TO_NONPRIV on an already > > non-privileged register should be totally fine. > > I have absolutely no idea. > > I'm somewhat thinking "CML being a separate platform" is a separate > problem from "CFL PCI ID macros including CML". > > I'm starting to think the PCI ID macros should be grouped by "does the > platform have a name of its own", That and/or "does bspec have a separate 'Confgurations <platform>' page?" > not by how those macros are actually > used by the driver. Keeping them separate at the PCI ID macro level just > reduces the pain in maintaining the PCI IDs, and lets us wiggle stuff > around in the driver how we see fit. Aye. > > And that spins back to Rodrigo's question, "Why only CML and not AML and > WHL as well?". Yeah, indeed. > > If we decide to stop treating CML as a separate platform in the > *driver*, that's another matter. Sure. Seeing it just got me wondering...
On Fri, 10 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, May 10, 2024 at 01:24:12PM +0300, Jani Nikula wrote: >> On Wed, 08 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > On Wed, May 08, 2024 at 02:45:10PM +0300, Jani Nikula wrote: >> >> On Wed, 08 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> >> > On Tue, May 07, 2024 at 09:47:16AM -0400, Rodrigo Vivi wrote: >> >> >> On Tue, May 07, 2024 at 03:56:48PM +0300, Jani Nikula wrote: >> >> >> > It's confusing for INTEL_CFL_IDS() to include all CML PCI IDs. Even if >> >> >> > we treat them the same in a lot of places, CML is a platform of its own, >> >> >> > and the lists of PCI IDs should not conflate them. >> >> [snip] >> >> >> >> Why only CML and not AML and WHL as well? >> >> > >> >> > Why do we even have CML as a separate platform? The only difference >> >> > I can see is is that we do allow_read_ctx_timestamp() for CML but >> >> > not for CFL. Does that even make sense? >> >> >> >> git blame tells me: >> >> >> >> 5f4ae2704d59 ("drm/i915: Identify Cometlake platform") >> >> dbc7e72897a4 ("drm/i915/gt: Make the CTX_TIMESTAMP readable on !rcs") >> > >> > Right. That explains why we need it on CML+. But is there some reason >> > we can't just do it on CFL as well, even if not strictly necessary? >> > I would assume that setting FORCE_TO_NONPRIV on an already >> > non-privileged register should be totally fine. >> >> I have absolutely no idea. >> >> I'm somewhat thinking "CML being a separate platform" is a separate >> problem from "CFL PCI ID macros including CML". >> >> I'm starting to think the PCI ID macros should be grouped by "does the >> platform have a name of its own", > > That and/or "does bspec have a separate 'Confgurations <platform>' page?" > >> not by how those macros are actually >> used by the driver. Keeping them separate at the PCI ID macro level just >> reduces the pain in maintaining the PCI IDs, and lets us wiggle stuff >> around in the driver how we see fit. > > Aye. > >> >> And that spins back to Rodrigo's question, "Why only CML and not AML and >> WHL as well?". Yeah, indeed. >> >> If we decide to stop treating CML as a separate platform in the >> *driver*, that's another matter. > > Sure. Seeing it just got me wondering... I sent a new series with just the PCI ID macro cleanups [1]. I meant to Cc: you and Rodrigo, but forgot. :( BR, Jani. [1] https://patchwork.freedesktop.org/series/133444/
diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c index 59f4aefc6bc1..2e2d15be4025 100644 --- a/arch/x86/kernel/early-quirks.c +++ b/arch/x86/kernel/early-quirks.c @@ -547,6 +547,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = { INTEL_BXT_IDS(&gen9_early_ops), INTEL_KBL_IDS(&gen9_early_ops), INTEL_CFL_IDS(&gen9_early_ops), + INTEL_CML_IDS(&gen9_early_ops), INTEL_GLK_IDS(&gen9_early_ops), INTEL_CNL_IDS(&gen9_early_ops), INTEL_ICL_11_IDS(&gen11_early_ops), diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c index 56a2e17d7d9e..3aa7d1cdd228 100644 --- a/drivers/gpu/drm/i915/display/intel_display_device.c +++ b/drivers/gpu/drm/i915/display/intel_display_device.c @@ -832,6 +832,7 @@ static const struct { INTEL_GLK_IDS(&glk_display), INTEL_KBL_IDS(&skl_display), INTEL_CFL_IDS(&skl_display), + INTEL_CML_IDS(&skl_display), INTEL_ICL_11_IDS(&icl_display), INTEL_EHL_IDS(&jsl_ehl_display), INTEL_JSL_IDS(&jsl_ehl_display), diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h index 85ce33ad6e26..5f52c504ffde 100644 --- a/include/drm/i915_pciids.h +++ b/include/drm/i915_pciids.h @@ -472,6 +472,12 @@ INTEL_VGA_DEVICE(0x9BCA, info), \ INTEL_VGA_DEVICE(0x9BCC, info) +#define INTEL_CML_IDS(info) \ + INTEL_CML_GT1_IDS(info), \ + INTEL_CML_GT2_IDS(info), \ + INTEL_CML_U_GT1_IDS(info), \ + INTEL_CML_U_GT2_IDS(info) + #define INTEL_KBL_IDS(info) \ INTEL_KBL_GT1_IDS(info), \ INTEL_KBL_GT2_IDS(info), \ @@ -535,11 +541,7 @@ INTEL_WHL_U_GT1_IDS(info), \ INTEL_WHL_U_GT2_IDS(info), \ INTEL_WHL_U_GT3_IDS(info), \ - INTEL_AML_CFL_GT2_IDS(info), \ - INTEL_CML_GT1_IDS(info), \ - INTEL_CML_GT2_IDS(info), \ - INTEL_CML_U_GT1_IDS(info), \ - INTEL_CML_U_GT2_IDS(info) + INTEL_AML_CFL_GT2_IDS(info) /* CNL */ #define INTEL_CNL_PORT_F_IDS(info) \
It's confusing for INTEL_CFL_IDS() to include all CML PCI IDs. Even if we treat them the same in a lot of places, CML is a platform of its own, and the lists of PCI IDs should not conflate them. Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- arch/x86/kernel/early-quirks.c | 1 + drivers/gpu/drm/i915/display/intel_display_device.c | 1 + include/drm/i915_pciids.h | 12 +++++++----- 3 files changed, 9 insertions(+), 5 deletions(-)