From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 586311B1B8 for ; Fri, 6 Oct 2017 00:23:00 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id B9F5420B77; Thu, 5 Oct 2017 18:22:59 -0400 (EDT) Received: from frontend2 ([10.202.2.161]) by compute1.internal (MEProxy); Thu, 05 Oct 2017 18:22:59 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=HK1Qag955f1UNMe HWcXU8EHcysNbD95jsz6l+ulNiSI=; b=JUaZwZ6NASXruFqWbOG3X3QYWSrw6v0 G7owNoKjtPwRDKIFK4tNKYCdz0KEtsL2x36g7YzU/llsrY5BWgYbCL6fwS8K0Keo ocX+ND4QF15V6NeUWG8caL35fmGP6yyv/xRmY8kLKJKnGFdKsShcVhMjPGGRqoB4 TQ/D37wXBVxI= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc:x-sasl-enc; s= fm1; bh=HK1Qag955f1UNMeHWcXU8EHcysNbD95jsz6l+ulNiSI=; b=ZuNJcPV0 Zi3y+Jiw6VynCqsX7yEevvFBkK90GGB7iDVQM+/URHxuR8s0yqMJnuN7NT3ou7OW LXupih8rAKpeGgOf6fy7QjQMre9QO/bjFOt8Pww12GoSxZqh/gBb6uLOIFcuXu70 81xCU2qLIVoZ0GhqT/rUMAhjnZTj6B3Xj9L3m3XYCPXmN+effWOqV1BTpXHVpCdz hHTNAMU6/A/jo6s7m93PgTeGOVQX+7whPqQy5K60S4Cbtbq4HcRe+qdXYnH30QHq u79WXYLpCDb7rXXIqge1vzLZ/LPB32YlWoJH9O3mSPOa9E3EDt4zZWxSjouTZUxR ZYpF3d2UzJBbaA== X-ME-Sender: X-Sasl-enc: L+Gj1TlHZGsvvNrBjEw6YxNC0wTUTVWF7HUEQImpr3wE 1507242179 Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 69BB824009; Thu, 5 Oct 2017 18:22:59 -0400 (EDT) From: Thomas Monjalon To: "Mokhtar, Amr" Cc: dev@dpdk.org, fbl@redhat.com, aconole@redhat.com, bluca@debian.org Date: Fri, 06 Oct 2017 00:22:58 +0200 Message-ID: <34263409.OrRmme2Ibo@xps> In-Reply-To: <3D3765A8CDB52A4C8B410430AA19CB236EC35318@IRSMSX104.ger.corp.intel.com> References: <1503668796-65832-1-git-send-email-amr.mokhtar@intel.com> <1887012.k95aipThBJ@xps> <3D3765A8CDB52A4C8B410430AA19CB236EC35318@IRSMSX104.ger.corp.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev) 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: , X-List-Received-Date: Thu, 05 Oct 2017 22:23:00 -0000 05/10/2017 23:55, Mokhtar, Amr: > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > 03/10/2017 16:29, Mokhtar, Amr: > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > 25/08/2017 15:46, Amr Mokhtar: > > > > > +int > > > > > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues, > > > > > + const struct rte_bbdev_conf *conf); > > > > > > > > I am not convinced by the "configure all" function in ethdev. > > > > We break the ABI each time we add a new feature to configure. > > > > And it does not really help to have all configurations in one struct. > > > > Would you mind to split the struct rte_bbdev_conf and split the > > > > function accordingly? > > > > > > There is nothing to split tbh. The only parameter it has is the socket_id. > > > And in fact, it's optional, can be null. The only config we need is num_queues. > > > > Indeed, there is nothing in this struct. > > If you need only to allocate queues, you just have to rename this function. > > > > > I don't see in the near future that we may need to add more config params. > > > As a side, in the time of the implementation we were trying to avoid > > > any diversions from the current design ideology of ethdev and cryptodev. > > > > There is no ideology in ethdev, just some mistakes ;) > > > > > Can we leave it for consideration with future releases? > > > > No it should be addressed from the beginning. > > > > When you will need to add something more to configure port-wise, you should > > add a new function instead of breaking the ABI of the global conf struct. > > That's why the configure option should be more specialized. > > > > Distro people were complaining about ABI breakage last week. > > This is exactly an example of how to avoid it from the beginning. > > > > Ok, got your point. I was looking at it from an API-only standpoint. > How about modifying it into? > int > rte_bbdev_setup_queues(uint16_t dev_id, uint16_t num_queues, int socket_id); Yes OK [...] > > > > > +struct __rte_cache_aligned rte_bbdev { > > > > > + rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue function */ > > > > > + rte_bbdev_dequeue_ops_t dequeue_ops; /**< Dequeue function */ > > > > > + const struct rte_bbdev_ops *dev_ops; /**< Functions exported by > > > > > +PMD > > > > */ > > > > > + struct rte_bbdev_data *data; /**< Pointer to device data */ > > > > > + bool attached; /**< If device is currently attached or not */ > > > > > > > > What "attached" means? > > > > I'm afraid you are trying to manage hotplug in the wrong layer. > > > > > > Hotplug is not supported in the current release. > > > > It is not answering the question. > > What is an "attached" device? > > "Attached" means that the PCI device was probed and the bbdev device slot is allocated. > For software devices, means that a virtual bbdev device (vdev) is allocated for bbdev. > Same way the "attached" approach used in cryptodev. Not sure to understand. If "attached" means "allocated", when is it false? [...] > > > > > +/** Structure specifying a single operation */ struct rte_bbdev_op { > > > > > + enum rte_bbdev_op_type type; /**< Type of this operation */ > > > > > + int status; /**< Status of operation that was performed */ > > > > > + struct rte_mempool *mempool; /**< Mempool which op instance is > > > > > +in > > > > */ > > > > > + void *opaque_data; /**< Opaque pointer for user data */ > > > > > + /** > > > > > + * Anonymous union of operation-type specific parameters. When > > > > allocated > > > > > + * using rte_bbdev_op_pool_create(), space is allocated for the > > > > > + * parameters at the end of each rte_bbdev_op structure, and the > > > > > + * pointers here point to it. > > > > > + */ > > > > > + RTE_STD_C11 > > > > > + union { > > > > > + void *generic; > > > > > + struct rte_bbdev_op_turbo_dec *turbo_dec; > > > > > + struct rte_bbdev_op_turbo_enc *turbo_enc; > > > > > + }; > > > > > +}; > > > > > > > > I am not sure it is a good idea to fit every operations in the same > > > > struct and the same functions. > > > > > > Due to the fact that our design adopts this idea that a device can > > > support both the encode and decode operations. > > > Then, at the time of PMD registration, the enqueue functions is allocated. > > > This enqueue() function is common for both operations. > > > This fitted operation structure is essential for the driver to decide on the > > operation. > > > > Sorry I do not understand why you must have a "generic operation". > > Please, could you try again to explain this design to someone not fully > > understanding how turbo enc/dec works? > > Oh, sorry, I was not paying attention that you're referring to "void *generic" > It is just a place-holder for any other operation types. Can be removed if you like. No I was not referring to void *generic. It is the same question as in the RFC. I don't understand the benefit of grouping different things in an union.