[Intel-gfx] [PATCH v4 1/5] drm/i915: remove the TODO in pin_and_fence_fb_obj

[Intel-gfx] [PATCH v4 1/5] drm/i915: remove the TODO in pin_and_fence_fb_obj

[Matthew Auld]

The copy is async (if there even is one), but when later updating the
GGTT we always sync against the binding, which will in turn sync against
any moves.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Jianshui Yu <jianshui.yu@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fb_pin.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
index c86e5d4ee016..0cd9e8cb078b 100644
--- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
+++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
@@ -141,7 +141,6 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
 		ret = i915_gem_object_attach_phys(obj, alignment);
 	else if (!ret && HAS_LMEM(dev_priv))
 		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
-	/* TODO: Do we need to sync when migration becomes async? */
 	if (!ret)
 		ret = i915_gem_object_pin_pages(obj);
 	if (ret)
-- 
2.37.3

[Intel-gfx] [PATCH v4 2/5] drm/i915/display: handle migration for dpt

[Matthew Auld]

On platforms like DG2, it looks like the dpt path here is missing the
migrate-to-lmem step on discrete platforms.

Fixes: 33e7a975103c ("drm/i915/xelpd: First stab at DPT support")
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Jianshui Yu <jianshui.yu@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fb_pin.c | 23 ++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
index 0cd9e8cb078b..32206bd359da 100644
--- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
+++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
@@ -26,10 +26,17 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
 	struct drm_device *dev = fb->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct i915_gem_ww_ctx ww;
 	struct i915_vma *vma;
 	u32 alignment;
 	int ret;
 
+	/*
+	 * We are not syncing against the binding (and potential migrations)
+	 * below, so this vm must never be async.
+	*/
+	GEM_WARN_ON(vm->bind_async_flags);
+
 	if (WARN_ON(!i915_gem_object_is_framebuffer(obj)))
 		return ERR_PTR(-EINVAL);
 
@@ -37,10 +44,20 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
 
 	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
 
-	ret = i915_gem_object_lock_interruptible(obj, NULL);
-	if (!ret) {
+	for_i915_gem_ww(&ww, ret, true) {
+		ret = i915_gem_object_lock(obj, &ww);
+		if (ret)
+			continue;
+
+		if (HAS_LMEM(dev_priv)) {
+			ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
+			if (ret)
+				continue;
+		}
+
 		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
-		i915_gem_object_unlock(obj);
+		if (ret)
+			continue;
 	}
 	if (ret) {
 		vma = ERR_PTR(ret);
-- 
2.37.3

[Intel-gfx] [PATCH v4 3/5] drm/i915: allow control over the flags when migrating

[Matthew Auld]

In the next patch we want to move the object (if the current resource is
not compatible), to the mappable part of lmem for some display buffers.
Currently that requires being able to unset the I915_BO_ALLOC_GPU_ONLY
hint.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Jianshui Yu <jianshui.yu@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c    | 37 ++++++++++++++++++-
 drivers/gpu/drm/i915/gem/i915_gem_object.h    |  4 ++
 .../gpu/drm/i915/gem/i915_gem_object_types.h  |  3 +-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c       |  5 ++-
 4 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 7ff9c7877bec..369006c5317f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -652,6 +652,41 @@ bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj,
 int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
 			    struct i915_gem_ww_ctx *ww,
 			    enum intel_region_id id)
+{
+	return __i915_gem_object_migrate(obj, ww, id, obj->flags);
+}
+
+/**
+ * __i915_gem_object_migrate - Migrate an object to the desired region id, with
+ * control of the extra flags
+ * @obj: The object to migrate.
+ * @ww: An optional struct i915_gem_ww_ctx. If NULL, the backend may
+ * not be successful in evicting other objects to make room for this object.
+ * @id: The region id to migrate to.
+ * @flags: The object flags. Normally just obj->flags.
+ *
+ * Attempt to migrate the object to the desired memory region. The
+ * object backend must support migration and the object may not be
+ * pinned, (explicitly pinned pages or pinned vmas). The object must
+ * be locked.
+ * On successful completion, the object will have pages pointing to
+ * memory in the new region, but an async migration task may not have
+ * completed yet, and to accomplish that, i915_gem_object_wait_migration()
+ * must be called.
+ *
+ * Note: the @ww parameter is not used yet, but included to make sure
+ * callers put some effort into obtaining a valid ww ctx if one is
+ * available.
+ *
+ * Return: 0 on success. Negative error code on failure. In particular may
+ * return -ENXIO on lack of region space, -EDEADLK for deadlock avoidance
+ * if @ww is set, -EINTR or -ERESTARTSYS if signal pending, and
+ * -EBUSY if the object is pinned.
+ */
+int __i915_gem_object_migrate(struct drm_i915_gem_object *obj,
+			      struct i915_gem_ww_ctx *ww,
+			      enum intel_region_id id,
+			      unsigned int flags)
 {
 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
 	struct intel_memory_region *mr;
@@ -672,7 +707,7 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
 		return 0;
 	}
 
-	return obj->ops->migrate(obj, mr);
+	return obj->ops->migrate(obj, mr, flags);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index a3b7551a57fc..6b9ecff42bb5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -612,6 +612,10 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj);
 int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
 			    struct i915_gem_ww_ctx *ww,
 			    enum intel_region_id id);
