[Intel-gfx] [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[ankitprasad . r . sharma]

From: Ankitprasad Sharma 

This patch adds support for clearing buffer objects via CPU/GTT. This
is particularly useful for clearing out the non shmem backed objects.
Currently intend to use this only for buffers allocated from stolen
region.

v2: Added kernel doc for i915_gem_clear_object(), corrected/removed
variable assignments (Tvrtko)

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma 
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 39 +++
 2 files changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e0f3f05..8db905a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2820,6 +2820,7 @@ int i915_gem_obj_prepare_shmem_read(struct 
drm_i915_gem_object *obj,
int *needs_clflush);
 
 int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
+int i915_gem_clear_object(struct drm_i915_gem_object *obj);
 
 static inline int __sg_page_count(struct scatterlist *sg)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1ac57ec..94533bf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5139,3 +5139,42 @@ fail:
drm_gem_object_unreference(>base);
return ERR_PTR(ret);
 }
+
+/**
+ * i915_gem_clear_object() - Clear buffer object via CPU/GTT
+ * @obj: Buffer object to be cleared
+ *
+ * Return: 0 - success, non-zero - failure
+ */
+int i915_gem_clear_object(struct drm_i915_gem_object *obj)
+{
+   int ret;
+   char __iomem *base;
+   size_t size = obj->base.size;
+   struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
+
+   WARN_ON(!mutex_is_locked(>base.dev->struct_mutex));
+   ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
+   if (ret)
+   return ret;
+
+   ret = i915_gem_object_put_fence(obj);
+   if (ret)
+   goto unpin;
+
+   /* Get the CPU virtual address of the buffer */
+   base = ioremap_wc(dev_priv->gtt.mappable_base +
+   i915_gem_obj_ggtt_offset(obj), size);
+   if (base == NULL) {
+   DRM_ERROR("Mapping of gem object to CPU failed!\n");
+   ret = -ENOSPC;
+   goto unpin;
+   }
+
+   memset_io(base, 0, size);
+
+   iounmap(base);
+unpin:
+   i915_gem_object_ggtt_unpin(obj);
+   return ret;
+}
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Tvrtko Ursulin]

On 09/15/2015 09:33 AM, ankitprasad.r.sha...@intel.com wrote:

From: Ankitprasad Sharma 

This patch adds support for clearing buffer objects via CPU/GTT. This
is particularly useful for clearing out the non shmem backed objects.
Currently intend to use this only for buffers allocated from stolen
region.

v2: Added kernel doc for i915_gem_clear_object(), corrected/removed
variable assignments (Tvrtko)

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma 
---
  drivers/gpu/drm/i915/i915_drv.h |  1 +
  drivers/gpu/drm/i915/i915_gem.c | 39 +++
  2 files changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e0f3f05..8db905a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2820,6 +2820,7 @@ int i915_gem_obj_prepare_shmem_read(struct 
drm_i915_gem_object *obj,
int *needs_clflush);

  int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
+int i915_gem_clear_object(struct drm_i915_gem_object *obj);

  static inline int __sg_page_count(struct scatterlist *sg)
  {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1ac57ec..94533bf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5139,3 +5139,42 @@ fail:
drm_gem_object_unreference(>base);
return ERR_PTR(ret);
  }
+
+/**
+ * i915_gem_clear_object() - Clear buffer object via CPU/GTT
+ * @obj: Buffer object to be cleared
+ *
+ * Return: 0 - success, non-zero - failure
+ */
+int i915_gem_clear_object(struct drm_i915_gem_object *obj)
+{
+   int ret;
+   char __iomem *base;
+   size_t size = obj->base.size;
+   struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
+
+   WARN_ON(!mutex_is_locked(>base.dev->struct_mutex));
+   ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
+   if (ret)
+   return ret;
+
+   ret = i915_gem_object_put_fence(obj);
+   if (ret)
+   goto unpin;
+
+   /* Get the CPU virtual address of the buffer */
+   base = ioremap_wc(dev_priv->gtt.mappable_base +
+   i915_gem_obj_ggtt_offset(obj), size);
+   if (base == NULL) {
+   DRM_ERROR("Mapping of gem object to CPU failed!\n");
+   ret = -ENOSPC;
+   goto unpin;
+   }
+
+   memset_io(base, 0, size);
+
+   iounmap(base);
+unpin:
+   i915_gem_object_ggtt_unpin(obj);
+   return ret;
+}



Reviewed-by: Tvrtko Ursulin 

Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Chris Wilson]

On Wed, Jul 22, 2015 at 04:01:33PM +0100, Tvrtko Ursulin wrote:
 +ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
 +if (ret) {
 +DRM_ERROR(Mapping of gem object to GTT failed\n);
 
 Maybe end the sentence with ! ?

Or better yet, don't do anything. The error is propagate back to the
user, and is going to be -EINTR.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Tvrtko Ursulin]

On 07/22/2015 04:06 PM, Chris Wilson wrote:

On Wed, Jul 22, 2015 at 04:01:33PM +0100, Tvrtko Ursulin wrote:

This I don't understand. Why it is OK to release a fence and never
re-establish it? Could that surprise some callers?


We always call get_fence() before we need it (and pin it as required),
because it can be revoked at anytime as they are a scarce resource.


Oookay, and why it is important to release it here? Does it warrant a 
comment or everyone knows this?


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Chris Wilson]

On Wed, Jul 22, 2015 at 04:01:33PM +0100, Tvrtko Ursulin wrote:
 This I don't understand. Why it is OK to release a fence and never
 re-establish it? Could that surprise some callers?

We always call get_fence() before we need it (and pin it as required),
because it can be revoked at anytime as they are a scarce resource.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Chris Wilson]

On Wed, Jul 22, 2015 at 04:16:20PM +0100, Tvrtko Ursulin wrote:
 
 On 07/22/2015 04:06 PM, Chris Wilson wrote:
 On Wed, Jul 22, 2015 at 04:01:33PM +0100, Tvrtko Ursulin wrote:
 This I don't understand. Why it is OK to release a fence and never
 re-establish it? Could that surprise some callers?
 
 We always call get_fence() before we need it (and pin it as required),
 because it can be revoked at anytime as they are a scarce resource.
 
 Oookay, and why it is important to release it here? Does it warrant
 a comment or everyone knows this?

There is fun to be had as the destination address is detiled by the
hardware meaning that it is possible for the writes into the last tile
row to be after the GTT address of the object. We should have allocated
sufficient space in the GTT to cover all fenced writes, but that is
actually only true on gen2/3.

There is also the fun that avoiding the fence (besides being a resource
that we may have to wait for and cause pagefaults elsewhere) is a bit
faster than writing through one.

So not using a fence is faster and less error prone.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Tvrtko Ursulin]

Hi,

On 07/22/2015 02:51 PM, ankitprasad.r.sha...@intel.com wrote:

From: Ankitprasad Sharma ankitprasad.r.sha...@intel.com

This patch adds support for clearing buffer objects via CPU/GTT. This
is particularly useful for clearing out the non shmem backed objects.
Currently intend to use this only for buffers allocated from stolen
region.

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma ankitprasad.r.sha...@intel.com
---
  drivers/gpu/drm/i915/i915_drv.h |  1 +
  drivers/gpu/drm/i915/i915_gem.c | 34 ++
  2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea9caf2..f6af9c0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2764,6 +2764,7 @@ int i915_gem_obj_prepare_shmem_read(struct 
drm_i915_gem_object *obj,
int *needs_clflush);

  int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
+int i915_gem_clear_object(struct drm_i915_gem_object *obj);

  static inline int __sg_page_count(struct scatterlist *sg)
  {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a2a4a27..fc434ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5469,3 +5469,37 @@ bool i915_gem_obj_is_pinned(struct drm_i915_gem_object 
*obj)
return false;
  }

+int i915_gem_clear_object(struct drm_i915_gem_object *obj)


Please add kernel doc for the function.


+{
+   int ret = 0;


No need to set to zero.


+   char __iomem *base;
+   int size = obj-base.size;


I think obj-base.size is a size_t at the moment so this could cause 
compiler to warn?



+   struct drm_i915_private *dev_priv = obj-base.dev-dev_private;


Again compiler should warn here due implicit pointer casting? I think 
correct is dev_priv = to_i915(obj-base.dev).



+   unsigned alignment = 0;


Don't even need this as variable since it is used only once.


+
+   ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
+   if (ret) {
+   DRM_ERROR(Mapping of gem object to GTT failed\n);


Maybe end the sentence with ! ?


+   return ret;
+   }
+
+   ret = i915_gem_object_put_fence(obj);
+   if (ret)
+   goto unpin;


This I don't understand. Why it is OK to release a fence and never 
re-establish it? Could that surprise some callers?


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[ankitprasad . r . sharma]

From: Ankitprasad Sharma ankitprasad.r.sha...@intel.com

This patch adds support for clearing buffer objects via CPU/GTT. This
is particularly useful for clearing out the non shmem backed objects.
Currently intend to use this only for buffers allocated from stolen
region.

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma ankitprasad.r.sha...@intel.com
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 34 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea9caf2..f6af9c0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2764,6 +2764,7 @@ int i915_gem_obj_prepare_shmem_read(struct 
drm_i915_gem_object *obj,
int *needs_clflush);
 
 int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
+int i915_gem_clear_object(struct drm_i915_gem_object *obj);
 
 static inline int __sg_page_count(struct scatterlist *sg)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a2a4a27..fc434ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5469,3 +5469,37 @@ bool i915_gem_obj_is_pinned(struct drm_i915_gem_object 
*obj)
return false;
 }
 
+int i915_gem_clear_object(struct drm_i915_gem_object *obj)
+{
+   int ret = 0;
+   char __iomem *base;
+   int size = obj-base.size;
+   struct drm_i915_private *dev_priv = obj-base.dev-dev_private;
+   unsigned alignment = 0;
+
+   ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
+   if (ret) {
+   DRM_ERROR(Mapping of gem object to GTT failed\n);
+   return ret;
+   }
+
+   ret = i915_gem_object_put_fence(obj);
+   if (ret)
+   goto unpin;
+
+   /* Get the CPU virtual address of the buffer */
+   base = ioremap_wc(dev_priv-gtt.mappable_base +
+ i915_gem_obj_ggtt_offset(obj), size);
+   if (base == NULL) {
+   DRM_ERROR(Mapping of gem object to CPU failed\n);
+   ret = -ENOSPC;
+   goto unpin;
+   }
+
+   memset_io(base, 0, size);
+
+   iounmap(base);
+unpin:
+   i915_gem_object_ggtt_unpin(obj);
+   return ret;
+}
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx