Re: [Intel-gfx] [PATCH v1 3/4] drm/i915: Split i915_gem_init_stolen()
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
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(>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 = >uncore;
resource_size_t reserved_base, stolen_top;
- resource_size_t reserved_total, reserved_size;
-
- mutex_init(>mm.stolen_lock);
-
- if (intel_vgpu_active(i915)) {
- drm_notice(>drm,
- "%s, disabling use of stolen memory\n",
- "iGVT-g active");
- return 0;
- }
-
- if (i915_vtd_active(i915) && GRAPHICS_VER(i915) < 8) {
- drm_notice(>drm,
- "%s, disabling use of stolen memory\n",
- "DMAR active");
- return 0;
- }
-
- if (resource_size(>region) == 0)
- return 0;
-
- if (i915_adjust_stolen(i915, >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
Re: [Intel-gfx] [PATCH v1 3/4] drm/i915: Split i915_gem_init_stolen()
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
>
> 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(>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.
>*/
> 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 = >uncore;
> resource_size_t reserved_base, stolen_top;
> - resource_size_t reserved_total, reserved_size;
> -
> - mutex_init(>mm.stolen_lock);
> -
> - if (intel_vgpu_active(i915)) {
> - drm_notice(>drm,
> -"%s, disabling use of stolen memory\n",
> -"iGVT-g active");
> - return 0;
> - }
> -
> - if (i915_vtd_active(i915) && GRAPHICS_VER(i915) < 8) {
> - drm_notice(>drm,
> -"%s, disabling use of stolen memory\n",
> -"DMAR active");
> - return 0;
> - }
> -
> - if (resource_size(>region) == 0)
> - return 0;
> -
> - if (i915_adjust_stolen(i915, >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)) {
