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 E15DB426AE; Tue, 3 Oct 2023 19:26:43 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8EA67402A2; Tue, 3 Oct 2023 19:26:43 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 36B7440262 for ; Tue, 3 Oct 2023 19:26:41 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1696354000; 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=BY0TFrCHCYqmbeCilOwDqW0F/zdiukAKYCEjZ+HGDFs=; b=Ne3OZXhiDpF3emtiJpFSb2wgKaMbcoWT+qVQvI+dqvGbmxZk83qHFzVMeiAvb4fYrfOykO ZgQkXFxKUjFUPXXAcQanyT6+oRjF1JOdnoHDya2rNNWzTgZh6O7THEp3Co1shtZTNcVZUa qVFyn5k2APNZ7RYrGYJq6pYwDECXAiY= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-31-VE4qd93JMBij2_XS0kecmw-1; Tue, 03 Oct 2023 13:26:30 -0400 X-MC-Unique: VE4qd93JMBij2_XS0kecmw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 289AA811E86; Tue, 3 Oct 2023 17:26:30 +0000 (UTC) Received: from [10.39.208.4] (unknown [10.39.208.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E5AAB140E953; Tue, 3 Oct 2023 17:26:28 +0000 (UTC) Message-ID: Date: Tue, 3 Oct 2023 19:26:27 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [PATCH v3 11/12] baseband/acc: add support for VRB2 engine error detection To: "Chautru, Nicolas" , "dev@dpdk.org" Cc: "hemant.agrawal@nxp.com" , "david.marchand@redhat.com" , "Vargas, Hernan" References: <20230929163516.3636499-1-nicolas.chautru@intel.com> <20230929163516.3636499-12-nicolas.chautru@intel.com> <34f4ca15-e155-00bd-8fbe-0cd2b4cd237c@redhat.com> From: Maxime Coquelin In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 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 10/3/23 19:22, Chautru, Nicolas wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Tuesday, October 3, 2023 8:16 AM >> To: Chautru, Nicolas ; dev@dpdk.org >> Cc: hemant.agrawal@nxp.com; david.marchand@redhat.com; Vargas, Hernan >> >> Subject: Re: [PATCH v3 11/12] baseband/acc: add support for VRB2 engine >> error detection >> >> >> >> On 9/29/23 18:35, Nicolas Chautru wrote: >>> Adding missing incremental functionality for the VRB2 variant. Notably >>> detection of engine error during the dequeue. Minor cosmetic edits. >>> >>> Signed-off-by: Nicolas Chautru >>> --- >>> drivers/baseband/acc/rte_vrb_pmd.c | 20 ++++++++++++-------- >>> drivers/baseband/acc/vrb1_pf_enum.h | 17 ++++++++++++----- >>> 2 files changed, 24 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c >>> b/drivers/baseband/acc/rte_vrb_pmd.c >>> index a9d3db86e6..3eb1a380fc 100644 >>> --- a/drivers/baseband/acc/rte_vrb_pmd.c >>> +++ b/drivers/baseband/acc/rte_vrb_pmd.c >>> @@ -1504,6 +1504,7 @@ vrb_fcw_td_fill(const struct rte_bbdev_dec_op >> *op, struct acc_fcw_td *fcw) >>> fcw->ea = op->turbo_dec.cb_params.e; >>> fcw->eb = op->turbo_dec.cb_params.e; >>> } >>> + >>> if (op->turbo_dec.rv_index == 0) >>> fcw->k0_start_col = ACC_FCW_TD_RVIDX_0; >>> else if (op->turbo_dec.rv_index == 1) @@ -2304,7 >> +2305,7 @@ >>> enqueue_ldpc_enc_n_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op >> **ops, >>> return num; >>> } >>> >>> -/* Enqueue one encode operations for device for a partial TB >>> +/* Enqueue one encode operations for VRB1 device for a partial TB >>> * all codes blocks have same configuration multiplexed on the same >> descriptor. >>> */ >>> static inline void >>> @@ -2649,7 +2650,7 @@ enqueue_dec_one_op_cb(struct acc_queue *q, >> struct rte_bbdev_dec_op *op, >>> return 1; >>> } >>> >>> -/** Enqueue one decode operations for device in CB mode */ >>> +/** Enqueue one decode operations for device in CB mode. */ >>> static inline int >>> vrb_enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct >> rte_bbdev_dec_op *op, >>> uint16_t total_enqueued_cbs, bool same_op) @@ -2801,7 >> +2802,6 @@ >>> vrb_enqueue_ldpc_dec_one_op_tb(struct acc_queue *q, struct >> rte_bbdev_dec_op *op, >>> desc->req.data_ptrs[0].blen = ACC_FCW_LD_BLEN; >>> rte_memcpy(&desc->req.fcw_ld, &desc_first->req.fcw_ld, >> ACC_FCW_LD_BLEN); >>> desc->req.fcw_ld.tb_trailer_size = (c - r - 1) * trail_len; >>> - >>> if (q->d->device_variant == VRB1_VARIANT) >>> ret = vrb1_dma_desc_ld_fill(op, &desc->req, &input, >>> h_output, &in_offset, &h_out_offset, >> @@ -3226,7 +3226,6 @@ >>> vrb_enqueue_ldpc_dec_cb(struct rte_bbdev_queue_data *q_data, >>> break; >>> } >>> avail -= 1; >>> - >> >> Is it intentionnally removed? > > Cosmetic but slightly more readable. I don’t have a strong rule for these. OK, if that's intentionnal that's OK to me. >> >>> rte_bbdev_log(INFO, "Op %d %d %d %d %d %d %d %d %d %d >> %d %d\n", >>> i, ops[i]->ldpc_dec.op_flags, ops[i]- >>> ldpc_dec.rv_index, >>> ops[i]->ldpc_dec.iter_max, ops[i]- >>> ldpc_dec.iter_count, @@ >>> -3354,6 +3353,7 @@ vrb_dequeue_enc_one_op_cb(struct acc_queue *q, >> struct rte_bbdev_enc_op **ref_op, >>> op->status |= ((rsp.input_err) ? (1 << RTE_BBDEV_DATA_ERROR) : 0); >>> op->status |= ((rsp.dma_err) ? (1 << RTE_BBDEV_DRV_ERROR) : 0); >>> op->status |= ((rsp.fcw_err) ? (1 << RTE_BBDEV_DRV_ERROR) : 0); >>> + op->status |= ((rsp.engine_hung) ? (1 << RTE_BBDEV_ENGINE_ERROR) >> : >>> +0); >>> >>> if (desc->req.last_desc_in_batch) { >>> (*aq_dequeued)++; >>> @@ -3470,6 +3470,7 @@ vrb_dequeue_enc_one_op_tb(struct acc_queue >> *q, struct rte_bbdev_enc_op **ref_op, >>> op->status |= ((rsp.input_err) ? (1 << >> RTE_BBDEV_DATA_ERROR) : 0); >>> op->status |= ((rsp.dma_err) ? (1 << RTE_BBDEV_DRV_ERROR) >> : 0); >>> op->status |= ((rsp.fcw_err) ? (1 << RTE_BBDEV_DRV_ERROR) : >> 0); >>> + op->status |= ((rsp.engine_hung) ? (1 << >> RTE_BBDEV_ENGINE_ERROR) : >>> +0); >>> >>> if (desc->req.last_desc_in_batch) { >>> (*aq_dequeued)++; >>> @@ -3516,6 +3517,8 @@ vrb_dequeue_dec_one_op_cb(struct >> rte_bbdev_queue_data *q_data, >>> op->status |= ((rsp.input_err) ? (1 << RTE_BBDEV_DATA_ERROR) : 0); >>> op->status |= ((rsp.dma_err) ? (1 << RTE_BBDEV_DRV_ERROR) : 0); >>> op->status |= ((rsp.fcw_err) ? (1 << RTE_BBDEV_DRV_ERROR) : 0); >>> + op->status |= rsp.engine_hung << RTE_BBDEV_ENGINE_ERROR; >>> + >>> if (op->status != 0) { >>> /* These errors are not expected. */ >>> q_data->queue_stats.dequeue_err_count++; >>> @@ -3569,6 +3572,7 @@ vrb_dequeue_ldpc_dec_one_op_cb(struct >> rte_bbdev_queue_data *q_data, >>> op->status |= rsp.input_err << RTE_BBDEV_DATA_ERROR; >>> op->status |= rsp.dma_err << RTE_BBDEV_DRV_ERROR; >>> op->status |= rsp.fcw_err << RTE_BBDEV_DRV_ERROR; >>> + op->status |= rsp.engine_hung << RTE_BBDEV_ENGINE_ERROR; >>> if (op->status != 0) >>> q_data->queue_stats.dequeue_err_count++; >>> >>> @@ -3650,6 +3654,7 @@ vrb_dequeue_dec_one_op_tb(struct acc_queue >> *q, struct rte_bbdev_dec_op **ref_op, >>> op->status |= ((rsp.input_err) ? (1 << >> RTE_BBDEV_DATA_ERROR) : 0); >>> op->status |= ((rsp.dma_err) ? (1 << RTE_BBDEV_DRV_ERROR) >> : 0); >>> op->status |= ((rsp.fcw_err) ? (1 << RTE_BBDEV_DRV_ERROR) : >> 0); >>> + op->status |= ((rsp.engine_hung) ? (1 << >> RTE_BBDEV_ENGINE_ERROR) : >>> +0); >> >> It kinf of highlights the need for refactoring I suggested in previous patch! It >> would have been done in one place. > > That is fair, some of the logic is fairly common indeed. > I would create now an internal ticket to refactor some of this for next release. Thanks. Thanks, Maxime >> >>> >>> if (check_bit(op->ldpc_dec.op_flags, >> RTE_BBDEV_LDPC_CRC_TYPE_24A_CHECK)) >>> tb_crc_check ^= desc->rsp.add_info_1; @@ -3701,7 >> +3706,6 @@ >>> vrb_dequeue_enc(struct rte_bbdev_queue_data *q_data, >>> if (avail == 0) >>> return 0; >>> op = acc_op_tail(q, 0); >>> - >>> cbm = op->turbo_enc.code_block_mode; >>> >>> for (i = 0; i < avail; i++) { >>> @@ -4041,9 +4045,8 @@ vrb_enqueue_fft_one_op(struct acc_queue *q, >> struct rte_bbdev_fft_op *op, >>> &in_offset, &out_offset, &win_offset, >> &pwr_offset); >>> } >>> #ifdef RTE_LIBRTE_BBDEV_DEBUG >>> - rte_memdump(stderr, "FCW", &desc->req.fcw_fft, >>> - sizeof(desc->req.fcw_fft)); >>> - rte_memdump(stderr, "Req Desc.", desc, sizeof(*desc)); >>> + rte_memdump(stderr, "FCW", fcw, 128); >>> + rte_memdump(stderr, "Req Desc.", desc, 128); >>> #endif >>> return 1; >>> } >>> @@ -4116,6 +4119,7 @@ vrb_dequeue_fft_one_op(struct >> rte_bbdev_queue_data *q_data, >>> op->status |= rsp.input_err << RTE_BBDEV_DATA_ERROR; >>> op->status |= rsp.dma_err << RTE_BBDEV_DRV_ERROR; >>> op->status |= rsp.fcw_err << RTE_BBDEV_DRV_ERROR; >>> + op->status |= rsp.engine_hung << RTE_BBDEV_ENGINE_ERROR; >>> if (op->status != 0) >>> q_data->queue_stats.dequeue_err_count++; >>> >>> diff --git a/drivers/baseband/acc/vrb1_pf_enum.h >>> b/drivers/baseband/acc/vrb1_pf_enum.h >>> index 82a36685e9..6dc359800f 100644 >>> --- a/drivers/baseband/acc/vrb1_pf_enum.h >>> +++ b/drivers/baseband/acc/vrb1_pf_enum.h >>> @@ -98,11 +98,18 @@ enum { >>> ACC_PF_INT_DMA_UL5G_DESC_IRQ = 8, >>> ACC_PF_INT_DMA_DL5G_DESC_IRQ = 9, >>> ACC_PF_INT_DMA_MLD_DESC_IRQ = 10, >>> - ACC_PF_INT_ARAM_ECC_1BIT_ERR = 11, >>> - ACC_PF_INT_PARITY_ERR = 12, >>> - ACC_PF_INT_QMGR_ERR = 13, >>> - ACC_PF_INT_INT_REQ_OVERFLOW = 14, >>> - ACC_PF_INT_APB_TIMEOUT = 15, >>> + ACC_PF_INT_ARAM_ACCESS_ERR = 11, >>> + ACC_PF_INT_ARAM_ECC_1BIT_ERR = 12, >>> + ACC_PF_INT_PARITY_ERR = 13, >>> + ACC_PF_INT_QMGR_OVERFLOW = 14, >>> + ACC_PF_INT_QMGR_ERR = 15, >>> + ACC_PF_INT_ATS_ERR = 22, >>> + ACC_PF_INT_ARAM_FUUL = 23, >>> + ACC_PF_INT_EXTRA_READ = 24, >>> + ACC_PF_INT_COMPLETION_TIMEOUT = 25, >>> + ACC_PF_INT_CORE_HANG = 26, >>> + ACC_PF_INT_DMA_HANG = 28, >>> + ACC_PF_INT_DS_HANG = 27, >>> }; >>> >>> #endif /* VRB1_PF_ENUM_H */ >> >> >> Reviewed-by: Maxime Coquelin >> >> Thanks, >> Maxime >