+int __i915_gem_object_migrate(struct drm_i915_gem_object *obj,
+			      struct i915_gem_ww_ctx *ww,
+			      enum intel_region_id id,
+			      unsigned int flags);
 
 bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj,
 				 enum intel_region_id id);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 40305e2bcd49..d0d6772e6f36 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -107,7 +107,8 @@ struct drm_i915_gem_object_ops {
 	 * pinning or for as long as the object lock is held.
 	 */
 	int (*migrate)(struct drm_i915_gem_object *obj,
-		       struct intel_memory_region *mr);
+		       struct intel_memory_region *mr,
+		       unsigned int flags);
 
 	void (*release)(struct drm_i915_gem_object *obj);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 3dc6acfcf4ec..5bed353ee9bc 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -848,9 +848,10 @@ static int __i915_ttm_migrate(struct drm_i915_gem_object *obj,
 }
 
 static int i915_ttm_migrate(struct drm_i915_gem_object *obj,
-			    struct intel_memory_region *mr)
+			    struct intel_memory_region *mr,
+			    unsigned int flags)
 {
-	return __i915_ttm_migrate(obj, mr, obj->flags);
+	return __i915_ttm_migrate(obj, mr, flags);
 }
 
 static void i915_ttm_put_pages(struct drm_i915_gem_object *obj,
-- 
2.37.3

[Intel-gfx] [PATCH v4 4/5] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Matthew Auld]

For these types of display buffers, we need to able to CPU access some
part of the backing memory in prepare_plane_clear_colors(). As a result
we need to ensure we always place in the mappable part of lmem, which
becomes necessary on small-bar systems.

v2(Nirmoy & Ville):
 - Add some commentary for why we need to CPU access the buffer.
 - Split out the other changes, so we just consider the display change
   here.
v3:
 - Handle this in the dpt path.

Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support")
Reported-by: Jianshui Yu <jianshui.yu@intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fb_pin.c | 24 +++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
index 32206bd359da..8197343300ee 100644
--- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
+++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
@@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
 			continue;
 
 		if (HAS_LMEM(dev_priv)) {
-			ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
+			unsigned int flags = obj->flags;
+
+			/*
+			 * For this type of buffer we need to able to read from the CPU
+			 * the clear color value found in the buffer, hence we need to
+			 * ensure it is always in the mappable part of lmem, if this is
+			 * a small-bar device.
+			 */
+			if (intel_fb_rc_ccs_cc_plane(fb) >= 0)
+				flags &= ~I915_BO_ALLOC_GPU_ONLY;
+			ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0,
+							flags);
 			if (ret)
 				continue;
 		}
