Re: [Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization
On 4/16/22 2:04 PM, Joe Perches wrote:
On Sat, 2022-04-16 at 13:48 -0700, Tom Rix wrote:
On 4/16/22 11:33 AM, Joe Perches wrote:
On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
In insert_mappable_node(), the parameter node is
cleared late in node's use with memset.
insert_mappable_node() is a singleton, called only
from i915_gem_gtt_prepare() which itself is only
called by i915_gem_gtt_pread() and
i915_gem_gtt_pwrite_fast() where the definition of
node originates.
Instead of using memset, initialize node to 0 at it's
definitions.
trivia: /it's/its/
Only reason _not_ to do this is memset is guaranteed to
zero any padding that might go to userspace.
But it doesn't seem there is any padding anyway nor is
the struct available to userspace.
So this seems fine though it might increase overall code
size a tiny bit.
I do have a caveat: see below:
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
[]
@@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
drm_i915_gem_object *obj,
goto err_ww;
} else if (!IS_ERR(vma)) {
node->start = i915_ggtt_offset(vma);
- node->flags = 0;
Why is this unneeded?
node = {} initializes all of node's elements to 0, including this one.
true, but could the call to insert_mappable_node combined with the
retry goto in i915_gem_gtt_prepare set this to non-zero?
Yikes!
I'll add that back.
Thanks for pointing it out.
Tom
Re: [Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization
On Sat, 2022-04-16 at 13:48 -0700, Tom Rix wrote:
> On 4/16/22 11:33 AM, Joe Perches wrote:
> > On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
> > > In insert_mappable_node(), the parameter node is
> > > cleared late in node's use with memset.
> > > insert_mappable_node() is a singleton, called only
> > > from i915_gem_gtt_prepare() which itself is only
> > > called by i915_gem_gtt_pread() and
> > > i915_gem_gtt_pwrite_fast() where the definition of
> > > node originates.
> > >
> > > Instead of using memset, initialize node to 0 at it's
> > > definitions.
> > trivia: /it's/its/
> >
> > Only reason _not_ to do this is memset is guaranteed to
> > zero any padding that might go to userspace.
> >
> > But it doesn't seem there is any padding anyway nor is
> > the struct available to userspace.
> >
> > So this seems fine though it might increase overall code
> > size a tiny bit.
> >
> > I do have a caveat: see below:
> >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > []
> > > @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
> > > drm_i915_gem_object *obj,
> > > goto err_ww;
> > > } else if (!IS_ERR(vma)) {
> > > node->start = i915_ggtt_offset(vma);
> > > - node->flags = 0;
> > Why is this unneeded?
>
> node = {} initializes all of node's elements to 0, including this one.
true, but could the call to insert_mappable_node combined with the
retry goto in i915_gem_gtt_prepare set this to non-zero?
Re: [Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization
On 4/16/22 11:33 AM, Joe Perches wrote:
On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
In insert_mappable_node(), the parameter node is
cleared late in node's use with memset.
insert_mappable_node() is a singleton, called only
from i915_gem_gtt_prepare() which itself is only
called by i915_gem_gtt_pread() and
i915_gem_gtt_pwrite_fast() where the definition of
node originates.
Instead of using memset, initialize node to 0 at it's
definitions.
trivia: /it's/its/
Only reason _not_ to do this is memset is guaranteed to
zero any padding that might go to userspace.
But it doesn't seem there is any padding anyway nor is
the struct available to userspace.
So this seems fine though it might increase overall code
size a tiny bit.
I do have a caveat: see below:
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
[]
@@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
drm_i915_gem_object *obj,
goto err_ww;
} else if (!IS_ERR(vma)) {
node->start = i915_ggtt_offset(vma);
- node->flags = 0;
Why is this unneeded?
node = {} initializes all of node's elements to 0, including this one.
Tom
from: drm_mm_insert_node_in_range which can set node->flags
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, >flags);
Re: [Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization
On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
> In insert_mappable_node(), the parameter node is
> cleared late in node's use with memset.
> insert_mappable_node() is a singleton, called only
> from i915_gem_gtt_prepare() which itself is only
> called by i915_gem_gtt_pread() and
> i915_gem_gtt_pwrite_fast() where the definition of
> node originates.
>
> Instead of using memset, initialize node to 0 at it's
> definitions.
trivia: /it's/its/
Only reason _not_ to do this is memset is guaranteed to
zero any padding that might go to userspace.
But it doesn't seem there is any padding anyway nor is
the struct available to userspace.
So this seems fine though it might increase overall code
size a tiny bit.
I do have a caveat: see below:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
[]
> @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
> drm_i915_gem_object *obj,
> goto err_ww;
> } else if (!IS_ERR(vma)) {
> node->start = i915_ggtt_offset(vma);
> - node->flags = 0;
Why is this unneeded?
from: drm_mm_insert_node_in_range which can set node->flags
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, >flags);
[Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization
In insert_mappable_node(), the parameter node is
cleared late in node's use with memset.
insert_mappable_node() is a singleton, called only
from i915_gem_gtt_prepare() which itself is only
called by i915_gem_gtt_pread() and
i915_gem_gtt_pwrite_fast() where the definition of
node originates.
Instead of using memset, initialize node to 0 at it's
definitions. And remove unneeded clearing of the flags
element.
Signed-off-by: Tom Rix
---
drivers/gpu/drm/i915/i915_gem.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2e10187cd0a0..7dbd0b325c43 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -69,7 +69,6 @@ insert_mappable_node(struct i915_ggtt *ggtt, struct
drm_mm_node *node, u32 size)
if (err)
return err;
- memset(node, 0, sizeof(*node));
err = drm_mm_insert_node_in_range(>vm.mm, node,
size, 0, I915_COLOR_UNEVICTABLE,
0, ggtt->mappable_end,
@@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
drm_i915_gem_object *obj,
goto err_ww;
} else if (!IS_ERR(vma)) {
node->start = i915_ggtt_offset(vma);
- node->flags = 0;
} else {
ret = insert_mappable_node(ggtt, node, PAGE_SIZE);
if (ret)
@@ -381,7 +379,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
intel_wakeref_t wakeref;
- struct drm_mm_node node;
+ struct drm_mm_node node = {};
void __user *user_data;
struct i915_vma *vma;
u64 remain, offset;
@@ -538,7 +536,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
struct intel_runtime_pm *rpm = >runtime_pm;
intel_wakeref_t wakeref;
- struct drm_mm_node node;
+ struct drm_mm_node node = {};
struct i915_vma *vma;
u64 remain, offset;
void __user *user_data;
--
2.27.0
