From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: [PATCH] drm/ttm: fix two regressions since move_notify changes Date: Wed, 25 Jan 2012 18:32:55 +0100 Message-ID: <4F203CC7.4010108@shipmail.org> References: <20120124223319.GA10002@homer.localdomain> <1327469662-6248-1-git-send-email-skeggsb@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from GOTHNET-SMTP2.gothnet.se (relay.gothnet.se [82.193.160.251]) by gabe.freedesktop.org (Postfix) with ESMTP id 1FADD9E74C for ; Wed, 25 Jan 2012 09:32:58 -0800 (PST) In-Reply-To: <1327469662-6248-1-git-send-email-skeggsb@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Ben Skeggs Cc: Ben Skeggs , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 01/25/2012 06:34 AM, Ben Skeggs wrote: > From: Ben Skeggs > > Both changes in dc97b3409a790d2a21aac6e5cdb99558b5944119 cause serious > regressions in the nouveau driver. > > move_notify() was originally able to presume that bo->mem is the old node, > and new_mem is the new node. The above commit moves the call to > move_notify() to after move() has been done, which means that now, sometimes, > new_mem isn't the new node at all, bo->mem is, and new_mem points at a > stale, possibly-just-been-killed-by-move node. > > This is clearly not a good situation. This patch reverts this change, and > replaces it with a cleanup in the move() failure path instead. > > The second issue is that the call to move_notify() from cleanup_memtype_use() > causes the TTM ghost objects to get passed into the driver. This is clearly > bad as the driver knows nothing about these "fake" TTM BOs, and ends up > accessing uninitialised memory. > > I worked around this in nouveau's move_notify() hook by ensuring the BO > destructor was nouveau's. I don't particularly like this solution, and > would rather TTM never pass the driver these objects. However, I don't > clearly understand the reason why we're calling move_notify() here anyway > and am happy to work around the problem in nouveau instead of breaking the > behaviour expected by other drivers. > > Signed-off-by: Ben Skeggs > Cc: Jerome Glisse As mentioned in the lengthy email discussion, I don't like the ttm change, but since we don't have time for anything better, Reviewed-by: Thomas Hellstrom > --- > drivers/gpu/drm/nouveau/nouveau_bo.c | 4 ++++ > drivers/gpu/drm/ttm/ttm_bo.c | 17 +++++++++++++---- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > index 724b41a..ec54364 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -812,6 +812,10 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem) > struct nouveau_bo *nvbo = nouveau_bo(bo); > struct nouveau_vma *vma; > > + /* ttm can now (stupidly) pass the driver bos it didn't create... */ > + if (bo->destroy != nouveau_bo_del_ttm) > + return; > + > list_for_each_entry(vma,&nvbo->vma_list, head) { > if (new_mem&& new_mem->mem_type == TTM_PL_VRAM) { > nouveau_vm_map(vma, new_mem->mm_node); > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index 2f0eab6..7c3a57d 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -404,6 +404,9 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > } > } > > + if (bdev->driver->move_notify) > + bdev->driver->move_notify(bo, mem); > + > if (!(old_man->flags& TTM_MEMTYPE_FLAG_FIXED)&& > !(new_man->flags& TTM_MEMTYPE_FLAG_FIXED)) > ret = ttm_bo_move_ttm(bo, evict, no_wait_reserve, no_wait_gpu, mem); > @@ -413,11 +416,17 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > else > ret = ttm_bo_move_memcpy(bo, evict, no_wait_reserve, no_wait_gpu, mem); > > - if (ret) > - goto out_err; > + if (ret) { > + if (bdev->driver->move_notify) { > + struct ttm_mem_reg tmp_mem = *mem; > + *mem = bo->mem; > + bo->mem = tmp_mem; > + bdev->driver->move_notify(bo, mem); > + bo->mem = *mem; > + } > > - if (bdev->driver->move_notify) > - bdev->driver->move_notify(bo, mem); > + goto out_err; > + } > > moved: > if (bo->evicted) {