[Intel-gfx] [PATCH 1/2] drm/i915: clear the shadow batch

[Intel-gfx] [PATCH 1/2] drm/i915: clear the shadow batch

[Matthew Auld]

The shadow batch is an internal object, which doesn't have any page
clearing, and since the batch_len can be smaller than the object, we
should take care to clear it.

Testcase: igt/gen9_exec_parse/shadow-peek
Fixes: 4f7af1948abc ("drm/i915: Support ro ppgtt mapped cmdparser shadow buffers")
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 8d88402387bd..ff3a0b8ccdd5 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1147,6 +1147,13 @@ static u32 *copy_batch(struct drm_i915_gem_object *dst_obj,
 	if (IS_ERR(dst))
 		return dst;
 
+	if (length < dst_obj->base.size) {
+		memset32(dst + length, 0,
+			 (dst_obj->base.size - length) / sizeof(u32));
+		__i915_gem_object_flush_map(dst_obj, length,
+					    dst_obj->base.size - length);
+	}
+
 	ret = i915_gem_object_pin_pages(src_obj);
 	if (ret) {
 		i915_gem_object_unpin_map(dst_obj);
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: clear the gpu reloc batch

[Matthew Auld]

The reloc batch is short lived but can exist in the user visible ppGTT,
and since it's backed by an internal object, which lacks page clearing,
we should take care to clear it upfront.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 0cf9e79325a8..a4ecd4b4e874 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1295,6 +1295,9 @@ static int __reloc_gpu_alloc(struct i915_execbuffer *eb,
 		goto err_pool;
 	}
 
+	memset64((void*)cmd, 0, pool->obj->base.size / sizeof(u64));
+	i915_gem_object_flush_map(pool->obj);
+
 	batch = i915_vma_instance(pool->obj, vma->vm, NULL);
 	if (IS_ERR(batch)) {
 		err = PTR_ERR(batch);
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: clear the gpu reloc batch

[Chris Wilson]

Quoting Matthew Auld (2020-12-24 14:34:55)
> The reloc batch is short lived but can exist in the user visible ppGTT,
> and since it's backed by an internal object, which lacks page clearing,
> we should take care to clear it upfront.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 0cf9e79325a8..a4ecd4b4e874 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1295,6 +1295,9 @@ static int __reloc_gpu_alloc(struct i915_execbuffer *eb,
>                 goto err_pool;
>         }
>  
> +       memset64((void*)cmd, 0, pool->obj->base.size / sizeof(u64));

Might as well make it memset32 to avoid the cast. I'm willing to bet
it's in the noise by that point on 4096+ object.

> +       i915_gem_object_flush_map(pool->obj);

We really don't want to have to flush it twice. Remove the limit on the
flush_map before submission.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: clear the shadow batch

[Chris Wilson]

Quoting Matthew Auld (2020-12-24 14:34:54)
> The shadow batch is an internal object, which doesn't have any page
> clearing, and since the batch_len can be smaller than the object, we
> should take care to clear it.
> 
> Testcase: igt/gen9_exec_parse/shadow-peek
> Fixes: 4f7af1948abc ("drm/i915: Support ro ppgtt mapped cmdparser shadow buffers")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 8d88402387bd..ff3a0b8ccdd5 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -1147,6 +1147,13 @@ static u32 *copy_batch(struct drm_i915_gem_object *dst_obj,
>         if (IS_ERR(dst))
>                 return dst;
>  
> +       if (length < dst_obj->base.size) {
> +               memset32(dst + length, 0,
> +                        (dst_obj->base.size - length) / sizeof(u32));
> +               __i915_gem_object_flush_map(dst_obj, length,
> +                                           dst_obj->base.size - length);
> +       }

I feel like we might as well remove the shadow_needs_clflush() and just
do a i915_gem_object_flush_map(shadow->obj) after parsing.

Then for simple chronological ordering, we can place this at the end of
copy_batch

memset32(dst + length, 0, (dst_obj->base.size - length) / sizeof(u32));

The cost of one more function call along here? Unnoticable.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] drm/i915: clear the shadow batch

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: clear the shadow batch
URL   : https://patchwork.freedesktop.org/series/85204/
State : failure

== Summary ==

Applying: drm/i915: clear the shadow batch
Applying: drm/i915: clear the gpu reloc batch
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 drm/i915: clear the gpu reloc batch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx