From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 5AF851B1A2 for ; Thu, 5 Oct 2017 23:55:26 +0200 (CEST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Oct 2017 14:55:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,482,1500966000"; d="scan'208";a="159935747" Received: from irsmsx153.ger.corp.intel.com ([163.33.192.75]) by fmsmga006.fm.intel.com with ESMTP; 05 Oct 2017 14:55:24 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.248]) by IRSMSX153.ger.corp.intel.com ([169.254.9.34]) with mapi id 14.03.0319.002; Thu, 5 Oct 2017 22:55:23 +0100 From: "Mokhtar, Amr" To: Thomas Monjalon CC: "dev@dpdk.org" , "fbl@redhat.com" , "aconole@redhat.com" , "bluca@debian.org" Thread-Topic: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev) Thread-Index: AQHTHaidyFtCgYsXxUqHDbe8mN+d6KK/h2qAgBK8FRCAACX8gIADkwEg Date: Thu, 5 Oct 2017 21:55:23 +0000 Message-ID: <3D3765A8CDB52A4C8B410430AA19CB236EC35318@IRSMSX104.ger.corp.intel.com> References: <1503668796-65832-1-git-send-email-amr.mokhtar@intel.com> <5517187.euiR5LjUcT@xps> <3D3765A8CDB52A4C8B410430AA19CB236EC341D7@IRSMSX104.ger.corp.intel.com> <1887012.k95aipThBJ@xps> In-Reply-To: <1887012.k95aipThBJ@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTU3ZmMzZDYtNzhkMC00MDU3LTkzMjEtY2JhMzIzYjk2MWI0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IlRqNnpkNURqMG52MnZ4cnFId3hMTmtUK0JcL2daa3dKUHBuNkpPQ05QK2Y0PSJ9 x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 21:55:26 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Tuesday 3 October 2017 16:18 > To: Mokhtar, Amr > Cc: dev@dpdk.org; fbl@redhat.com; aconole@redhat.com; bluca@debian.org > Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev) >=20 > 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. >=20 > Indeed, there is nothing in this struct. > If you need only to allocate queues, you just have to rename this functio= n. >=20 > > I don't see in the near future that we may need to add more config para= ms. > > 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= . >=20 > There is no ideology in ethdev, just some mistakes ;) >=20 > > Can we leave it for consideration with future releases? >=20 > No it should be addressed from the beginning. >=20 > When you will need to add something more to configure port-wise, you shou= ld > add a new function instead of breaking the ABI of the global conf struct. > That's why the configure option should be more specialized. >=20 > Distro people were complaining about ABI breakage last week. > This is exactly an example of how to avoid it from the beginning. >=20 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)= ; >=20 > > > [...] > > > > +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. >=20 > It is not answering the question. > What is an "attached" device? "Attached" means that the PCI device was probed and the bbdev device slot i= s allocated. For software devices, means that a virtual bbdev device (vdev) is allocated= for bbdev. Same way the "attached" approach used in cryptodev. >=20 >=20 > > > [...] > > > > +/** 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 allocat= ed. > > This enqueue() function is common for both operations. > > This fitted operation structure is essential for the driver to decide o= n the > operation. >=20 > 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 *gener= ic" It is just a place-holder for any other operation types. Can be removed if = you like.