[VSP-Tests PATCH 0/3] Run as user, and python3 support

[VSP-Tests PATCH 0/3] Run as user, and python3 support

[Kieran Bingham]

My development target now runs as a user, rather than root and fails to
obtain timestamps through the /proc/timer_list which requires root
access, and generates a warning on every log output.

Whilst not fatal, this can be fixed by using a c-based implementation to
read the monotonic timestamps without parsing proc manually with awk.

This has the extra advantage of not spawning extra processes for every
line that is logged, and simplifies the timestamp handling.

Furthermore, python2 is no longer available on my platforms so the
gen-lut.py script is updated to run as python3.

I am aware that there is a second python2 tool in this repository, for
converting histograms, however I have not yet identified what files this
process so I have not completed the python3 migration for that file yet.

If someone wants to complete this, I have the initial conversion, or
alternatively - if someone has a set of appropriate histograms to give
me I can update and validate the tool myself.


Kieran Bingham (3):
  gen-lut: Update for python3
  src: monotonic-ts: Monotonic timestamp logging
  scripts/logger: Use new monotonic-ts tool

 data/frames/gen-lut.py | 18 +++++++++---------
 scripts/logger.sh      | 20 ++++----------------
 src/Makefile           | 10 +++++++---
 src/monotonic-ts.c     | 37 +++++++++++++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 28 deletions(-)
 create mode 100644 src/monotonic-ts.c

-- 
2.25.1

[VSP-Tests PATCH 1/3] gen-lut: Update for python3

[Kieran Bingham]

Python2 has now gone end-of-life and is discontinued.

Update the gen-lut utility to use python3 directly, converting xrange
usages to range, and using bytearray to store the tables and write them
directly removing the discontinued file object.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 data/frames/gen-lut.py | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/data/frames/gen-lut.py b/data/frames/gen-lut.py
index 07889b11f4ac..335b9f1613bc 100755
--- a/data/frames/gen-lut.py
+++ b/data/frames/gen-lut.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/python3
 # SPDX-License-Identifier: GPL-2.0-or-later
 # SPDX-FileCopyrightText: 2016 Renesas Electronics Corporation
 
@@ -49,26 +49,26 @@ def clu_value(x, y, z, scale, a, freq, weights):
 	return (z, y, x, 0)
 
 def generate_clu(config):
-	clu = []
+	clu = bytearray()
 
-	for z in xrange(17):
-		for y in xrange(17):
-			for x in xrange(17):
+	for z in range(17):
+		for y in range(17):
+			for x in range(17):
 				clu.extend(clu_value(x, y, z, **config[1]))
 
-	file('clu-%s.bin' % config[0], 'wb').write(''.join([chr(c) for c in clu]))
+	open('clu-%s.bin' % config[0], 'wb').write(clu)
 
 
 def gamma(vin, gamma, scale):
 	return int(255 * scale * math.pow(vin / 255., gamma))
 
 def generate_lut(config):
-	lut = []
-	for i in xrange(256):
+	lut = bytearray()
+	for i in range(256):
 		lut.extend([gamma(i, g, config[1]) for g in config[2:]])
 		lut.append(0)
 
-	file('lut-%s.bin' % config[0], 'wb').write(''.join([chr(c) for c in lut]))
+	open('lut-%s.bin' % config[0], 'wb').write(lut)
 
 
 def main(argv):
-- 
2.25.1

[VSP-Tests PATCH 2/3] src: monotonic-ts: Monotonic timestamp logging

[Kieran Bingham]

Introduce a new utility which prefixes a monotonic timestamp rendered in the
same format as the kernel logs to all lines fed in through stdin.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/Makefile       | 10 +++++++---
 src/monotonic-ts.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)
 create mode 100644 src/monotonic-ts.c

