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 12DC6C433EF for ; Tue, 14 Jun 2022 20:01:25 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 629D310E4EF; Tue, 14 Jun 2022 20:01:25 +0000 (UTC) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id A830810E4EF; Tue, 14 Jun 2022 20:01:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1655236883; x=1686772883; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=S1DLlztIhrvH9P5+KlXb1RUdgFdcy45/PGP2ufSJfPI=; b=kl/fe63M7yyGw+hc2dWo26yeLgBltk9krGNeCpo5njlaccBqerbckAUO 8YHvq5gNm1cKScezJQ0kXCJeEoiuvwUhe/C7OgPEynhiis314+mXMd8CR 974xL8yMMHUIHfHwLSae74G2iDjzJcIHap36CR4qFZUgPpFq881YMw9ID T4Qr5m/xeDBxRrl7F75tU4BUf+2VdXHqTOy/+3xj7Cr+wLzd/XVc2U16p m1682zHdN3EOfHcT3foTQNjjF7/mWO8LLX5RhZxXQRmll1ioC4sWNqR/B yloM8b5ZCicXRKFgn7lvBY2GzPA7fYzjq9JXJlzWWcJGKj7yaWNrCgG04 g==; X-IronPort-AV: E=McAfee;i="6400,9594,10378"; a="365072446" X-IronPort-AV: E=Sophos;i="5.91,300,1647327600"; d="scan'208";a="365072446" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jun 2022 13:01:23 -0700 X-IronPort-AV: E=Sophos;i="5.91,300,1647327600"; d="scan'208";a="830606857" Received: from adixit-mobl1.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.75.167]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jun 2022 13:01:22 -0700 Date: Tue, 14 Jun 2022 13:01:22 -0700 Message-ID: <87y1xzf20d.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Zhanjun Dong In-Reply-To: <87zgiffbvl.wl-ashutosh.dixit@intel.com> References: <20220602172119.96324-1-zhanjun.dong@intel.com> <87zgiffbvl.wl-ashutosh.dixit@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [Intel-gfx] [PATCH] drm/i915/guc: Check ctx while waiting for response 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: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, alan.previn.teres.alexis@intel.com Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Tue, 14 Jun 2022 09:28:14 -0700, Dixit, Ashutosh wrote: > On Thu, 02 Jun 2022 10:21:19 -0700, Zhanjun Dong wrote: > > > @@ -481,12 +481,14 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) > > #define GUC_CTB_RESPONSE_TIMEOUT_SHORT_MS 10 > > #define GUC_CTB_RESPONSE_TIMEOUT_LONG_MS 1000 > > #define done \ > > - (FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ > > + (!intel_guc_ct_enabled(ct) || FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ > > GUC_HXG_ORIGIN_GUC) > > err = wait_for_us(done, GUC_CTB_RESPONSE_TIMEOUT_SHORT_MS); > > if (err) > > err = wait_for(done, GUC_CTB_RESPONSE_TIMEOUT_LONG_MS); > > #undef done > > + if (!intel_guc_ct_enabled(ct)) > > + err = -ECANCELED; > > Also, I really don't like intel_guc_ct_enabled() being called in two > places. Is there a possibility that intel_guc_ct_enabled() can return false > in the first place (causing the wait to exit) and then return true in the > second place (so we don't return -ECANCELED)? > > Is it possible to change the status of the request to something else from > intel_guc_ct_disable() (or wherever ct->enabled is set to false) rather > than introducing intel_guc_ct_enabled() checks here. Changing the status of > the request when CT goes down would cause the wait's to exit here. And then > we can check that special request status signifying CT went down? I think there are free bits in the request status fields which can be used. But setting the request status say from intel_guc_ct_enabled() might not be straightforward since you have to locate waiting requests and also there may be multiple such requests (waiting in different threads). Maybe an easier way might be to do something like: bool foo(ct, req) { if (!intel_guc_ct_enabled(ct)) { req->status = CT_WENT_AWAY; return true; } return false; } Now in your patch we can substitute foo() instead of !intel_guc_ct_enabled(ct) so that we have: #define done \ (foo() || FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ GUC_HXG_ORIGIN_GUC) And then check for req->status == CT_WENT_AWAY (most likely in ct_send()). 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 1C459C43334 for ; Tue, 14 Jun 2022 20:01:29 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 929D910E617; Tue, 14 Jun 2022 20:01:25 +0000 (UTC) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id A830810E4EF; Tue, 14 Jun 2022 20:01:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1655236883; x=1686772883; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=S1DLlztIhrvH9P5+KlXb1RUdgFdcy45/PGP2ufSJfPI=; b=kl/fe63M7yyGw+hc2dWo26yeLgBltk9krGNeCpo5njlaccBqerbckAUO 8YHvq5gNm1cKScezJQ0kXCJeEoiuvwUhe/C7OgPEynhiis314+mXMd8CR 974xL8yMMHUIHfHwLSae74G2iDjzJcIHap36CR4qFZUgPpFq881YMw9ID T4Qr5m/xeDBxRrl7F75tU4BUf+2VdXHqTOy/+3xj7Cr+wLzd/XVc2U16p m1682zHdN3EOfHcT3foTQNjjF7/mWO8LLX5RhZxXQRmll1ioC4sWNqR/B yloM8b5ZCicXRKFgn7lvBY2GzPA7fYzjq9JXJlzWWcJGKj7yaWNrCgG04 g==; X-IronPort-AV: E=McAfee;i="6400,9594,10378"; a="365072446" X-IronPort-AV: E=Sophos;i="5.91,300,1647327600"; d="scan'208";a="365072446" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jun 2022 13:01:23 -0700 X-IronPort-AV: E=Sophos;i="5.91,300,1647327600"; d="scan'208";a="830606857" Received: from adixit-mobl1.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.75.167]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jun 2022 13:01:22 -0700 Date: Tue, 14 Jun 2022 13:01:22 -0700 Message-ID: <87y1xzf20d.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Zhanjun Dong Subject: Re: [PATCH] drm/i915/guc: Check ctx while waiting for response In-Reply-To: <87zgiffbvl.wl-ashutosh.dixit@intel.com> References: <20220602172119.96324-1-zhanjun.dong@intel.com> <87zgiffbvl.wl-ashutosh.dixit@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII 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-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, alan.previn.teres.alexis@intel.com, michal.wajdeczko@intel.com Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Tue, 14 Jun 2022 09:28:14 -0700, Dixit, Ashutosh wrote: > On Thu, 02 Jun 2022 10:21:19 -0700, Zhanjun Dong wrote: > > > @@ -481,12 +481,14 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) > > #define GUC_CTB_RESPONSE_TIMEOUT_SHORT_MS 10 > > #define GUC_CTB_RESPONSE_TIMEOUT_LONG_MS 1000 > > #define done \ > > - (FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ > > + (!intel_guc_ct_enabled(ct) || FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ > > GUC_HXG_ORIGIN_GUC) > > err = wait_for_us(done, GUC_CTB_RESPONSE_TIMEOUT_SHORT_MS); > > if (err) > > err = wait_for(done, GUC_CTB_RESPONSE_TIMEOUT_LONG_MS); > > #undef done > > + if (!intel_guc_ct_enabled(ct)) > > + err = -ECANCELED; > > Also, I really don't like intel_guc_ct_enabled() being called in two > places. Is there a possibility that intel_guc_ct_enabled() can return false > in the first place (causing the wait to exit) and then return true in the > second place (so we don't return -ECANCELED)? > > Is it possible to change the status of the request to something else from > intel_guc_ct_disable() (or wherever ct->enabled is set to false) rather > than introducing intel_guc_ct_enabled() checks here. Changing the status of > the request when CT goes down would cause the wait's to exit here. And then > we can check that special request status signifying CT went down? I think there are free bits in the request status fields which can be used. But setting the request status say from intel_guc_ct_enabled() might not be straightforward since you have to locate waiting requests and also there may be multiple such requests (waiting in different threads). Maybe an easier way might be to do something like: bool foo(ct, req) { if (!intel_guc_ct_enabled(ct)) { req->status = CT_WENT_AWAY; return true; } return false; } Now in your patch we can substitute foo() instead of !intel_guc_ct_enabled(ct) so that we have: #define done \ (foo() || FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ GUC_HXG_ORIGIN_GUC) And then check for req->status == CT_WENT_AWAY (most likely in ct_send()).