Re: [igt-dev] [PATCH i-g-t] lib/igt_core: Enable extra kernel logs for audio debug

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 18 Mar 2019, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
> On Thu, 14 Mar 2019 03:12:25 -0700, Jani Nikula wrote:
>>
>> On Wed, 13 Mar 2019, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
>> >
>> > For debug of audio issues in power management and driver reload tests,
>> > additional kernel logs may be useful, both in dmesg as well as
>> > ftrace. Add the infrastructure to generate these logs which can be
>> > enabled/disabled at sub-test granularity. Also enable these logs for
>> > selected sub-tests. The ftrace buffer is dumped to stdout to avoid
>> > changes in igt_runner and other CI infrastructure.
>>
>> I think I'd just provide the helpers and add them in igt_fixture instead
>> of introducing new "extra klog" subtest helpers. I don't think we need
>> that granularity.
>
> Actually the v1 commit message is misleading. The real reason for the
> "subtest granularity" is mentioned in the v2 commit message:
>
>     "At present igt_runner and other CI infrastructure does not capture the
>     ftrace buffer. Therefore, to avoid changes to igt_runner and the CI
>     infrastructure the ftrace buffer is dumped to stdout. This is done
>     after each sub-test so the ftrace output for a subtest can be
>     associated with that subtest."
>
>> OTOH I think we do need more granularity in enabling the dynamic debug
>> and the tracing separately.
>
> I think there is no real way to enable such granularity with the approach
> taken in this patch. Dynamic debug and tracing functions can be separated
> out. But there will have to be a single function which enables both of
> these together as has been done here. Unless we change the approach taken
> here. But then what is that approach?

I fail to see why enabling completely orthogonal features such as
dynamic debug and tracing should be conflated into a single function.

>> The *skl* style wildcard dynamid debug enables/disables also seem not
>> specific enough.
>
> Perhaps something like this is better?
>
> echo "file drivers/sound/soc/intel/skylake/* +p" > \
>      <debugfs>/dynamic_debug/control

I think it should include more than just skylake, but the point was, the
original wildcards included *everything*, not just sound.

>
>> Another review comment: Please don't use system(). Implement this in C,
>> not shell script
>
> Agreed. But let us agree on the overall approach taken in this patch first
> and also take Chris' patch into account. If we are in agreement about the
> high level approach making such changes is not an issue.
>
> Please note that this patch is in response to the request to generate the
> sort of logs mentioned in [1] in CI, so that hard to reproduce audio issues
> can be debugged.

That's fine and good, but please let's not restrict ourselves to the
audio use case. Imagine having to debug issues other than audio, with
different combinations of dynamic debug, tracing, and perhaps something
else required.

I don't think we want igt_subtest_extra_klog() which enables
*everything* when the extra debugs to be enabled are really test
specific. Audio tests need these. Some other tests might need something
else *without* any audio tracing/debugging. Etc.


BR,
Jani.



>
> Thanks for the reviews!
>
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=109189#c4
>
> --
> Ashutosh Dixit, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev