[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <d0f293a4-18fc-4c82-8dfc-286c4913948b@redhat.com>
Date: Sat, 16 Apr 2022 15:25:20 -0700
From: Tom Rix <trix@...hat.com>
To: Joe Perches <joe@...ches.com>, jani.nikula@...ux.intel.com,
joonas.lahtinen@...ux.intel.com, rodrigo.vivi@...el.com,
tvrtko.ursulin@...ux.intel.com, airlied@...ux.ie, daniel@...ll.ch
Cc: intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [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
>
>
Powered by blists - more mailing lists