diff --git a/src/Makefile b/src/Makefile
index d7f901f58be6..67216e81ffc4 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -7,18 +7,22 @@ CFLAGS	?= -O0 -g -W -Wall -Wno-unused-parameter -Iinclude
 LDFLAGS	?=
 LIBS	:= -lm
 GEN-IMAGE := gen-image
+MONOTONIC_TS := monotonic-ts
 
 %.o : %.c
 	$(CC) $(CFLAGS) -c -o $@ $<
 
-all: $(GEN-IMAGE)
+all: $(GEN-IMAGE) $(MONOTONIC_TS)
 
 $(GEN-IMAGE): gen-image.o
 	$(CC) $(LDFLAGS) -o $@ $^ $(LIBS)
 
+$(MONOTONIC_TS): monotonic-ts.o
+	$(CC) $(LDFLAGS) -o $@ $^ $(LIBS)
+
 clean:
 	-rm -f *.o
-	-rm -f $(GEN-IMAGE)
+	-rm -f $(GEN-IMAGE) $(MONOTONIC_TS)
 
 install:
-	cp $(GEN-IMAGE) $(INSTALL_DIR)/
+	cp $(GEN-IMAGE) $(MONOTONIC_TS) $(INSTALL_DIR)/
diff --git a/src/monotonic-ts.c b/src/monotonic-ts.c
new file mode 100644
index 000000000000..fcb671e06d27
--- /dev/null
+++ b/src/monotonic-ts.c
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* SPDX-FileCopyrightText: 2020 Kieran Bingham <kieran.bingham@ideasonboard.com> */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <time.h>
+
+int main(int argc, char ** argv)
+{
+	struct timespec tp;
+	char *line = NULL;
+	size_t size = 0;
+	const char *label = "";
+
+	if (argc > 1)
+		label = argv[1];
+
+	/*
+	 * Explicitly set line buffering on stdin to be sure it is delivered
+	 * in a timely fashion for our timestamping purposes when data is fed
+	 * through a pipe.
+	 */
+	setlinebuf(stdin);
+
+	do {
+		if (getline(&line, &size, stdin) <= 0)
+			break;
+
+		clock_gettime(CLOCK_MONOTONIC, &tp);
+		printf("[%ld.%.9ld]%s %s", tp.tv_sec, tp.tv_nsec, label, line);
+	} while (!feof(stdin));
+
+	free(line);
+
+	return 0;
+}
+
-- 
2.25.1

[VSP-Tests PATCH 3/3] scripts/logger: Use new monotonic-ts tool

[Kieran Bingham]

Utilise the new monotonic timestamping tool to remove the manual parsing of
timestamps via /proc/timer_list which can only be read by root.

This also simplifies the processing required and contains all timestamping
actions within a single process space.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 scripts/logger.sh | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/scripts/logger.sh b/scripts/logger.sh
index 97e1f582da2b..452ebc8c82ba 100755
--- a/scripts/logger.sh
+++ b/scripts/logger.sh
@@ -2,23 +2,11 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 # SPDX-FileCopyrightText: 2016 Renesas Electronics Corporation
 
-now() {
-	awk '/^now/ {time=$3; printf("[%u.%06u]", time / 1000000000, (time % 1000000000) / 1000) ; exit}' /proc/timer_list
-}
-
 label=${1:+ [$1]}
 
 TRACE_MARKER=/sys/kernel/debug/tracing/trace_marker
-if [ -e $TRACE_MARKER ]; then
-	extra_log_files=$TRACE_MARKER
+if [ -e $TRACE_MARKER ] && [ $(id -u) == 0 ]; then
+	./monotonic-ts $label | tee -a $TRACE_MARKER
+else
+	./monotonic-ts $label
 fi
-
-while read line ; do
-	newline="$(now)$label $line"
-
-	echo "$newline"
-
-	for f in $extra_log_files; do
-		echo "$newline" >> $f;
-	done;
-done
-- 
2.25.1

Re: [VSP-Tests PATCH 1/3] gen-lut: Update for python3

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wed, Sep 16, 2020 at 03:43:00PM +0100, Kieran Bingham wrote:
> Python2 has now gone end-of-life and is discontinued.
> 
> Update the gen-lut utility to use python3 directly, converting xrange
> usages to range, and using bytearray to store the tables and write them
> directly removing the discontinued file object.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  data/frames/gen-lut.py | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/data/frames/gen-lut.py b/data/frames/gen-lut.py
> index 07889b11f4ac..335b9f1613bc 100755
> --- a/data/frames/gen-lut.py
> +++ b/data/frames/gen-lut.py
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python
> +#!/usr/bin/python3
>  # SPDX-License-Identifier: GPL-2.0-or-later
>  # SPDX-FileCopyrightText: 2016 Renesas Electronics Corporation
>  
> @@ -49,26 +49,26 @@ def clu_value(x, y, z, scale, a, freq, weights):
>  	return (z, y, x, 0)
>  
>  def generate_clu(config):
> -	clu = []
> +	clu = bytearray()
>  
> -	for z in xrange(17):
> -		for y in xrange(17):
> -			for x in xrange(17):
> +	for z in range(17):
> +		for y in range(17):
> +			for x in range(17):
>  				clu.extend(clu_value(x, y, z, **config[1]))
>  
> -	file('clu-%s.bin' % config[0], 'wb').write(''.join([chr(c) for c in clu]))
> +	open('clu-%s.bin' % config[0], 'wb').write(clu)
>  
>  
>  def gamma(vin, gamma, scale):
>  	return int(255 * scale * math.pow(vin / 255., gamma))
>  
>  def generate_lut(config):
> -	lut = []
> -	for i in xrange(256):
> +	lut = bytearray()
> +	for i in range(256):
>  		lut.extend([gamma(i, g, config[1]) for g in config[2:]])
>  		lut.append(0)
>  
> -	file('lut-%s.bin' % config[0], 'wb').write(''.join([chr(c) for c in lut]))
> +	open('lut-%s.bin' % config[0], 'wb').write(lut)
>  
>  
>  def main(argv):

-- 
Regards,

Laurent Pinchart

Re: [VSP-Tests PATCH 2/3] src: monotonic-ts: Monotonic timestamp logging

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wed, Sep 16, 2020 at 03:43:01PM +0100, Kieran Bingham wrote:
> Introduce a new utility which prefixes a monotonic timestamp rendered in the
> same format as the kernel logs to all lines fed in through stdin.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/Makefile       | 10 +++++++---
>  src/monotonic-ts.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 3 deletions(-)
>  create mode 100644 src/monotonic-ts.c
> 
> diff --git a/src/Makefile b/src/Makefile
> index d7f901f58be6..67216e81ffc4 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -7,18 +7,22 @@ CFLAGS	?= -O0 -g -W -Wall -Wno-unused-parameter -Iinclude
>  LDFLAGS	?=
>  LIBS	:= -lm
>  GEN-IMAGE := gen-image
> +MONOTONIC_TS := monotonic-ts

s/MONOTONIC_TS/MONOTONIC-TS/ to match GEN-IMAGE ?
>  
>  %.o : %.c
>  	$(CC) $(CFLAGS) -c -o $@ $<
>  
> -all: $(GEN-IMAGE)
> +all: $(GEN-IMAGE) $(MONOTONIC_TS)
>  
>  $(GEN-IMAGE): gen-image.o
>  	$(CC) $(LDFLAGS) -o $@ $^ $(LIBS)
>  
> +$(MONOTONIC_TS): monotonic-ts.o
> +	$(CC) $(LDFLAGS) -o $@ $^ $(LIBS)
> +
>  clean:
>  	-rm -f *.o
> -	-rm -f $(GEN-IMAGE)
> +	-rm -f $(GEN-IMAGE) $(MONOTONIC_TS)
>  
>  install:
> -	cp $(GEN-IMAGE) $(INSTALL_DIR)/
> +	cp $(GEN-IMAGE) $(MONOTONIC_TS) $(INSTALL_DIR)/

I'd split this on two lines but I'm not sure why, so feel free to ignore
this :-)

> diff --git a/src/monotonic-ts.c b/src/monotonic-ts.c
> new file mode 100644
> index 000000000000..fcb671e06d27
> --- /dev/null
> +++ b/src/monotonic-ts.c
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* SPDX-FileCopyrightText: 2020 Kieran Bingham <kieran.bingham@ideasonboard.com> */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <time.h>
> +
> +int main(int argc, char ** argv)
> +{
> +	struct timespec tp;
> +	char *line = NULL;
> +	size_t size = 0;
> +	const char *label = "";
> +
> +	if (argc > 1)
> +		label = argv[1];
> +
> +	/*
> +	 * Explicitly set line buffering on stdin to be sure it is delivered
> +	 * in a timely fashion for our timestamping purposes when data is fed
> +	 * through a pipe.
> +	 */
> +	setlinebuf(stdin);
> +
> +	do {
> +		if (getline(&line, &size, stdin) <= 0)
> +			break;
> +
> +		clock_gettime(CLOCK_MONOTONIC, &tp);
> +		printf("[%ld.%.9ld]%s %s", tp.tv_sec, tp.tv_nsec, label, line);
> +	} while (!feof(stdin));
> +
> +	free(line);
> +
> +	return 0;
> +}
> +

Extra blank line.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [VSP-Tests PATCH 3/3] scripts/logger: Use new monotonic-ts tool

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wed, Sep 16, 2020 at 03:43:02PM +0100, Kieran Bingham wrote:
> Utilise the new monotonic timestamping tool to remove the manual parsing of
> timestamps via /proc/timer_list which can only be read by root.
> 
> This also simplifies the processing required and contains all timestamping
> actions within a single process space.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  scripts/logger.sh | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/scripts/logger.sh b/scripts/logger.sh
> index 97e1f582da2b..452ebc8c82ba 100755
> --- a/scripts/logger.sh
> +++ b/scripts/logger.sh
> @@ -2,23 +2,11 @@
>  # SPDX-License-Identifier: GPL-2.0-or-later
>  # SPDX-FileCopyrightText: 2016 Renesas Electronics Corporation
>  
> -now() {
> -	awk '/^now/ {time=$3; printf("[%u.%06u]", time / 1000000000, (time % 1000000000) / 1000) ; exit}' /proc/timer_list
> -}
> -
>  label=${1:+ [$1]}
>  
>  TRACE_MARKER=/sys/kernel/debug/tracing/trace_marker
> -if [ -e $TRACE_MARKER ]; then
> -	extra_log_files=$TRACE_MARKER
> +if [ -e $TRACE_MARKER ] && [ $(id -u) == 0 ]; then
> +	./monotonic-ts $label | tee -a $TRACE_MARKER
> +else
> +	./monotonic-ts $label
>  fi
> -
> -while read line ; do
> -	newline="$(now)$label $line"
> -
> -	echo "$newline"
> -
> -	for f in $extra_log_files; do
> -		echo "$newline" >> $f;
> -	done;
> -done

In logger.sh line 8:
if [ -e $TRACE_MARKER ] && [ $(id -u) == 0 ]; then
                             ^------^ SC2046: Quote this to prevent word splitting.
                                      ^-- SC2039: In POSIX sh, == in place of = is undefined.


In logger.sh line 9:
        ./monotonic-ts $label | tee -a $TRACE_MARKER
                       ^----^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
        ./monotonic-ts "$label" | tee -a $TRACE_MARKER


In logger.sh line 11:
        ./monotonic-ts $label
                       ^----^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
        ./monotonic-ts "$label"

For more information:
  https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, == in place of = is ...
  https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [VSP-Tests PATCH 2/3] src: monotonic-ts: Monotonic timestamp logging

[Kieran Bingham]

Hi Laurent,

On 17/09/2020 02:17, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Sep 16, 2020 at 03:43:01PM +0100, Kieran Bingham wrote:
>> Introduce a new utility which prefixes a monotonic timestamp rendered in the
>> same format as the kernel logs to all lines fed in through stdin.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/Makefile       | 10 +++++++---
>>  src/monotonic-ts.c | 37 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+), 3 deletions(-)
>>  create mode 100644 src/monotonic-ts.c
>>
>> diff --git a/src/Makefile b/src/Makefile
>> index d7f901f58be6..67216e81ffc4 100644
>> --- a/src/Makefile
>> +++ b/src/Makefile
>> @@ -7,18 +7,22 @@ CFLAGS	?= -O0 -g -W -Wall -Wno-unused-parameter -Iinclude
>>  LDFLAGS	?=
>>  LIBS	:= -lm
>>  GEN-IMAGE := gen-image
>> +MONOTONIC_TS := monotonic-ts
> 
> s/MONOTONIC_TS/MONOTONIC-TS/ to match GEN-IMAGE ?

Hrm, habits of thinking I can't use a hyphen here, yet - clearly we can ;-)

>>  
>>  %.o : %.c
>>  	$(CC) $(CFLAGS) -c -o $@ $<
>>  
>> -all: $(GEN-IMAGE)
>> +all: $(GEN-IMAGE) $(MONOTONIC_TS)
>>  
>>  $(GEN-IMAGE): gen-image.o
>>  	$(CC) $(LDFLAGS) -o $@ $^ $(LIBS)
>>  
>> +$(MONOTONIC_TS): monotonic-ts.o
>> +	$(CC) $(LDFLAGS) -o $@ $^ $(LIBS)
>> +
>>  clean:
>>  	-rm -f *.o
>> -	-rm -f $(GEN-IMAGE)
>> +	-rm -f $(GEN-IMAGE) $(MONOTONIC_TS)
>>  
>>  install:
>> -	cp $(GEN-IMAGE) $(INSTALL_DIR)/
>> +	cp $(GEN-IMAGE) $(MONOTONIC_TS) $(INSTALL_DIR)/
> 
> I'd split this on two lines but I'm not sure why, so feel free to ignore
> this :-)

It was two lines, and I looked at it and thought it should be one.
But two makes it easier to extend without modifying the existing
lines... so I actaully prefer two...

>> diff --git a/src/monotonic-ts.c b/src/monotonic-ts.c
>> new file mode 100644
>> index 000000000000..fcb671e06d27
>> --- /dev/null
>> +++ b/src/monotonic-ts.c
>> @@ -0,0 +1,37 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* SPDX-FileCopyrightText: 2020 Kieran Bingham <kieran.bingham@ideasonboard.com> */
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <time.h>
>> +
>> +int main(int argc, char ** argv)
>> +{
>> +	struct timespec tp;
>> +	char *line = NULL;
>> +	size_t size = 0;
>> +	const char *label = "";
>> +
>> +	if (argc > 1)
>> +		label = argv[1];
>> +
>> +	/*
>> +	 * Explicitly set line buffering on stdin to be sure it is delivered
>> +	 * in a timely fashion for our timestamping purposes when data is fed
>> +	 * through a pipe.
>> +	 */
>> +	setlinebuf(stdin);
>> +
>> +	do {
>> +		if (getline(&line, &size, stdin) <= 0)
>> +			break;
>> +
>> +		clock_gettime(CLOCK_MONOTONIC, &tp);
>> +		printf("[%ld.%.9ld]%s %s", tp.tv_sec, tp.tv_nsec, label, line);
>> +	} while (!feof(stdin));
>> +
>> +	free(line);
>> +
>> +	return 0;
>> +}
>> +
> 
> Extra blank line.

It's ok - you can have that one for free ;-)


> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, will update.


-- 
Regards
--
Kieran