Messages in this thread |  | | Subject | Re: [PATCH] drm/i915: change node clearing from memset to initialization | From | Tom Rix <> | Date | Sat, 16 Apr 2022 15:25:20 -0700 |
| |
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
> >
|  |