diff mbox series

[3/7] drm/i915/dg2: DG2 MBD config

Message ID 20220518130716.10936-4-anshuman.gupta@intel.com (mailing list archive)
State New, archived
Headers show
Series DG2 VRAM_SR Support | expand

Commit Message

Anshuman Gupta May 18, 2022, 1:07 p.m. UTC
Add DG2 Motherboard Down Config check support.

BSpec: 44477
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_opregion.c | 2 ++
 drivers/gpu/drm/i915/i915_drv.h               | 9 +++++++++
 2 files changed, 11 insertions(+)

Comments

Jani Nikula May 19, 2022, 9:26 a.m. UTC | #1
On Wed, 18 May 2022, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> Add DG2 Motherboard Down Config check support.
>
> BSpec: 44477
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_opregion.c | 2 ++
>  drivers/gpu/drm/i915/i915_drv.h               | 9 +++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> index 3dcd54517b89..dec7628522c5 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -1271,6 +1271,8 @@ intel_opregion_vram_sr_required(struct drm_i915_private *i915)
>  
>  	if (IS_DG1(i915))
>  		return intel_opregion_dg1_mbd_config(i915);
> +	else if (IS_DG2_MBD(i915))
> +		return true;
>  
>  	return false;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 10f273800645..c5ecc490dced 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1071,6 +1071,15 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>  	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G11)
>  #define IS_DG2_G12(dev_priv) \
>  	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G12)
> +/*
> + * FIXME: Need to define a new SUBPLATFORM INTEL_SUBPLATFORM_DG2_MBD
> + * for PCI id range 5690..5695, but G10, G11 SUBPLATFROM conflicts
> + * with those pci id range.
> + */
> +#define DG2_MBD_CONFIG_MASK	GENMASK(7, 4)
> +#define DG2_MBD_CONFIG_VAL	FIELD_PREP(DG2_MBD_CONFIG_MASK, 9)
> +#define IS_DG2_MBD(dev_priv) (IS_DG2(dev_priv) && \
> +			      (INTEL_DEVID(dev_priv) & DG2_MBD_CONFIG_MASK) == DG2_MBD_CONFIG_VAL)

No. Please don't do *any* magic masking or comparison of PCI ID bits or
bitfields.

BR,
Jani.

>  #define IS_ADLS_RPLS(dev_priv) \
>  	IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_S, INTEL_SUBPLATFORM_RPL)
>  #define IS_ADLP_N(dev_priv) \
Anshuman Gupta May 30, 2022, 4:44 a.m. UTC | #2
> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Thursday, May 19, 2022 2:57 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Nilawar, Badal <badal.nilawar@intel.com>; Ewins, Jon
> <jon.ewins@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Deak, Imre
> <imre.deak@intel.com>; Gupta, Anshuman <anshuman.gupta@intel.com>
> Subject: Re: [PATCH 3/7] drm/i915/dg2: DG2 MBD config
> 
> On Wed, 18 May 2022, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> > Add DG2 Motherboard Down Config check support.
> >
> > BSpec: 44477
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_opregion.c | 2 ++
> >  drivers/gpu/drm/i915/i915_drv.h               | 9 +++++++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c
> > b/drivers/gpu/drm/i915/display/intel_opregion.c
> > index 3dcd54517b89..dec7628522c5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> > +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> > @@ -1271,6 +1271,8 @@ intel_opregion_vram_sr_required(struct
> > drm_i915_private *i915)
> >
> >  	if (IS_DG1(i915))
> >  		return intel_opregion_dg1_mbd_config(i915);
> > +	else if (IS_DG2_MBD(i915))
> > +		return true;
> >
> >  	return false;
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 10f273800645..c5ecc490dced
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1071,6 +1071,15 @@ IS_SUBPLATFORM(const struct drm_i915_private
> *i915,
> >  	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G11)
> #define
> > IS_DG2_G12(dev_priv) \
> >  	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G12)
> > +/*
> > + * FIXME: Need to define a new SUBPLATFORM
> INTEL_SUBPLATFORM_DG2_MBD
> > + * for PCI id range 5690..5695, but G10, G11 SUBPLATFROM conflicts
> > + * with those pci id range.
> > + */
> > +#define DG2_MBD_CONFIG_MASK	GENMASK(7, 4)
> > +#define DG2_MBD_CONFIG_VAL
> 	FIELD_PREP(DG2_MBD_CONFIG_MASK, 9)
> > +#define IS_DG2_MBD(dev_priv) (IS_DG2(dev_priv) && \
> > +			      (INTEL_DEVID(dev_priv) &
> DG2_MBD_CONFIG_MASK) ==
> > +DG2_MBD_CONFIG_VAL)
> 
> No. Please don't do *any* magic masking or comparison of PCI ID bits or
> bitfields.
Hi Matt,
We need to distinguish between DG2 NB MBD and rest such that i915 can figure out opregion vram_sr is requires for the DG2 platform.  Could you please suggestion on that.
Thanks,
Anshuman Gupta.
> 
> BR,
> Jani.
> 
> >  #define IS_ADLS_RPLS(dev_priv) \
> >  	IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_S,
> INTEL_SUBPLATFORM_RPL)
> > #define IS_ADLP_N(dev_priv) \
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
Matt Roper May 31, 2022, 3:39 p.m. UTC | #3
On Sun, May 29, 2022 at 09:44:38PM -0700, Gupta, Anshuman wrote:
> 
> 
> > -----Original Message-----
> > From: Nikula, Jani <jani.nikula@intel.com>
> > Sent: Thursday, May 19, 2022 2:57 PM
> > To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: Nilawar, Badal <badal.nilawar@intel.com>; Ewins, Jon
> > <jon.ewins@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Deak, Imre
> > <imre.deak@intel.com>; Gupta, Anshuman <anshuman.gupta@intel.com>
> > Subject: Re: [PATCH 3/7] drm/i915/dg2: DG2 MBD config
> > 
> > On Wed, 18 May 2022, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> > > Add DG2 Motherboard Down Config check support.
> > >
> > > BSpec: 44477
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_opregion.c | 2 ++
> > >  drivers/gpu/drm/i915/i915_drv.h               | 9 +++++++++
> > >  2 files changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c
> > > b/drivers/gpu/drm/i915/display/intel_opregion.c
> > > index 3dcd54517b89..dec7628522c5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> > > @@ -1271,6 +1271,8 @@ intel_opregion_vram_sr_required(struct
> > > drm_i915_private *i915)
> > >
> > >  	if (IS_DG1(i915))
> > >  		return intel_opregion_dg1_mbd_config(i915);
> > > +	else if (IS_DG2_MBD(i915))
> > > +		return true;
> > >
> > >  	return false;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h index 10f273800645..c5ecc490dced
> > > 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1071,6 +1071,15 @@ IS_SUBPLATFORM(const struct drm_i915_private
> > *i915,
> > >  	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G11)
> > #define
> > > IS_DG2_G12(dev_priv) \
> > >  	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G12)
> > > +/*
> > > + * FIXME: Need to define a new SUBPLATFORM
> > INTEL_SUBPLATFORM_DG2_MBD
> > > + * for PCI id range 5690..5695, but G10, G11 SUBPLATFROM conflicts
> > > + * with those pci id range.
> > > + */
> > > +#define DG2_MBD_CONFIG_MASK	GENMASK(7, 4)
> > > +#define DG2_MBD_CONFIG_VAL
> > 	FIELD_PREP(DG2_MBD_CONFIG_MASK, 9)
> > > +#define IS_DG2_MBD(dev_priv) (IS_DG2(dev_priv) && \
> > > +			      (INTEL_DEVID(dev_priv) &
> > DG2_MBD_CONFIG_MASK) ==
> > > +DG2_MBD_CONFIG_VAL)
> > 
> > No. Please don't do *any* magic masking or comparison of PCI ID bits or
> > bitfields.
> Hi Matt,
> We need to distinguish between DG2 NB MBD and rest such that i915 can
> figure out opregion vram_sr is requires for the DG2 platform.  Could
> you please suggestion on that.

