From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 03EDCC47084 for ; Tue, 25 May 2021 13:37:41 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id BA5236140F for ; Tue, 25 May 2021 13:37:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BA5236140F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C14AA89CD7; Tue, 25 May 2021 13:37:38 +0000 (UTC) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 39E3189C96; Tue, 25 May 2021 13:37:37 +0000 (UTC) IronPort-SDR: nel6YYkeB8QgMioWpFFmwBw6ig84PA3Nln+iJg9+614XeOm2xkjN4/3GqVDJp6B02ACjP7f+w9 Ti3RyjagcxLg== X-IronPort-AV: E=McAfee;i="6200,9189,9995"; a="202211187" X-IronPort-AV: E=Sophos;i="5.82,328,1613462400"; d="scan'208";a="202211187" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 May 2021 06:37:32 -0700 IronPort-SDR: oeNNfTENPVKCqm0o3j/YOHMpdcM+XqG/TqNd6otZaGDXbIKwsUs/iRKp25e2WaLNp5U0aVJDXO DNZZ1paZhcVw== X-IronPort-AV: E=Sophos;i="5.82,328,1613462400"; d="scan'208";a="476426302" Received: from tmuluk-mobl.ger.corp.intel.com ([10.249.254.198]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 May 2021 06:37:29 -0700 Message-ID: <30dc2d316b643a07babbb3a985b6ff2bbf533345.camel@linux.intel.com> Subject: Re: [Intel-gfx] [PATCH v3 09/12] drm/ttm: Document and optimize ttm_bo_pipeline_gutting() From: Thomas =?ISO-8859-1?Q?Hellstr=F6m?= To: Matthew Auld Date: Tue, 25 May 2021 15:37:27 +0200 In-Reply-To: References: <20210521153253.518037-1-thomas.hellstrom@linux.intel.com> <20210521153253.518037-10-thomas.hellstrom@linux.intel.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Intel Graphics Development , Christian =?ISO-8859-1?Q?K=F6nig?= , ML dri-devel Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Tue, 2021-05-25 at 12:00 +0100, Matthew Auld wrote: > On Fri, 21 May 2021 at 16:33, Thomas Hellström > wrote: > > > > If the bo is idle when calling ttm_bo_pipeline_gutting(), we > > unnecessarily > > create a ghost object and push it out to delayed destroy. > > Fix this by adding a path for idle, and document the function. > > > > Also avoid having the bo end up in a bad state vulnerable to user- > > space > > triggered kernel BUGs if the call to ttm_tt_create() fails. > > > > Finally reuse ttm_bo_pipeline_gutting() in ttm_bo_evict(). > > > > Cc: Christian König > > Signed-off-by: Thomas Hellström > > --- > >  drivers/gpu/drm/ttm/ttm_bo.c      | 20 ++++++------ > >  drivers/gpu/drm/ttm/ttm_bo_util.c | 52 > > ++++++++++++++++++++++++++++--- > >  drivers/gpu/drm/ttm/ttm_tt.c      |  5 +++ > >  include/drm/ttm/ttm_tt.h          | 10 ++++++ > >  4 files changed, 73 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c > > b/drivers/gpu/drm/ttm/ttm_bo.c > > index ca1b098b6a56..a8fa3375b8aa 100644 > > --- a/drivers/gpu/drm/ttm/ttm_bo.c > > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > > @@ -501,10 +501,15 @@ static int ttm_bo_evict(struct > > ttm_buffer_object *bo, > >         bdev->funcs->evict_flags(bo, &placement); > > > >         if (!placement.num_placement && > > !placement.num_busy_placement) { > > -               ttm_bo_wait(bo, false, false); > > +               ret = ttm_bo_wait(bo, true, false); > > +               if (ret) > > +                       return ret; > > > > -               ttm_bo_cleanup_memtype_use(bo); > > -               return ttm_tt_create(bo, false); > > +               /* > > +                * Since we've already synced, this frees backing > > store > > +                * immediately. > > +                */ > > +               return ttm_bo_pipeline_gutting(bo); > >         } > > > >         ret = ttm_bo_mem_space(bo, &placement, &evict_mem, ctx); > > @@ -974,13 +979,8 @@ int ttm_bo_validate(struct ttm_buffer_object > > *bo, > >         /* > >          * Remove the backing store if no placement is given. > >          */ > > -       if (!placement->num_placement && !placement- > > >num_busy_placement) { > > -               ret = ttm_bo_pipeline_gutting(bo); > > -               if (ret) > > -                       return ret; > > - > > -               return ttm_tt_create(bo, false); > > -       } > > +       if (!placement->num_placement && !placement- > > >num_busy_placement) > > +               return ttm_bo_pipeline_gutting(bo); > > > >         /* > >          * Check whether we need to move buffer. > > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c > > b/drivers/gpu/drm/ttm/ttm_bo_util.c > > index 4a7d3d672f9a..7fa9b3a852eb 100644 > > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c > > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c > > @@ -585,26 +585,70 @@ int ttm_bo_move_accel_cleanup(struct > > ttm_buffer_object *bo, > >  } > >  EXPORT_SYMBOL(ttm_bo_move_accel_cleanup); > > > > +/** > > + * ttm_bo_pipeline_gutting - purge the contents of a bo > > + * @bo: The buffer object > > + * > > + * Purge the contents of a bo, async if the bo is not idle. > > + * After a successful call, the bo is left unpopulated in > > + * system placement. The function may wait uninterruptible > > + * for idle on OOM. > > + * > > + * Return: 0 if successful, negative error code on failure. > > + */ > >  int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo) > >  { > >         static const struct ttm_place sys_mem = { .mem_type = > > TTM_PL_SYSTEM }; > >         struct ttm_buffer_object *ghost; > > +       struct ttm_tt *ttm; > >         int ret; > > > > -       ret = ttm_buffer_object_transfer(bo, &ghost); > > +       /* If already idle, no need for ghost object dance. */ > > +       ret = ttm_bo_wait(bo, false, true); > > +       if (ret != -EBUSY) { > > +               if (!bo->ttm) { > > +                       ret = ttm_tt_create(bo, true); > > Why do we now unconditionally add clearing? Below also. Here we've dropped the bo content and we add but do not populate a page vector. Now if someone resurrects this object we obtain new pages and those must be cleared, at least that's the intention. /Thomas