[Accel-config] Re: [PATCH v2 1/1] accel-config/dsa_test: Add device and wq selection for test

On 9/24/2021 4:34 AM, Tony Zhu wrote:
> Add -d to input device like dsa0/wq0.0 which allows the device and wq selection.
>
> Signed-off-by: Tony Zhu <tony.zhu(a)intel.com>
> Reviewed-by: Dave Jiang <dave.jiang(a)intel.com>
> ---
>   test/dsa.c      | 63 +++++++++++++++++++++++++++++++++++++++++++++----
>   test/dsa.h      |  2 +-
>   test/dsa_test.c | 14 +++++++++--
>   3 files changed, 71 insertions(+), 8 deletions(-)
>
> diff --git a/test/dsa.c b/test/dsa.c
> index ce6f6a1..83e4b4e 100644
> --- a/test/dsa.c
> +++ b/test/dsa.c
> @@ -146,6 +146,55 @@ static struct accfg_wq *dsa_get_wq(struct dsa_context *ctx,
>   	return NULL;
>   }
>   
> +static struct accfg_wq *dsa_get_wq_byid(struct dsa_context *ctx,
> +		int dev_id, int wq_id)

Can you align the function indentation such as:

+static struct accfg_wq *dsa_get_wq_byid(struct dsa_context *ctx,
+					 int dev_id, int wq_id)


> +{
> +	struct accfg_device *device;
> +	struct accfg_wq *wq;
> +	int rc;
> +
> +	accfg_device_foreach(ctx->ctx, device) {
> +		enum accfg_device_state dstate;
> +
> +		/* Make sure that the device is enabled */
> +		dstate = accfg_device_get_state(device);
> +		if (dstate != ACCFG_DEVICE_ENABLED)
> +			continue;

Since you are only checking it here once, you can drop the variable

if (accfg_device_get_state(device) != ACCFG_DEVICE_ENABLED)
	continue;

> +
> +		/* Match the device to the id requested */
> +		if (accfg_device_get_id(device) != dev_id &&
> +				dev_id != -1)

Preferably if you can line up the arguments:

if (accfg_device_get_id(device) != dev_id &&
     dev_id != -1)

Also, can we define -1?
#define ACCFG_DEVICE_INVALID_ID -1

> +			continue;
> +
> +		accfg_wq_foreach(device, wq) {
> +			enum accfg_wq_state wstate;
> +			enum accfg_wq_type type;
> +
> +			/* Get a workqueue that's enabled */
> +			wstate = accfg_wq_get_state(wq);
> +			if (wstate != ACCFG_WQ_ENABLED)
> +				continue;

Probably no need to have wstate variable. Just check directly.


> +
> +			/* The wq type should be user */
> +			type = accfg_wq_get_type(wq);
> +			if (type != ACCFG_WQT_USER)
> +				continue;

Same here for type.


> +
> +			/* Make sure the wq id is correct */
> +			if(wq_id != accfg_wq_get_id(wq))
> +				continue;
> +
> +			rc = dsa_setup_wq(ctx, wq);
> +			if (rc < 0)
> +				return NULL;
> +
> +			return wq;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>   static uint32_t bsr(uint32_t val)
>   {
>   	uint32_t msb;
> @@ -154,7 +203,7 @@ static uint32_t bsr(uint32_t val)
>   	return msb - 1;
>   }
>   
> -int dsa_alloc(struct dsa_context *ctx, int shared)
> +int dsa_alloc(struct dsa_context *ctx, int shared, int dev_id, int wq_id)
>   {
>   	struct accfg_device *dev;
>   
> @@ -162,14 +211,18 @@ int dsa_alloc(struct dsa_context *ctx, int shared)
>   	if (ctx->wq_reg)
>   		return 0;
>   
> -	ctx->wq = dsa_get_wq(ctx, -1, shared);
> +	if(wq_id != -1){
> +		ctx->wq = dsa_get_wq_byid(ctx, dev_id, wq_id);
> +	}else{
> +		ctx->wq = dsa_get_wq(ctx, dev_id, shared);
> +	}

Fix spacing here. Also single line does not need {}:

if (wq_id != -1)
	ctx->wq = dsa_get_wq_byid(ctx, dev_id, wq_id);
else
	ctx->wq = dsa_get_wq(...);


> +
>   	if (!ctx->wq) {
>   		err("No usable wq found\n");
>   		return -ENODEV;
>   	}
>   	dev = accfg_wq_get_device(ctx->wq);
> -
> -	ctx->dedicated = !shared;
> +	ctx->dedicated = accfg_wq_get_mode(ctx->wq);
>   	ctx->wq_size = accfg_wq_get_size(ctx->wq);
>   	ctx->wq_idx = accfg_wq_get_id(ctx->wq);
>   	ctx->bof = accfg_wq_get_block_on_fault(ctx->wq);
> @@ -182,7 +235,7 @@ int dsa_alloc(struct dsa_context *ctx, int shared)
>   	ctx->max_xfer_bits = bsr(ctx->max_xfer_size);
>   
>   	info("alloc wq %d shared %d size %d addr %p batch sz %#x xfer sz %#x\n",
> -			ctx->wq_idx, shared, ctx->wq_size, ctx->wq_reg,
> +			ctx->wq_idx, ctx->dedicated, ctx->wq_size, ctx->wq_reg,
>   			ctx->max_batch_size, ctx->max_xfer_size);
>   
>   	return 0;
> diff --git a/test/dsa.h b/test/dsa.h
> index 5db0c02..393221b 100644
> --- a/test/dsa.h
> +++ b/test/dsa.h
> @@ -211,7 +211,7 @@ int memcmp_pattern(const void *src, const uint64_t pattern, size_t len);
>   int dsa_enqcmd(struct dsa_context *ctx, struct dsa_hw_desc *hw);
>   
>   struct dsa_context *dsa_init(void);
> -int dsa_alloc(struct dsa_context *ctx, int shared);
> +int dsa_alloc(struct dsa_context *ctx, int shared, int dev_id, int wq_id);
>   int alloc_task(struct dsa_context *ctx);
>   struct task *__alloc_task(void);
>   int init_task(struct task *tsk, int tflags, int opcode,
> diff --git a/test/dsa_test.c b/test/dsa_test.c
> index 5f7a7f1..a2bffe8 100644
> --- a/test/dsa_test.c
> +++ b/test/dsa_test.c
> @@ -23,6 +23,7 @@ static void usage(void)
>   	"-o <opcode>     ; opcode, same value as in DSA spec\n"
>   	"-b <opcode> ; if batch opcode, opcode in the batch\n"
>   	"-c <batch_size> ; if batch opcode, number of descriptors for batch\n"
> +	"-d              ; wq device such as dsa0/wq0.0\n"
>   	"-t <ms timeout> ; ms to wait for descs to complete\n"
>   	"-v              ; verbose\n"
>   	"-h              ; print this message\n");
> @@ -110,8 +111,10 @@ int main(int argc, char *argv[])
>   	int tflags = TEST_FLAGS_BOF;
>   	int opt;
>   	unsigned int bsize = 0;
> +	char dev_type[MAX_DEV_LEN];
> +	int wq_id = -1, dev_id=-1, dev_wq_id=-1;
>   
> -	while ((opt = getopt(argc, argv, "w:l:f:o:b:c:t:p:vh")) != -1) {
> +	while ((opt = getopt(argc, argv, "w:l:f:o:b:c:d:t:p:vh")) != -1) {
>   		switch (opt) {
>   		case 'w':
>   			wq_type = atoi(optarg);
> @@ -131,6 +134,13 @@ int main(int argc, char *argv[])
>   		case 'c':
>   			bsize = strtoul(optarg, NULL, 0);
>   			break;
> +		case 'd':
> +			if (sscanf(optarg, "%[a-z]%u/%*[a-z]%u.%u", dev_type, \
> +						&dev_id, &dev_wq_id, &wq_id) != 4) {
> +				err("invalid input device:dev_wq_id:%d ,wq_id:%d\n", dev_wq_id, wq_id);
> +				return -EINVAL;
> +			}
> +			break;
>   		case 't':
>   			ms_timeout = strtoul(optarg, NULL, 0);
>   			break;
> @@ -150,7 +160,7 @@ int main(int argc, char *argv[])
>   	if (dsa == NULL)
>   		return -ENOMEM;
>   
> -	rc = dsa_alloc(dsa, wq_type);
> +	rc = dsa_alloc(dsa, wq_type, dev_id, wq_id);
>   	if (rc < 0)
>   		return -ENOMEM;
>