diff mbox series

[v3,1/2] drm/i915/display: Add MTL subplatforms definition

Message ID 20241213143408.3051070-2-dnyaneshwar.bhadane@intel.com (mailing list archive)
State New
Headers show
Series drm/i915/cx0_phy: Update HDMI TMDS C20 algorithm value | expand

Commit Message

Bhadane, Dnyaneshwar Dec. 13, 2024, 2:34 p.m. UTC 
Separate MTL-U platform PCI ids in one define macro.

Add the MTL U/ARL U as subplatform member in MTL platform description
structure to use display.platform.<platform> from intel_display
structure instead of IS_<PLATFORM>() in display code path.

Signed-off-by: Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com>
---
 .../drm/i915/display/intel_display_device.c   | 21 +++++++++++++++++++
 .../drm/i915/display/intel_display_device.h   |  2 ++
 include/drm/intel/pciids.h                    |  5 ++++-
 3 files changed, 27 insertions(+), 1 deletion(-)

Comments

Jani Nikula Dec. 16, 2024, 11:18 a.m. UTC | #1 
On Fri, 13 Dec 2024, Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com> wrote:
> Separate MTL-U platform PCI ids in one define macro.
>
> Add the MTL U/ARL U as subplatform member in MTL platform description
> structure to use display.platform.<platform> from intel_display
> structure instead of IS_<PLATFORM>() in display code path.
>
> Signed-off-by: Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com>
> ---
>  .../drm/i915/display/intel_display_device.c   | 21 +++++++++++++++++++
>  .../drm/i915/display/intel_display_device.h   |  2 ++
>  include/drm/intel/pciids.h                    |  5 ++++-
>  3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> index 68cb7f9b9ef3..5dc689a8b1ae 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> @@ -1357,6 +1357,16 @@ static const struct intel_display_device_info xe2_hpd_display = {
>  		BIT(PORT_TC1) | BIT(PORT_TC2) | BIT(PORT_TC3) | BIT(PORT_TC4),
>  };
>  
> +static const u16 arl_u_ids[] = {
> +	INTEL_ARL_U_IDS(ID),
> +	0
> +};
> +
> +static const u16 mtl_u_ids[] = {
> +	INTEL_MTL_U_IDS(ID),
> +	0
> +};

We don't have arrowlake platform definition. They're all just
meteorlakes. Do you actually need the mtl-u/arl-u distinction, or do you
just need mtl+arl vs. mtl-u+arl-u distinction?

I.e. could we just have 

static const u16 mtl_u_ids[] = {
	INTEL_MTL_U_IDS(ID),
	INTEL_ARL_U_IDS(ID),
	0
};

And call them all mtl-u?