@@ -156,8 +167,17 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
 	ret = i915_gem_object_lock(obj, &ww);
 	if (!ret && phys_cursor)
 		ret = i915_gem_object_attach_phys(obj, alignment);
-	else if (!ret && HAS_LMEM(dev_priv))
+	else if (!ret && HAS_LMEM(dev_priv)) {
+		/*
+		 * For this type of ccs buffer we need to able to read from the
+		 * CPU the clear color value found in the buffer, which might
+		 * require moving to the mappable part of lmem first, but here
+		 * we should be using dpt for this.
+		 */
+		WARN_ON_ONCE(intel_fb_rc_ccs_cc_plane(fb) >= 0);
+
 		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
+	}
 	if (!ret)
 		ret = i915_gem_object_pin_pages(obj);
 	if (ret)
-- 
2.37.3

[Intel-gfx] [PATCH v4 5/5] drm/i915: check memory is mappable in read_from_page

[Matthew Auld]

On small-bar systems we could be given something non-mappable here,
which leads to nasty oops. Make this nicer by checking if the resource
is mappable or not, and return an error otherwise.

v2: drop GEM_BUG_ON(flags & I915_BO_ALLOC_GPU_ONLY)

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Jianshui Yu <jianshui.yu@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 369006c5317f..62495d5d0038 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -444,6 +444,16 @@ i915_gem_object_read_from_page_iomap(struct drm_i915_gem_object *obj, u64 offset
 	io_mapping_unmap(src_map);
 }
 
+static bool object_has_mappable_iomem(struct drm_i915_gem_object *obj)
+{
+	GEM_BUG_ON(!i915_gem_object_has_iomem(obj));
+
+	if (IS_DGFX(to_i915(obj->base.dev)))
+		return i915_ttm_resource_mappable(i915_gem_to_ttm(obj)->resource);
+
+	return true;
+}
+
 /**
  * i915_gem_object_read_from_page - read data from the page of a GEM object
  * @obj: GEM object to read from
@@ -466,7 +476,7 @@ int i915_gem_object_read_from_page(struct drm_i915_gem_object *obj, u64 offset,
 
 	if (i915_gem_object_has_struct_page(obj))
 		i915_gem_object_read_from_page_kmap(obj, offset, dst, size);
-	else if (i915_gem_object_has_iomem(obj))
+	else if (i915_gem_object_has_iomem(obj) && object_has_mappable_iomem(obj))
 		i915_gem_object_read_from_page_iomap(obj, offset, dst, size);
 	else
 		return -ENODEV;
-- 
2.37.3

Re: [Intel-gfx] [PATCH v4 2/5] drm/i915/display: handle migration for dpt

[Ville Syrjälä]

On Tue, Oct 04, 2022 at 11:33:08AM +0100, Matthew Auld wrote:
> On platforms like DG2, it looks like the dpt path here is missing the
> migrate-to-lmem step on discrete platforms.
> 
> Fixes: 33e7a975103c ("drm/i915/xelpd: First stab at DPT support")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Jianshui Yu <jianshui.yu@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Nirmoy Das <nirmoy.das@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fb_pin.c | 23 ++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> index 0cd9e8cb078b..32206bd359da 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> @@ -26,10 +26,17 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
>  	struct drm_device *dev = fb->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct i915_gem_ww_ctx ww;
>  	struct i915_vma *vma;
>  	u32 alignment;
>  	int ret;
>  
> +	/*
> +	 * We are not syncing against the binding (and potential migrations)
> +	 * below, so this vm must never be async.
> +	*/
> +	GEM_WARN_ON(vm->bind_async_flags);

Not sure why this is different between the dpt and non-dpt paths?

