Re: [Intel-gfx] [PATCH v5 1/5] drm/i915: add needs_compact_pt flag

[Robert Beckett]

On 26/01/2022 13:49, Thomas Hellström (Intel) wrote:
> On 1/25/22 20:35, Robert Beckett wrote:
> > From: Ramalingam C <ramalingam.c@xxxxxxxxx>
> >
> > Add a new platform flag, needs_compact_pt, to mark the requirement of
> > compact pt layout support for the ppGTT when using 64K GTT pages.
> >
> > With this flag has_64k_pages will only indicate requirement of 64K
> > GTT page sizes or larger for device local memory access.
> >
> > Suggested-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> > Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> > Signed-off-by: Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h          | 10 +++++++---
> >   drivers/gpu/drm/i915/i915_pci.c          |  2 ++
> >   drivers/gpu/drm/i915/intel_device_info.h |  1 +
> >   3 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 44c1f98144b4..1258b7779705 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1512,12 +1512,16 @@ IS_SUBPLATFORM(const struct drm_i915_private 
> > *i915,
> >   /*
> >    * Set this flag, when platform requires 64K GTT page sizes or 
> > larger for
> > - * device local memory access. Also this flag implies that we require or
> > - * at least support the compact PT layout for the ppGTT when using 
> > the 64K
> > - * GTT pages.
>
> Why do we remove these comment lines?
Because HAS_64K_PAGES now means just 64K page, it no longer means also 
requires compact pt.
This is to support other products that will have 64K but not have the 
PDE non-sharing restriction in future.

Those lines moved to the next change NEEDS_COMPACT_PT, which is now 
separate.

> > + * device local memory access.
> >    */
> >   #define HAS_64K_PAGES(dev_priv) (INTEL_INFO(dev_priv)->has_64k_pages)
> > +/* Set this flag when platform doesn't allow both 64k pages and 4k 
> > pages in
>
> First line of multi-line comments should be empty.
thanks. I'm surprised checkpatch didnt spot that.

> > + * the same PT. this flag means we need to support compact PT layout 
> > for the
> > + * ppGTT when using the 64K GTT pages.
> > + */
> > +#define NEEDS_COMPACT_PT(dev_priv) 
> > (INTEL_INFO(dev_priv)->needs_compact_pt)
> > +
> >   #define HAS_IPC(dev_priv)         
> > (INTEL_INFO(dev_priv)->display.has_ipc)
> >   #define HAS_REGION(i915, i) (INTEL_INFO(i915)->memory_regions & (i))
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c 
> > b/drivers/gpu/drm/i915/i915_pci.c
> > index 4081fd50ba9d..799b56569ef5 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -1028,6 +1028,7 @@ static const struct intel_device_info 
> > xehpsdv_info = {
> >       PLATFORM(INTEL_XEHPSDV),
> >       .display = { },
> >       .has_64k_pages = 1,
> > +    .needs_compact_pt = 1,
> >       .platform_engine_mask =
> >           BIT(RCS0) | BIT(BCS0) |
> >           BIT(VECS0) | BIT(VECS1) | BIT(VECS2) | BIT(VECS3) |
> > @@ -1045,6 +1046,7 @@ static const struct intel_device_info dg2_info = {
> >       .media.rel = 55,
> >       PLATFORM(INTEL_DG2),
> >       .has_64k_pages = 1,
> > +    .needs_compact_pt = 1,
> >       .platform_engine_mask =
> >           BIT(RCS0) | BIT(BCS0) |
> >           BIT(VECS0) | BIT(VECS1) |
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h 
> > b/drivers/gpu/drm/i915/intel_device_info.h
> > index 3699b1c539ea..c8aaf646430c 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -130,6 +130,7 @@ enum intel_ppgtt_type {
> >       /* Keep has_* in alphabetical order */ \
> >       func(has_64bit_reloc); \
> >       func(has_64k_pages); \
> > +    func(needs_compact_pt); \
> >       func(gpu_reset_clobbers_display); \
> >       func(has_reset_engine); \
> >       func(has_global_mocs); \