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=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 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 91519C47082 for ; Tue, 8 Jun 2021 08:23:25 +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 585936127A for ; Tue, 8 Jun 2021 08:23:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 585936127A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=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 A894B89D83; Tue, 8 Jun 2021 08:23:24 +0000 (UTC) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id CDC5A89D83; Tue, 8 Jun 2021 08:23:23 +0000 (UTC) IronPort-SDR: xxXkrzJYH3jlX0l8eeREAf2oxfm0+o585elQP0o3pRH62Sa9zb/HHneY4SZc5bvXonRSILseCL wRKYnXQP+dRw== X-IronPort-AV: E=McAfee;i="6200,9189,10008"; a="290428428" X-IronPort-AV: E=Sophos;i="5.83,257,1616482800"; d="scan'208";a="290428428" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jun 2021 01:23:22 -0700 IronPort-SDR: gbNd+rh4H01Ywa9N/HuBzWAjEhINXtVGJTbkMfuPT+MgURndxxW3JPtKp0OEbO2jgY61nesf0v KmsyAcVw0ewA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.83,257,1616482800"; d="scan'208";a="476517412" Received: from irvmail001.ir.intel.com ([10.43.11.63]) by FMSMGA003.fm.intel.com with ESMTP; 08 Jun 2021 01:23:20 -0700 Received: from [10.249.139.189] (mwajdecz-MOBL.ger.corp.intel.com [10.249.139.189]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id 1588NJcn012500; Tue, 8 Jun 2021 09:23:19 +0100 Subject: Re: [PATCH 03/13] drm/i915/guc: Update CTB response status definition To: Daniele Ceraolo Spurio , Matthew Brost , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org References: <20210607180356.165785-1-matthew.brost@intel.com> <20210607180356.165785-4-matthew.brost@intel.com> <6a89048d-4075-ccdc-1bc7-cbcc06c0e972@intel.com> From: Michal Wajdeczko Message-ID: <7e32ac2f-7dea-5116-33e3-46ed18fe5983@intel.com> Date: Tue, 8 Jun 2021 10:23:19 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.10.2 MIME-Version: 1.0 In-Reply-To: <6a89048d-4075-ccdc-1bc7-cbcc06c0e972@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US 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: john.c.harrison@intel.com Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 08.06.2021 02:05, Daniele Ceraolo Spurio wrote: > > > On 6/7/2021 11:03 AM, Matthew Brost wrote: >> From: Michal Wajdeczko >> >> Format of the STATUS dword in CTB response message now follows >> definition of the HXG header. Update our code and remove any >> obsolete legacy definitions. > > This is kind of hard to review on its own against the specs, because > there are larger changes to the CTB flows which AFAICS are part of patch > 8. If what you wanted to do here was a simple replacement of defines to > keep the later patch simpler, then, considering all patches are going to > be squashed anyway: > > Reviewed-by: Daniele Ceraolo Spurio > > One suggestion below. > >> >> GuC: 55.0.0 >> Signed-off-by: Michal Wajdeczko >> Acked-by: Piotr Piórkowski >> --- >>   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c   | 14 ++++++++------ >>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 17 ----------------- >>   2 files changed, 8 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> index 8f7b148fef58..3f7f48611487 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> @@ -477,7 +477,9 @@ static int wait_for_ct_request_update(struct >> ct_request *req, u32 *status) >>        * up to that length of time, then switch to a slower sleep-wait >> loop. >>        * No GuC command should ever take longer than 10ms. >>        */ >> -#define done INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status)) >> +#define done \ >> +    (FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ >> +     GUC_HXG_ORIGIN_GUC) >>       err = wait_for_us(done, 10); >>       if (err) >>           err = wait_for(done, 10); >> @@ -532,21 +534,21 @@ static int ct_send(struct intel_guc_ct *ct, >>       if (unlikely(err)) >>           goto unlink; >>   -    if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status)) { >> +    if (FIELD_GET(GUC_HXG_MSG_0_TYPE, *status) != >> GUC_HXG_TYPE_RESPONSE_SUCCESS) { >>           err = -EIO; >>           goto unlink; >>       } >>         if (response_buf) { >>           /* There shall be no data in the status */ >> -        WARN_ON(INTEL_GUC_MSG_TO_DATA(request.status)); >> +        WARN_ON(FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, >> request.status)); >>           /* Return actual response len */ >>           err = request.response_len; >>       } else { >>           /* There shall be no response payload */ >>           WARN_ON(request.response_len); >>           /* Return data decoded from the status dword */ >> -        err = INTEL_GUC_MSG_TO_DATA(*status); >> +        err = FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, *status); > > Given that the same FIELD_GET() are repeated multiple times, IMO we > could've kept some helper macros, something like: > > INTEL_GUC_HXG_RESPONSE_TO_DATA(hxg) \ >     FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, hxg) > > INTEL_GUC_HXG_ORIGIN_IS_GUC(hxg) \ >     (FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg) == GUC_HXG_ORIGIN_GUC) > > INTEL_GUC_HXG_TYPE(hxg) \ >     FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg) > > Which could be useful in the mmio code as well. > Not sure how this changes in patch 8 though, I might put some more > comments on that patch. sure, we can add some of above helper macros, but not part of the ABI definitions, but in fwif.h where we have other wrappers and better to add them later as optional improvement, when all refactoring dust settles down > > Daniele > >>       } >>     unlink: >> @@ -741,8 +743,8 @@ static int ct_handle_response(struct intel_guc_ct >> *ct, struct ct_incoming_msg *r >>       status = response->msg[2]; >>       datalen = len - 2; >>   -    /* Format of the status follows RESPONSE message */ >> -    if (unlikely(!INTEL_GUC_MSG_IS_RESPONSE(status))) { >> +    /* Format of the status dword follows HXG header */ >> +    if (unlikely(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, status) != >> GUC_HXG_ORIGIN_GUC)) { >>           CT_ERROR(ct, "Corrupted response (status %#x)\n", status); >>           return -EPROTO; >>       } >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h >> index e9a9d85e2aa3..fb04e2211b79 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h >> @@ -414,23 +414,6 @@ struct guc_shared_ctx_data { >>       struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM]; >>   } __packed; >>   -#define __INTEL_GUC_MSG_GET(T, m) \ >> -    (((m) & INTEL_GUC_MSG_ ## T ## _MASK) >> INTEL_GUC_MSG_ ## T ## >> _SHIFT) >> -#define INTEL_GUC_MSG_TO_TYPE(m)    __INTEL_GUC_MSG_GET(TYPE, m) >> -#define INTEL_GUC_MSG_TO_DATA(m)    __INTEL_GUC_MSG_GET(DATA, m) >> -#define INTEL_GUC_MSG_TO_CODE(m)    __INTEL_GUC_MSG_GET(CODE, m) >> - >> -#define __INTEL_GUC_MSG_TYPE_IS(T, m) \ >> -    (INTEL_GUC_MSG_TO_TYPE(m) == INTEL_GUC_MSG_TYPE_ ## T) >> -#define INTEL_GUC_MSG_IS_REQUEST(m)    >> __INTEL_GUC_MSG_TYPE_IS(REQUEST, m) >> -#define INTEL_GUC_MSG_IS_RESPONSE(m)    >> __INTEL_GUC_MSG_TYPE_IS(RESPONSE, m) >> - >> -#define INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(m) \ >> -     (typecheck(u32, (m)) && \ >> -      ((m) & (INTEL_GUC_MSG_TYPE_MASK | INTEL_GUC_MSG_CODE_MASK)) == \ >> -      ((INTEL_GUC_MSG_TYPE_RESPONSE << INTEL_GUC_MSG_TYPE_SHIFT) | \ >> -       (INTEL_GUC_RESPONSE_STATUS_SUCCESS << INTEL_GUC_MSG_CODE_SHIFT))) >> - >>   /* This action will be programmed in C1BC - SOFT_SCRATCH_15_REG */ >>   enum intel_guc_recv_message { >>       INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED = BIT(1), >