From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <amr.mokhtar@intel.com>
Received: from mga01.intel.com (mga01.intel.com [192.55.52.88])
by dpdk.org (Postfix) with ESMTP id 5AF851B1A2
for <dev@dpdk.org>; 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" <amr.mokhtar@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>
CC: "dev@dpdk.org" <dev@dpdk.org>, "fbl@redhat.com" <fbl@redhat.com>,
"aconole@redhat.com" <aconole@redhat.com>, "bluca@debian.org"
<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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <amr.mokhtar@intel.com>
> 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.