diff mbox series

[v2,02/27] drm/i915/lnl: Add display definitions

Message ID 20230907153757.2249452-3-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show
Series Enable Lunar Lake display | expand

Commit Message

Lucas De Marchi Sept. 7, 2023, 3:37 p.m. UTC 
From: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>

Add Lunar Lake platform definitions for i915 display. The support for
LNL will be added to the xe driver, with i915 only driving the display
side. Therefore define IS_LUNARLAKE to 0 to disable it when building the
i915 module.

v2: Use a LPDP_FEATURES macro (Matt Roper)

Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_device.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Matt Roper Sept. 7, 2023, 4:10 p.m. UTC | #1 
On Thu, Sep 07, 2023 at 08:37:32AM -0700, Lucas De Marchi wrote:
> From: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> 
> Add Lunar Lake platform definitions for i915 display. The support for
> LNL will be added to the xe driver, with i915 only driving the display
> side. Therefore define IS_LUNARLAKE to 0 to disable it when building the
> i915 module.

This final sentence no longer matches the patch.  But it might be worth
adding a different sentence saying something like "Xe2 display is
derived from the Xe_LPD+ IP; additional feature deltas will be
introduced in subsequent patches."

> 
> v2: Use a LPDP_FEATURES macro (Matt Roper)
> 
> Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_device.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> index 089674e2f1d2..feafb0f94b06 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> @@ -768,6 +768,12 @@ static const struct intel_display_device_info xe_lpdp_display = {
>  	.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A) | BIT(INTEL_FBC_B),
>  };
>  
> +static const struct intel_display_device_info xe2_lpd_display = {
> +	XE_LPDP_FEATURES,
> +
> +	.__runtime_defaults.ip.ver = 20,

There's no need to set a default value here, right?  If we've managed to
match this IP block, we already read out the GMD ID version and matched
it against the table below.  We'll be assigning the real value directly
and shouldn't need this for anything.


Matt

> +};
> +
>  /*
>   * Separate detection for no display cases to keep the display id array simple.
>   *
> @@ -847,6 +853,7 @@ static const struct {
>  	const struct intel_display_device_info *display;
>  } gmdid_display_map[] = {
>  	{ 14,  0, &xe_lpdp_display },
> +	{ 20,  0, &xe2_lpd_display },
>  };
>  
>  static const struct intel_display_device_info *
> -- 
> 2.40.1
>
Lucas De Marchi Sept. 8, 2023, 11:25 p.m. UTC | #2 
On Thu, Sep 07, 2023 at 09:10:44AM -0700, Matt Roper wrote:
>On Thu, Sep 07, 2023 at 08:37:32AM -0700, Lucas De Marchi wrote:
>> From: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
>>
>> Add Lunar Lake platform definitions for i915 display. The support for
>> LNL will be added to the xe driver, with i915 only driving the display
>> side. Therefore define IS_LUNARLAKE to 0 to disable it when building the
>> i915 module.
>
>This final sentence no longer matches the patch.  But it might be worth
>adding a different sentence saying something like "Xe2 display is
>derived from the Xe_LPD+ IP; additional feature deltas will be
>introduced in subsequent patches."
>
>>
>> v2: Use a LPDP_FEATURES macro (Matt Roper)
>>
>> Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display_device.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
>> index 089674e2f1d2..feafb0f94b06 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
>> @@ -768,6 +768,12 @@ static const struct intel_display_device_info xe_lpdp_display = {
>>  	.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A) | BIT(INTEL_FBC_B),
>>  };
>>
>> +static const struct intel_display_device_info xe2_lpd_display = {
>> +	XE_LPDP_FEATURES,
>> +
>> +	.__runtime_defaults.ip.ver = 20,
>
>There's no need to set a default value here, right?  If we've managed to

unless we have a broken check for display version before this is
initialized. I will give it a try and see what happens.

But if we remove it here, we should also remove on previous patch.  As
far as I can see, it's true for Xe-LPD+ too. If we have a wrong check
for version, I'd rather prefer it broken and a loud warning than it
matching version 14 due to using the macro above.

Lucas De Marchi

>match this IP block, we already read out the GMD ID version and matched
>it against the table below.  We'll be assigning the real value directly
>and shouldn't need this for anything.
>
>
>Matt
>
>> +};
>> +
>>  /*
>>   * Separate detection for no display cases to keep the display id array simple.
>>   *
>> @@ -847,6 +853,7 @@ static const struct {
>>  	const struct intel_display_device_info *display;
>>  } gmdid_display_map[] = {
>>  	{ 14,  0, &xe_lpdp_display },
>> +	{ 20,  0, &xe2_lpd_display },
>>  };
>>
>>  static const struct intel_display_device_info *
>> --
>> 2.40.1
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation
Matt Roper Sept. 8, 2023, 11:37 p.m. UTC | #3 
On Fri, Sep 08, 2023 at 06:25:04PM -0500, Lucas De Marchi wrote:
> On Thu, Sep 07, 2023 at 09:10:44AM -0700, Matt Roper wrote:
> > On Thu, Sep 07, 2023 at 08:37:32AM -0700, Lucas De Marchi wrote:
> > > From: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> > > 
> > > Add Lunar Lake platform definitions for i915 display. The support for
> > > LNL will be added to the xe driver, with i915 only driving the display
> > > side. Therefore define IS_LUNARLAKE to 0 to disable it when building the
> > > i915 module.
> > 
> > This final sentence no longer matches the patch.  But it might be worth
> > adding a different sentence saying something like "Xe2 display is
> > derived from the Xe_LPD+ IP; additional feature deltas will be
> > introduced in subsequent patches."
> > 
> > > 
> > > v2: Use a LPDP_FEATURES macro (Matt Roper)
> > > 
> > > Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display_device.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> > > index 089674e2f1d2..feafb0f94b06 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> > > @@ -768,6 +768,12 @@ static const struct intel_display_device_info xe_lpdp_display = {
> > >  	.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A) | BIT(INTEL_FBC_B),
> > >  };
> > > 
> > > +static const struct intel_display_device_info xe2_lpd_display = {
> > > +	XE_LPDP_FEATURES,
> > > +
> > > +	.__runtime_defaults.ip.ver = 20,
> > 
> > There's no need to set a default value here, right?  If we've managed to
> 
> unless we have a broken check for display version before this is
> initialized. I will give it a try and see what happens.
> 
> But if we remove it here, we should also remove on previous patch.  As
> far as I can see, it's true for Xe-LPD+ too. If we have a wrong check
> for version, I'd rather prefer it broken and a loud warning than it
> matching version 14 due to using the macro above.

Agreed, we shouldn't have it on Xe_LPD+ either.  I meant to mention
that, but I guess I forgot.


Matt

> 
> Lucas De Marchi
> 
> > match this IP block, we already read out the GMD ID version and matched
> > it against the table below.  We'll be assigning the real value directly
> > and shouldn't need this for anything.
> > 
> > 
> > Matt
> > 
> > > +};
> > > +
> > >  /*
> > >   * Separate detection for no display cases to keep the display id array simple.
> > >   *
> > > @@ -847,6 +853,7 @@ static const struct {
> > >  	const struct intel_display_device_info *display;
> > >  } gmdid_display_map[] = {
> > >  	{ 14,  0, &xe_lpdp_display },
> > > +	{ 20,  0, &xe2_lpd_display },
> > >  };
> > > 
> > >  static const struct intel_display_device_info *
> > > --
> > > 2.40.1
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation
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 089674e2f1d2..feafb0f94b06 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.c
+++ b/drivers/gpu/drm/i915/display/intel_display_device.c
@@ -768,6 +768,12 @@  static const struct intel_display_device_info xe_lpdp_display = {
 	.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A) | BIT(INTEL_FBC_B),
 };
 
+static const struct intel_display_device_info xe2_lpd_display = {
+	XE_LPDP_FEATURES,
+
+	.__runtime_defaults.ip.ver = 20,
+};
+
 /*
  * Separate detection for no display cases to keep the display id array simple.
  *
@@ -847,6 +853,7 @@  static const struct {
 	const struct intel_display_device_info *display;
 } gmdid_display_map[] = {
 	{ 14,  0, &xe_lpdp_display },
+	{ 20,  0, &xe2_lpd_display },
 };
 
 static const struct intel_display_device_info *