> +
>  	if (WARN_ON(!i915_gem_object_is_framebuffer(obj)))
>  		return ERR_PTR(-EINVAL);
>  
> @@ -37,10 +44,20 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
>  
>  	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
>  
> -	ret = i915_gem_object_lock_interruptible(obj, NULL);
> -	if (!ret) {
> +	for_i915_gem_ww(&ww, ret, true) {
> +		ret = i915_gem_object_lock(obj, &ww);
> +		if (ret)
> +			continue;
> +
> +		if (HAS_LMEM(dev_priv)) {
> +			ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> +			if (ret)
> +				continue;
> +		}
> +
>  		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
> -		i915_gem_object_unlock(obj);
> +		if (ret)
> +			continue;
>  	}

The non-dpt path has the whole thing under the same lock.
Is there a reason we're not doing the same thing for both?

I guess some kind of unification effort would be nice to
avoid the codepaths diverging for no good reason.

Maybe even some refactoring would be nice to share more code,
but IIRC all the fence/mappable stuff in the lower levels
of the ggtt paths is what got in the way of just reusing
more of the ggtt code directly.

>  	if (ret) {
>  		vma = ERR_PTR(ret);
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v4 4/5] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Ville Syrjälä]

On Tue, Oct 04, 2022 at 11:33:10AM +0100, Matthew Auld wrote:
> For these types of display buffers, we need to able to CPU access some
> part of the backing memory in prepare_plane_clear_colors(). As a result
> we need to ensure we always place in the mappable part of lmem, which
> becomes necessary on small-bar systems.
> 
> v2(Nirmoy & Ville):
>  - Add some commentary for why we need to CPU access the buffer.
>  - Split out the other changes, so we just consider the display change
>    here.
> v3:
>  - Handle this in the dpt path.
> 
> Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support")
> Reported-by: Jianshui Yu <jianshui.yu@intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Nirmoy Das <nirmoy.das@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fb_pin.c | 24 +++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> index 32206bd359da..8197343300ee 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
>  			continue;
>  
>  		if (HAS_LMEM(dev_priv)) {
> -			ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> +			unsigned int flags = obj->flags;
> +
> +			/*
> +			 * For this type of buffer we need to able to read from the CPU
> +			 * the clear color value found in the buffer, hence we need to
> +			 * ensure it is always in the mappable part of lmem, if this is
> +			 * a small-bar device.
> +			 */
> +			if (intel_fb_rc_ccs_cc_plane(fb) >= 0)
> +				flags &= ~I915_BO_ALLOC_GPU_ONLY;
> +			ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0,
> +							flags);
>  			if (ret)
>  				continue;
>  		}
> @@ -156,8 +167,17 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
>  	ret = i915_gem_object_lock(obj, &ww);
>  	if (!ret && phys_cursor)
>  		ret = i915_gem_object_attach_phys(obj, alignment);
> -	else if (!ret && HAS_LMEM(dev_priv))
> +	else if (!ret && HAS_LMEM(dev_priv)) {
> +		/*
> +		 * For this type of ccs buffer we need to able to read from the
> +		 * CPU the clear color value found in the buffer, which might
> +		 * require moving to the mappable part of lmem first, but here
> +		 * we should be using dpt for this.
> +		 */
> +		WARN_ON_ONCE(intel_fb_rc_ccs_cc_plane(fb) >= 0);

DPT isn't availalable on DG1.

> +
>  		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> +	}
>  	if (!ret)
>  		ret = i915_gem_object_pin_pages(obj);
>  	if (ret)
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v4 4/5] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Matthew Auld]

