| Message ID | 20221130000608.519574-1-broonie@kernel.org |
|---|---|
| Headers | show |
| Series | kselftest/alsa: pcm-test improvements | expand |
On Wed, 30 Nov 2022 14:42:49 +0100, Mark Brown wrote: > > On Wed, Nov 30, 2022 at 12:06:08AM +0000, Mark Brown wrote: > > > + { "8k.1.big", "S16_LE", 8000, 2, 2000, 16000 }, > > + { "8k.2.big", "S16_LE", 8000, 2, 4000, 32000 }, > > + { "44k1.2.big", "S16_LE", 44100, 2, 22050, 192000 }, > > + { "48k.2.small", "S16_LE", 48000, 2, 512, 4096 }, > > + { "48k.2.big", "S16_LE", 48000, 2, 24000, 192000 }, > > + { "48k.6.big", "S16_LE", 48000, 6, 48000, 576000 }, > > + { "96k.2.big", "S16_LE", 96000, 2, 48000, 384000 }, > > It looks like the period/buffer size numbers need some tuning other than > for 44.1kHz and 48kHz 6 channel, if I do any more fiddling with this > series I'll split the problematic ones out into a separate commit if I > don't get things sorted (but we could just leave things as-is and fix > incrementally too I guess). I'm fine in either way. I wait for Jaroslav's review for now. thanks, Takashi
On 30. 11. 22 1:06, Mark Brown wrote: > This series provides a bunch of quick updates which should make the > coverage from pcm-test a bit more useful, it adds some support for > skipping tests when the hardware/driver is unable to support the > requested configuration and then expands the set of cases we cover to > include more sample rates and channel counts. This should exercise > switching between 8kHz and 44.1kHz based rates and ensure that clocking > doesn't get confused by non-stereo channel counts, both of which are I > expect common real world errors, at least for embedded cards. The current code allows to override "test.time1 {} test.time2 {}" blocks in the configuration files which is equivalent to "test { time1 {} time2 {} }". This changeset will introduce configuration lookups like "pcm.0.0.PLAYBACK.44k1.2.big {}" which creates another configuration structure. The '.' (compound level delimiter) should not be used in the test name. My original idea for the next improvement was to parse the "pcm.0.0.PLAYBACK.test" compound and gather the tests for the given pcm. If this compound is missing, we can continue with the hard-coded defaults. About the skips - the test should probably keep to support also the exact parameters. For example - if the hardware must support 6 channels, it should not be a skip but an error. Everything may be broken, including the PCM configuration refining. I just sent the patch with my changes for comments [1]. It's just the base code which may be extended with your requirements. The skips may be implemented using configuration field like 'skip_if_rate_error yes' or so. Let me know, if I can stack your changes on top, or perhaps, you may be willing to adapt them. Jaroslav [1] https://lore.kernel.org/alsa-devel/20221201173333.2494019-1-perex@perex.cz/
On Thu, Dec 01, 2022 at 06:42:22PM +0100, Jaroslav Kysela wrote: > The current code allows to override "test.time1 {} test.time2 {}" blocks in > the configuration files which is equivalent to "test { time1 {} time2 {} }". Right, I was leaving that in place but just renaming so that the intent of the test was clearer and expanding the standard coverage - trying to make it clearer what the test was trying to accomplish when someone comes along trying to do something later on. It did however cross my mind that we might be better off having the tests read from the config file be in addition to the standard tests rather than overriding them, I think that'd work out a lot clearer in the end. > This changeset will introduce configuration lookups like > "pcm.0.0.PLAYBACK.44k1.2.big {}" which creates another configuration > structure. The '.' (compound level delimiter) should not be used in the test > name. I see, we could use another delimiter there easily enough (though if we segregated the built in and loaded test configurations I'm not sure it'd matter so much). > My original idea for the next improvement was to parse the > "pcm.0.0.PLAYBACK.test" compound and gather the tests for the given pcm. If > this compound is missing, we can continue with the hard-coded defaults. While it is useful to be able to specify additional tests through configuration I don't think we should be relying on that for coverage, we should have a more substantial baseline of tests so that systems like KernelCI get reasonable coverage without having to get changes individually integrated for boards (and then wait for them to filter out into the trees being tested). It doesn't scale out so well over the number of systems that we might be running on, especially if we come up with new tests and have to loop back over existing boards, and isn't really idiomatic for kselftest. I'm also a bit worried about the way we currently override the built in tests, it creates additional potential for confusion when looking at results if the test might've been turned into something different by the configuration file. > About the skips - the test should probably keep to support also the exact > parameters. For example - if the hardware must support 6 channels, it should > not be a skip but an error. Everything may be broken, including the PCM > configuration refining. Yes, there's a tension there between hard coded tests and the explicitly specified per board ones. I think the solution here is to add two tests for things we read from the configuration file rather than just adding by default, one verifying that we managed to configure the settings we asked for and one for the actual test. > I just sent the patch with my changes for comments [1]. It's just the base > code which may be extended with your requirements. The skips may be > implemented using configuration field like 'skip_if_rate_error yes' or so. > Let me know, if I can stack your changes on top, or perhaps, you may be > willing to adapt them.
On Thu, 01 Dec 2022 18:42:22 +0100, Jaroslav Kysela wrote: > > Let me know, if I can stack your changes on top, or perhaps, you may > be willing to adapt them. As Mark has already sent a v2 series, I applied his v2 at first. Could you rebase and resubmit on top of my for-next branch? thanks, Takashi
On Thu, Dec 01, 2022 at 08:06:22PM +0100, Takashi Iwai wrote: > On Thu, 01 Dec 2022 18:42:22 +0100, > Jaroslav Kysela wrote: > > > > Let me know, if I can stack your changes on top, or perhaps, you may > > be willing to adapt them. > > As Mark has already sent a v2 series, I applied his v2 at first. > Could you rebase and resubmit on top of my for-next branch? Oh, this is getting a little confusing - I'd just picked Jaroslav's patch into my tree and was in the middle redoing my ideas on top of his code! I might have something more later this evening... I think we can converge here, let me continue taking a look.
On Thu, 01 Dec 2022 21:29:48 +0100, Mark Brown wrote: > > On Thu, Dec 01, 2022 at 08:06:22PM +0100, Takashi Iwai wrote: > > On Thu, 01 Dec 2022 18:42:22 +0100, > > Jaroslav Kysela wrote: > > > > > > Let me know, if I can stack your changes on top, or perhaps, you may > > > be willing to adapt them. > > > > As Mark has already sent a v2 series, I applied his v2 at first. > > Could you rebase and resubmit on top of my for-next branch? > > Oh, this is getting a little confusing - I'd just picked Jaroslav's > patch into my tree and was in the middle redoing my ideas on top of his > code! I might have something more later this evening... I think we can > converge here, let me continue taking a look. Ah then it was my misunderstanding, and everything should be fine now ;) Thanks! Takashi
On Fri, 02 Dec 2022 08:52:03 +0100, Takashi Iwai wrote: > > On Thu, 01 Dec 2022 21:29:48 +0100, > Mark Brown wrote: > > > > On Thu, Dec 01, 2022 at 08:06:22PM +0100, Takashi Iwai wrote: > > > On Thu, 01 Dec 2022 18:42:22 +0100, > > > Jaroslav Kysela wrote: > > > > > > > > Let me know, if I can stack your changes on top, or perhaps, you may > > > > be willing to adapt them. > > > > > > As Mark has already sent a v2 series, I applied his v2 at first. > > > Could you rebase and resubmit on top of my for-next branch? > > > > Oh, this is getting a little confusing - I'd just picked Jaroslav's > > patch into my tree and was in the middle redoing my ideas on top of his > > code! I might have something more later this evening... I think we can > > converge here, let me continue taking a look. > > Ah then it was my misunderstanding, and everything should be fine now > ;) Thanks! Erm, you meant sent as *v3*. I've seen now. As the v2 patches were already merged, could you rather rebase and resubmit? I'd like to avoid rebase the full series that are already included in linux-next. Apologies for the mess. Takashi
On 02. 12. 22 8:54, Takashi Iwai wrote: > On Fri, 02 Dec 2022 08:52:03 +0100, > Takashi Iwai wrote: >> >> On Thu, 01 Dec 2022 21:29:48 +0100, >> Mark Brown wrote: >>> >>> On Thu, Dec 01, 2022 at 08:06:22PM +0100, Takashi Iwai wrote: >>>> On Thu, 01 Dec 2022 18:42:22 +0100, >>>> Jaroslav Kysela wrote: >>>>> >>>>> Let me know, if I can stack your changes on top, or perhaps, you may >>>>> be willing to adapt them. >>>> >>>> As Mark has already sent a v2 series, I applied his v2 at first. >>>> Could you rebase and resubmit on top of my for-next branch? >>> >>> Oh, this is getting a little confusing - I'd just picked Jaroslav's >>> patch into my tree and was in the middle redoing my ideas on top of his >>> code! I might have something more later this evening... I think we can >>> converge here, let me continue taking a look. >> >> Ah then it was my misunderstanding, and everything should be fine now >> ;) Thanks! > > Erm, you meant sent as *v3*. I've seen now. > > As the v2 patches were already merged, could you rather rebase and > resubmit? I'd like to avoid rebase the full series that are already > included in linux-next. It's rebased. The first patch from the set drops the previous Mark's changes. Jaroslav
On Fri, Dec 02, 2022 at 09:56:39AM +0100, Jaroslav Kysela wrote: > On 02. 12. 22 8:54, Takashi Iwai wrote: > > Takashi Iwai wrote: > > > > Oh, this is getting a little confusing - I'd just picked Jaroslav's > > > > patch into my tree and was in the middle redoing my ideas on top of his > > > > code! I might have something more later this evening... I think we can > > > > converge here, let me continue taking a look. > > > Ah then it was my misunderstanding, and everything should be fine now > > > ;) Thanks! > > Erm, you meant sent as *v3*. I've seen now. > > As the v2 patches were already merged, could you rather rebase and > > resubmit? I'd like to avoid rebase the full series that are already > > included in linux-next. > It's rebased. The first patch from the set drops the previous Mark's changes. Indeed, there was so many collisions with Jaroslav's patches which it just seemed like the most straightforward way to do things (plus I'd already written a good chunk of the new version by the time you applied my v2). Probably only a small bit of the skipping code would end up remaining anyway.
This series provides a bunch of quick updates which should make the coverage from pcm-test a bit more useful, it adds some support for skipping tests when the hardware/driver is unable to support the requested configuration and then expands the set of cases we cover to include more sample rates and channel counts. This should exercise switching between 8kHz and 44.1kHz based rates and ensure that clocking doesn't get confused by non-stereo channel counts, both of which are I expect common real world errors, at least for embedded cards. Mark Brown (6): kselftest/alsa: Refactor pcm-test to list the tests to run in a struct kselftest/alsa: Report failures to set the requested sample rate as skips kselftest/alsa: Report failures to set the requested channels as skips kselftest/alsa: Don't any configuration in the sample config kselftest/alsa: Provide more meaningful names for tests kselftest/alsa: Add more coverage of sample rates and channel counts .../alsa/conf.d/Lenovo_ThinkPad_P1_Gen2.conf | 35 ++++---- tools/testing/selftests/alsa/pcm-test.c | 88 +++++++++++++------ 2 files changed, 81 insertions(+), 42 deletions(-) base-commit: 1d8025ec722d5e011f9299c46274eb21fb54a428