From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id F3672A034C; Wed, 21 Sep 2022 09:13:18 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D921E40697; Wed, 21 Sep 2022 09:13:18 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id D9BE84014F for ; Wed, 21 Sep 2022 09:13:17 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1663744397; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CWfTsOOK5EwjxxO8kGCky3E/zrevrhXnvy9fgTUZevU=; b=dbriMRJfjpchYlqDMm3CTgHP9/bBCdaEy/AenoJx5KWZcRfnagBadL7iVA1zgPCze7uqso s7c7j61NH7B2F5XKuKsc2R1Ra9AaMlDX6b5gwW7m9fmvAgrwT6vApzksxsS5WFT36vQlDu I5iuuv1SnJUs5lpEWw5ASuq8U5lGgyw= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-466-9Y_cvnX2PYKvL1InD3EAEA-1; Wed, 21 Sep 2022 03:13:14 -0400 X-MC-Unique: 9Y_cvnX2PYKvL1InD3EAEA-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8F4FD3C0D189; Wed, 21 Sep 2022 07:13:13 +0000 (UTC) Received: from [10.39.208.16] (unknown [10.39.208.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 680AB492CA2; Wed, 21 Sep 2022 07:13:11 +0000 (UTC) Message-ID: <58a28ff9-03a5-450c-6cbb-438f25c5dc36@redhat.com> Date: Wed, 21 Sep 2022 09:13:10 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 To: Nic Chautru , dev@dpdk.org, thomas@monjalon.net Cc: trix@redhat.com, mdr@ashroe.eu, bruce.richardson@intel.com, hemant.agrawal@nxp.com, david.marchand@redhat.com, stephen@networkplumber.org, hernan.vargas@intel.com References: <1663292106-45320-1-git-send-email-nicolas.chautru@intel.com> <1663292106-45320-2-git-send-email-nicolas.chautru@intel.com> From: Maxime Coquelin Subject: Re: [PATCH v3 01/13] baseband/acc100: refactor to segregate common code In-Reply-To: <1663292106-45320-2-git-send-email-nicolas.chautru@intel.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 9/16/22 03:34, Nic Chautru wrote: > Refactoring all shareable common code to be used by future PMD > (including ACC200 in this patchset as well as taking into account > following PMDs in roadmap) by gathering such structures or inline methods. > Cleaning up the enum files to remove un-used registers definitions. > No functionality change. > > Signed-off-by: Nic Chautru You usually sign-off with Nicolas, but some of the patches of this series are with Nic. > Acked-by: Bruce Richardson > --- > app/test-bbdev/test_bbdev_perf.c | 6 +- > drivers/baseband/acc100/acc100_pf_enum.h | 939 ------------- > drivers/baseband/acc100/acc100_pmd.h | 449 +------ > drivers/baseband/acc100/acc101_pmd.h | 10 - > drivers/baseband/acc100/acc_common.h | 1388 +++++++++++++++++++ > drivers/baseband/acc100/rte_acc100_cfg.h | 70 +- > drivers/baseband/acc100/rte_acc100_pmd.c | 1856 ++++++++------------------ > drivers/baseband/acc100/rte_acc_common_cfg.h | 101 ++ > 8 files changed, 2069 insertions(+), 2750 deletions(-) > create mode 100644 drivers/baseband/acc100/acc_common.h > create mode 100644 drivers/baseband/acc100/rte_acc_common_cfg.h Overall, I think the patch should be split. For example: - One patch for rings rework as it introduces new helpers (and this patch should make use of them in ACC100). - One patch for the structs renaming and move to common file. - One patch for registers - ... It will make easier for the reviewer to identify discrepancies, and also will help to identify regressions when using git bisect. I have not done a full review of this patch, but something caught my eye wrt to available entries calculation in rings: > diff --git a/drivers/baseband/acc100/acc100_pf_enum.h b/drivers/baseband/acc100/acc100_pf_enum.h > index 2fba667..f4e5002 100644 > --- a/drivers/baseband/acc100/acc100_pf_enum.h > +++ b/drivers/baseband/acc100/acc100_pf_enum.h > @@ -14,32 +14,6 @@ ... > + > +/* Number of available descriptor in ring to enqueue */ > +static inline uint32_t > +acc_ring_avail_enq(struct acc_queue *q) > +{ > + return (q->sw_ring_depth - 1 + q->sw_ring_tail - q->sw_ring_head) % q->sw_ring_depth; > +} > + > +/* Number of available descriptor in ring to dequeue */ > +static inline uint32_t > +acc_ring_avail_deq(struct acc_queue *q) > +{ > + return (q->sw_ring_depth + q->sw_ring_head - q->sw_ring_tail) % q->sw_ring_depth; > +} It is surprising to me that the number of available descriptors calculation for enqueue and dequeue are différent. Could you please explain why there a off-by-one delta between enc and dec? If we look at other avail calculations in ACC100 enqueue, we get this: int32_t avail = q->sw_ring_depth + q->sw_ring_tail - q->sw_ring_head; It looks like there is indeed a off-by-one delta even for different avail enqueue calculation. Also, these new helpers are introduced but are not used in the patch.