[PATCHv2 0/5] VSP-Tests: Add suspend resume tests and helpers

[PATCHv2 0/5] VSP-Tests: Add suspend resume tests and helpers

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Following on from previous useful feedback, I have reworked the supporting
scripts and tests.

Provide two tests for suspend/resume cycles. One will verify the VSP1 is
functional with a test before and after a suspend cycle. The other will
maintain an active pipeline which must succeed despite a suspend resume cycle
occuring in the middle of the test.

Along side these tests are a couple of helpers that I have locally and thought
they might be useful to others, so I'm posting for review. A test suite runner
simplifies executing all vsp-unit tests, and provides the ability to easily
repeat the test suite (for example to run an overnight longevity test).

'bin2png.sh' is a wrapper around the existing tools that helps convert the test
files generated by VSP-Tests into png files for easy viewing.

Finally, I have extended 'logger.sh' to also log to the FTrace buffer. As I have
been making greater use of ftrace for register write capture, and driver flow -
this addition helps separate multiple tests from the ftrace kernelshark view.

Kieran Bingham (5):
  scripts: Test suite runner
  scripts: Provide bin2png.sh helper
  logger: Log to the FTrace buffer if tracing is enabled
  tests: Test suspend/resume on idle pipelines
  tests: Test suspend/resume on active pipelines

 scripts/Makefile            |   2 +-
 scripts/bin2png.sh          |  36 ++++++++++++++++
 scripts/logger.sh           |  13 +++++-
 scripts/vsp-tests.sh        |  49 +++++++++++++++++++++
 tests/vsp-unit-test-0019.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++
 tests/vsp-unit-test-0020.sh |  97 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 296 insertions(+), 2 deletions(-)
 create mode 100755 scripts/bin2png.sh
 create mode 100755 scripts/vsp-tests.sh
 create mode 100755 tests/vsp-unit-test-0019.sh
 create mode 100755 tests/vsp-unit-test-0020.sh

-- 
2.7.4

[PATCHv2 1/5] scripts: Test suite runner

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide a utility script to execute all vsp unit tests, as well
as the option to execute multiple iterations of the suite.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
v2
- remove spurious uses of ';'
- fix output logging

 scripts/vsp-tests.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100755 scripts/vsp-tests.sh

