| Message ID | 20220125193530.3272386-2-bob.beckett@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | discrete card 64K page support | expand |
On 1/25/22 20:35, Robert Beckett wrote: > From: Ramalingam C <ramalingam.c@intel.com> > > 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@intel.com> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > Signed-off-by: Robert Beckett <bob.beckett@collabora.com> > --- > 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? > + * 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. > + * 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); \
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@intel.com> >> >> 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@intel.com> >> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >> --- >> 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); \
On 1/26/22 18:11, Robert Beckett wrote: > > > 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@intel.com> >>> >>> 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@intel.com> >>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >>> --- >>> 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. Yes, NEEDS_COMPACT_PT indicates that compact is *required* but does "HAS_64K_PAGES" still mean compact is supported? That information is lost. /Thomas
On 27/01/2022 09:37, Thomas Hellström (Intel) wrote: > > On 1/26/22 18:11, Robert Beckett wrote: >> >> >> 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@intel.com> >>>> >>>> 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@intel.com> >>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >>>> --- >>>> 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. > > Yes, NEEDS_COMPACT_PT indicates that compact is *required* but does > "HAS_64K_PAGES" still mean compact is supported? That information is lost. Not any more. I discussed the ambiguity of the original wording with mauld on irc. We came to the conclusion that HAS_64K_PAGES should mean just that, that the hw has support for 64K pages, and says nothing about compact-pt at all. NEEDS_COMPACT_PT means that the hw has compact-pt support and the driver is required to use it as it is a hw limitation. There will be other devices that can support compact-pt but do not mandate its use. In this case, the current code would not use them, but there is potential for some future opportunistic use of that in the driver if desired (currently expected to include accelerated move/clear). If any opportunistic use is added to the driver, a new flag can be added along with the code that uses it to indicate compact-pt availability that is not mandatory (HAS_COMPACT_PT most likely), but as there is no code requiring it currently it should not be added yet, and the comments left as this patch does. > > /Thomas > >
On 1/31/22 15:19, Robert Beckett wrote: > > > On 27/01/2022 09:37, Thomas Hellström (Intel) wrote: >> >> On 1/26/22 18:11, Robert Beckett wrote: >>> >>> >>> 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@intel.com> >>>>> >>>>> 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@intel.com> >>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >>>>> --- >>>>> 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. >> >> Yes, NEEDS_COMPACT_PT indicates that compact is *required* but does >> "HAS_64K_PAGES" still mean compact is supported? That information is >> lost. > Not any more. > I discussed the ambiguity of the original wording with mauld on irc. > We came to the conclusion that HAS_64K_PAGES should mean just that, > that the hw has support for 64K pages, and says nothing about > compact-pt at all. > > NEEDS_COMPACT_PT means that the hw has compact-pt support and the > driver is required to use it as it is a hw limitation. > > There will be other devices that can support compact-pt but do not > mandate its use. In this case, the current code would not use them, > but there is potential for some future opportunistic use of that in > the driver if desired (currently expected to include accelerated > move/clear). If any opportunistic use is added to the driver, a new > flag can be added along with the code that uses it to indicate > compact-pt availability that is not mandatory (HAS_COMPACT_PT most > likely), but as there is no code requiring it currently it should not > be added yet, and the comments left as this patch does. > Ok, Thanks for the clarification. Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> >> /Thomas >> >>
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. + * 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 + * 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); \