[Intel-gfx] [PATCH v1 3/4] drm/i915: Split i915_gem_init_stolen()
Lucas De Marchi
lucas.demarchi at intel.com
Fri Sep 16 16:06:31 UTC 2022
On Fri, Sep 16, 2022 at 05:50:33PM +0530, Iddamsetty, Aravind wrote:
>
>
>On 16-09-2022 02:09, Lucas De Marchi wrote:
>> Add some helpers: adjust_stolen(), request_smem_stolen_() and
>> init_reserved_stolen() that are now called by i915_gem_init_stolen() to
>> initialize each part of the Data Stolen Memory region. Main goal is to
>> split the reserved part, also known as WOPCM, as its calculation changes
>> often per platform.
>>
>> This also fixes a bug in graphics version < 5 (in theory, not tested,
>> due to no machine available): it would bail out on stolen creation due
>> to "Stolen reserved area outside stolen memory". Other than that, no
>> change in behavior.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>> index c34065fe2ecc..0e57a6d81534 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>> @@ -77,22 +77,26 @@ void i915_gem_stolen_remove_node(struct drm_i915_private *i915,
>> mutex_unlock(&i915->mm.stolen_lock);
>> }
>>
>> -static int i915_adjust_stolen(struct drm_i915_private *i915,
>> - struct resource *dsm)
>> +static bool valid_stolen_size(struct resource *dsm)
>> +{
>> + return dsm->start != 0 && dsm->end > dsm->start;
>> +}
>> +
>> +static int adjust_stolen(struct drm_i915_private *i915,
>> + struct resource *dsm)
>> {
>> struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
>> struct intel_uncore *uncore = ggtt->vm.gt->uncore;
>> - struct resource *r;
>>
>> - if (dsm->start == 0 || dsm->end <= dsm->start)
>> + if (!valid_stolen_size(dsm))
>> return -EINVAL;
>>
>> /*
>> + * Make sure we don't clobber the GTT if it's within stolen memory
>> + *
>> * TODO: We have yet too encounter the case where the GTT wasn't at the
>> * end of stolen. With that assumption we could simplify this.
>> */
>> -
>> - /* Make sure we don't clobber the GTT if it's within stolen memory */
>> if (GRAPHICS_VER(i915) <= 4 &&
>> !IS_G33(i915) && !IS_PINEVIEW(i915) && !IS_G4X(i915)) {
>> struct resource stolen[2] = {*dsm, *dsm};
>> @@ -131,10 +135,20 @@ static int i915_adjust_stolen(struct drm_i915_private *i915,
>> }
>> }
>>
>> + if (!valid_stolen_size(dsm))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int request_smem_stolen(struct drm_i915_private *i915,
>> + struct resource *dsm)
>> +{
>> + struct resource *r;
>> +
>> /*
>> - * With stolen lmem, we don't need to check if the address range
>> - * overlaps with the non-stolen system memory range, since lmem is local
>> - * to the gpu.
>> + * With stolen lmem, we don't need to request if the address range
>replace /if/for
>> + * since lmem is local to the gpu.
humn.. it seems I skip some words here.
With stolen lmem, we don't need to request system memory since the
stolen region is local to the gpu.
>> */
>> if (HAS_LMEM(i915))
>> return 0;
>> @@ -392,39 +406,22 @@ static void icl_get_stolen_reserved(struct drm_i915_private *i915,
>> }
>> }
>>
>> -static int i915_gem_init_stolen(struct intel_memory_region *mem)
>> +/*
>> + * Initialize i915->dsm_reserved to contain the reserved space within the Data
>> + * Stolen Memory. This is a range on the top of DSM that is reserved, not to
>> + * be used by driver, so must be excluded from the region passed to the
>> + * allocator later. In the spec this is also called as WOPCM.
>> + *
>> + * Our expectation is that the reserved space is at the top of the stolen
>> + * region, as it has been the case for every platform, and *never* at the
>> + * bottom, so the calculation here can be simplified.
>> + */
>> +static int init_reserved_stolen(struct drm_i915_private *i915)
>> {
>> - struct drm_i915_private *i915 = mem->i915;
>> struct intel_uncore *uncore = &i915->uncore;
>> resource_size_t reserved_base, stolen_top;
>> - resource_size_t reserved_total, reserved_size;
>> -
>> - mutex_init(&i915->mm.stolen_lock);
>> -
>> - if (intel_vgpu_active(i915)) {
>> - drm_notice(&i915->drm,
>> - "%s, disabling use of stolen memory\n",
>> - "iGVT-g active");
>> - return 0;
>> - }
>> -
>> - if (i915_vtd_active(i915) && GRAPHICS_VER(i915) < 8) {
>> - drm_notice(&i915->drm,
>> - "%s, disabling use of stolen memory\n",
>> - "DMAR active");
>> - return 0;
>> - }
>> -
>> - if (resource_size(&mem->region) == 0)
>> - return 0;
>> -
>> - if (i915_adjust_stolen(i915, &mem->region))
>> - return 0;
>> -
>> - GEM_BUG_ON(i915->dsm.start == 0);
>> - GEM_BUG_ON(i915->dsm.end <= i915->dsm.start);
>> -
>> - i915->dsm = mem->region;
>> + resource_size_t reserved_size;
>> + int ret = 0;
>>
>> stolen_top = i915->dsm.end + 1;
>> reserved_base = stolen_top;
>> @@ -453,19 +450,17 @@ static int i915_gem_init_stolen(struct intel_memory_region *mem)
>> } else if (GRAPHICS_VER(i915) >= 5 || IS_G4X(i915)) {
>> g4x_get_stolen_reserved(i915, uncore,
>> &reserved_base, &reserved_size);
>> + } else {
>> + /* No reserved region */
>> + goto bail_out;
>
>better to have a WARN_ON here about STOLEN region wrongly passed on the
>region list.
see the follow up I sent on this patch. The bail out here should be outside
this else and is a normal condition. This is about the reserved space
within stolen (aka wopcm), and having it 0 is normal even for the
platforms above that have a function to read the stolen reserved region.
a WARN_ON based on the regions in device_info would be wrong as having
stolen doesn't mean there is a wopcm. Btw I think a great source of
confusion is using "reserved stolen" for this part because we then think
it's about reserving the stolen memory when in fact this is about a
region inside the stolen.
>
>> }
>>
>> - /*
>> - * Our expectation is that the reserved space is at the top of the
>> - * stolen region and *never* at the bottom. If we see !reserved_base,
>> - * it likely means we failed to read the registers correctly.
>> - */
>> - if (!reserved_base) {
>> + if (!reserved_base || reserved_base == stolen_top) {
>> drm_err(&i915->drm,
>> "inconsistent reservation %pa + %pa; ignoring\n",
>> &reserved_base, &reserved_size);
>> - reserved_base = stolen_top;
>> - reserved_size = 0;
>> + ret = -EINVAL;
>> + goto bail_out;
>> }
>>
>> i915->dsm_reserved =
>> @@ -475,19 +470,55 @@ static int i915_gem_init_stolen(struct intel_memory_region *mem)
>> drm_err(&i915->drm,
>> "Stolen reserved area %pR outside stolen memory %pR\n",
>> &i915->dsm_reserved, &i915->dsm);
>> + ret = -EINVAL;
>> + goto bail_out;
>> + }
>> +
>> + return 0;
>> +
>> +bail_out:
>> + i915->dsm_reserved =
>> + (struct resource)DEFINE_RES_MEM(reserved_base, 0);
>> +
>> + return ret;
>> +}
>> +
>> +static int i915_gem_init_stolen(struct intel_memory_region *mem)
>> +{
>> + struct drm_i915_private *i915 = mem->i915;
>> +
>> + mutex_init(&i915->mm.stolen_lock);
>> +
>> + if (intel_vgpu_active(i915)) {
>> + drm_notice(&i915->drm,
>> + "%s, disabling use of stolen memory\n",
>> + "iGVT-g active");
>> + return 0;
>> + }
>> +
>> + if (i915_vtd_active(i915) && GRAPHICS_VER(i915) < 8) {
>> + drm_notice(&i915->drm,
>> + "%s, disabling use of stolen memory\n",
>> + "DMAR active");
>> return 0;
>> }
>>
>> + if (adjust_stolen(i915, &mem->region))
>> + return 0;
>> +
>> + if (request_smem_stolen(i915, &mem->region))
>> + return 0;
>
>why do we want to subside the errors returned in adjust_stolen and
>request_smem_stolen?
This was already there: i915_gem_init_stolen() was only returning 0.
This patch only makes it clearer since it's now a smaller function.
Basically this was the function doing the "log when
appropriate, but consider it non-fatal". On the next patch I move this
decision to the caller.
Lucas De Marchi
>
>> +
>> + i915->dsm = mem->region;
>> +
>> + if (init_reserved_stolen(i915))
>similarly here.
>
>Thanks,
>Aravind.
>> + return 0;
>> +
>> /* Exclude the reserved region from driver use */
>> - mem->region.end = reserved_base - 1;
>> + mem->region.end = i915->dsm_reserved.start - 1;
>> mem->io_size = min(mem->io_size, resource_size(&mem->region));
>>
>> - /* It is possible for the reserved area to end before the end of stolen
>> - * memory, so just consider the start. */
>> - reserved_total = stolen_top - reserved_base;
>> -
>> - i915->stolen_usable_size =
>> - resource_size(&i915->dsm) - reserved_total;
>> + i915->stolen_usable_size = resource_size(&mem->region);
>>
>> drm_dbg(&i915->drm,
>> "Memory reserved for graphics device: %lluK, usable: %lluK\n",
>> @@ -759,11 +790,6 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
>> if (GEM_WARN_ON(resource_size(&mem->region) == 0))
>> return -ENODEV;
>>
>> - /*
>> - * TODO: For stolen lmem we mostly just care about populating the dsm
>> - * related bits and setting up the drm_mm allocator for the range.
>> - * Perhaps split up i915_gem_init_stolen() for this.
>> - */
>> err = i915_gem_init_stolen(mem);
>> if (err)
>> return err;
>>
More information about the Intel-gfx
mailing list