diff mbox series

[i-g-t,10/21] gem_wsim: Extract str to engine lookup

Message ID 20190508121058.27038-11-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Media scalability tooling | expand

Commit Message

Tvrtko Ursulin May 8, 2019, 12:10 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

Comments

Chris Wilson May 10, 2019, 1:20 p.m. UTC | #1 
Quoting Tvrtko Ursulin (2019-05-08 13:10:47)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  benchmarks/gem_wsim.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index 5245692df6eb..f654decb24cc 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -318,6 +318,18 @@ wsim_err(const char *fmt, ...)
>         } \
>  }
>  
> +static int str_to_engine(const char *str)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(ring_str_map); i++) {
> +               if (!strcasecmp(str, ring_str_map[i]))
> +                       return i;
> +       }
> +
> +       return -1;
> +}
> +
>  static struct workload *
>  parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
>  {
> @@ -480,22 +492,18 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
>                 }
>  
>                 if ((field = strtok_r(fstart, ".", &fctx)) != NULL) {
> -                       unsigned int old_valid = valid;
> -
>                         fstart = NULL;
>  
> -                       for (i = 0; i < ARRAY_SIZE(ring_str_map); i++) {
> -                               if (!strcasecmp(field, ring_str_map[i])) {
> -                                       step.engine = i;
> -                                       if (step.engine == BCS)
> -                                               bcs_used = true;
> -                                       valid++;
> -                                       break;
> -                               }
> -                       }
> -
> -                       check_arg(old_valid == valid,
> +                       i = str_to_engine(field);
> +                       check_arg(i < 0,
>                                   "Invalid engine id at step %u!\n", nr_steps);
> +                       if (i >= 0)
> +                               valid++;

check_arg() returned already for all i < 0, no?
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Tvrtko Ursulin May 13, 2019, 1:08 p.m. UTC | #2 
On 10/05/2019 14:20, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-08 13:10:47)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   benchmarks/gem_wsim.c | 34 +++++++++++++++++++++-------------
>>   1 file changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>> index 5245692df6eb..f654decb24cc 100644
>> --- a/benchmarks/gem_wsim.c
>> +++ b/benchmarks/gem_wsim.c
>> @@ -318,6 +318,18 @@ wsim_err(const char *fmt, ...)
>>          } \
>>   }
>>   
>> +static int str_to_engine(const char *str)
>> +{
>> +       unsigned int i;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(ring_str_map); i++) {
>> +               if (!strcasecmp(str, ring_str_map[i]))
>> +                       return i;
>> +       }
>> +
>> +       return -1;
>> +}
>> +
>>   static struct workload *
>>   parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
>>   {
>> @@ -480,22 +492,18 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
>>                  }
>>   
>>                  if ((field = strtok_r(fstart, ".", &fctx)) != NULL) {
>> -                       unsigned int old_valid = valid;
>> -
>>                          fstart = NULL;
>>   
>> -                       for (i = 0; i < ARRAY_SIZE(ring_str_map); i++) {
>> -                               if (!strcasecmp(field, ring_str_map[i])) {
>> -                                       step.engine = i;
>> -                                       if (step.engine == BCS)
>> -                                               bcs_used = true;
>> -                                       valid++;
>> -                                       break;
>> -                               }
>> -                       }
>> -
>> -                       check_arg(old_valid == valid,
>> +                       i = str_to_engine(field);
>> +                       check_arg(i < 0,
>>                                    "Invalid engine id at step %u!\n", nr_steps);
>> +                       if (i >= 0)
>> +                               valid++;
> 
> check_arg() returned already for all i < 0, no?

Yes, and it looks the very next patch removes the if. I'll pull it here.

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks!

Regards,

Tvrtko
diff mbox series

Patch

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index 5245692df6eb..f654decb24cc 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -318,6 +318,18 @@  wsim_err(const char *fmt, ...)
 	} \
 }
 
+static int str_to_engine(const char *str)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(ring_str_map); i++) {
+		if (!strcasecmp(str, ring_str_map[i]))
+			return i;
+	}
+
+	return -1;
+}
+
 static struct workload *
 parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 {
@@ -480,22 +492,18 @@  parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 		}
 
 		if ((field = strtok_r(fstart, ".", &fctx)) != NULL) {
-			unsigned int old_valid = valid;
-
 			fstart = NULL;
 
-			for (i = 0; i < ARRAY_SIZE(ring_str_map); i++) {
-				if (!strcasecmp(field, ring_str_map[i])) {
-					step.engine = i;
-					if (step.engine == BCS)
-						bcs_used = true;
-					valid++;
-					break;
-				}
-			}
-
-			check_arg(old_valid == valid,
+			i = str_to_engine(field);
+			check_arg(i < 0,
 				  "Invalid engine id at step %u!\n", nr_steps);
+			if (i >= 0)
+				valid++;
+
+			step.engine = i;
+
+			if (step.engine == BCS)
+				bcs_used = true;
 		}
 
 		if ((field = strtok_r(fstart, ".", &fctx)) != NULL) {