On 04/10/2022 12:28, Ville Syrjälä wrote:
> On Tue, Oct 04, 2022 at 11:33:10AM +0100, Matthew Auld wrote:
>> For these types of display buffers, we need to able to CPU access some
>> part of the backing memory in prepare_plane_clear_colors(). As a result
>> we need to ensure we always place in the mappable part of lmem, which
>> becomes necessary on small-bar systems.
>>
>> v2(Nirmoy & Ville):
>>   - Add some commentary for why we need to CPU access the buffer.
>>   - Split out the other changes, so we just consider the display change
>>     here.
>> v3:
>>   - Handle this in the dpt path.
>>
>> Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support")
>> Reported-by: Jianshui Yu <jianshui.yu@intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_fb_pin.c | 24 +++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
>> index 32206bd359da..8197343300ee 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
>> @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
>>   			continue;
>>   
>>   		if (HAS_LMEM(dev_priv)) {
>> -			ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
>> +			unsigned int flags = obj->flags;
>> +
>> +			/*
>> +			 * For this type of buffer we need to able to read from the CPU
>> +			 * the clear color value found in the buffer, hence we need to
>> +			 * ensure it is always in the mappable part of lmem, if this is
>> +			 * a small-bar device.
>> +			 */
>> +			if (intel_fb_rc_ccs_cc_plane(fb) >= 0)
>> +				flags &= ~I915_BO_ALLOC_GPU_ONLY;
>> +			ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0,
>> +							flags);
>>   			if (ret)
>>   				continue;
>>   		}
>> @@ -156,8 +167,17 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
>>   	ret = i915_gem_object_lock(obj, &ww);
>>   	if (!ret && phys_cursor)
>>   		ret = i915_gem_object_attach_phys(obj, alignment);
>> -	else if (!ret && HAS_LMEM(dev_priv))
>> +	else if (!ret && HAS_LMEM(dev_priv)) {
>> +		/*
>> +		 * For this type of ccs buffer we need to able to read from the
>> +		 * CPU the clear color value found in the buffer, which might
>> +		 * require moving to the mappable part of lmem first, but here
>> +		 * we should be using dpt for this.
>> +		 */
>> +		WARN_ON_ONCE(intel_fb_rc_ccs_cc_plane(fb) >= 0);
> 
> DPT isn't availalable on DG1.

Hmm, does it also support the DG2_RC_CCS_CC modifier?

> 
>> +
>>   		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
>> +	}
>>   	if (!ret)
>>   		ret = i915_gem_object_pin_pages(obj);
>>   	if (ret)
>> -- 
>> 2.37.3
> 

Re: [Intel-gfx] [PATCH v4 2/5] drm/i915/display: handle migration for dpt

[Matthew Auld]

On 04/10/2022 12:22, Ville Syrjälä wrote:
> On Tue, Oct 04, 2022 at 11:33:08AM +0100, Matthew Auld wrote:
>> On platforms like DG2, it looks like the dpt path here is missing the
>> migrate-to-lmem step on discrete platforms.
>>
>> Fixes: 33e7a975103c ("drm/i915/xelpd: First stab at DPT support")
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Jianshui Yu <jianshui.yu@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_fb_pin.c | 23 ++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
>> index 0cd9e8cb078b..32206bd359da 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
>> @@ -26,10 +26,17 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
>>   	struct drm_device *dev = fb->dev;
>>   	struct drm_i915_private *dev_priv = to_i915(dev);
>>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>> +	struct i915_gem_ww_ctx ww;
>>   	struct i915_vma *vma;
>>   	u32 alignment;
>>   	int ret;
>>   
>> +	/*
>> +	 * We are not syncing against the binding (and potential migrations)
>> +	 * below, so this vm must never be async.
>> +	*/
>> +	GEM_WARN_ON(vm->bind_async_flags);
> 
> Not sure why this is different between the dpt and non-dpt paths?

It looks like dpt is using vma_pin() below which doesn't have the 
wait_for_bind() stuff, like we do for ggtt_pin().

