From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: airlied@redhat.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/5] drm/vmwgfx: unbind in vmw_ttm_unpopulate
Date: Fri, 23 Jul 2021 11:13:09 +0200 [thread overview]
Message-ID: <c83ebc42-567c-4f4c-d6da-53ff21739222@gmail.com> (raw)
In-Reply-To: <YPqCBUDiibBWUs2/@phenom.ffwll.local>
Am 23.07.21 um 10:47 schrieb Daniel Vetter:
> On Thu, Jul 22, 2021 at 02:41:23PM +0200, Christian König wrote:
>> Doing this in vmw_ttm_destroy() is to late.
>>
>> It turned out that this is not a good idea at all because it leaves pointers
>> to freed up system memory pages in the GART tables of the drivers.
> So I wanted to review this series, and I can't reconcile your claim here
> with the demidlayering Dave has done. The driver patches here don't
> ouright undo what Dave has done, but that means the bug has been
> preexisting since forever (or is due to some other change?), and your
> commit message is a bit confusing here.
>
> The final patch just undoes the demidlayering from Dave, and I really
> don't see where there's even a functional change there.
>
> And even these patches here don't really change a hole lot with the
> calling sequence for at least final teardown: ttm_tt_destroy_common calls
> ttm_tt_unpopulate as the first thing, so at least there there's no change.
>
> Can you pls elaborate more clearly what exactly you're fixing and what
> exactly needs to be reordered and where this bug is from (commit sha1)? As
> is I'm playing detective and the evidence presented is extremely since and
> I can't reconcile it at all.
>
> I mean I know you don't like typing commit message and documentation, but
> it does get occasionally rather frustrating on the reviewer side if I have
> to interpolate between some very sparse hints for this stuff :-/
Yeah, when have seen the history it's rather obvious what's wrong here
and I expected Dave to review it himself.
Previously we had three states in TTM for a tt object: Allocated ->
Populated -> Bound which on destruction where done in the order unbind
-> unpopulate -> free.
Dave moved handling of the bound state into the drivers since it is
basically a driver decision and not a TTM decision what should be bound
and what not (that part perfectly makes sense).
The problem is that he also moved doing the unbind into the free
callback instead of the unpopulate callback. This result in stale page
pointers in the GART if that unpopulate operation isn't immediately
followed by a free.
Thinking more about it if we do populated->unpopulated->populated then
we would also have stale pointers to the old pages which is even worse.
This is also not de-midlayering since we already have a proper
ttm_tt_init()/ttm_tt_fini() functions which should work nicely for the
tt object.
Christian.
> -Daniel
>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
>> index b0973c27e774..904031d03dbe 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
>> @@ -526,14 +526,9 @@ static void vmw_ttm_destroy(struct ttm_device *bdev, struct ttm_tt *ttm)
>> struct vmw_ttm_tt *vmw_be =
>> container_of(ttm, struct vmw_ttm_tt, dma_ttm);
>>
>> - vmw_ttm_unbind(bdev, ttm);
>> ttm_tt_destroy_common(bdev, ttm);
>> vmw_ttm_unmap_dma(vmw_be);
>> - if (vmw_be->dev_priv->map_mode == vmw_dma_alloc_coherent)
>> - ttm_tt_fini(&vmw_be->dma_ttm);
>> - else
>> - ttm_tt_fini(ttm);
>> -
>> + ttm_tt_fini(ttm);
>> if (vmw_be->mob)
>> vmw_mob_destroy(vmw_be->mob);
>>
>> @@ -578,6 +573,8 @@ static void vmw_ttm_unpopulate(struct ttm_device *bdev,
>> dma_ttm);
>> unsigned int i;
>>
>> + vmw_ttm_unbind(bdev, ttm);
>> +
>> if (vmw_tt->mob) {
>> vmw_mob_destroy(vmw_tt->mob);
>> vmw_tt->mob = NULL;
>> --
>> 2.25.1
>>
next prev parent reply other threads:[~2021-07-23 9:13 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-22 12:41 [PATCH 1/5] drm/vmwgfx: unbind in vmw_ttm_unpopulate Christian König
2021-07-22 12:41 ` [PATCH 2/5] drm/amdgpu: unbind in amdgpu_ttm_tt_unpopulate Christian König
2021-07-22 12:41 ` [PATCH 3/5] drm/nouveau: unbind in nouveau_ttm_tt_unpopulate Christian König
2021-07-22 12:41 ` [PATCH 4/5] drm/radeon: unbind in radeon_ttm_tt_unpopulate() Christian König
2021-07-22 12:41 ` [PATCH 5/5] drm/ttm: revert "flip tt destroy ordering." Christian König
2021-07-23 8:47 ` [PATCH 1/5] drm/vmwgfx: unbind in vmw_ttm_unpopulate Daniel Vetter
2021-07-23 9:13 ` Christian König [this message]
2021-07-23 9:21 ` Daniel Vetter
2021-07-23 9:40 ` Christian König
2021-07-26 0:03 ` Dave Airlie
2021-07-26 19:35 ` Christian König
2021-07-26 20:03 ` Dave Airlie
2021-07-27 9:28 ` Daniel Vetter
2021-07-28 11:18 ` Christian König
-- strict thread matches above, loose matches on Subject: below --
2021-07-28 13:05 Christian König
2021-08-23 11:05 ` Christian König
2021-08-23 11:15 ` Thomas Hellström
2021-08-26 8:49 ` Daniel Vetter
2021-08-26 10:11 ` Christian König
2021-08-26 12:50 ` Daniel Vetter
2021-07-22 8:55 Christian König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c83ebc42-567c-4f4c-d6da-53ff21739222@gmail.com \
--to=ckoenig.leichtzumerken@gmail.com \
--cc=airlied@redhat.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.