[v2,2/3] drm/i915: Split i915_gem_init_stolen()

Commit Message

Lucas De Marchi Sept. 16, 2022, 5:36 p.m. UTC

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 within the stolen, also known as
WOPCM, as its calculation changes often per platform and is a big source
of confusion when handling stolen memory.

Signed-off-by: Lucas De Marchi <[e-mail]>

Comments

Wayne Boyer Sept. 20, 2022, 7:16 p.m. UTC | #1

On 9/16/22 10:36 AM, 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 within the stolen, also known as
> WOPCM, as its calculation changes often per platform and is a big source
> of confusion when handling stolen memory.
> 
> Signed-off-by: Lucas De Marchi <[e-mail]>
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index 3665f9b035bb..6edf4e374f54 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

nit: as long as you're updating this comment block, s/too/to/

Otherwise,
Reviewed-by: Wayne Boyer <[e-mail]>

>   	 * 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 system memory for the
> +	 * address range since it's 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;
> -
> -	i915->dsm = mem->region;
> -
> -	if (i915_adjust_stolen(i915, &i915->dsm))
> -		return 0;
> -
> -	GEM_BUG_ON(i915->dsm.start == 0);
> -	GEM_BUG_ON(i915->dsm.end <= i915->dsm.start);
> +	resource_size_t reserved_size;
> +	int ret = 0;
>   
>   	stolen_top = i915->dsm.end + 1;
>   	reserved_base = stolen_top;
> @@ -455,17 +452,16 @@ static int i915_gem_init_stolen(struct intel_memory_region *mem)
>   					&reserved_base, &reserved_size);
>   	}
>   
> -	/*
> -	 * 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.
> -	 */
> +	/* No reserved stolen */
> +	if (reserved_base == stolen_top)
> +		goto bail_out;
> +
>   	if (!reserved_base) {
>   		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 +471,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;
> +
> +	i915->dsm = mem->region;
> +
> +	if (init_reserved_stolen(i915))
> +		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 +791,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;
>

Patch

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index 3665f9b035bb..6edf4e374f54 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 system memory for the
+	 * address range since it's 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;
-
-	i915->dsm = mem->region;
-
-	if (i915_adjust_stolen(i915, &i915->dsm))
-		return 0;
-
-	GEM_BUG_ON(i915->dsm.start == 0);
-	GEM_BUG_ON(i915->dsm.end <= i915->dsm.start);
+	resource_size_t reserved_size;
+	int ret = 0;
 
 	stolen_top = i915->dsm.end + 1;
 	reserved_base = stolen_top;
@@ -455,17 +452,16 @@  static int i915_gem_init_stolen(struct intel_memory_region *mem)
 					&reserved_base, &reserved_size);
 	}
 
-	/*
-	 * 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.
-	 */
+	/* No reserved stolen */
+	if (reserved_base == stolen_top)
+		goto bail_out;
+
 	if (!reserved_base) {
 		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 +471,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;
+
+	i915->dsm = mem->region;
+
+	if (init_reserved_stolen(i915))
+		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 +791,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;