> +
>  /*
>   * Do not initialize the .info member of the platform desc for GMD ID based
>   * platforms. Their display will be probed automatically based on the IP version
> @@ -1364,6 +1374,17 @@ static const struct intel_display_device_info xe2_hpd_display = {
>   */
>  static const struct platform_desc mtl_desc = {
>  	PLATFORM(meteorlake),
> +	.subplatforms = (const struct subplatform_desc[]) {
> +		{
> +			SUBPLATFORM(meteorlake, u),
> +			.pciidlist = mtl_u_ids,
> +		},
> +		{
> +			SUBPLATFORM(arrowlake, u),
> +			.pciidlist = arl_u_ids,

You're defining subplatfroms for meteorlake. All the platform parameters
for SUBPLATFORM() *must* match the PLATFORM() above.

> +		},
> +		{},
> +	}
>  };
>  
>  static const struct platform_desc lnl_desc = {
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 9a333d9e6601..87a614e2dfab 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -96,6 +96,8 @@ struct pci_dev;
>  	func(dg2_g12) \
>  	/* Display ver 14 (based on GMD ID) */ \
>  	func(meteorlake) \
> +	func(meteorlake_u) \
> +	func(arrowlake_u) \

The naming needs to be <platform>_<subplatform>. We don't have arrowlake
platform, so we can't have arrowlake_u.

Either we can just put all mtl+arl u's together in meteorlake_u, or we
define arl-u as meteorlake_arrowlake_u with meteorlake being the
platform and arrowlake_u the subplatform.

>  	/* Display ver 20 (based on GMD ID) */ \
>  	func(lunarlake) \
>  	/* Display ver 14.1 (based on GMD ID) */ \
> diff --git a/include/drm/intel/pciids.h b/include/drm/intel/pciids.h
> index c6518b0992cf..f29034ccb36c 100644
> --- a/include/drm/intel/pciids.h
> +++ b/include/drm/intel/pciids.h
> @@ -811,9 +811,12 @@
>  	INTEL_ARL_S_IDS(MACRO__, ## __VA_ARGS__)
>  
>  /* MTL */
> +#define INTEL_MTL_U_IDS(MACRO__, ...) \
> +	MACRO__(0x7D45, ## __VA_ARGS__)
> +
>  #define INTEL_MTL_IDS(MACRO__, ...) \
>  	MACRO__(0x7D40, ## __VA_ARGS__), \
> -	MACRO__(0x7D45, ## __VA_ARGS__), \
> +	INTEL_MTL_U_IDS(MACRO__, ## __VA_ARGS__), \
>  	MACRO__(0x7D55, ## __VA_ARGS__), \
>  	MACRO__(0x7D60, ## __VA_ARGS__), \
>  	MACRO__(0x7DD5, ## __VA_ARGS__)
Bhadane, Dnyaneshwar Dec. 17, 2024, 7:16 a.m. UTC | #2 
> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Monday, December 16, 2024 4:48 PM
> To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>
> Subject: Re: [PATCH v3 1/2] drm/i915/display: Add MTL subplatforms definition
> 
> On Fri, 13 Dec 2024, Dnyaneshwar Bhadane
> <dnyaneshwar.bhadane@intel.com> wrote:
> > Separate MTL-U platform PCI ids in one define macro.
> >
> > Add the MTL U/ARL U as subplatform member in MTL platform description
> > structure to use display.platform.<platform> from intel_display
> > structure instead of IS_<PLATFORM>() in display code path.
> >
> > Signed-off-by: Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_device.c   | 21 +++++++++++++++++++
> >  .../drm/i915/display/intel_display_device.h   |  2 ++
> >  include/drm/intel/pciids.h                    |  5 ++++-
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c
> > b/drivers/gpu/drm/i915/display/intel_display_device.c
> > index 68cb7f9b9ef3..5dc689a8b1ae 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> > @@ -1357,6 +1357,16 @@ static const struct intel_display_device_info
> xe2_hpd_display = {
> >  		BIT(PORT_TC1) | BIT(PORT_TC2) | BIT(PORT_TC3) |
> BIT(PORT_TC4),  };
> >
> > +static const u16 arl_u_ids[] = {
> > +	INTEL_ARL_U_IDS(ID),
> > +	0
> > +};
> > +
> > +static const u16 mtl_u_ids[] = {
> > +	INTEL_MTL_U_IDS(ID),
> > +	0
> > +};
> 
> We don't have arrowlake platform definition. They're all just meteorlakes. Do
> you actually need the mtl-u/arl-u distinction, or do you just need mtl+arl vs.
> mtl-u+arl-u distinction?
Hi Jani, 
#1 No, don't need arl-u/mtl-u distinction then can be one-unit mtl-u+arl-u vs mtl+arl. 
I was trying to make readability on conditions to match with specs description. 
> 
> I.e. could we just have
> 
> static const u16 mtl_u_ids[] = {
> 	INTEL_MTL_U_IDS(ID),
> 	INTEL_ARL_U_IDS(ID),
> 	0
> };
> 
> And call them all mtl-u?
#2
Thank you, this suggestion looking good. 
For now, we don't need the arrowlake-u as separate and can be club with meteorlake-u.
I will follow this approach in next revision. 

but in future if we separate out arrowlake-u, then we should be careful to replace (arrowlake-u || meteorlake-u)
wherever needed.
> 
> > +
> >  /*
> >   * Do not initialize the .info member of the platform desc for GMD ID based
> >   * platforms. Their display will be probed automatically based on the
> > IP version @@ -1364,6 +1374,17 @@ static const struct
> intel_display_device_info xe2_hpd_display = {
> >   */
> >  static const struct platform_desc mtl_desc = {
> >  	PLATFORM(meteorlake),
> > +	.subplatforms = (const struct subplatform_desc[]) {
> > +		{
> > +			SUBPLATFORM(meteorlake, u),
> > +			.pciidlist = mtl_u_ids,
> > +		},
> > +		{
> > +			SUBPLATFORM(arrowlake, u),
> > +			.pciidlist = arl_u_ids,
> 
> You're defining subplatfroms for meteorlake. All the platform parameters for
> SUBPLATFORM() *must* match the PLATFORM() above.

> 
> > +		},
> > +		{},
> > +	}
> >  };
> >
> >  static const struct platform_desc lnl_desc = { diff --git
> > a/drivers/gpu/drm/i915/display/intel_display_device.h
> > b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index 9a333d9e6601..87a614e2dfab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -96,6 +96,8 @@ struct pci_dev;
> >  	func(dg2_g12) \
> >  	/* Display ver 14 (based on GMD ID) */ \
> >  	func(meteorlake) \
> > +	func(meteorlake_u) \
> > +	func(arrowlake_u) \
> 
> The naming needs to be <platform>_<subplatform>. We don't have arrowlake
> platform, so we can't have arrowlake_u.
> 
> Either we can just put all mtl+arl u's together in meteorlake_u, or we define
> arl-u as meteorlake_arrowlake_u with meteorlake being the platform and
> arrowlake_u the subplatform.
This is good note to take for future, 
If they are very specific need of identifying arrowlake-u we should use this approach. 
For now, removing arrowlake_u and combining with meteorlake u subplatform as per comment #2.

Thank you, Jani

BR, 
Dnyaneshwar, 

>Jani
> 
> >  	/* Display ver 20 (based on GMD ID) */ \
> >  	func(lunarlake) \
> >  	/* Display ver 14.1 (based on GMD ID) */ \ diff --git
> > a/include/drm/intel/pciids.h b/include/drm/intel/pciids.h index
> > c6518b0992cf..f29034ccb36c 100644
> > --- a/include/drm/intel/pciids.h
> > +++ b/include/drm/intel/pciids.h
> > @@ -811,9 +811,12 @@
> >  	INTEL_ARL_S_IDS(MACRO__, ## __VA_ARGS__)
> >
> >  /* MTL */
> > +#define INTEL_MTL_U_IDS(MACRO__, ...) \
> > +	MACRO__(0x7D45, ## __VA_ARGS__)
> > +
> >  #define INTEL_MTL_IDS(MACRO__, ...) \
> >  	MACRO__(0x7D40, ## __VA_ARGS__), \
> > -	MACRO__(0x7D45, ## __VA_ARGS__), \
> > +	INTEL_MTL_U_IDS(MACRO__, ## __VA_ARGS__), \
> >  	MACRO__(0x7D55, ## __VA_ARGS__), \
> >  	MACRO__(0x7D60, ## __VA_ARGS__), \
> >  	MACRO__(0x7DD5, ## __VA_ARGS__)
> 
> --
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
index 68cb7f9b9ef3..5dc689a8b1ae 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.c
+++ b/drivers/gpu/drm/i915/display/intel_display_device.c
@@ -1357,6 +1357,16 @@  static const struct intel_display_device_info xe2_hpd_display = {
 		BIT(PORT_TC1) | BIT(PORT_TC2) | BIT(PORT_TC3) | BIT(PORT_TC4),
 };
 
+static const u16 arl_u_ids[] = {
+	INTEL_ARL_U_IDS(ID),
+	0
+};
+
+static const u16 mtl_u_ids[] = {
+	INTEL_MTL_U_IDS(ID),
+	0
+};
+
 /*
  * Do not initialize the .info member of the platform desc for GMD ID based
  * platforms. Their display will be probed automatically based on the IP version
@@ -1364,6 +1374,17 @@  static const struct intel_display_device_info xe2_hpd_display = {
  */
 static const struct platform_desc mtl_desc = {
 	PLATFORM(meteorlake),
+	.subplatforms = (const struct subplatform_desc[]) {
+		{
+			SUBPLATFORM(meteorlake, u),
+			.pciidlist = mtl_u_ids,
+		},
+		{
+			SUBPLATFORM(arrowlake, u),
+			.pciidlist = arl_u_ids,
+		},
+		{},
+	}
 };
 
 static const struct platform_desc lnl_desc = {
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index 9a333d9e6601..87a614e2dfab 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -96,6 +96,8 @@  struct pci_dev;
 	func(dg2_g12) \
 	/* Display ver 14 (based on GMD ID) */ \
 	func(meteorlake) \
+	func(meteorlake_u) \
+	func(arrowlake_u) \
 	/* Display ver 20 (based on GMD ID) */ \
 	func(lunarlake) \
 	/* Display ver 14.1 (based on GMD ID) */ \
diff --git a/include/drm/intel/pciids.h b/include/drm/intel/pciids.h
index c6518b0992cf..f29034ccb36c 100644
--- a/include/drm/intel/pciids.h
+++ b/include/drm/intel/pciids.h
@@ -811,9 +811,12 @@ 
 	INTEL_ARL_S_IDS(MACRO__, ## __VA_ARGS__)
 
 /* MTL */
+#define INTEL_MTL_U_IDS(MACRO__, ...) \
+	MACRO__(0x7D45, ## __VA_ARGS__)
+
 #define INTEL_MTL_IDS(MACRO__, ...) \
 	MACRO__(0x7D40, ## __VA_ARGS__), \
-	MACRO__(0x7D45, ## __VA_ARGS__), \
+	INTEL_MTL_U_IDS(MACRO__, ## __VA_ARGS__), \
 	MACRO__(0x7D55, ## __VA_ARGS__), \
 	MACRO__(0x7D60, ## __VA_ARGS__), \
 	MACRO__(0x7DD5, ## __VA_ARGS__)