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 F00FDA0032; Mon, 18 Jul 2022 15:09:10 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DDAC74069F; Mon, 18 Jul 2022 15:09:10 +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 60DD340041 for ; Mon, 18 Jul 2022 15:09:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1658149748; 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=VH2XRcifMeAXfew8Z+h8Kbyc+ftHLsPrMjgz5sa4cx8=; b=QkUk/Ws/Jz/di0mCXkgEJdLxQhzHeTKEBS83m7V/cFH+90umBUpx7qvKfrU37P7YOH2oWx 6KQ57PuVxoUZgz92ZIaKai0ZtRwM9qO7L4o0QeIOHCPHrhweYQAS440CN1ej9NvV8g7Ph4 iYUi1oweplxUOyf8L/963d96bxQd8K4= Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-9-LEnP33BQNw2ZMl8kyLIJBw-1; Mon, 18 Jul 2022 09:09:07 -0400 X-MC-Unique: LEnP33BQNw2ZMl8kyLIJBw-1 Received: by mail-qt1-f200.google.com with SMTP id r5-20020ac85c85000000b0031ecf611c64so8436784qta.23 for ; Mon, 18 Jul 2022 06:09:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=VH2XRcifMeAXfew8Z+h8Kbyc+ftHLsPrMjgz5sa4cx8=; b=DZTzXMN096QoiItElKJaxK39zcTt/fZjEO+Cul0QIUYgky/NLZaJM0xIO7J1gDIAx5 fPLJhM0I6nXM57pyc2KLQ1Bx/MOJn6u3EGbGiU54eL1OEz1+0vBlV57f+PqCzU/K00bu Rf+ZA49cc23c2AcJQFcpKI5w8T1Zud0aU/hq1E/UL9R78j5gXHQp/i/e8eDodaKl1XU0 hN3PEDeGO2f2fLXiXf0IznZqSxYUH7yMsi1aYOT/iwXoF2NGc8WoGQTI7OoF0keLe88g UraysLN2uPHb0bgIm0JSW/ap2gdkxClFAwVtijN/3cDfAOw4T2pCwMCXz5rmuQW2oPUu YjNA== X-Gm-Message-State: AJIora/UzUuj9rT7Oyaph1mw0KGn47nJC5MFwgXiKLYbjMPzPEsh+soy s40Lo04jhMCWdbaLtssAjE/RLtVXWQkNWDG7is0PKDQ3/e1QSw+5bePwyJm78NFpq4ZZdFhd3X9 S4kQ= X-Received: by 2002:ae9:c305:0:b0:6b5:b618:370a with SMTP id n5-20020ae9c305000000b006b5b618370amr17764211qkg.90.1658149747369; Mon, 18 Jul 2022 06:09:07 -0700 (PDT) X-Google-Smtp-Source: AGRyM1uLAfxKuyRAlkWucmtpW9OVZguwJb5inbSzlvRHY+4R9nTD4uU9+4QhSlD/BvKdlnYZy16d1Q== X-Received: by 2002:ae9:c305:0:b0:6b5:b618:370a with SMTP id n5-20020ae9c305000000b006b5b618370amr17764183qkg.90.1658149747077; Mon, 18 Jul 2022 06:09:07 -0700 (PDT) Received: from localhost.localdomain (024-205-208-113.res.spectrum.com. [24.205.208.113]) by smtp.gmail.com with ESMTPSA id o21-20020a05620a2a1500b006b5cdbbfccfsm9946008qkp.79.2022.07.18.06.09.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 18 Jul 2022 06:09:06 -0700 (PDT) Subject: Re: [PATCH v4 2/7] bbdev: add device status info To: "Chautru, Nicolas" , "dev@dpdk.org" , "thomas@monjalon.net" , "gakhil@marvell.com" , "hemant.agrawal@nxp.com" Cc: "maxime.coquelin@redhat.com" , "mdr@ashroe.eu" , "Richardson, Bruce" , "david.marchand@redhat.com" , "stephen@networkplumber.org" References: <1655491040-183649-6-git-send-email-nicolas.chautru@intel.com> <1657067022-54373-1-git-send-email-nicolas.chautru@intel.com> <1657067022-54373-3-git-send-email-nicolas.chautru@intel.com> <87a53618-992c-2309-c622-09dbc671a201@redhat.com> From: Tom Rix Message-ID: <46a71a21-60d9-b800-84b6-12178a679942@redhat.com> Date: Mon, 18 Jul 2022 06:09:03 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: 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; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US 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 7/7/22 10:15 AM, Chautru, Nicolas wrote: > Hi Tom, > >> -----Original Message----- >> From: Tom Rix >> Sent: Thursday, July 7, 2022 6:37 AM >> To: Chautru, Nicolas ; dev@dpdk.org; >> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com >> Cc: maxime.coquelin@redhat.com; mdr@ashroe.eu; Richardson, Bruce >> ; david.marchand@redhat.com; >> stephen@networkplumber.org >> Subject: Re: [PATCH v4 2/7] bbdev: add device status info >> >> >> On 7/6/22 2:16 PM, Chautru, Nicolas wrote: >>>> -----Original Message----- >>>> From: Tom Rix >>>> Subject: Re: [PATCH v4 2/7] bbdev: add device status info >>>> >>>> >>>> On 7/5/22 5:23 PM, Nicolas Chautru wrote: >>>>> Added device status information, so that the PMD can expose >>>>> information related to the underlying accelerator device status. >>>>> Minor order change in structure to fit into padding hole. >>>>> >>>>> Signed-off-by: Nicolas Chautru >>>>> --- >>>>> drivers/baseband/acc100/rte_acc100_pmd.c | 1 + >>>>> drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c | 1 + >>>>> drivers/baseband/fpga_lte_fec/fpga_lte_fec.c | 1 + >>>>> drivers/baseband/la12xx/bbdev_la12xx.c | 1 + >>>>> drivers/baseband/null/bbdev_null.c | 1 + >>>>> drivers/baseband/turbo_sw/bbdev_turbo_software.c | 1 + >>>>> lib/bbdev/rte_bbdev.c | 24 +++++++++++++++ >>>>> lib/bbdev/rte_bbdev.h | 35 ++++++++++++++++++++-- >>>>> lib/bbdev/version.map | 6 ++++ >>>>> 9 files changed, 69 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c >>>>> b/drivers/baseband/acc100/rte_acc100_pmd.c >>>>> index de7e4bc..17ba798 100644 >>>>> --- a/drivers/baseband/acc100/rte_acc100_pmd.c >>>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c >>>>> @@ -1060,6 +1060,7 @@ >>>>> >>>>> /* Read and save the populated config from ACC100 registers */ >>>>> fetch_acc100_config(dev); >>>>> + dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED; >>>>> >>>>> /* This isn't ideal because it reports the maximum number of >>>>> queues >>>> but >>>>> * does not provide info on how many can be uplink/downlink or >>>>> different diff --git >>>>> a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c >>>>> b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c >>>>> index 82ae6ba..57b12af 100644 >>>>> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c >>>>> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c >>>>> @@ -369,6 +369,7 @@ >>>>> dev_info->capabilities = bbdev_capabilities; >>>>> dev_info->cpu_flag_reqs = NULL; >>>>> dev_info->data_endianness = RTE_LITTLE_ENDIAN; >>>>> + dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED; >>>>> >>>>> /* Calculates number of queues assigned to device */ >>>>> dev_info->max_num_queues = 0; >>>>> diff --git a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c >>>>> b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c >>>>> index 21d3529..2a330c4 100644 >>>>> --- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c >>>>> +++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c >>>>> @@ -645,6 +645,7 @@ struct __rte_cache_aligned fpga_queue { >>>>> dev_info->capabilities = bbdev_capabilities; >>>>> dev_info->cpu_flag_reqs = NULL; >>>>> dev_info->data_endianness = RTE_LITTLE_ENDIAN; >>>>> + dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED; >>>>> >>>>> /* Calculates number of queues assigned to device */ >>>>> dev_info->max_num_queues = 0; >>>>> diff --git a/drivers/baseband/la12xx/bbdev_la12xx.c >>>>> b/drivers/baseband/la12xx/bbdev_la12xx.c >>>>> index 4d1bd16..c1f88c6 100644 >>>>> --- a/drivers/baseband/la12xx/bbdev_la12xx.c >>>>> +++ b/drivers/baseband/la12xx/bbdev_la12xx.c >>>>> @@ -100,6 +100,7 @@ struct bbdev_la12xx_params { >>>>> dev_info->capabilities = bbdev_capabilities; >>>>> dev_info->cpu_flag_reqs = NULL; >>>>> dev_info->min_alignment = 64; >>>>> + dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED; >>>>> >>>>> rte_bbdev_log_debug("got device info from %u", dev->data->dev_id); >>>>> } >>>>> diff --git a/drivers/baseband/null/bbdev_null.c >>>>> b/drivers/baseband/null/bbdev_null.c >>>>> index 248e129..94a1976 100644 >>>>> --- a/drivers/baseband/null/bbdev_null.c >>>>> +++ b/drivers/baseband/null/bbdev_null.c >>>>> @@ -82,6 +82,7 @@ struct bbdev_queue { >>>>> * here for code completeness. >>>>> */ >>>>> dev_info->data_endianness = RTE_LITTLE_ENDIAN; >>>>> + dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED; >>>>> >>>>> rte_bbdev_log_debug("got device info from %u", dev->data->dev_id); >>>>> } >>>>> diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c >>>>> b/drivers/baseband/turbo_sw/bbdev_turbo_software.c >>>>> index af7bc41..dbc5524 100644 >>>>> --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c >>>>> +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c >>>>> @@ -254,6 +254,7 @@ struct turbo_sw_queue { >>>>> dev_info->min_alignment = 64; >>>>> dev_info->harq_buffer_size = 0; >>>>> dev_info->data_endianness = RTE_LITTLE_ENDIAN; >>>>> + dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED; >>>>> >>>>> rte_bbdev_log_debug("got device info from %u\n", dev->data- >>>>> dev_id); >>>>> } >>>>> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index >>>>> 22bd894..555bda9 100644 >>>>> --- a/lib/bbdev/rte_bbdev.c >>>>> +++ b/lib/bbdev/rte_bbdev.c >>>>> @@ -25,6 +25,8 @@ >>>>> >>>>> /* Number of supported operation types */ >>>>> #define BBDEV_OP_TYPE_COUNT 5 >>>>> +/* Number of supported device status */ #define >>>>> +BBDEV_DEV_STATUS_COUNT 9 >>>>> >>>>> /* BBDev library logging ID */ >>>>> RTE_LOG_REGISTER_DEFAULT(bbdev_logtype, NOTICE); @@ -1132,3 >>>> +1134,25 >>>>> @@ struct rte_mempool * >>>>> rte_bbdev_log(ERR, "Invalid operation type"); >>>>> return NULL; >>>>> } >>>>> + >>>>> +const char * >>>>> +rte_bbdev_device_status_str(enum rte_bbdev_device_status status) { >>>>> + static const char * const dev_sta_string[] = { >>>>> + "RTE_BBDEV_DEV_NOSTATUS", >>>>> + "RTE_BBDEV_DEV_NOT_SUPPORTED", >>>>> + "RTE_BBDEV_DEV_RESET", >>>>> + "RTE_BBDEV_DEV_CONFIGURED", >>>>> + "RTE_BBDEV_DEV_ACTIVE", >>>>> + "RTE_BBDEV_DEV_FATAL_ERR", >>>>> + "RTE_BBDEV_DEV_RESTART_REQ", >>>>> + "RTE_BBDEV_DEV_RECONFIG_REQ", >>>>> + "RTE_BBDEV_DEV_CORRECT_ERR", >>>>> + }; >>>>> + >>>>> + if (status < BBDEV_DEV_STATUS_COUNT) >>>>> + return dev_sta_string[status]; >>>>> + >>>>> + rte_bbdev_log(ERR, "Invalid device status"); >>>>> + return NULL; >>>>> +} >>>>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index >>>>> b88c881..9b1ffa4 100644 >>>>> --- a/lib/bbdev/rte_bbdev.h >>>>> +++ b/lib/bbdev/rte_bbdev.h >>>>> @@ -223,6 +223,21 @@ struct rte_bbdev_queue_conf { >>>>> int >>>>> rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id); >>>>> >>>>> +/** >>>>> + * Flags indicate the status of the device */ enum >>>>> +rte_bbdev_device_status { >>>>> + RTE_BBDEV_DEV_NOSTATUS, /**< Nothing being reported */ >>>>> + RTE_BBDEV_DEV_NOT_SUPPORTED, /**< Device status is not >>>> supported on the PMD */ >>>> If this was 0, you may not need to explicitly set. >>> This helps to have the lack of status being equivalent to a cleared register. >> NOSTATUS is fine, just change >> >> NOT_SUPPORTED = 0, > Let me rephrase. Currently RTE_BBDEV_DEV_NOSTATUS is zero explicitly which can be valuable to match a clear register. > RTE_BBDEV_DEV_NOT_SUPPORTED would not be zero. > Are you suggesting I should put explictly a RTE_BBDEV_DEV_NOSTATUS = 0? Isn't it implicit for any compiler that the first enum starts from zero? However you want to do it, try taking advantage of zero-ed memory.  By choosing for this enum to be non-zero, it has to be explicitly set. If it was 0 it would be implicitly set, assuming dev is zero-ed. Tom > >> Tom >> >>>>> + RTE_BBDEV_DEV_RESET, /**< Device in reset and un-configured >>>> state */ >>>>> + RTE_BBDEV_DEV_CONFIGURED, /**< Device is configured and >>>> ready to use */ >>>>> + RTE_BBDEV_DEV_ACTIVE, /**< Device is configured and VF is >>>> being used */ >>>>> + RTE_BBDEV_DEV_FATAL_ERR, /**< Device has hit a fatal >>>> uncorrectable error */ >>>>> + RTE_BBDEV_DEV_RESTART_REQ, /**< Device requires application to >>>> restart */ >>>>> + RTE_BBDEV_DEV_RECONFIG_REQ, /**< Device requires application >>>> to reconfigure queues */ >>>>> + RTE_BBDEV_DEV_CORRECT_ERR, /**< Warning of a correctable >>>> error event happened */ >>>> Last patch was padded, do something consistent here. >>> We only pad if we have to. Here there is no array whose size would be >> dimensioned by the size of that enum. >>>>> +}; >>>>> + >>>>> /** Device statistics. */ >>>>> struct rte_bbdev_stats { >>>>> uint64_t enqueued_count; /**< Count of all operations enqueued >>>>> */ @@ -285,12 +300,14 @@ struct rte_bbdev_driver_info { >>>>> /** Set if device supports per-queue interrupts */ >>>>> bool queue_intr_supported; >>>>> /** Minimum alignment of buffers, in bytes */ >>>>> - uint16_t min_alignment; >>>>> - /** HARQ memory available in kB */ >>>>> + /** Device Status */ >>>>> + enum rte_bbdev_device_status device_status; >>>> New elements should be added to the end to improve backward >> compatibility. >>> Same comment in different patch. I would like to know if there is a real >> recommendation from DPDK on this. I have heard opposite view as well. >>> In that very case we are breaking the ABI in that new serie for 22.11 (sizes >> and offsets are changing). >>>> Tom >>>> >>>>> uint32_t harq_buffer_size; >>>>> /** Byte endianness (RTE_BIG_ENDIAN/RTE_LITTLE_ENDIAN) >>>> supported >>>>> * for input/output data >>>>> */ >>>>> + uint16_t min_alignment; >>>>> + /** HARQ memory available in kB */ >>>>> uint8_t data_endianness; >>>>> /** Default queue configuration used if none is supplied */ >>>>> struct rte_bbdev_queue_conf default_queue_conf; @@ -827,6 >>>> +844,20 >>>>> @@ typedef void (*rte_bbdev_cb_fn)(uint16_t dev_id, >>>>> rte_bbdev_queue_intr_ctl(uint16_t dev_id, uint16_t queue_id, int >>>>> epfd, int >>>> op, >>>>> void *data); >>>>> >>>>> +/** >>>>> + * Converts device status from enum to string >>>>> + * >>>>> + * @param status >>>>> + * Device status as enum >>>>> + * >>>>> + * @returns >>>>> + * Operation type as string or NULL if op_type is invalid >>>>> + * >>>>> + */ >>>>> +__rte_experimental >>>>> +const char* >>>>> +rte_bbdev_device_status_str(enum rte_bbdev_device_status status); >>>>> + >>>>> #ifdef __cplusplus >>>>> } >>>>> #endif >>>>> diff --git a/lib/bbdev/version.map b/lib/bbdev/version.map index >>>>> cce3f3c..9ac3643 100644 >>>>> --- a/lib/bbdev/version.map >>>>> +++ b/lib/bbdev/version.map >>>>> @@ -39,3 +39,9 @@ DPDK_22 { >>>>> >>>>> local: *; >>>>> }; >>>>> + >>>>> +EXPERIMENTAL { >>>>> + global: >>>>> + >>>>> + rte_bbdev_device_status_str; >>>>> +};