All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data
@ 2019-05-14 13:13 Simon Ser
  2019-05-14 13:51 ` Ville Syrjälä
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Simon Ser @ 2019-05-14 13:13 UTC (permalink / raw)
  To: igt-dev

audio_signal_detect uses gsl_fft_real_radix2_transform which mutates the data
array. This can be surprising when calling audio_signal_detect and then read
again the data (e.g. for another check).

Instead of mutating the array, make audio_signal_detect less error-prone by
taking a const parameter. Do an internal copy before calling the gsl function.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_audio.c | 18 +++++++++++++++---
 lib/igt_audio.h |  2 +-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/lib/igt_audio.c b/lib/igt_audio.c
index fd8cf07c0de3..d0e34b275fa2 100644
--- a/lib/igt_audio.c
+++ b/lib/igt_audio.c
@@ -251,11 +251,14 @@ void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
  * Checks that frequencies specified in signal, and only those, are included
  * in the input data.
  *
- * sampling_rate is given in Hz. data_len is the number of elements in data.
+ * sampling_rate is given in Hz. samples_len is the number of elements in
+ * samples.
  */
 bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
-			 int channel, double *data, size_t data_len)
+			 int channel, const double *samples, size_t samples_len)
 {
+	double *data;
+	size_t data_len = samples_len;
 	size_t bin_power_len = data_len / 2 + 1;
 	double bin_power[bin_power_len];
 	bool detected[FREQS_MAX];
@@ -264,12 +267,19 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
 	size_t i, j;
 	bool above, success;

+	/* gsl will mutate the array in-place, so make a copy */
+	data = malloc(samples_len * sizeof(double));
+	memcpy(data, samples, samples_len * sizeof(double));
+
 	/* Allowed error in Hz due to FFT step */
 	freq_accuracy = sampling_rate / data_len;
 	igt_debug("Allowed freq. error: %d Hz\n", freq_accuracy);

 	ret = gsl_fft_real_radix2_transform(data, 1, data_len);
-	igt_assert(ret == 0);
+	if (ret != 0) {
+		free(data);
+		igt_assert(0);
+	}

 	/* Compute the power received by every bin of the FFT, and record the
 	 * maximum power received as a way to normalize all the others.
@@ -372,6 +382,8 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
 		}
 	}

+	free(data);
+
 	return success;
 }

diff --git a/lib/igt_audio.h b/lib/igt_audio.h
index 466e772a75a4..d5ba1caaca63 100644
--- a/lib/igt_audio.h
+++ b/lib/igt_audio.h
@@ -43,7 +43,7 @@ void audio_signal_reset(struct audio_signal *signal);
 void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
 		       size_t buffer_len);
 bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
-			 int channel, double *data, size_t data_len);
+			 int channel, const double *samples, size_t samples_len);
 size_t audio_extract_channel_s32_le(double *dst, size_t dst_cap,
 				    int32_t *src, size_t src_len,
 				    int n_channels, int channel);
--
2.21.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data
  2019-05-14 13:13 [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data Simon Ser
@ 2019-05-14 13:51 ` Ville Syrjälä
  2019-05-15  7:50   ` Arkadiusz Hiler
  2019-05-14 14:21 ` [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_audio: make audio_signal_detect take const data (rev2) Patchwork
  2019-05-14 21:10 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
  2 siblings, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2019-05-14 13:51 UTC (permalink / raw)
  To: Simon Ser; +Cc: igt-dev

On Tue, May 14, 2019 at 04:13:24PM +0300, Simon Ser wrote:
> audio_signal_detect uses gsl_fft_real_radix2_transform which mutates the data
> array. This can be surprising when calling audio_signal_detect and then read
> again the data (e.g. for another check).
> 
> Instead of mutating the array, make audio_signal_detect less error-prone by
> taking a const parameter. Do an internal copy before calling the gsl function.
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
>  lib/igt_audio.c | 18 +++++++++++++++---
>  lib/igt_audio.h |  2 +-
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/igt_audio.c b/lib/igt_audio.c
> index fd8cf07c0de3..d0e34b275fa2 100644
> --- a/lib/igt_audio.c
> +++ b/lib/igt_audio.c
> @@ -251,11 +251,14 @@ void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
>   * Checks that frequencies specified in signal, and only those, are included
>   * in the input data.
>   *
> - * sampling_rate is given in Hz. data_len is the number of elements in data.
> + * sampling_rate is given in Hz. samples_len is the number of elements in
> + * samples.
>   */
>  bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> -			 int channel, double *data, size_t data_len)
> +			 int channel, const double *samples, size_t samples_len)
>  {
> +	double *data;
> +	size_t data_len = samples_len;
>  	size_t bin_power_len = data_len / 2 + 1;
>  	double bin_power[bin_power_len];
>  	bool detected[FREQS_MAX];
> @@ -264,12 +267,19 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
>  	size_t i, j;
>  	bool above, success;
> 
> +	/* gsl will mutate the array in-place, so make a copy */
> +	data = malloc(samples_len * sizeof(double));
> +	memcpy(data, samples, samples_len * sizeof(double));

Maybe some kind of memdup() might be useful. Not sure how many instances
of this pattern we have in igt.

> +
>  	/* Allowed error in Hz due to FFT step */
>  	freq_accuracy = sampling_rate / data_len;
>  	igt_debug("Allowed freq. error: %d Hz\n", freq_accuracy);
> 
>  	ret = gsl_fft_real_radix2_transform(data, 1, data_len);
> -	igt_assert(ret == 0);
> +	if (ret != 0) {
> +		free(data);
> +		igt_assert(0);
> +	}

It's an assert. Do we really care about cleanup?

Patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
>  	/* Compute the power received by every bin of the FFT, and record the
>  	 * maximum power received as a way to normalize all the others.
> @@ -372,6 +382,8 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
>  		}
>  	}
> 
> +	free(data);
> +
>  	return success;
>  }
> 
> diff --git a/lib/igt_audio.h b/lib/igt_audio.h
> index 466e772a75a4..d5ba1caaca63 100644
> --- a/lib/igt_audio.h
> +++ b/lib/igt_audio.h
> @@ -43,7 +43,7 @@ void audio_signal_reset(struct audio_signal *signal);
>  void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
>  		       size_t buffer_len);
>  bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> -			 int channel, double *data, size_t data_len);
> +			 int channel, const double *samples, size_t samples_len);
>  size_t audio_extract_channel_s32_le(double *dst, size_t dst_cap,
>  				    int32_t *src, size_t src_len,
>  				    int n_channels, int channel);
> --
> 2.21.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_audio: make audio_signal_detect take const data (rev2)
  2019-05-14 13:13 [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data Simon Ser
  2019-05-14 13:51 ` Ville Syrjälä
@ 2019-05-14 14:21 ` Patchwork
  2019-05-14 21:10 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
  2 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2019-05-14 14:21 UTC (permalink / raw)
  To: Simon Ser; +Cc: igt-dev

== Series Details ==

Series: lib/igt_audio: make audio_signal_detect take const data (rev2)
URL   : https://patchwork.freedesktop.org/series/60585/
State : success

== Summary ==

CI Bug Log - changes from IGT_4988 -> IGTPW_2982
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/60585/revisions/2/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2982 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      [PASS][1] -> [DMESG-FAIL][2] ([fdo#110235])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_hangcheck:
    - fi-kbl-8809g:       [DMESG-FAIL][3] -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/fi-kbl-8809g/igt@i915_selftest@live_hangcheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/fi-kbl-8809g/igt@i915_selftest@live_hangcheck.html

  
  [fdo#110235]: https://bugs.freedesktop.org/show_bug.cgi?id=110235


Participating hosts (51 -> 46)
------------------------------

  Additional (2): fi-icl-u2 fi-pnv-d510 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * IGT: IGT_4988 -> IGTPW_2982

  CI_DRM_6080: 6c6b621677cc0d3616de3dab025d967aa2c43877 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2982: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/
  IGT_4988: 2f6303d13e09b2457762540383c7f36cecd02bbf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for lib/igt_audio: make audio_signal_detect take const data (rev2)
  2019-05-14 13:13 [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data Simon Ser
  2019-05-14 13:51 ` Ville Syrjälä
  2019-05-14 14:21 ` [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_audio: make audio_signal_detect take const data (rev2) Patchwork
@ 2019-05-14 21:10 ` Patchwork
  2 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2019-05-14 21:10 UTC (permalink / raw)
  To: Simon Ser; +Cc: igt-dev

== Series Details ==

Series: lib/igt_audio: make audio_signal_detect take const data (rev2)
URL   : https://patchwork.freedesktop.org/series/60585/
State : success

== Summary ==

CI Bug Log - changes from IGT_4988_full -> IGTPW_2982_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/60585/revisions/2/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2982_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@i2c:
    - shard-iclb:         [PASS][1] -> [DMESG-WARN][2] ([fdo#109982])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb8/igt@i915_pm_rpm@i2c.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@i915_pm_rpm@i2c.html

  * igt@kms_flip@2x-flip-vs-suspend-interruptible:
    - shard-hsw:          [PASS][3] -> [INCOMPLETE][4] ([fdo#103540])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-hsw6/igt@kms_flip@2x-flip-vs-suspend-interruptible.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-hsw2/igt@kms_flip@2x-flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-render:
    - shard-iclb:         [PASS][5] -> [FAIL][6] ([fdo#103167]) +2 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-render.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-render.html

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#109441]) +4 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb7/igt@kms_psr@psr2_cursor_plane_onoff.html

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-apl:          [PASS][9] -> [DMESG-WARN][10] ([fdo#108566]) +2 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-apl1/igt@kms_vblank@pipe-c-ts-continuation-suspend.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-apl3/igt@kms_vblank@pipe-c-ts-continuation-suspend.html

  
#### Possible fixes ####

  * igt@gem_wait@basic-wait-all:
    - shard-iclb:         [INCOMPLETE][11] ([fdo#107713]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb8/igt@gem_wait@basic-wait-all.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@gem_wait@basic-wait-all.html

  * igt@gem_workarounds@suspend-resume:
    - shard-apl:          [DMESG-WARN][13] ([fdo#108566]) -> [PASS][14] +3 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-apl8/igt@gem_workarounds@suspend-resume.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-apl4/igt@gem_workarounds@suspend-resume.html

  * igt@kms_color@pipe-b-ctm-negative:
    - shard-iclb:         [DMESG-WARN][15] -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_color@pipe-b-ctm-negative.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb7/igt@kms_color@pipe-b-ctm-negative.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [SKIP][17] ([fdo#109349]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb5/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt:
    - shard-iclb:         [FAIL][19] ([fdo#103167]) -> [PASS][20] +4 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html

  * igt@kms_frontbuffer_tracking@psr-rgb101010-draw-blt:
    - shard-iclb:         [FAIL][21] ([fdo#109247]) -> [PASS][22] +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-blt.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-blt.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [FAIL][23] ([fdo#103166]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb1/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@primary_mmap_gtt:
    - shard-iclb:         [FAIL][25] ([fdo#107383] / [fdo#110215]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_psr@primary_mmap_gtt.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb7/igt@kms_psr@primary_mmap_gtt.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][27] ([fdo#99912]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-apl2/igt@kms_setmode@basic.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-apl4/igt@kms_setmode@basic.html
    - shard-kbl:          [FAIL][29] ([fdo#99912]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-kbl6/igt@kms_setmode@basic.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-kbl4/igt@kms_setmode@basic.html

  * igt@kms_universal_plane@universal-plane-pipe-b-functional:
    - shard-apl:          [FAIL][31] ([fdo#110037]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-apl7/igt@kms_universal_plane@universal-plane-pipe-b-functional.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-apl3/igt@kms_universal_plane@universal-plane-pipe-b-functional.html
    - shard-kbl:          [FAIL][33] ([fdo#110037]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-kbl4/igt@kms_universal_plane@universal-plane-pipe-b-functional.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-kbl2/igt@kms_universal_plane@universal-plane-pipe-b-functional.html

  
#### Warnings ####

  * igt@gem_mmap_gtt@forked-big-copy-odd:
    - shard-iclb:         [TIMEOUT][35] ([fdo#109673]) -> [INCOMPLETE][36] ([fdo#107713] / [fdo#109100])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb8/igt@gem_mmap_gtt@forked-big-copy-odd.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb5/igt@gem_mmap_gtt@forked-big-copy-odd.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#109982]: https://bugs.freedesktop.org/show_bug.cgi?id=109982
  [fdo#110037]: https://bugs.freedesktop.org/show_bug.cgi?id=110037
  [fdo#110215]: https://bugs.freedesktop.org/show_bug.cgi?id=110215
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 6)
------------------------------

  Missing    (1): shard-skl 


Build changes
-------------

  * IGT: IGT_4988 -> IGTPW_2982

  CI_DRM_6080: 6c6b621677cc0d3616de3dab025d967aa2c43877 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2982: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/
  IGT_4988: 2f6303d13e09b2457762540383c7f36cecd02bbf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data
  2019-05-14 13:51 ` Ville Syrjälä
@ 2019-05-15  7:50   ` Arkadiusz Hiler
  0 siblings, 0 replies; 5+ messages in thread
From: Arkadiusz Hiler @ 2019-05-15  7:50 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: igt-dev

On Tue, May 14, 2019 at 04:51:29PM +0300, Ville Syrjälä wrote:
> On Tue, May 14, 2019 at 04:13:24PM +0300, Simon Ser wrote:
> > audio_signal_detect uses gsl_fft_real_radix2_transform which mutates the data
> > array. This can be surprising when calling audio_signal_detect and then read
> > again the data (e.g. for another check).
> > 
> > Instead of mutating the array, make audio_signal_detect less error-prone by
> > taking a const parameter. Do an internal copy before calling the gsl function.
> > 
> > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > ---
> >  lib/igt_audio.c | 18 +++++++++++++++---
> >  lib/igt_audio.h |  2 +-
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/igt_audio.c b/lib/igt_audio.c
> > index fd8cf07c0de3..d0e34b275fa2 100644
> > --- a/lib/igt_audio.c
> > +++ b/lib/igt_audio.c
> > @@ -251,11 +251,14 @@ void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
> >   * Checks that frequencies specified in signal, and only those, are included
> >   * in the input data.
> >   *
> > - * sampling_rate is given in Hz. data_len is the number of elements in data.
> > + * sampling_rate is given in Hz. samples_len is the number of elements in
> > + * samples.
> >   */
> >  bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> > -			 int channel, double *data, size_t data_len)
> > +			 int channel, const double *samples, size_t samples_len)
> >  {
> > +	double *data;
> > +	size_t data_len = samples_len;
> >  	size_t bin_power_len = data_len / 2 + 1;
> >  	double bin_power[bin_power_len];
> >  	bool detected[FREQS_MAX];
> > @@ -264,12 +267,19 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> >  	size_t i, j;
> >  	bool above, success;
> > 
> > +	/* gsl will mutate the array in-place, so make a copy */
> > +	data = malloc(samples_len * sizeof(double));
> > +	memcpy(data, samples, samples_len * sizeof(double));
> 
> Maybe some kind of memdup() might be useful. Not sure how many instances
> of this pattern we have in igt.
> 
> > +
> >  	/* Allowed error in Hz due to FFT step */
> >  	freq_accuracy = sampling_rate / data_len;
> >  	igt_debug("Allowed freq. error: %d Hz\n", freq_accuracy);
> > 
> >  	ret = gsl_fft_real_radix2_transform(data, 1, data_len);
> > -	igt_assert(ret == 0);
> > +	if (ret != 0) {
> > +		free(data);
> > +		igt_assert(0);
> > +	}
> 
> It's an assert. Do we really care about cleanup?

igt_assert, so basically we jump out of test and continue executing.

we wouldn't leak much and matters a little when running a whole binary
but it makes some code analysis tools less angry

> Patch is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >  	/* Compute the power received by every bin of the FFT, and record the
> >  	 * maximum power received as a way to normalize all the others.
> > @@ -372,6 +382,8 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> >  		}
> >  	}
> > 
> > +	free(data);
> > +
> >  	return success;
> >  }
> > 
> > diff --git a/lib/igt_audio.h b/lib/igt_audio.h
> > index 466e772a75a4..d5ba1caaca63 100644
> > --- a/lib/igt_audio.h
> > +++ b/lib/igt_audio.h
> > @@ -43,7 +43,7 @@ void audio_signal_reset(struct audio_signal *signal);
> >  void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
> >  		       size_t buffer_len);
> >  bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> > -			 int channel, double *data, size_t data_len);
> > +			 int channel, const double *samples, size_t samples_len);
> >  size_t audio_extract_channel_s32_le(double *dst, size_t dst_cap,
> >  				    int32_t *src, size_t src_len,
> >  				    int n_channels, int channel);
> > --
> > 2.21.0
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-05-15  7:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-14 13:13 [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data Simon Ser
2019-05-14 13:51 ` Ville Syrjälä
2019-05-15  7:50   ` Arkadiusz Hiler
2019-05-14 14:21 ` [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_audio: make audio_signal_detect take const data (rev2) Patchwork
2019-05-14 21:10 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.