> 
>> +
>>   	if (WARN_ON(!i915_gem_object_is_framebuffer(obj)))
>>   		return ERR_PTR(-EINVAL);
>>   
>> @@ -37,10 +44,20 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
>>   
>>   	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
>>   
>> -	ret = i915_gem_object_lock_interruptible(obj, NULL);
>> -	if (!ret) {
>> +	for_i915_gem_ww(&ww, ret, true) {
>> +		ret = i915_gem_object_lock(obj, &ww);
>> +		if (ret)
>> +			continue;
>> +
>> +		if (HAS_LMEM(dev_priv)) {
>> +			ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
>> +			if (ret)
>> +				continue;
>> +		}
>> +
>>   		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
>> -		i915_gem_object_unlock(obj);
>> +		if (ret)
>> +			continue;
>>   	}
> 
> The non-dpt path has the whole thing under the same lock.
> Is there a reason we're not doing the same thing for both?
> 
> I guess some kind of unification effort would be nice to
> avoid the codepaths diverging for no good reason.

Can do, I'll take a look.

> 
> Maybe even some refactoring would be nice to share more code,
> but IIRC all the fence/mappable stuff in the lower levels
> of the ggtt paths is what got in the way of just reusing
> more of the ggtt code directly.
> 
>>   	if (ret) {
>>   		vma = ERR_PTR(ret);
>> -- 
>> 2.37.3
> 

Re: [Intel-gfx] [PATCH v4 4/5] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Ville Syrjälä]

On Tue, Oct 04, 2022 at 12:51:11PM +0100, Matthew Auld wrote:
> On 04/10/2022 12:28, Ville Syrjälä wrote:
> > On Tue, Oct 04, 2022 at 11:33:10AM +0100, Matthew Auld wrote:
> >> For these types of display buffers, we need to able to CPU access some
> >> part of the backing memory in prepare_plane_clear_colors(). As a result
> >> we need to ensure we always place in the mappable part of lmem, which
> >> becomes necessary on small-bar systems.
> >>
> >> v2(Nirmoy & Ville):
> >>   - Add some commentary for why we need to CPU access the buffer.
> >>   - Split out the other changes, so we just consider the display change
> >>     here.
> >> v3:
> >>   - Handle this in the dpt path.
> >>
> >> Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support")
> >> Reported-by: Jianshui Yu <jianshui.yu@intel.com>
> >> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Nirmoy Das <nirmoy.das@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_fb_pin.c | 24 +++++++++++++++++++--
> >>   1 file changed, 22 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> >> index 32206bd359da..8197343300ee 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> >> @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
> >>   			continue;
> >>   
> >>   		if (HAS_LMEM(dev_priv)) {
> >> -			ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> >> +			unsigned int flags = obj->flags;
> >> +
> >> +			/*
> >> +			 * For this type of buffer we need to able to read from the CPU
> >> +			 * the clear color value found in the buffer, hence we need to
> >> +			 * ensure it is always in the mappable part of lmem, if this is
> >> +			 * a small-bar device.
> >> +			 */
> >> +			if (intel_fb_rc_ccs_cc_plane(fb) >= 0)
> >> +				flags &= ~I915_BO_ALLOC_GPU_ONLY;
> >> +			ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0,
> >> +							flags);
> >>   			if (ret)
> >>   				continue;
> >>   		}
> >> @@ -156,8 +167,17 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> >>   	ret = i915_gem_object_lock(obj, &ww);
> >>   	if (!ret && phys_cursor)
> >>   		ret = i915_gem_object_attach_phys(obj, alignment);
> >> -	else if (!ret && HAS_LMEM(dev_priv))
> >> +	else if (!ret && HAS_LMEM(dev_priv)) {
> >> +		/*
> >> +		 * For this type of ccs buffer we need to able to read from the
> >> +		 * CPU the clear color value found in the buffer, which might
> >> +		 * require moving to the mappable part of lmem first, but here
> >> +		 * we should be using dpt for this.
> >> +		 */
> >> +		WARN_ON_ONCE(intel_fb_rc_ccs_cc_plane(fb) >= 0);
> > 
> > DPT isn't availalable on DG1.
> 
> Hmm, does it also support the DG2_RC_CCS_CC modifier?

I believe it should support I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC.

> 
> > 
> >> +
> >>   		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> >> +	}
> >>   	if (!ret)
> >>   		ret = i915_gem_object_pin_pages(obj);
> >>   	if (ret)
> >> -- 
> >> 2.37.3
> > 

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v4 2/5] drm/i915/display: handle migration for dpt

