diff mbox series

[1/5] drm/i915: don't include CML PCI IDs in CFL

Message ID bebbdad2decb22f3db29e6bc66746b4a05e1387b.1715086509.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: PCI ID macro and subplatform changes | expand

Commit Message

Jani Nikula May 7, 2024, 12:56 p.m. UTC
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(-)

Comments

Rodrigo Vivi May 7, 2024, 1:47 p.m. UTC | #1
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
>
Jani Nikula May 8, 2024, 8:33 a.m. UTC | #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
>>
Ville Syrjala May 8, 2024, 10:57 a.m. UTC | #3
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?
Jani Nikula May 8, 2024, 11:45 a.m. UTC | #4
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.
Ville Syrjala May 8, 2024, 12:01 p.m. UTC | #5
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.
Rodrigo Vivi May 8, 2024, 12:38 p.m. UTC | #6
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
Jani Nikula May 10, 2024, 10:24 a.m. UTC | #7
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.
Ville Syrjala May 10, 2024, 10:34 a.m. UTC | #8
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...
Jani Nikula May 10, 2024, 11:24 a.m. UTC | #9
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 mbox series

Patch

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) \