diff --git a/scripts/vsp-tests.sh b/scripts/vsp-tests.sh
new file mode 100755
index 000000000000..8b21176ccc09
--- /dev/null
+++ b/scripts/vsp-tests.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+
+##
+## VSP Tests runner
+##
+## Automatically execute all vsp-unit tests
+## Move test failure results to a specific folder for
+## the running kernel version
+##
+## An argument can be provided to specify the number of
+## iterations to perform
+##
+## usage:
+##  ./vsp-tests.sh <n>
+##
+##   n: Number of iterations to execute test suite
+##
+
+KERNEL_VERSION=`uname -r`
+
+run_test() {
+	echo $1
+	./$1
+
+	if [ $(ls *.bin 2>/dev/null | wc -l) != 0 ]
+	then
+		RESULTS_DIR=$KERNEL_VERSION/test-$1/$2/
+
+		echo "Moving *.bin to $RESULTS_DIR"
+		mkdir -p $RESULTS_DIR
+		mv *.bin $RESULTS_DIR
+		for f in $RESULTS_DIR/*.bin
+		do
+			./bin2png.sh "$f"
+		done
+	fi
+}
+
+run_suite() {
+	echo "Test loop $1"
+
+	for test in vsp-unit-test*.sh; do
+		run_test $test $1;
+	done;
+}
+
+for loop in `seq 1 1 $1`; do
+	run_suite $loop
+done;
-- 
2.7.4

[PATCHv2 2/5] scripts: Provide bin2png.sh helper

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Identify the size and format from the test output filename, and pass
to raw2rgbpnm for conversion to a PNM file.

>From there we can convert easily to a PNG output file.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
v2:

- use 'convert' to proces png files to png
- strip '.bin' from target filenames

 scripts/Makefile   |  2 +-
 scripts/bin2png.sh | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100755 scripts/bin2png.sh

diff --git a/scripts/Makefile b/scripts/Makefile
index 8c452f4c54ce..6586b2989aed 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -1,4 +1,4 @@
-SCRIPTS=logger.sh vsp-lib.sh
+SCRIPTS=$(wildcard *.sh)
 
 all:
 
diff --git a/scripts/bin2png.sh b/scripts/bin2png.sh
new file mode 100755
index 000000000000..bde1ddfa3eab
--- /dev/null
+++ b/scripts/bin2png.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+FILE="$1"
+
+PNM=$(echo $FILE | sed -e 's|\.bin$|.pnm|')
+PNG=$(echo $FILE | sed -e 's|\.bin$|.png|')
+
+fmt=$(echo $FILE | sed -e 's|.*-\([[:alnum:]]*\)-\([0-9]*x[0-9]*\).*.bin|\1|')
+size=$(echo $FILE | sed -e 's|.*-\([[:alnum:]]*\)-\([0-9]*x[0-9]*\).*.bin|\2|')
+
+case $fmt in
+	yuv410m|yvu410m|yuv411m|yuv420m|yvu420m|yuv422m|yvu422m|yuv444m|yvu444m)
+		fmt=`echo $fmt | tr '[:lower:]' '[:upper:]'`
+		fmt=`echo $fmt | tr 'M' 'P'`
+		;;
+	nv12m|nv21m|nv16m|nv61m)
+		fmt=`echo $fmt | tr '[:lower:]' '[:upper:]'`
+		fmt=`echo $fmt | tr -d 'M'`
+		;;
+	argb555|xrgb555)
+		fmt=RGB555X
+		;;
+	argb32|xrgb32)
+		fmt=RGB32
+		;;
+	abgr32|xbgr32)
+		fmt=BGR32
+		;;
+	*)
+		fmt=`echo $fmt | tr '[:lower:]' '[:upper:]'`
+		;;
+esac
+
+raw2rgbpnm -s $size -f $fmt $FILE $PNM && \
+	convert $PNM $PNG
+rm $PNM
-- 
2.7.4

[PATCHv2 3/5] logger: Log to the FTrace buffer if tracing is enabled

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Extend the logger such that it will detect the tracing system, and also
append print statement to this ring buffer.

This provides the relevant logging output interspersed in the ftrace
logs for an effective solution to identifying the actions that caused
the traces to occur

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

diff --git a/scripts/logger.sh b/scripts/logger.sh
index 8123f0c9f6e3..8412b0ba9a08 100755
--- a/scripts/logger.sh
+++ b/scripts/logger.sh
@@ -6,6 +6,17 @@ now() {
 
 label=${1:+ [$1]}
 
+TRACE_MARKER=/sys/kernel/debug/tracing/trace_marker
+if [ -e $TRACE_MARKER ]; then
+	extra_log_files=$TRACE_MARKER
+fi
+
 while read line ; do
-	echo "$(now)$label $line"
+	newline="$(now)$label $line"
+
+	echo "$newline"
+
+	for f in $extra_log_files; do
+		echo "$newline" >> $f;
+	done;
 done
-- 
2.7.4

[PATCHv2 4/5] tests: Test suspend/resume on idle pipelines

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide a test to verify the hardware is functional both before and
after entering a suspend / resume cycle. Make use of the
/sys/power/pm_test functionality provided by CONFIG_PM_DEBUG to perform
the testing

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
v2:

- extended pipeline test to run twice to verify stop->start process
- added return value checking to track test progress
- modified test reporting to be once per 'suspend mode'
- removed trailing ';' uses
- verify the pm_test mode is available, or skip

 tests/vsp-unit-test-0019.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100755 tests/vsp-unit-test-0019.sh

diff --git a/tests/vsp-unit-test-0019.sh b/tests/vsp-unit-test-0019.sh
new file mode 100755
index 000000000000..728dcdda29e6
--- /dev/null
+++ b/tests/vsp-unit-test-0019.sh
@@ -0,0 +1,101 @@
+#!/bin/sh
+
+#
+# Test power-management suspend/resume whilst pipelines are idle
+#
+# Utilise the basic RPF->WPF packing test case as a measure that the hardware
+# is operable while we perform test suspend and resume, and verify that it is
+# still operable after resume.
+#
+# Format iteration loops are maintained, even with only one format so that this
+# test can be easily extended to try further formats if needed in the future.
+#
+
+source vsp-lib.sh
+
+features="rpf.0 wpf.0"
+
+# Two formats are specified so that the test is run twice.
+# This ensures that stop -> start works both before and after suspend
+formats="RGB24 RGB24"
+
+# These can be extracted from /sys/power/pm_test
+suspend_modes="freezer devices platform processors core"
+
+test_wpf_packing() {
+	pipe_configure rpf-wpf 0 0
+	format_configure rpf-wpf 0 0 ARGB32 1024x768 $format
+
+	vsp_runner rpf.0 &
+	vsp_runner wpf.0
+
+	local result=$(compare_frames)
+	[ x$result == xpass ] && return 0 || return 1
+}
+
+test_hw_pipe() {
+	local format
+	local result
+
+	for format in $formats ; do
+		test_wpf_packing $format
+		result=$?
+
+		# return early on failure
+		[ $result != 0 ] && return 1
+	done
+
+	return 0
+}
+
+test_suspend_resume() {
+	local result=0
+
+	test_start "non-active pipeline suspend/resume in suspend:$mode"
+
+	# Verify the test is available
+	grep -q $mode /sys/power/pm_test
+	if [ $? != 0 ]; then
+		test_complete skip
+		return
+	fi
+
+	# Test the hardware each side of suspend resume
+	test_hw_pipe
+	result=$((result+$?))
+
+	# Set the test mode
+	echo $mode > /sys/power/pm_test
+
+	# Comence suspend
+	# The pm_test framework will automatically resume after 5 seconds
+	echo mem > /sys/power/state
+
+	# Verify the hardware is still operational
+	test_hw_pipe
+	result=$((result+$?))
+
+	if [ $result != 0 ]; then
+		test_complete "failed"
+	else
+		test_complete "passed"
+	fi
+}
+
+test_main() {
+	local mode
+
+	# Check for pm-suspend test option
+	if [ ! -e /sys/power/pm_test ] ; then
+		echo "$0: Suspend Resume testing requires CONFIG_PM_DEBUG"
+		test_complete skip
+		return
+	fi
+
+	for mode in $suspend_modes ; do
+		test_suspend_resume $mode
+	done
+}
+
+test_init $0 "$features"
+test_run
-- 
2.7.4

[PATCHv2 5/5] tests: Test suspend/resume on active pipelines

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide a test to verify the hardware completes a functional test whilst
performing a suspend resume cycle in parallel. Make use of the
/sys/power/pm_test functionality provided by CONFIG_PM_DEBUG to perform
the testing

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
v2:

- removed format iteration loop
- modified test reporting to be once per 'suspend mode'
- verify the pm_test mode is available, or skip

 tests/vsp-unit-test-0020.sh | 97 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)
 create mode 100755 tests/vsp-unit-test-0020.sh

diff --git a/tests/vsp-unit-test-0020.sh b/tests/vsp-unit-test-0020.sh
new file mode 100755
index 000000000000..c9e6b79e5d06
--- /dev/null
+++ b/tests/vsp-unit-test-0020.sh
@@ -0,0 +1,97 @@
+#!/bin/sh
+
+#
+# Test power-management suspend/resume whilst pipelines are active
+#
+# Utilise the basic RPF->WPF packing test case as a measure that the hardware
+# is operable while we perform test suspend and resume, and verify that it is
+# still successful even with a suspend resume cycle in the middle of the test.
+#
+
+source vsp-lib.sh
+
+features="rpf.0 wpf.0"
+
+# These can be extracted from /sys/power/pm_test
+suspend_modes="freezer devices platform processors core"
+
+# This extended function performs the same
+# as it's non-extended name-sake - but runs the pipeline
+# for 300 frames. The suspend action occurs between frame #150~#200
+
+test_extended_wpf_packing() {
+	pipe_configure rpf-wpf 0 0
+	format_configure rpf-wpf 0 0 ARGB32 1024x768 $format
+
+	vsp_runner rpf.0 --count=300 &
+	vsp_runner wpf.0 --count=300 --skip=297
+
+	local result=$(compare_frames)
+
+	test_complete $result
+}
+
+test_hw_pipe() {
+	local format
+
+	for format in $formats ; do
+		test_extended_wpf_packing RGB24
+	done
+}
+
+test_suspend_resume() {
+	local result
+	local test_pid
+
+	test_start "Testing active pipeline suspend/resume in suspend:$mode"
+
+	# Verify the test is available
+	grep -q $mode /sys/power/pm_test
+	if [ $? != 0 ]; then
+		test_complete skip
+		return
+	fi
+
+	# Set the hardware running in parallel while we suspend
+	test_hw_pipe &
+	test_pid=$!
+
+	# Make sure the pipeline has time to start
+	sleep 1
+
+	# Set the test mode
+	echo $mode > /sys/power/pm_test
+
+	# Comence suspend
+	# The pm_test framework will automatically resume after 5 seconds
+	echo mem > /sys/power/state
+
+	# Wait for the pipeline to complete
+	wait $test_pid
+	result=$?
+
+	if [ $result == 0 ]; then
+		test_complete pass
+	else
+		test_complete fail
+	fi
+}
+
+test_main() {
+	local mode;
+	local suspend_test_failures
+
+	# Check for pm-suspend test option
+	if [ ! -e /sys/power/pm_test ] ; then
+		echo "$0: Suspend Resume testing requires CONFIG_PM_DEBUG"
+		test_complete skip
+		return
+	fi;
+
+	for mode in $suspend_modes ; do
+		test_suspend_resume $mode
+	done;
+}
+
+test_init $0 "$features"
+test_run
-- 
2.7.4

Re: [PATCHv2 1/5] scripts: Test suite runner

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday 01 Dec 2016 21:31:45 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Provide a utility script to execute all vsp unit tests, as well
> as the option to execute multiple iterations of the suite.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v2
> - remove spurious uses of ';'
> - fix output logging
> 
>  scripts/vsp-tests.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>  create mode 100755 scripts/vsp-tests.sh
> 
> diff --git a/scripts/vsp-tests.sh b/scripts/vsp-tests.sh
> new file mode 100755
> index 000000000000..8b21176ccc09
> --- /dev/null
> +++ b/scripts/vsp-tests.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +
> +##
> +## VSP Tests runner
> +##
> +## Automatically execute all vsp-unit tests
> +## Move test failure results to a specific folder for
> +## the running kernel version
> +##
> +## An argument can be provided to specify the number of
> +## iterations to perform
> +##
> +## usage:
> +##  ./vsp-tests.sh <n>
> +##
> +##   n: Number of iterations to execute test suite
> +##
> +
> +KERNEL_VERSION=`uname -r`
> +
> +run_test() {
> +	echo $1

I would print "- $1" to make the output easier to read.

> +	./$1
> +
> +	if [ $(ls *.bin 2>/dev/null | wc -l) != 0 ]
> +	then
> +		RESULTS_DIR=$KERNEL_VERSION/test-$1/$2/

Let's make local variables local.

> +
> +		echo "Moving *.bin to $RESULTS_DIR"

I would remove this message, it doesn't add much value.

> +		mkdir -p $RESULTS_DIR
> +		mv *.bin $RESULTS_DIR
> +		for f in $RESULTS_DIR/*.bin
> +		do
> +			./bin2png.sh "$f"
> +		done

Converting binary files to png on the target would slow down the tests. How 
about performing the conversion on the host instead ?

> +	fi
> +}
> +
> +run_suite() {
> +	echo "Test loop $1"

How about "--- Test loop $1 ---" here for the same reason as above ?

No need to resubmit, I'll fix while applying.

> +
> +	for test in vsp-unit-test*.sh; do
> +		run_test $test $1;
> +	done;
> +}
> +
> +for loop in `seq 1 1 $1`; do
> +	run_suite $loop
> +done;

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 2/5] scripts: Provide bin2png.sh helper

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday 01 Dec 2016 21:31:46 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Identify the size and format from the test output filename, and pass
> to raw2rgbpnm for conversion to a PNM file.
> 
> From there we can convert easily to a PNG output file.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v2:
> 
> - use 'convert' to proces png files to png
> - strip '.bin' from target filenames
> 
>  scripts/Makefile   |  2 +-
>  scripts/bin2png.sh | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)
>  create mode 100755 scripts/bin2png.sh
> 
> diff --git a/scripts/Makefile b/scripts/Makefile
> index 8c452f4c54ce..6586b2989aed 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -1,4 +1,4 @@
> -SCRIPTS=logger.sh vsp-lib.sh
> +SCRIPTS=$(wildcard *.sh)
> 
>  all:
> 
> diff --git a/scripts/bin2png.sh b/scripts/bin2png.sh
> new file mode 100755
> index 000000000000..bde1ddfa3eab
> --- /dev/null
> +++ b/scripts/bin2png.sh
> @@ -0,0 +1,36 @@
> +#!/bin/sh
> +
> +FILE="$1"
> +
> +PNM=$(echo $FILE | sed -e 's|\.bin$|.pnm|')

You can write this as

PNM=${FILE/.bin/.pnm}

> +PNG=$(echo $FILE | sed -e 's|\.bin$|.png|')

Ditto.

> +fmt=$(echo $FILE | sed -e
> 's|.*-\([[:alnum:]]*\)-\([0-9]*x[0-9]*\).*.bin|\1|') +size=$(echo $FILE |
> sed -e 's|.*-\([[:alnum:]]*\)-\([0-9]*x[0-9]*\).*.bin|\2|') +
> +case $fmt in
> +	yuv410m|yvu410m|yuv411m|yuv420m|yvu420m|yuv422m|yvu422m|yuv444m|
yvu444m)
> +		fmt=`echo $fmt | tr '[:lower:]' '[:upper:]'`
> +		fmt=`echo $fmt | tr 'M' 'P'`
> +		;;
> +	nv12m|nv21m|nv16m|nv61m)
> +		fmt=`echo $fmt | tr '[:lower:]' '[:upper:]'`
> +		fmt=`echo $fmt | tr -d 'M'`
> +		;;
> +	argb555|xrgb555)
> +		fmt=RGB555X

raw2rgbpnm doesn't support RGB555X, I think you should use RGB555.

> +		;;
> +	argb32|xrgb32)
> +		fmt=RGB32
> +		;;
> +	abgr32|xbgr32)
> +		fmt=BGR32
> +		;;

You could group all those cases by just removing the leading A or X.

No need to resubmit, I'll fix while applying.

> +	*)
> +		fmt=`echo $fmt | tr '[:lower:]' '[:upper:]'`
> +		;;
> +esac
> +
> +raw2rgbpnm -s $size -f $fmt $FILE $PNM && \
> +	convert $PNM $PNG
> +rm $PNM

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 2/5] scripts: Provide bin2png.sh helper

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Feb 13, 2017 at 3:03 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> +PNM=$(echo $FILE | sed -e 's|\.bin$|.pnm|')
>
> You can write this as
>
> PNM=${FILE/.bin/.pnm}

/ doesn't just match the suffix, try with waste.bin.picture.bin :-)

PNM=${FILE%.bin}.pnm

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCHv2 3/5] logger: Log to the FTrace buffer if tracing is enabled

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday 01 Dec 2016 21:31:47 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Extend the logger such that it will detect the tracing system, and also
> append print statement to this ring buffer.
> 
> This provides the relevant logging output interspersed in the ftrace
> logs for an effective solution to identifying the actions that caused
> the traces to occur
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

> ---
>  scripts/logger.sh | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/logger.sh b/scripts/logger.sh
> index 8123f0c9f6e3..8412b0ba9a08 100755
> --- a/scripts/logger.sh
> +++ b/scripts/logger.sh
> @@ -6,6 +6,17 @@ now() {
> 
>  label=${1:+ [$1]}
> 
> +TRACE_MARKER=/sys/kernel/debug/tracing/trace_marker
> +if [ -e $TRACE_MARKER ]; then
> +	extra_log_files=$TRACE_MARKER
> +fi
> +
>  while read line ; do
> -	echo "$(now)$label $line"
> +	newline="$(now)$label $line"
> +
> +	echo "$newline"
> +
> +	for f in $extra_log_files; do
> +		echo "$newline" >> $f;
> +	done;
>  done

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 2/5] scripts: Provide bin2png.sh helper

[Laurent Pinchart]

Hi Geert,

On Monday 13 Feb 2017 15:17:10 Geert Uytterhoeven wrote:
> On Mon, Feb 13, 2017 at 3:03 PM, Laurent Pinchart wrote:
> >> +PNM=$(echo $FILE | sed -e 's|\.bin$|.pnm|')
> > 
> > You can write this as
> > 
> > PNM=${FILE/.bin/.pnm}
> 
> / doesn't just match the suffix, try with waste.bin.picture.bin :-)

There's a single '.bin' string in the file names we deal with, but in general 
you're right, yes.

> PNM=${FILE%.bin}.pnm

I've used PNM={FILE/%bin/pnm}

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 5/5] tests: Test suspend/resume on active pipelines

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday 01 Dec 2016 21:31:49 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Provide a test to verify the hardware completes a functional test whilst
> performing a suspend resume cycle in parallel. Make use of the
> /sys/power/pm_test functionality provided by CONFIG_PM_DEBUG to perform
> the testing
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v2:
> 
> - removed format iteration loop
> - modified test reporting to be once per 'suspend mode'
> - verify the pm_test mode is available, or skip
> 
>  tests/vsp-unit-test-0020.sh | 97 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
>  create mode 100755 tests/vsp-unit-test-0020.sh
> 
> diff --git a/tests/vsp-unit-test-0020.sh b/tests/vsp-unit-test-0020.sh
> new file mode 100755
> index 000000000000..c9e6b79e5d06
> --- /dev/null
> +++ b/tests/vsp-unit-test-0020.sh
> @@ -0,0 +1,97 @@
> +#!/bin/sh
> +
> +#
> +# Test power-management suspend/resume whilst pipelines are active
> +#
> +# Utilise the basic RPF->WPF packing test case as a measure that the
> hardware +# is operable while we perform test suspend and resume, and
> verify that it is +# still successful even with a suspend resume cycle in
> the middle of the test. +#
> +
> +source vsp-lib.sh
> +
> +features="rpf.0 wpf.0"
> +
> +# These can be extracted from /sys/power/pm_test
> +suspend_modes="freezer devices platform processors core"
> +
> +# This extended function performs the same
> +# as it's non-extended name-sake - but runs the pipeline
> +# for 300 frames. The suspend action occurs between frame #150~#200
> +
> +test_extended_wpf_packing() {
> +	pipe_configure rpf-wpf 0 0
> +	format_configure rpf-wpf 0 0 ARGB32 1024x768 $format
> +
> +	vsp_runner rpf.0 --count=300 &
> +	vsp_runner wpf.0 --count=300 --skip=297
> +
> +	local result=$(compare_frames)
> +
> +	test_complete $result

This will result in test_complete being called twice. You probably want

        [ x$result == xpass ] && return 0 || return 1

as in patch 4/5.

> +}
> +
> +test_hw_pipe() {
> +	local format
> +
> +	for format in $formats ; do

The formats variable isn't defined, I think you can just get rid of the loop 
and call test_extended_wpf_packing just once.

> +		test_extended_wpf_packing RGB24
> +	done
> +}
> +
> +test_suspend_resume() {
> +	local result
> +	local test_pid
> +
> +	test_start "Testing active pipeline suspend/resume in suspend:$mode"
> +
> +	# Verify the test is available
> +	grep -q $mode /sys/power/pm_test
> +	if [ $? != 0 ]; then
> +		test_complete skip
> +		return
> +	fi
> +
> +	# Set the hardware running in parallel while we suspend
> +	test_hw_pipe &
> +	test_pid=$!
> +
> +	# Make sure the pipeline has time to start
> +	sleep 1
> +
> +	# Set the test mode
> +	echo $mode > /sys/power/pm_test
> +
> +	# Comence suspend

s/Comence/Commence/

> +	# The pm_test framework will automatically resume after 5 seconds
> +	echo mem > /sys/power/state
> +
> +	# Wait for the pipeline to complete
> +	wait $test_pid
> +	result=$?
> +
> +	if [ $result == 0 ]; then
> +		test_complete pass
> +	else
> +		test_complete fail
> +	fi
> +}
> +
> +test_main() {
> +	local mode;

No need for the trailing ;

> +	local suspend_test_failures

This variablee is unused.

> +
> +	# Check for pm-suspend test option
> +	if [ ! -e /sys/power/pm_test ] ; then
> +		echo "$0: Suspend Resume testing requires CONFIG_PM_DEBUG"
> +		test_complete skip

You haven't called test_start so you shouldn't call test_complete.

Some of those comments apply to patch 4/5 too.

There's no need to resubmit, I'll fix this while applying.

> +		return
> +	fi;
> +
> +	for mode in $suspend_modes ; do
> +		test_suspend_resume $mode
> +	done;
> +}
> +
> +test_init $0 "$features"
> +test_run

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 2/5] scripts: Provide bin2png.sh helper

[Kieran Bingham]

Hi Laurent,

Thanks for your integration and cleanups.

One issue arisen to resolve:

On 13/02/17 14:03, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thursday 01 Dec 2016 21:31:46 Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> Identify the size and format from the test output filename, and pass
>> to raw2rgbpnm for conversion to a PNM file.
>>
>> From there we can convert easily to a PNG output file.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> ---
>> v2:
>>
>> - use 'convert' to proces png files to png
>> - strip '.bin' from target filenames
>>
>>  scripts/Makefile   |  2 +-
>>  scripts/bin2png.sh | 36 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 37 insertions(+), 1 deletion(-)
>>  create mode 100755 scripts/bin2png.sh
>>
>> diff --git a/scripts/Makefile b/scripts/Makefile
>> index 8c452f4c54ce..6586b2989aed 100644
>> --- a/scripts/Makefile
>> +++ b/scripts/Makefile
>> @@ -1,4 +1,4 @@
>> -SCRIPTS=logger.sh vsp-lib.sh
>> +SCRIPTS=$(wildcard *.sh)
>>
>>  all:
>>
>> diff --git a/scripts/bin2png.sh b/scripts/bin2png.sh
>> new file mode 100755
>> index 000000000000..bde1ddfa3eab
>> --- /dev/null
>> +++ b/scripts/bin2png.sh
>> @@ -0,0 +1,36 @@
>> +#!/bin/sh
>> +
>> +FILE="$1"
>> +
>> +PNM=$(echo $FILE | sed -e 's|\.bin$|.pnm|')
> 
> You can write this as
> 
> PNM=${FILE/.bin/.pnm}
> 
>> +PNG=$(echo $FILE | sed -e 's|\.bin$|.png|')
> 
> Ditto.

This change breaks POSIX SH compliance, as reported by shellcheck:

In scripts/bin2png.sh line 7:
        local pnm=${file/%bin/pnm}
                  ^-- SC2039: In POSIX sh, string replacement is not supported.


In scripts/bin2png.sh line 8:
        local png=${file/%bin/png}
                  ^-- SC2039: In POSIX sh, string replacement is not supported.

It also breaks on my system which uses a strict posix compliant shell :D

> 
>> +fmt=$(echo $FILE | sed -e
>> 's|.*-\([[:alnum:]]*\)-\([0-9]*x[0-9]*\).*.bin|\1|') +size=$(echo $FILE |
>> sed -e 's|.*-\([[:alnum:]]*\)-\([0-9]*x[0-9]*\).*.bin|\2|') +
>> +case $fmt in
>> +	yuv410m|yvu410m|yuv411m|yuv420m|yvu420m|yuv422m|yvu422m|yuv444m|
> yvu444m)
>> +		fmt=`echo $fmt | tr '[:lower:]' '[:upper:]'`
>> +		fmt=`echo $fmt | tr 'M' 'P'`
>> +		;;
>> +	nv12m|nv21m|nv16m|nv61m)
>> +		fmt=`echo $fmt | tr '[:lower:]' '[:upper:]'`
>> +		fmt=`echo $fmt | tr -d 'M'`
>> +		;;
>> +	argb555|xrgb555)
>> +		fmt=RGB555X
> 
> raw2rgbpnm doesn't support RGB555X, I think you should use RGB555.
> 
>> +		;;
>> +	argb32|xrgb32)
>> +		fmt=RGB32
>> +		;;
>> +	abgr32|xbgr32)
>> +		fmt=BGR32
>> +		;;
> 
> You could group all those cases by just removing the leading A or X.

Yes, that is a nice cleanup!

> 
> No need to resubmit, I'll fix while applying.
> 
>> +	*)
>> +		fmt=`echo $fmt | tr '[:lower:]' '[:upper:]'`

I see you now convert to upper case in one early stage as well ... that's a good
call.

>> +		;;
>> +esac
>> +
>> +raw2rgbpnm -s $size -f $fmt $FILE $PNM && \
>> +	convert $PNM $PNG
>> +rm $PNM

So - just a revert of the shell string replacement change required. I'll queue
up a patch :D
--
Kieran

Re: [PATCHv2 2/5] scripts: Provide bin2png.sh helper

[Geert Uytterhoeven]

Hi Kieran,

On Mon, Feb 20, 2017 at 6:47 PM, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> This change breaks POSIX SH compliance, as reported by shellcheck:
>
> In scripts/bin2png.sh line 7:
>         local pnm=${file/%bin/pnm}
>                   ^-- SC2039: In POSIX sh, string replacement is not supported.
>
>
> In scripts/bin2png.sh line 8:
>         local png=${file/%bin/png}
>                   ^-- SC2039: In POSIX sh, string replacement is not supported.
>
> It also breaks on my system which uses a strict posix compliant shell :D

Does "local pnm=${file%bin}pnm" work?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCHv2 2/5] scripts: Provide bin2png.sh helper

[Kieran Bingham]

Hi Geert,

On 20/02/17 20:45, Geert Uytterhoeven wrote:
> Hi Kieran,
> 
> On Mon, Feb 20, 2017 at 6:47 PM, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
>> This change breaks POSIX SH compliance, as reported by shellcheck:
>>
>> In scripts/bin2png.sh line 7:
>>         local pnm=${file/%bin/pnm}
>>                   ^-- SC2039: In POSIX sh, string replacement is not supported.
>>
>>
>> In scripts/bin2png.sh line 8:
>>         local png=${file/%bin/png}
>>                   ^-- SC2039: In POSIX sh, string replacement is not supported.
>>
>> It also breaks on my system which uses a strict posix compliant shell :D
> 
> Does "local pnm=${file%bin}pnm" work?

Yes, it does indeed seem to work so far!

Thanks for the hint.

I'll test a bit more and hopefully send a patch tomorrow :D

--
Kieran

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>