If you truly need to distinguish a specific sub-type of DG2, then
creating a proper subplatform (with its own list of PCI IDs, matched in
intel_device_info_subplatform_init() is the way to go.  As far as I
recall, it should be okay to have multiple subplatform bits set for a
platform, so for example you can have both the G11 and a MBD subplatform
bits set on a device and they can each be tested independently without
interfering with each other.

I'm not too familiar with the feature you're working on here.  Is there
a way we can detect whether it's supported by querying the pcode?  Or
what happens if you send your pcode request on a platform that doesn't
support it?  Do you just get a regular error back so that the driver
would know to give up and move on, or would it actually cause some kind
of behavioral problem?


Matt

> Thanks,
> Anshuman Gupta.
> > 
> > BR,
> > Jani.
> > 
> > >  #define IS_ADLS_RPLS(dev_priv) \
> > >  	IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_S,
> > INTEL_SUBPLATFORM_RPL)
> > > #define IS_ADLP_N(dev_priv) \
> > 
> > --
> > Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
index 3dcd54517b89..dec7628522c5 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.c
+++ b/drivers/gpu/drm/i915/display/intel_opregion.c
@@ -1271,6 +1271,8 @@  intel_opregion_vram_sr_required(struct drm_i915_private *i915)
 
 	if (IS_DG1(i915))
 		return intel_opregion_dg1_mbd_config(i915);
+	else if (IS_DG2_MBD(i915))
+		return true;
 
 	return false;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 10f273800645..c5ecc490dced 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1071,6 +1071,15 @@  IS_SUBPLATFORM(const struct drm_i915_private *i915,
 	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G11)
 #define IS_DG2_G12(dev_priv) \
 	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G12)
+/*
+ * FIXME: Need to define a new SUBPLATFORM INTEL_SUBPLATFORM_DG2_MBD
+ * for PCI id range 5690..5695, but G10, G11 SUBPLATFROM conflicts
+ * with those pci id range.
+ */
+#define DG2_MBD_CONFIG_MASK	GENMASK(7, 4)
+#define DG2_MBD_CONFIG_VAL	FIELD_PREP(DG2_MBD_CONFIG_MASK, 9)
+#define IS_DG2_MBD(dev_priv) (IS_DG2(dev_priv) && \
+			      (INTEL_DEVID(dev_priv) & DG2_MBD_CONFIG_MASK) == DG2_MBD_CONFIG_VAL)
 #define IS_ADLS_RPLS(dev_priv) \
 	IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_S, INTEL_SUBPLATFORM_RPL)
 #define IS_ADLP_N(dev_priv) \