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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 95089C61DA4 for ; Mon, 6 Feb 2023 09:36:07 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7099710E33F; Mon, 6 Feb 2023 09:36:02 +0000 (UTC) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9F96A10E16B; Mon, 6 Feb 2023 09:35:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1675676159; x=1707212159; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=OV4hu9Cv4oGEnss556qD7ZX4r/ehOr4vxxDxMa+/ABo=; b=Wfbjp2AIM3/y2oVw5ILHnF1Kq8tZ5DhZAScTXfT05edJthfId7wChJ7S G96AVGMw+a1U1W3SoZQ/xvKSpuU0PkSUFr726Zqudz+c6UIN8OXP2Wnif gqUa5l72c32TjHE6daTlIDUPst4rkiZ94NN0IYIJ2Om4BalBOB1vjjhyn Zsxx+zitbsnOtJKjj6hntXjN8MH4hbAv2pJdiaO/5MSib1Ej40DxLAFyL EXiGXBRfzqYmgwduB84WeCzTvnEaGBXbru08ZTE60pkf2H+qxygVTNpLY 9ahguB4wap75to8vwKea3rTZjQ7wZ6Rm2qkkbJY04b+cXyCrgZa7MM5vU A==; X-IronPort-AV: E=McAfee;i="6500,9779,10612"; a="330458337" X-IronPort-AV: E=Sophos;i="5.97,276,1669104000"; d="scan'208";a="330458337" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Feb 2023 01:35:59 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10612"; a="668360569" X-IronPort-AV: E=Sophos;i="5.97,276,1669104000"; d="scan'208";a="668360569" Received: from vastrong-mobl.amr.corp.intel.com (HELO [10.213.203.226]) ([10.213.203.226]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Feb 2023 01:35:54 -0800 Message-ID: Date: Mon, 6 Feb 2023 09:35:52 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Content-Language: en-US To: Rob Clark , dri-devel@lists.freedesktop.org, Matthew Brost References: <20230203164937.4035503-1-robdclark@gmail.com> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Intel-gfx] [PATCH] drm/i915: Move fd_install after last use of fence X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rob Clark , =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= , "Jason A. Donenfeld" , Andrzej Hajda , jason.ekstrand@collabora.com, intel-gfx@lists.freedesktop.org, open list , Matthew Auld , Daniel Vetter , Rodrigo Vivi , David Airlie , Nirmoy Das Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On 03/02/2023 18:15, Rob Clark wrote: > On Fri, Feb 3, 2023 at 8:49 AM Rob Clark wrote: >> >> From: Rob Clark >> >> Because eb_composite_fence_create() drops the fence_array reference >> after creation of the sync_file, only the sync_file holds a ref to the >> fence. But fd_install() makes that reference visable to userspace, so >> it must be the last thing we do with the fence. >> > > Fixes: 00dae4d3d35d ("drm/i915: Implement SINGLE_TIMELINE with a syncobj (v4)") This is correct and the fix looks good to me. Reviewed-by: Tvrtko Ursulin CI is green so I will merge it, thanks again for a fix Rob! Followup up question for Matthew Brost however is whether the composite fence flow could be simplified. This block here comes late in i915_gem_do_execbuffer and may mislead the user the composite fence is held to the end of the function: if (!out_fence && eb.composite_fence) dma_fence_put(eb.composite_fence); Question is would it work to remove the !out_fence condition from here, and remove "consumption" of the reference from eb_composite_fence_create success path. Regards, Tvrtko >> Signed-off-by: Rob Clark >> --- >> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> index f266b68cf012..0f2e056c02dd 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> @@ -3476,38 +3476,38 @@ i915_gem_do_execbuffer(struct drm_device *dev, >> >> err_request: >> eb_requests_get(&eb); >> err = eb_requests_add(&eb, err); >> >> if (eb.fences) >> signal_fence_array(&eb, eb.composite_fence ? >> eb.composite_fence : >> &eb.requests[0]->fence); >> >> + if (unlikely(eb.gem_context->syncobj)) { >> + drm_syncobj_replace_fence(eb.gem_context->syncobj, >> + eb.composite_fence ? >> + eb.composite_fence : >> + &eb.requests[0]->fence); >> + } >> + >> if (out_fence) { >> if (err == 0) { >> fd_install(out_fence_fd, out_fence->file); >> args->rsvd2 &= GENMASK_ULL(31, 0); /* keep in-fence */ >> args->rsvd2 |= (u64)out_fence_fd << 32; >> out_fence_fd = -1; >> } else { >> fput(out_fence->file); >> } >> } >> >> - if (unlikely(eb.gem_context->syncobj)) { >> - drm_syncobj_replace_fence(eb.gem_context->syncobj, >> - eb.composite_fence ? >> - eb.composite_fence : >> - &eb.requests[0]->fence); >> - } >> - >> if (!out_fence && eb.composite_fence) >> dma_fence_put(eb.composite_fence); >> >> eb_requests_put(&eb); >> >> err_vma: >> eb_release_vmas(&eb, true); >> WARN_ON(err == -EDEADLK); >> i915_gem_ww_ctx_fini(&eb.ww); >> >> -- >> 2.38.1 >>