Re: [Intel-gfx] [Intel-xe] [PATCH v2 02/27] drm/i915/lnl: Add display definitions

2023-09-08 Thread Matt Roper
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 
> > > 
> > > 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 
> > > 
> > > Signed-off-by: Lucas De Marchi 
> > > ---
> > >  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

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation


Re: [Intel-gfx] [Intel-xe] [PATCH v2 02/27] drm/i915/lnl: Add display definitions

2023-09-08 Thread Lucas De Marchi

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 

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 
Signed-off-by: Lucas De Marchi 
---
 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