| Message ID | 20220126152155.3070602-20-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for small BAR recovery | expand |
On 1/26/22 16:21, Matthew Auld wrote: > Just pass along the probed io_size. The backend should be able to > utilize the entire range here, even if some of it is non-mappable. Changes here LGTM. > > It does leave open with what to do with stolen local-memory. Are objects in stolen local required to be mappable? /Thomas > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/i915/gt/intel_region_lmem.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c > index 2c7ec7ff79fd..b788fc2b3df8 100644 > --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c > +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c > @@ -200,6 +200,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) > struct intel_memory_region *mem; > resource_size_t min_page_size; > resource_size_t io_start; > + resource_size_t io_size; > resource_size_t lmem_size; > int err; > > @@ -210,7 +211,8 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) > lmem_size = intel_uncore_read64(uncore, GEN12_GSMBASE); > > io_start = pci_resource_start(pdev, 2); > - if (GEM_WARN_ON(lmem_size > pci_resource_len(pdev, 2))) > + io_size = min(pci_resource_len(pdev, 2), lmem_size); > + if (!io_size) > return ERR_PTR(-ENODEV); > > min_page_size = HAS_64K_PAGES(i915) ? I915_GTT_PAGE_SIZE_64K : > @@ -220,7 +222,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) > lmem_size, > min_page_size, > io_start, > - lmem_size, > + io_size, > INTEL_MEMORY_LOCAL, > 0, > &intel_region_lmem_ops);
On 03/02/2022 09:48, Thomas Hellström wrote: > > On 1/26/22 16:21, Matthew Auld wrote: >> Just pass along the probed io_size. The backend should be able to >> utilize the entire range here, even if some of it is non-mappable. > Changes here LGTM. >> >> It does leave open with what to do with stolen local-memory. > > Are objects in stolen local required to be mappable? From a quick look I don't really see such users on discrete, outside of maybe intelfb_create(), where I guess the initial fb might be located in stolen on DG1. But from DG2+ it looks like it will just be located in normal LMEM. For that I was thinking we add something like i915_gem_object_create_region_at(), and somehow wire that up to the {fpfn, lpfn}... > > /Thomas > > >> >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_region_lmem.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c >> b/drivers/gpu/drm/i915/gt/intel_region_lmem.c >> index 2c7ec7ff79fd..b788fc2b3df8 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c >> +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c >> @@ -200,6 +200,7 @@ static struct intel_memory_region >> *setup_lmem(struct intel_gt *gt) >> struct intel_memory_region *mem; >> resource_size_t min_page_size; >> resource_size_t io_start; >> + resource_size_t io_size; >> resource_size_t lmem_size; >> int err; >> @@ -210,7 +211,8 @@ static struct intel_memory_region >> *setup_lmem(struct intel_gt *gt) >> lmem_size = intel_uncore_read64(uncore, GEN12_GSMBASE); >> io_start = pci_resource_start(pdev, 2); >> - if (GEM_WARN_ON(lmem_size > pci_resource_len(pdev, 2))) >> + io_size = min(pci_resource_len(pdev, 2), lmem_size); >> + if (!io_size) >> return ERR_PTR(-ENODEV); >> min_page_size = HAS_64K_PAGES(i915) ? I915_GTT_PAGE_SIZE_64K : >> @@ -220,7 +222,7 @@ static struct intel_memory_region >> *setup_lmem(struct intel_gt *gt) >> lmem_size, >> min_page_size, >> io_start, >> - lmem_size, >> + io_size, >> INTEL_MEMORY_LOCAL, >> 0, >> &intel_region_lmem_ops);
On Thu, 2022-02-03 at 11:18 +0000, Matthew Auld wrote: > On 03/02/2022 09:48, Thomas Hellström wrote: > > > > On 1/26/22 16:21, Matthew Auld wrote: > > > Just pass along the probed io_size. The backend should be able to > > > utilize the entire range here, even if some of it is non- > > > mappable. > > Changes here LGTM. > > > > > > It does leave open with what to do with stolen local-memory. > > > > Are objects in stolen local required to be mappable? > > From a quick look I don't really see such users on discrete, outside > of > maybe intelfb_create(), where I guess the initial fb might be located > in > stolen on DG1. But from DG2+ it looks like it will just be located in > normal LMEM. For that I was thinking we add something like > i915_gem_object_create_region_at(), and somehow wire that up to the > {fpfn, lpfn}... So we could then skip STOLEN completely on DG2+? Could we then also do the same on DG1, at least assuming that creating and pinning an object for that initial fb would be done before any other pinning into LMEM? /Thomas
On 03/02/2022 13:56, Thomas Hellström wrote: > On Thu, 2022-02-03 at 11:18 +0000, Matthew Auld wrote: >> On 03/02/2022 09:48, Thomas Hellström wrote: >>> >>> On 1/26/22 16:21, Matthew Auld wrote: >>>> Just pass along the probed io_size. The backend should be able to >>>> utilize the entire range here, even if some of it is non- >>>> mappable. >>> Changes here LGTM. >>>> >>>> It does leave open with what to do with stolen local-memory. >>> >>> Are objects in stolen local required to be mappable? >> >> From a quick look I don't really see such users on discrete, outside >> of >> maybe intelfb_create(), where I guess the initial fb might be located >> in >> stolen on DG1. But from DG2+ it looks like it will just be located in >> normal LMEM. For that I was thinking we add something like >> i915_gem_object_create_region_at(), and somehow wire that up to the >> {fpfn, lpfn}... > > So we could then skip STOLEN completely on DG2+? Could we then also do > the same on DG1, at least assuming that creating and pinning an object > for that initial fb would be done before any other pinning into LMEM? It looks like fbc is the main user on discrete, AFAICT, but that doesn't seem to use the gem object interface, and instead just plugs into the underlying drm_mm directly. So AFAIK we still want stolen on DG2/DG1 for that. > > /Thomas > >
diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c index 2c7ec7ff79fd..b788fc2b3df8 100644 --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c @@ -200,6 +200,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) struct intel_memory_region *mem; resource_size_t min_page_size; resource_size_t io_start; + resource_size_t io_size; resource_size_t lmem_size; int err; @@ -210,7 +211,8 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) lmem_size = intel_uncore_read64(uncore, GEN12_GSMBASE); io_start = pci_resource_start(pdev, 2); - if (GEM_WARN_ON(lmem_size > pci_resource_len(pdev, 2))) + io_size = min(pci_resource_len(pdev, 2), lmem_size); + if (!io_size) return ERR_PTR(-ENODEV); min_page_size = HAS_64K_PAGES(i915) ? I915_GTT_PAGE_SIZE_64K : @@ -220,7 +222,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) lmem_size, min_page_size, io_start, - lmem_size, + io_size, INTEL_MEMORY_LOCAL, 0, &intel_region_lmem_ops);
Just pass along the probed io_size. The backend should be able to utilize the entire range here, even if some of it is non-mappable. It does leave open with what to do with stolen local-memory. Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/i915/gt/intel_region_lmem.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)