| Message ID | 20231004183625.1307100-1-jonathan.cavitt@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/4] drm/i915: Add GuC TLB Invalidation pci tags | expand |
Hi Jonathan, On Wed, Oct 04, 2023 at 11:36:22AM -0700, Jonathan Cavitt wrote: > Add pci (device info) tags for if GuC TLB Invalidation is enabled. > Since GuC based TLB invalidation is only strictly necessary for MTL > resently, only enable GuC based TLB invalidations for MTL. > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> Jani was mentioning that the pci tags is not a proper title. No need to resend, I think I will merge this series, so that, if you agree, I can change /pci tags/pci flag/ before pushing. In any case. Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Andi > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_pci.c | 1 + > drivers/gpu/drm/i915/intel_device_info.h | 3 ++- > 3 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2b7a6db4d0d44..1e25cc1e3dba1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -807,4 +807,5 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > #define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \ > GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) > > +#define HAS_GUC_TLB_INVALIDATION(i915) (INTEL_INFO(i915)->has_guc_tlb_invalidation) > #endif > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index df7c261410f79..c3a5d5efb45d1 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -837,6 +837,7 @@ static const struct intel_device_info mtl_info = { > .memory_regions = REGION_SMEM | REGION_STOLEN_LMEM, > .platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(CCS0), > .require_force_probe = 1, > + .has_guc_tlb_invalidation = 1, > MTL_CACHELEVEL, > }; > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index 39817490b13fd..ad54db0a22470 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -173,7 +173,8 @@ enum intel_ppgtt_type { > func(has_coherent_ggtt); \ > func(tuning_thread_rr_after_dep); \ > func(unfenced_needs_alignment); \ > - func(hws_needs_physical); > + func(hws_needs_physical); \ > + func(has_guc_tlb_invalidation); > > struct intel_ip_version { > u8 ver; > -- > 2.25.1
Why is there no cover letter for this patch series? It is at v5 but there is no history of what has changed from one version to the next. That makes it much harder to review. John. On 10/4/2023 11:36, Jonathan Cavitt wrote: > Add pci (device info) tags for if GuC TLB Invalidation is enabled. > Since GuC based TLB invalidation is only strictly necessary for MTL > resently, only enable GuC based TLB invalidations for MTL. > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_pci.c | 1 + > drivers/gpu/drm/i915/intel_device_info.h | 3 ++- > 3 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2b7a6db4d0d44..1e25cc1e3dba1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -807,4 +807,5 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > #define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \ > GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) > > +#define HAS_GUC_TLB_INVALIDATION(i915) (INTEL_INFO(i915)->has_guc_tlb_invalidation) > #endif > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index df7c261410f79..c3a5d5efb45d1 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -837,6 +837,7 @@ static const struct intel_device_info mtl_info = { > .memory_regions = REGION_SMEM | REGION_STOLEN_LMEM, > .platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(CCS0), > .require_force_probe = 1, > + .has_guc_tlb_invalidation = 1, > MTL_CACHELEVEL, > }; > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index 39817490b13fd..ad54db0a22470 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -173,7 +173,8 @@ enum intel_ppgtt_type { > func(has_coherent_ggtt); \ > func(tuning_thread_rr_after_dep); \ > func(unfenced_needs_alignment); \ > - func(hws_needs_physical); > + func(hws_needs_physical); \ > + func(has_guc_tlb_invalidation); > > struct intel_ip_version { > u8 ver;
On 10/4/2023 12:03, Andi Shyti wrote: > Hi Jonathan, > > On Wed, Oct 04, 2023 at 11:36:22AM -0700, Jonathan Cavitt wrote: >> Add pci (device info) tags for if GuC TLB Invalidation is enabled. >> Since GuC based TLB invalidation is only strictly necessary for MTL >> resently, only enable GuC based TLB invalidations for MTL. >> >> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> > Jani was mentioning that the pci tags is not a proper title. > > No need to resend, I think I will merge this series, so that, if > you agree, I can change /pci tags/pci flag/ before pushing. Have all the review comments been addressed? Surely it can't be pushed until it has at least an ack from everyone who has expressed concerns about the changes? John. > > In any case. > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > > Andi > >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/i915_pci.c | 1 + >> drivers/gpu/drm/i915/intel_device_info.h | 3 ++- >> 3 files changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 2b7a6db4d0d44..1e25cc1e3dba1 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -807,4 +807,5 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >> #define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \ >> GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) >> >> +#define HAS_GUC_TLB_INVALIDATION(i915) (INTEL_INFO(i915)->has_guc_tlb_invalidation) >> #endif >> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c >> index df7c261410f79..c3a5d5efb45d1 100644 >> --- a/drivers/gpu/drm/i915/i915_pci.c >> +++ b/drivers/gpu/drm/i915/i915_pci.c >> @@ -837,6 +837,7 @@ static const struct intel_device_info mtl_info = { >> .memory_regions = REGION_SMEM | REGION_STOLEN_LMEM, >> .platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(CCS0), >> .require_force_probe = 1, >> + .has_guc_tlb_invalidation = 1, >> MTL_CACHELEVEL, >> }; >> >> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h >> index 39817490b13fd..ad54db0a22470 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.h >> +++ b/drivers/gpu/drm/i915/intel_device_info.h >> @@ -173,7 +173,8 @@ enum intel_ppgtt_type { >> func(has_coherent_ggtt); \ >> func(tuning_thread_rr_after_dep); \ >> func(unfenced_needs_alignment); \ >> - func(hws_needs_physical); >> + func(hws_needs_physical); \ >> + func(has_guc_tlb_invalidation); >> >> struct intel_ip_version { >> u8 ver; >> -- >> 2.25.1
Hi John, > > > Add pci (device info) tags for if GuC TLB Invalidation is enabled. > > > Since GuC based TLB invalidation is only strictly necessary for MTL > > > resently, only enable GuC based TLB invalidations for MTL. > > > > > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> > > Jani was mentioning that the pci tags is not a proper title. > > > > No need to resend, I think I will merge this series, so that, if > > you agree, I can change /pci tags/pci flag/ before pushing. > Have all the review comments been addressed? Surely it can't be pushed until > it has at least an ack from everyone who has expressed concerns about the > changes? this particular patch did not receive any comment so far, except for the "pci tags" from Jani. This solution was somehow hinted by Tvrtko in one of the previous review, I guess. Personally I think that having a pci flag just for this is a bit of an overkill, but I don't have a strong opinion about it. Andi
On Wed, Oct 04, 2023 at 09:24:12PM +0200, Andi Shyti wrote: > Hi John, > > > > > Add pci (device info) tags for if GuC TLB Invalidation is enabled. > > > > Since GuC based TLB invalidation is only strictly necessary for MTL > > > > resently, only enable GuC based TLB invalidations for MTL. > > > > > > > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> > > > Jani was mentioning that the pci tags is not a proper title. > > > > > > No need to resend, I think I will merge this series, so that, if > > > you agree, I can change /pci tags/pci flag/ before pushing. > > Have all the review comments been addressed? Surely it can't be pushed until > > it has at least an ack from everyone who has expressed concerns about the > > changes? > > this particular patch did not receive any comment so far, except > for the "pci tags" from Jani. > > This solution was somehow hinted by Tvrtko in one of the previous > review, I guess. > > Personally I think that having a pci flag just for this is a bit > of an overkill, but I don't have a strong opinion about it. Drive-by comment: you probably only want to turn on the feature flag for MTL at the end of the series, not at the beginning. Otherwise bisects that land somewhere in the middle might have half the necessary changes but not all of them. Matt > > Andi
> > > > Add pci (device info) tags for if GuC TLB Invalidation is enabled. > > > > Since GuC based TLB invalidation is only strictly necessary for MTL > > > > resently, only enable GuC based TLB invalidations for MTL. > > > > > > > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> > > > Jani was mentioning that the pci tags is not a proper title. > > > > > > No need to resend, I think I will merge this series, so that, if > > > you agree, I can change /pci tags/pci flag/ before pushing. > > Have all the review comments been addressed? Surely it can't be pushed until > > it has at least an ack from everyone who has expressed concerns about the > > changes? and... of course... I won't push it until all the reviews and acks are in place... I just wanted to save Jonathan a v6 just for this change if, in a furtunate case, there won't be other reviews. Andi > this particular patch did not receive any comment so far, except > for the "pci tags" from Jani. > > This solution was somehow hinted by Tvrtko in one of the previous > review, I guess. > > Personally I think that having a pci flag just for this is a bit > of an overkill, but I don't have a strong opinion about it. > > Andi
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2b7a6db4d0d44..1e25cc1e3dba1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -807,4 +807,5 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, #define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \ GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) +#define HAS_GUC_TLB_INVALIDATION(i915) (INTEL_INFO(i915)->has_guc_tlb_invalidation) #endif diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index df7c261410f79..c3a5d5efb45d1 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -837,6 +837,7 @@ static const struct intel_device_info mtl_info = { .memory_regions = REGION_SMEM | REGION_STOLEN_LMEM, .platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(CCS0), .require_force_probe = 1, + .has_guc_tlb_invalidation = 1, MTL_CACHELEVEL, }; diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index 39817490b13fd..ad54db0a22470 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -173,7 +173,8 @@ enum intel_ppgtt_type { func(has_coherent_ggtt); \ func(tuning_thread_rr_after_dep); \ func(unfenced_needs_alignment); \ - func(hws_needs_physical); + func(hws_needs_physical); \ + func(has_guc_tlb_invalidation); struct intel_ip_version { u8 ver;
Add pci (device info) tags for if GuC TLB Invalidation is enabled. Since GuC based TLB invalidation is only strictly necessary for MTL resently, only enable GuC based TLB invalidations for MTL. Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_pci.c | 1 + drivers/gpu/drm/i915/intel_device_info.h | 3 ++- 3 files changed, 4 insertions(+), 1 deletion(-)