From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8610CA04B5; Wed, 30 Sep 2020 21:03:28 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 74BE31D57B; Wed, 30 Sep 2020 21:03:26 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by dpdk.org (Postfix) with ESMTP id CB5F81D57A for ; Wed, 30 Sep 2020 21:03:23 +0200 (CEST) Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1601492602; 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=6/JRsXv0H/ig7oakDLnoT1i4t1uFRA5WFUTdVMVL7ck=; b=dxwbBjIwl9R8wv50CvqqX4DUXlsDaGlAxI6aVvSswoCN5BCwIMtCbU2rxmARSJr7vbfgkr 3JqFHt+mcC5mJ5Knu2f7QN2K2pISRc0YfhhpccjCqmQC7wT+WTgQQkJfAxK6NL3xW36GhT zQMrGd8yY1FnNV0G9o8MC1RtZqmFu60= Received: from mail-oo1-f69.google.com (mail-oo1-f69.google.com [209.85.161.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-462-5TnTAoNRNkagZ_Wa2B-Khw-1; Wed, 30 Sep 2020 15:03:19 -0400 X-MC-Unique: 5TnTAoNRNkagZ_Wa2B-Khw-1 Received: by mail-oo1-f69.google.com with SMTP id h2so1384402oop.10 for ; Wed, 30 Sep 2020 12:03:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=6/JRsXv0H/ig7oakDLnoT1i4t1uFRA5WFUTdVMVL7ck=; b=oEsxxpYq6DxBkXB0N8o7/cRVvxOUxjts3A30JGFwtmoLUlOyphvF63yVQLlaRnrT4x s68bqtLll6kKra6c81t++8IyzM4OJ0XlDqMHg4yISXasAvr56QJa+sql2dyYVU84eFqO FGJogvoy/M1G2ez8TvLTE7atD23nVxmvZYDuTd1JDbJd0xlUq9xHSvn865m9uJ8nhaoW nJsT155ytYhyhe3KhMnnrJXAOC+mH8DVwLWxYMVmcBFzLYPMy/Cxa89vy6B4gFFgbTfu wDV92L6tCTtwQdqkWMA/d/V+iS6rW1MesAQiyyyG00/HVetrhePE2ln35eAYQ8D0qKYt rvUg== X-Gm-Message-State: AOAM530KGDNLosYv9ga04dUvgXWbxH32BxH2WYHqwM+9iB0+3hH6mV6j RARohcRJMS4StcW1uc4XlT6s5y8JKkgPg5RIVCUp7h6YOWN86CKUAuMQFLu2THZDhkTVy6AdHKf K5BI= X-Received: by 2002:a05:6830:23bc:: with SMTP id m28mr2345733ots.10.1601492597882; Wed, 30 Sep 2020 12:03:17 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzIK1wSN5EGyz3OfO2CMY7CmoCG6Sd0QpoBanHLudTgV62LnBXnNtPAvoTVewoVxK8xrOT1JQ== X-Received: by 2002:a05:6830:23bc:: with SMTP id m28mr2345701ots.10.1601492597513; Wed, 30 Sep 2020 12:03:17 -0700 (PDT) Received: from trix.remote.csb (075-142-250-213.res.spectrum.com. [75.142.250.213]) by smtp.gmail.com with ESMTPSA id k73sm606390otk.63.2020.09.30.12.03.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 30 Sep 2020 12:03:16 -0700 (PDT) To: Nicolas Chautru , dev@dpdk.org, akhil.goyal@nxp.com Cc: bruce.richardson@intel.com, rosen.xu@intel.com, dave.burley@accelercomm.com, aidan.goddard@accelercomm.com, ferruh.yigit@intel.com, tianjiao.liu@intel.com References: <1597796731-57841-12-git-send-email-nicolas.chautru@intel.com> <1601339385-117424-1-git-send-email-nicolas.chautru@intel.com> <1601339385-117424-9-git-send-email-nicolas.chautru@intel.com> From: Tom Rix Message-ID: <5f42d8ad-e19d-0601-da5a-492b1b84333b@redhat.com> Date: Wed, 30 Sep 2020 12:03:14 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <1601339385-117424-9-git-send-email-nicolas.chautru@intel.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=trix@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v9 08/10] baseband/acc100: add interrupt support to PMD X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 9/28/20 5:29 PM, Nicolas Chautru wrote: > Adding capability and functions to support MSI > interrupts, call backs and inforing. > > Signed-off-by: Nicolas Chautru > Acked-by: Liu Tianjiao > --- > drivers/baseband/acc100/rte_acc100_pmd.c | 288 ++++++++++++++++++++++++++++++- > drivers/baseband/acc100/rte_acc100_pmd.h | 15 ++ > 2 files changed, 300 insertions(+), 3 deletions(-) > > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c b/drivers/baseband/acc100/rte_acc100_pmd.c > index 7d4c3df..b6d9e7c 100644 > --- a/drivers/baseband/acc100/rte_acc100_pmd.c > +++ b/drivers/baseband/acc100/rte_acc100_pmd.c > @@ -339,6 +339,213 @@ > free_base_addresses(base_addrs, i); > } > > +/* > + * Find queue_id of a device queue based on details from the Info Ring. > + * If a queue isn't found UINT16_MAX is returned. > + */ > +static inline uint16_t > +get_queue_id_from_ring_info(struct rte_bbdev_data *data, > + const union acc100_info_ring_data ring_data) > +{ > + uint16_t queue_id; > + > + for (queue_id = 0; queue_id < data->num_queues; ++queue_id) { > + struct acc100_queue *acc100_q = > + data->queues[queue_id].queue_private; > + if (acc100_q != NULL && acc100_q->aq_id == ring_data.aq_id && > + acc100_q->qgrp_id == ring_data.qg_id && > + acc100_q->vf_id == ring_data.vf_id) > + return queue_id; If num_queues is large, this linear search will be slow. Consider changing the search algorithm. > + } > + > + return UINT16_MAX; the interrupt handlers that use this function do not a great job of handling this error. > +} > + > +/* Checks PF Info Ring to find the interrupt cause and handles it accordingly */ > +static inline void > +acc100_check_ir(struct acc100_device *acc100_dev) > +{ > + volatile union acc100_info_ring_data *ring_data; > + uint16_t info_ring_head = acc100_dev->info_ring_head; > + if (acc100_dev->info_ring == NULL) > + return; > + > + ring_data = acc100_dev->info_ring + (acc100_dev->info_ring_head & > + ACC100_INFO_RING_MASK); > + > + while (ring_data->valid) { > + if ((ring_data->int_nb < ACC100_PF_INT_DMA_DL_DESC_IRQ) || ( > + ring_data->int_nb > > + ACC100_PF_INT_DMA_DL5G_DESC_IRQ)) > + rte_bbdev_log(WARNING, "InfoRing: ITR:%d Info:0x%x", > + ring_data->int_nb, ring_data->detailed_info); > + /* Initialize Info Ring entry and move forward */ > + ring_data->val = 0; > + info_ring_head++; > + ring_data = acc100_dev->info_ring + > + (info_ring_head & ACC100_INFO_RING_MASK); These three statements are common for the ring handling, consider a macro or inline function. > + } > +} > + > +/* Checks PF Info Ring to find the interrupt cause and handles it accordingly */ > +static inline void > +acc100_pf_interrupt_handler(struct rte_bbdev *dev) > +{ > + struct acc100_device *acc100_dev = dev->data->dev_private; > + volatile union acc100_info_ring_data *ring_data; > + struct acc100_deq_intr_details deq_intr_det; > + > + ring_data = acc100_dev->info_ring + (acc100_dev->info_ring_head & > + ACC100_INFO_RING_MASK); > + > + while (ring_data->valid) { > + > + rte_bbdev_log_debug( > + "ACC100 PF Interrupt received, Info Ring data: 0x%x", > + ring_data->val); > + > + switch (ring_data->int_nb) { > + case ACC100_PF_INT_DMA_DL_DESC_IRQ: > + case ACC100_PF_INT_DMA_UL_DESC_IRQ: > + case ACC100_PF_INT_DMA_UL5G_DESC_IRQ: > + case ACC100_PF_INT_DMA_DL5G_DESC_IRQ: > + deq_intr_det.queue_id = get_queue_id_from_ring_info( > + dev->data, *ring_data); > + if (deq_intr_det.queue_id == UINT16_MAX) { > + rte_bbdev_log(ERR, > + "Couldn't find queue: aq_id: %u, qg_id: %u, vf_id: %u", > + ring_data->aq_id, > + ring_data->qg_id, > + ring_data->vf_id); > + return; > + } > + rte_bbdev_pmd_callback_process(dev, > + RTE_BBDEV_EVENT_DEQUEUE, &deq_intr_det); > + break; > + default: > + rte_bbdev_pmd_callback_process(dev, > + RTE_BBDEV_EVENT_ERROR, NULL); > + break; > + } > + > + /* Initialize Info Ring entry and move forward */ > + ring_data->val = 0; > + ++acc100_dev->info_ring_head; > + ring_data = acc100_dev->info_ring + > + (acc100_dev->info_ring_head & > + ACC100_INFO_RING_MASK); > + } > +} > + > +/* Checks VF Info Ring to find the interrupt cause and handles it accordingly */ > +static inline void > +acc100_vf_interrupt_handler(struct rte_bbdev *dev) very similar to pf case, consider combining. > +{ > + struct acc100_device *acc100_dev = dev->data->dev_private; > + volatile union acc100_info_ring_data *ring_data; > + struct acc100_deq_intr_details deq_intr_det; > + > + ring_data = acc100_dev->info_ring + (acc100_dev->info_ring_head & > + ACC100_INFO_RING_MASK); > + > + while (ring_data->valid) { > + > + rte_bbdev_log_debug( > + "ACC100 VF Interrupt received, Info Ring data: 0x%x", > + ring_data->val); > + > + switch (ring_data->int_nb) { > + case ACC100_VF_INT_DMA_DL_DESC_IRQ: > + case ACC100_VF_INT_DMA_UL_DESC_IRQ: > + case ACC100_VF_INT_DMA_UL5G_DESC_IRQ: > + case ACC100_VF_INT_DMA_DL5G_DESC_IRQ: > + /* VFs are not aware of their vf_id - it's set to 0 in > + * queue structures. > + */ > + ring_data->vf_id = 0; > + deq_intr_det.queue_id = get_queue_id_from_ring_info( > + dev->data, *ring_data); > + if (deq_intr_det.queue_id == UINT16_MAX) { > + rte_bbdev_log(ERR, > + "Couldn't find queue: aq_id: %u, qg_id: %u", > + ring_data->aq_id, > + ring_data->qg_id); > + return; > + } > + rte_bbdev_pmd_callback_process(dev, > + RTE_BBDEV_EVENT_DEQUEUE, &deq_intr_det); > + break; > + default: > + rte_bbdev_pmd_callback_process(dev, > + RTE_BBDEV_EVENT_ERROR, NULL); > + break; > + } > + > + /* Initialize Info Ring entry and move forward */ > + ring_data->valid = 0; > + ++acc100_dev->info_ring_head; > + ring_data = acc100_dev->info_ring + (acc100_dev->info_ring_head > + & ACC100_INFO_RING_MASK); > + } > +} > + > +/* Interrupt handler triggered by ACC100 dev for handling specific interrupt */ > +static void > +acc100_dev_interrupt_handler(void *cb_arg) > +{ > + struct rte_bbdev *dev = cb_arg; > + struct acc100_device *acc100_dev = dev->data->dev_private; > + > + /* Read info ring */ > + if (acc100_dev->pf_device) > + acc100_pf_interrupt_handler(dev); combined like .. acc100_interrupt_handler(dev, is_pf) > + else > + acc100_vf_interrupt_handler(dev); > +} > + > +/* Allocate and setup inforing */ > +static int > +allocate_inforing(struct rte_bbdev *dev) consider renaming allocate_info_ring > +{ > + struct acc100_device *d = dev->data->dev_private; > + const struct acc100_registry_addr *reg_addr; > + rte_iova_t info_ring_phys; > + uint32_t phys_low, phys_high; > + > + if (d->info_ring != NULL) > + return 0; /* Already configured */ > + > + /* Choose correct registry addresses for the device type */ > + if (d->pf_device) > + reg_addr = &pf_reg_addr; > + else > + reg_addr = &vf_reg_addr; > + /* Allocate InfoRing */ > + d->info_ring = rte_zmalloc_socket("Info Ring", > + ACC100_INFO_RING_NUM_ENTRIES * > + sizeof(*d->info_ring), RTE_CACHE_LINE_SIZE, > + dev->data->socket_id); > + if (d->info_ring == NULL) { > + rte_bbdev_log(ERR, > + "Failed to allocate Info Ring for %s:%u", > + dev->device->driver->name, > + dev->data->dev_id); The callers do not check that this fails. > + return -ENOMEM; > + } > + info_ring_phys = rte_malloc_virt2iova(d->info_ring); > + > + /* Setup Info Ring */ > + phys_high = (uint32_t)(info_ring_phys >> 32); > + phys_low = (uint32_t)(info_ring_phys); > + acc100_reg_write(d, reg_addr->info_ring_hi, phys_high); > + acc100_reg_write(d, reg_addr->info_ring_lo, phys_low); > + acc100_reg_write(d, reg_addr->info_ring_en, ACC100_REG_IRQ_EN_ALL); > + d->info_ring_head = (acc100_reg_read(d, reg_addr->info_ring_ptr) & > + 0xFFF) / sizeof(union acc100_info_ring_data); > + return 0; > +} > + > + > /* Allocate 64MB memory used for all software rings */ > static int > acc100_setup_queues(struct rte_bbdev *dev, uint16_t num_queues, int socket_id) > @@ -426,6 +633,7 @@ > acc100_reg_write(d, reg_addr->tail_ptrs_dl4g_hi, phys_high); > acc100_reg_write(d, reg_addr->tail_ptrs_dl4g_lo, phys_low); > > + allocate_inforing(dev); need to check here > d->harq_layout = rte_zmalloc_socket("HARQ Layout", > ACC100_HARQ_LAYOUT * sizeof(*d->harq_layout), > RTE_CACHE_LINE_SIZE, dev->data->socket_id); > @@ -437,13 +645,53 @@ > return 0; > } > > +static int > +acc100_intr_enable(struct rte_bbdev *dev) > +{ > + int ret; > + struct acc100_device *d = dev->data->dev_private; > + > + /* Only MSI are currently supported */ > + if (dev->intr_handle->type == RTE_INTR_HANDLE_VFIO_MSI || > + dev->intr_handle->type == RTE_INTR_HANDLE_UIO) { > + > + allocate_inforing(dev); need to check here > + > + ret = rte_intr_enable(dev->intr_handle); > + if (ret < 0) { > + rte_bbdev_log(ERR, > + "Couldn't enable interrupts for device: %s", > + dev->data->name); > + rte_free(d->info_ring); > + return ret; > + } > + ret = rte_intr_callback_register(dev->intr_handle, > + acc100_dev_interrupt_handler, dev); > + if (ret < 0) { > + rte_bbdev_log(ERR, > + "Couldn't register interrupt callback for device: %s", > + dev->data->name); > + rte_free(d->info_ring); does intr need to be disabled here ? > + return ret; > + } > + > + return 0; > + } > + > + rte_bbdev_log(ERR, "ACC100 (%s) supports only VFIO MSI interrupts", > + dev->data->name); > + return -ENOTSUP; > +} > + > /* Free 64MB memory used for software rings */ > static int > acc100_dev_close(struct rte_bbdev *dev) > { > struct acc100_device *d = dev->data->dev_private; > + acc100_check_ir(d); > if (d->sw_rings_base != NULL) { > rte_free(d->tail_ptrs); > + rte_free(d->info_ring); > rte_free(d->sw_rings_base); > d->sw_rings_base = NULL; > } > @@ -643,6 +891,7 @@ > RTE_BBDEV_TURBO_CRC_TYPE_24B | > RTE_BBDEV_TURBO_HALF_ITERATION_EVEN | > RTE_BBDEV_TURBO_EARLY_TERMINATION | > + RTE_BBDEV_TURBO_DEC_INTERRUPTS | > RTE_BBDEV_TURBO_NEG_LLR_1_BIT_IN | > RTE_BBDEV_TURBO_MAP_DEC | > RTE_BBDEV_TURBO_DEC_TB_CRC_24B_KEEP | > @@ -663,6 +912,7 @@ > RTE_BBDEV_TURBO_CRC_24B_ATTACH | > RTE_BBDEV_TURBO_RV_INDEX_BYPASS | > RTE_BBDEV_TURBO_RATE_MATCH | > + RTE_BBDEV_TURBO_ENC_INTERRUPTS | > RTE_BBDEV_TURBO_ENC_SCATTER_GATHER, > .num_buffers_src = > RTE_BBDEV_TURBO_MAX_CODE_BLOCKS, > @@ -676,7 +926,8 @@ > .capability_flags = > RTE_BBDEV_LDPC_RATE_MATCH | > RTE_BBDEV_LDPC_CRC_24B_ATTACH | > - RTE_BBDEV_LDPC_INTERLEAVER_BYPASS, > + RTE_BBDEV_LDPC_INTERLEAVER_BYPASS | > + RTE_BBDEV_LDPC_ENC_INTERRUPTS, > .num_buffers_src = > RTE_BBDEV_LDPC_MAX_CODE_BLOCKS, > .num_buffers_dst = > @@ -701,7 +952,8 @@ > RTE_BBDEV_LDPC_DECODE_BYPASS | > RTE_BBDEV_LDPC_DEC_SCATTER_GATHER | > RTE_BBDEV_LDPC_HARQ_6BIT_COMPRESSION | > - RTE_BBDEV_LDPC_LLR_COMPRESSION, > + RTE_BBDEV_LDPC_LLR_COMPRESSION | > + RTE_BBDEV_LDPC_DEC_INTERRUPTS, > .llr_size = 8, > .llr_decimals = 1, > .num_buffers_src = > @@ -751,14 +1003,39 @@ > #else > dev_info->harq_buffer_size = 0; > #endif > + acc100_check_ir(d); > +} > + > +static int > +acc100_queue_intr_enable(struct rte_bbdev *dev, uint16_t queue_id) > +{ > + struct acc100_queue *q = dev->data->queues[queue_id].queue_private; > + > + if (dev->intr_handle->type != RTE_INTR_HANDLE_VFIO_MSI && > + dev->intr_handle->type != RTE_INTR_HANDLE_UIO) > + return -ENOTSUP; > + > + q->irq_enable = 1; > + return 0; > +} > + > +static int > +acc100_queue_intr_disable(struct rte_bbdev *dev, uint16_t queue_id) > +{ > + struct acc100_queue *q = dev->data->queues[queue_id].queue_private; > + q->irq_enable = 0; A -ENOTSUP above, should need similar check here. > + return 0; > } > > static const struct rte_bbdev_ops acc100_bbdev_ops = { > .setup_queues = acc100_setup_queues, > + .intr_enable = acc100_intr_enable, > .close = acc100_dev_close, > .info_get = acc100_dev_info_get, > .queue_setup = acc100_queue_setup, > .queue_release = acc100_queue_release, > + .queue_intr_enable = acc100_queue_intr_enable, > + .queue_intr_disable = acc100_queue_intr_disable > }; > > /* ACC100 PCI PF address map */ > @@ -3018,8 +3295,10 @@ > ? (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); > - if (op->status != 0) > + if (op->status != 0) { > q_data->queue_stats.dequeue_err_count++; > + acc100_check_ir(q->d); > + } > > /* CRC invalid if error exists */ > if (!op->status) > @@ -3076,6 +3355,9 @@ > op->status |= 1 << RTE_BBDEV_SYNDROME_ERROR; > op->ldpc_dec.iter_count = (uint8_t) rsp.iter_cnt; > > + if (op->status & (1 << RTE_BBDEV_DRV_ERROR)) > + acc100_check_ir(q->d); > + > /* Check if this is the last desc in batch (Atomic Queue) */ > if (desc->req.last_desc_in_batch) { > (*aq_dequeued)++; > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.h b/drivers/baseband/acc100/rte_acc100_pmd.h > index 78686c1..8980fa5 100644 > --- a/drivers/baseband/acc100/rte_acc100_pmd.h > +++ b/drivers/baseband/acc100/rte_acc100_pmd.h > @@ -559,7 +559,14 @@ struct acc100_device { > /* Virtual address of the info memory routed to the this function under > * operation, whether it is PF or VF. > */ > + union acc100_info_ring_data *info_ring; Need a comment that this array needs a sentinel ? Tom > + > union acc100_harq_layout_data *harq_layout; > + /* Virtual Info Ring head */ > + uint16_t info_ring_head; > + /* Number of bytes available for each queue in device, depending on > + * how many queues are enabled with configure() > + */ > uint32_t sw_ring_size; > uint32_t ddr_size; /* Size in kB */ > uint32_t *tail_ptrs; /* Base address of response tail pointer buffer */ > @@ -575,4 +582,12 @@ struct acc100_device { > bool configured; /**< True if this ACC100 device is configured */ > }; > > +/** > + * Structure with details about RTE_BBDEV_EVENT_DEQUEUE event. It's passed to > + * the callback function. > + */ > +struct acc100_deq_intr_details { > + uint16_t queue_id; > +}; > + > #endif /* _RTE_ACC100_PMD_H_ */