Re: [Intel-gfx] [PATCH v2 3/9] drm/i915/dg2: Add DG2_NB_MBD subplatform

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@intel.com>
> Sent: Friday, June 17, 2022 5:43 AM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Nilawar,
> Badal <badal.nilawar@intel.com>; Ewins, Jon <jon.ewins@intel.com>; Vivi,
> Rodrigo <rodrigo.vivi@intel.com>; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>;
> Tangudu, Tilak <tilak.tangudu@intel.com>
> Subject: Re: [PATCH v2 3/9] drm/i915/dg2: Add DG2_NB_MBD subplatform
> 
> On Thu, Jun 16, 2022 at 05:31:00PM +0530, Anshuman Gupta wrote:
> > DG2 NB SKU need to distinguish between MBD and AIC to probe the VRAM
> > Self Refresh feature support. Adding those sub platform accordingly.
> >
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h          |  3 +++
> >  drivers/gpu/drm/i915/intel_device_info.c | 21 +++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_device_info.h | 11 +++++++----
> >  include/drm/i915_pciids.h                | 23 ++++++++++++++++-------
> >  4 files changed, 47 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index a5bc6a774c5a..f1f8699eedfd
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1007,10 +1007,13 @@ IS_SUBPLATFORM(const struct drm_i915_private
> > *i915,  #define IS_PONTEVECCHIO(dev_priv) IS_PLATFORM(dev_priv,
> > INTEL_PONTEVECCHIO)
> >
> >  #define IS_DG2_G10(dev_priv) \
> > +	IS_SUBPLATFORM(dev_priv, INTEL_DG2,
> INTEL_SUBPLATFORM_G10_NB_MBD) ||
> > +\
> >  	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G10)
> #define
> > IS_DG2_G11(dev_priv) \
> > +	IS_SUBPLATFORM(dev_priv, INTEL_DG2,
> INTEL_SUBPLATFORM_G11_NB_MBD) ||
> > +\
> >  	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G11)
> #define
> > IS_DG2_G12(dev_priv) \
> > +	IS_SUBPLATFORM(dev_priv, INTEL_DG2,
> INTEL_SUBPLATFORM_G12_NB_MBD) ||
> > +\
> >  	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G12)
> #define
> > IS_ADLS_RPLS(dev_priv) \
> >  	IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_S,
> INTEL_SUBPLATFORM_RPL)
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> > b/drivers/gpu/drm/i915/intel_device_info.c
> > index f0bf23726ed8..93da555adc4e 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -187,6 +187,18 @@ static const u16 subplatform_rpl_ids[] = {
> >  	INTEL_RPLP_IDS(0),
> >  };
> >
> > +static const u16 subplatform_g10_mb_mbd_ids[] = {
> > +	INTEL_DG2_G10_NB_MBD_IDS(0),
> > +};
> > +
> > +static const u16 subplatform_g11_mb_mbd_ids[] = {
> > +	INTEL_DG2_G11_NB_MBD_IDS(0),
> > +};
> > +
> > +static const u16 subplatform_g12_mb_mbd_ids[] = {
> > +	INTEL_DG2_G12_NB_MBD_IDS(0),
> > +};
> 
> We only need a single MBD subplatform, not three new subplatforms.
> Unless I'm forgetting something, a single device ID can be assigned two two
> independent subplatforms at the same time.  So the decision about whether to
> set the G10, G11, or G12 bit is one decision.  The decision about whether to set
> the MBD bit is a completely separate decision that doesn't care about the
> G10/G11/G12 stuff.
> 
> > +
> >  static const u16 subplatform_g10_ids[] = {
> >  	INTEL_DG2_G10_IDS(0),
> >  	INTEL_ATS_M150_IDS(0),
> > @@ -246,6 +258,15 @@ void intel_device_info_subplatform_init(struct
> drm_i915_private *i915)
> >  	} else if (find_devid(devid, subplatform_rpl_ids,
> >  			      ARRAY_SIZE(subplatform_rpl_ids))) {
> >  		mask = BIT(INTEL_SUBPLATFORM_RPL);
> > +	} else if (find_devid(devid, subplatform_g10_mb_mbd_ids,
> > +			      ARRAY_SIZE(subplatform_g10_mb_mbd_ids))) {
> > +		mask = BIT(INTEL_SUBPLATFORM_G10_NB_MBD);
> > +	} else if (find_devid(devid, subplatform_g11_mb_mbd_ids,
> > +			      ARRAY_SIZE(subplatform_g11_mb_mbd_ids))) {
> > +		mask = BIT(INTEL_SUBPLATFORM_G11_NB_MBD);
> > +	} else if (find_devid(devid, subplatform_g12_mb_mbd_ids,
> > +			      ARRAY_SIZE(subplatform_g12_mb_mbd_ids))) {
> > +		mask = BIT(INTEL_SUBPLATFORM_G12_NB_MBD);
> 
> Assuming you consolidate MBD back down to just a single extra subplatform,
> the lookup and bit setting should happen in a separate 'if'
> statement (not an 'else' block).
> 
>         if (find_devid(devid, subplatform_mbd_ids,
>                        ARRAY_SIZE(subplatform_mbd_ids)))
>                 mask |= BIT(INTEL_SUBPLATFORM_MBD);
Thanks Matt , Jani and Tvrtko for review comment,
I will create only INTEL_SUBPLATFORM_MBD and address it.
Regards,
Anshuman Gupta.
> 
> 
> Matt
> 
> >  	} else if (find_devid(devid, subplatform_g10_ids,
> >  			      ARRAY_SIZE(subplatform_g10_ids))) {
> >  		mask = BIT(INTEL_SUBPLATFORM_G10);
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> > b/drivers/gpu/drm/i915/intel_device_info.h
> > index 08341174ee0a..c929e2d7e59c 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -97,7 +97,7 @@ enum intel_platform {
> >   * it is fine for the same bit to be used on multiple parent platforms.
> >   */
> >
> > -#define INTEL_SUBPLATFORM_BITS (3)
> > +#define INTEL_SUBPLATFORM_BITS (6)
> >  #define INTEL_SUBPLATFORM_MASK (BIT(INTEL_SUBPLATFORM_BITS) - 1)
> >
> >  /* HSW/BDW/SKL/KBL/CFL */
> > @@ -111,9 +111,12 @@ enum intel_platform {
> >  #define INTEL_SUBPLATFORM_UY	(0)
> >
> >  /* DG2 */
> > -#define INTEL_SUBPLATFORM_G10	0
> > -#define INTEL_SUBPLATFORM_G11	1
> > -#define INTEL_SUBPLATFORM_G12	2
> > +#define INTEL_SUBPLATFORM_G10_NB_MBD	0
> > +#define INTEL_SUBPLATFORM_G11_NB_MBD	1
> > +#define INTEL_SUBPLATFORM_G12_NB_MBD	2
> > +#define INTEL_SUBPLATFORM_G10	3
> > +#define INTEL_SUBPLATFORM_G11	4
> > +#define INTEL_SUBPLATFORM_G12	5
> >
> >  /* ADL */
> >  #define INTEL_SUBPLATFORM_RPL	0
> > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> > index 4585fed4e41e..198be417bb2d 100644
> > --- a/include/drm/i915_pciids.h
> > +++ b/include/drm/i915_pciids.h
> > @@ -693,32 +693,41 @@
> >  	INTEL_VGA_DEVICE(0xA7A9, info)
> >
> >  /* DG2 */
> > -#define INTEL_DG2_G10_IDS(info) \
> > +#define INTEL_DG2_G10_NB_MBD_IDS(info) \
> >  	INTEL_VGA_DEVICE(0x5690, info), \
> >  	INTEL_VGA_DEVICE(0x5691, info), \
> > -	INTEL_VGA_DEVICE(0x5692, info), \
> > +	INTEL_VGA_DEVICE(0x5692, info)
> > +
> > +#define INTEL_DG2_G11_NB_MBD_IDS(info) \
> > +	INTEL_VGA_DEVICE(0x5693, info), \
> > +	INTEL_VGA_DEVICE(0x5694, info), \
> > +	INTEL_VGA_DEVICE(0x5695, info)
> > +
> > +#define INTEL_DG2_G12_NB_MBD_IDS(info) \
> > +	INTEL_VGA_DEVICE(0x5696, info), \
> > +	INTEL_VGA_DEVICE(0x5697, info)
> > +
> > +#define INTEL_DG2_G10_IDS(info) \
> >  	INTEL_VGA_DEVICE(0x56A0, info), \
> >  	INTEL_VGA_DEVICE(0x56A1, info), \
> >  	INTEL_VGA_DEVICE(0x56A2, info)
> >
> >  #define INTEL_DG2_G11_IDS(info) \
> > -	INTEL_VGA_DEVICE(0x5693, info), \
> > -	INTEL_VGA_DEVICE(0x5694, info), \
> > -	INTEL_VGA_DEVICE(0x5695, info), \
> >  	INTEL_VGA_DEVICE(0x56A5, info), \
> >  	INTEL_VGA_DEVICE(0x56A6, info), \
> >  	INTEL_VGA_DEVICE(0x56B0, info), \
> >  	INTEL_VGA_DEVICE(0x56B1, info)
> >
> >  #define INTEL_DG2_G12_IDS(info) \
> > -	INTEL_VGA_DEVICE(0x5696, info), \
> > -	INTEL_VGA_DEVICE(0x5697, info), \
> >  	INTEL_VGA_DEVICE(0x56A3, info), \
> >  	INTEL_VGA_DEVICE(0x56A4, info), \
> >  	INTEL_VGA_DEVICE(0x56B2, info), \
> >  	INTEL_VGA_DEVICE(0x56B3, info)
> >
> >  #define INTEL_DG2_IDS(info) \
> > +	INTEL_DG2_G10_NB_MBD_IDS(info), \
> > +	INTEL_DG2_G11_NB_MBD_IDS(info), \
> > +	INTEL_DG2_G12_NB_MBD_IDS(info), \
> >  	INTEL_DG2_G10_IDS(info), \
> >  	INTEL_DG2_G11_IDS(info), \
> >  	INTEL_DG2_G12_IDS(info)
> > --
> > 2.26.2
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation