[Intel-gfx] [PATCH v2 1/2] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Intel-gfx] [PATCH v2 1/2] 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.

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   | 11 ++++--
 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 ++-
 5 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
index c86e5d4ee016..f83cf41ddd63 100644
--- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
+++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
@@ -139,9 +139,14 @@ 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))
-		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
-	/* TODO: Do we need to sync when migration becomes async? */
+	else if (!ret && HAS_LMEM(dev_priv)) {
+		unsigned int flags = obj->flags;
+
+		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)
 		ret = i915_gem_object_pin_pages(obj);
 	if (ret)
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 v2 2/2] 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>
---
 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 v2 1/2] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 02:47:30PM +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.
> 
> 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   | 11 ++++--
>  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 ++-
>  5 files changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> index c86e5d4ee016..f83cf41ddd63 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> @@ -139,9 +139,14 @@ 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))
> -		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> -	/* TODO: Do we need to sync when migration becomes async? */

Why is the TODO being removed?

> +	else if (!ret && HAS_LMEM(dev_priv)) {
> +		unsigned int flags = obj->flags;
> +

It might not be super obvious what is going on here, so maybe add
a comment stating the CPU needs to read the clear color from the bo?

> +		if (intel_fb_rc_ccs_cc_plane(fb) >= 0)
> +			flags &= ~I915_BO_ALLOC_GPU_ONLY;

Hmm. Do we require the clear color plane to be in the same bo as the
rest of the planes? I know we require the main and aux to be in the
same bo, but dunno why we would require that also of the clear color
plane (apart from being lazy perhaps).

> +		ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0,
> +						flags);
> +	}
>  	if (!ret)
>  		ret = i915_gem_object_pin_pages(obj);
>  	if (ret)
> 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

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 05:00:40PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 30, 2022 at 02:47:30PM +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.
> > 
> > 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   | 11 ++++--
> >  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 ++-
> >  5 files changed, 53 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > index c86e5d4ee016..f83cf41ddd63 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > @@ -139,9 +139,14 @@ 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))
> > -		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> > -	/* TODO: Do we need to sync when migration becomes async? */
> 
> Why is the TODO being removed?
> 
> > +	else if (!ret && HAS_LMEM(dev_priv)) {
> > +		unsigned int flags = obj->flags;
> > +
> 
> It might not be super obvious what is going on here, so maybe add
> a comment stating the CPU needs to read the clear color from the bo?
> 
> > +		if (intel_fb_rc_ccs_cc_plane(fb) >= 0)
> > +			flags &= ~I915_BO_ALLOC_GPU_ONLY;
> 
> Hmm. Do we require the clear color plane to be in the same bo as the
> rest of the planes? I know we require the main and aux to be in the
> same bo, but dunno why we would require that also of the clear color
> plane (apart from being lazy perhaps).

I guess we must since we call this only once for the whole fb.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Matthew Auld]

On 30/09/2022 15:00, Ville Syrjälä wrote:
> On Fri, Sep 30, 2022 at 02:47:30PM +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.
>>
>> 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   | 11 ++++--
>>   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 ++-
>>   5 files changed, 53 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
>> index c86e5d4ee016..f83cf41ddd63 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
>> @@ -139,9 +139,14 @@ 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))
>> -		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
>> -	/* TODO: Do we need to sync when migration becomes async? */
> 
> Why is the TODO being removed?

Just because we now know we do a fence sync below, when binding into the 
GGTT (this comment was from before we had async moves/migrations). I can 
a make a note of that in the commit message. Or perhaps change the 
comment to "Should we rather make this async, currently we sync below 
which is maybe not optimal?" :)

> 
>> +	else if (!ret && HAS_LMEM(dev_priv)) {
>> +		unsigned int flags = obj->flags;
>> +
> 
> It might not be super obvious what is going on here, so maybe add
> a comment stating the CPU needs to read the clear color from the bo?

Sure.

> 
>> +		if (intel_fb_rc_ccs_cc_plane(fb) >= 0)
>> +			flags &= ~I915_BO_ALLOC_GPU_ONLY;
> 
> Hmm. Do we require the clear color plane to be in the same bo as the
> rest of the planes? I know we require the main and aux to be in the
> same bo, but dunno why we would require that also of the clear color
> plane (apart from being lazy perhaps).

I have no idea :)

> 
>> +		ret = __i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0,
>> +						flags);
>> +	}
>>   	if (!ret)
>>   		ret = i915_gem_object_pin_pages(obj);
>>   	if (ret)
>> 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
> 

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: check memory is mappable in read_from_page

[Das, Nirmoy]

Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

On 9/30/2022 3:47 PM, Matthew Auld wrote:
> 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>
> ---
>   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;

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Das, Nirmoy]

Should this be split into two patch, display+gem if that doesn't seem 
like a code churn.

For non display part Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

On 9/30/2022 3:47 PM, 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.
>
> 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   | 11 ++++--
>   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 ++-
>   5 files changed, 53 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> index c86e5d4ee016..f83cf41ddd63 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> @@ -139,9 +139,14 @@ 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))
> -		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> -	/* TODO: Do we need to sync when migration becomes async? */
> +	else if (!ret && HAS_LMEM(dev_priv)) {
> +		unsigned int flags = obj->flags;
> +
> +		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)
>   		ret = i915_gem_object_pin_pages(obj);
>   	if (ret)
> 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,

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 03:10:41PM +0100, Matthew Auld wrote:
> On 30/09/2022 15:00, Ville Syrjälä wrote:
> > On Fri, Sep 30, 2022 at 02:47:30PM +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.
> >>
> >> 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   | 11 ++++--
> >>   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 ++-
> >>   5 files changed, 53 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> >> index c86e5d4ee016..f83cf41ddd63 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> >> @@ -139,9 +139,14 @@ 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))
> >> -		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> >> -	/* TODO: Do we need to sync when migration becomes async? */
> > 
> > Why is the TODO being removed?
> 
> Just because we now know we do a fence sync below, when binding into the 
> GGTT (this comment was from before we had async moves/migrations). I can 
> a make a note of that in the commit message. Or perhaps change the 
> comment to "Should we rather make this async, currently we sync below 
> which is maybe not optimal?" :)

Shrug. As long as the commit message will get some explanation
we should be good. Could even be a separate patch since it
seems entirely orthogonal to the actual contents of this patch.

-- 
Ville Syrjälä
Intel