Re: [dpdk-dev] [PATCH v8] dmadev: introduce DMA device library

[fengchengwen <fengchengwen@huawei.com>]

Thanks Jerin, comment inline

On 2021/7/20 13:03, Jerin Jacob wrote:
> On Tue, Jul 20, 2021 at 6:48 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>>
>> This patch introduce 'dmadevice' which is a generic type of DMA
>> device.
>>
>> The APIs of dmadev library exposes some generic operations which can
>> enable configuration and I/O with the DMA devices.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

[snip]

>> +int
>> +rte_dmadev_info_get(uint16_t dev_id, struct rte_dmadev_info *dev_info)
>> +{
>> +       const struct rte_dmadev *dev = &rte_dmadevices[dev_id];
>> +       int ret;
>> +
>> +       RTE_DMADEV_VALID_DEV_ID_OR_ERR_RET(dev_id, -EINVAL);
>> +       if (dev_info == NULL)
>> +               return -EINVAL;
>> +
>> +       RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_info_get, -ENOTSUP);
>> +       memset(dev_info, 0, sizeof(struct rte_dmadev_info));
>> +       ret = (*dev->dev_ops->dev_info_get)(dev, dev_info,
>> +                                           sizeof(struct rte_dmadev_info));
>> +       if (ret != 0)
>> +               return ret;
>> +
>> +       dev_info->device = dev->device;
>> +       dev_info->nb_vchans = dev->data->dev_conf.max_vchans;
> 
> This will be updated after configure stage.

Yes, the dev_info->nb_vchans hold the number of virtual DMA channel configured.
Do you mean add one comment here ?

> 
> 
>> +
>> +       return 0;
>> +}
>> +
>> +int
>> +rte_dmadev_configure(uint16_t dev_id, const struct rte_dmadev_conf *dev_conf)
>> +{
>> +       struct rte_dmadev *dev = &rte_dmadevices[dev_id];
>> +       struct rte_dmadev_info info;
>> +       int ret;
>> +
>> +       RTE_DMADEV_VALID_DEV_ID_OR_ERR_RET(dev_id, -EINVAL);
>> +       if (dev_conf == NULL)
>> +               return -EINVAL;
>> +
>> +       ret = rte_dmadev_info_get(dev_id, &info);
>> +       if (ret != 0) {
>> +               RTE_DMADEV_LOG(ERR, "Device %u get device info fail\n", dev_id);
>> +               return -EINVAL;
>> +       }
>> +       if (dev_conf->max_vchans == 0) {
>> +               RTE_DMADEV_LOG(ERR,
>> +                       "Device %u configure zero vchans\n", dev_id);
>> +               return -EINVAL;
>> +       }
>> +       if (dev_conf->max_vchans > info.max_vchans) {
>> +               RTE_DMADEV_LOG(ERR,
>> +                       "Device %u configure too many vchans\n", dev_id);
>> +               return -EINVAL;
>> +       }
>> +       if (dev_conf->enable_silent &&
>> +           !(info.dev_capa & RTE_DMADEV_CAPA_SILENT)) {
>> +               RTE_DMADEV_LOG(ERR, "Device %u don't support silent\n", dev_id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (dev->data->dev_started != 0) {
>> +               RTE_DMADEV_LOG(ERR,
>> +                       "Device %u must be stopped to allow configuration\n",
>> +                       dev_id);
>> +               return -EBUSY;
>> +       }
> 
> ethdev and other device class common code handles the reconfigure case. i.e
> the application configures N vchan first and reconfigures to N - M
> then free the resources
> attached to M - N. Please do the same here.

DMA is a simple device, I think it's OK to reconfigure at driver-level.

PS: If we need support reconfigure at dmadev-level, dmadev should hold the vchan-configuration,
and invoke driver's vchan_release to release resources. This may introduce more complexity.


>> +int
>> +rte_dmadev_dump(uint16_t dev_id, FILE *f)
>> +{
>> +       const struct rte_dmadev *dev = &rte_dmadevices[dev_id];
>> +       struct rte_dmadev_info info;
>> +       int ret;
>> +
>> +       RTE_DMADEV_VALID_DEV_ID_OR_ERR_RET(dev_id, -EINVAL);
>> +       if (f == NULL)
>> +               return -EINVAL;
>> +
>> +       ret = rte_dmadev_info_get(dev_id, &info);
>> +       if (ret != 0) {
>> +               RTE_DMADEV_LOG(ERR, "Device %u get device info fail\n", dev_id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       fprintf(f, "DMA Dev %u, '%s' [%s]\n",
>> +               dev->data->dev_id,
>> +               dev->data->dev_name,
>> +               dev->data->dev_started ? "started" : "stopped");
>> +       fprintf(f, "  dev_capa: 0x%" PRIx64 "\n", info.dev_capa);
>> +       fprintf(f, "  max_vchans_supported: %u\n", info.max_vchans);
>> +       fprintf(f, "  max_vchans_configured: %u\n", info.nb_vchans);
>> +       fprintf(f, "  silent_mode: %s\n",
>> +               dev->data->dev_conf.enable_silent ? "on" : "off");
> 
> Probably iterate over each vchan and dumping the at least direction
> will be usefull.

dmadev hasn't hold vchan-configuration, Need more discussion.