[Ville Syrjälä]

On Tue, Oct 04, 2022 at 12:54:33PM +0100, Matthew Auld wrote:
> On 04/10/2022 12:22, Ville Syrjälä wrote:
> > On Tue, Oct 04, 2022 at 11:33:08AM +0100, Matthew Auld wrote:
> >> On platforms like DG2, it looks like the dpt path here is missing the
> >> migrate-to-lmem step on discrete platforms.
> >>
> >> Fixes: 33e7a975103c ("drm/i915/xelpd: First stab at DPT support")
> >> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> >> Cc: Jianshui Yu <jianshui.yu@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Nirmoy Das <nirmoy.das@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_fb_pin.c | 23 ++++++++++++++++++---
> >>   1 file changed, 20 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> >> index 0cd9e8cb078b..32206bd359da 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> >> @@ -26,10 +26,17 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
> >>   	struct drm_device *dev = fb->dev;
> >>   	struct drm_i915_private *dev_priv = to_i915(dev);
> >>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >> +	struct i915_gem_ww_ctx ww;
> >>   	struct i915_vma *vma;
> >>   	u32 alignment;
> >>   	int ret;
> >>   
> >> +	/*
> >> +	 * We are not syncing against the binding (and potential migrations)
> >> +	 * below, so this vm must never be async.
> >> +	*/
> >> +	GEM_WARN_ON(vm->bind_async_flags);
> > 
> > Not sure why this is different between the dpt and non-dpt paths?
> 
> It looks like dpt is using vma_pin() below which doesn't have the 
> wait_for_bind() stuff, like we do for ggtt_pin().

So I guess more accident than by design.

> 
> > 
> >> +
> >>   	if (WARN_ON(!i915_gem_object_is_framebuffer(obj)))
> >>   		return ERR_PTR(-EINVAL);
> >>   
> >> @@ -37,10 +44,20 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
> >>   
> >>   	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
> >>   
> >> -	ret = i915_gem_object_lock_interruptible(obj, NULL);
> >> -	if (!ret) {
> >> +	for_i915_gem_ww(&ww, ret, true) {
> >> +		ret = i915_gem_object_lock(obj, &ww);
> >> +		if (ret)
> >> +			continue;
> >> +
> >> +		if (HAS_LMEM(dev_priv)) {
> >> +			ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> >> +			if (ret)
> >> +				continue;
> >> +		}
> >> +
> >>   		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
> >> -		i915_gem_object_unlock(obj);
> >> +		if (ret)
> >> +			continue;
> >>   	}
> > 
> > The non-dpt path has the whole thing under the same lock.
> > Is there a reason we're not doing the same thing for both?
> > 
> > I guess some kind of unification effort would be nice to
> > avoid the codepaths diverging for no good reason.
> 
> Can do, I'll take a look.

BTW I'm not saying we have to do it right now, but at least something
to keep in mind. And to that end would be nice to understand what are
the real differences vs. differnces due to accidents of the development
history. It would be nice to not have to maintain two totally separate
codepaths if possible.

> > 
> > Maybe even some refactoring would be nice to share more code,
> > but IIRC all the fence/mappable stuff in the lower levels
> > of the ggtt paths is what got in the way of just reusing
> > more of the ggtt code directly.
> > 
> >>   	if (ret) {
> >>   		vma = ERR_PTR(ret);
> >> -- 
> >> 2.37.3
> > 

-- 
Ville Syrjälä
Intel