[PATCH igt] module_reload: remove snd_hda_intel

[PATCH igt] module_reload: remove snd_hda_intel

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The audio driver uses the power well provided by our driver, so on
Haswell we can't "rmmod i915" if we don't "rmmod snd_hda_intel" first.

The problem with removing snd_hda_intel is that we also need to kill
its users. On the specific machine I tested, the only user seem to be
alsactl, but on other machines this may change. IMHO we should leave
the "kill user space" step to whoever is running the script, but
Daniel asked me to put it here so we have a better chance of Just
Working on QA's machines.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/module_reload | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/module_reload b/tests/module_reload
index 3d8cb85..c1fd395 100755
--- a/tests/module_reload
+++ b/tests/module_reload
@@ -18,6 +18,10 @@ if ! echo 0 > /sys/class/vtconsole/vtcon1/bind ; then
 	exit 77
 fi
 
+# The sound driver uses our power well
+pkill alsactl
+rmmod snd_hda_intel &> /dev/null
+
 #ignore errors in ips - gen5 only
 rmmod intel_ips &> /dev/null
 rmmod i915
@@ -38,6 +42,8 @@ fi
 modprobe i915
 echo 1 > /sys/class/vtconsole/vtcon1/bind
 
+modprobe snd_hda_intel
+
 # try to run something
 $SOURCE_DIR/gem_exec_nop > /dev/null && echo "module successfully loaded again"
 
-- 
1.8.3.1

Re: [PATCH igt] module_reload: remove snd_hda_intel

[Ben Widawsky]

On Mon, Oct 21, 2013 at 11:56:09AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The audio driver uses the power well provided by our driver, so on
> Haswell we can't "rmmod i915" if we don't "rmmod snd_hda_intel" first.
> 
> The problem with removing snd_hda_intel is that we also need to kill
> its users. On the specific machine I tested, the only user seem to be
> alsactl, but on other machines this may change. IMHO we should leave
> the "kill user space" step to whoever is running the script, but
> Daniel asked me to put it here so we have a better chance of Just
> Working on QA's machines.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  tests/module_reload | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/module_reload b/tests/module_reload
> index 3d8cb85..c1fd395 100755
> --- a/tests/module_reload
> +++ b/tests/module_reload
> @@ -18,6 +18,10 @@ if ! echo 0 > /sys/class/vtconsole/vtcon1/bind ; then
>  	exit 77
>  fi
>  
> +# The sound driver uses our power well
> +pkill alsactl
> +rmmod snd_hda_intel &> /dev/null
> +
>  #ignore errors in ips - gen5 only
>  rmmod intel_ips &> /dev/null
>  rmmod i915
> @@ -38,6 +42,8 @@ fi
>  modprobe i915
>  echo 1 > /sys/class/vtconsole/vtcon1/bind
>  
> +modprobe snd_hda_intel
> +
>  # try to run something
>  $SOURCE_DIR/gem_exec_nop > /dev/null && echo "module successfully loaded again"
>  

I don't really pretend to understand how module dependencies are
calculated. However, I would think the dependency should be *somewhere*
and so modprobe -r i915 should work.

Don't read that as a nak. Rather, a wtf is with the existing stuff?

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH igt] module_reload: remove snd_hda_intel

[Paulo Zanoni]

2013/10/23 Ben Widawsky <ben@bwidawsk.net>:
> On Mon, Oct 21, 2013 at 11:56:09AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> The audio driver uses the power well provided by our driver, so on
>> Haswell we can't "rmmod i915" if we don't "rmmod snd_hda_intel" first.
>>
>> The problem with removing snd_hda_intel is that we also need to kill
>> its users. On the specific machine I tested, the only user seem to be
>> alsactl, but on other machines this may change. IMHO we should leave
>> the "kill user space" step to whoever is running the script, but
>> Daniel asked me to put it here so we have a better chance of Just
>> Working on QA's machines.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  tests/module_reload | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/tests/module_reload b/tests/module_reload
>> index 3d8cb85..c1fd395 100755
>> --- a/tests/module_reload
>> +++ b/tests/module_reload
>> @@ -18,6 +18,10 @@ if ! echo 0 > /sys/class/vtconsole/vtcon1/bind ; then
>>       exit 77
>>  fi
>>
>> +# The sound driver uses our power well
>> +pkill alsactl
>> +rmmod snd_hda_intel &> /dev/null
>> +
>>  #ignore errors in ips - gen5 only
>>  rmmod intel_ips &> /dev/null
>>  rmmod i915
>> @@ -38,6 +42,8 @@ fi
>>  modprobe i915
>>  echo 1 > /sys/class/vtconsole/vtcon1/bind
>>
>> +modprobe snd_hda_intel
>> +
>>  # try to run something
>>  $SOURCE_DIR/gem_exec_nop > /dev/null && echo "module successfully loaded again"
>>
>
> I don't really pretend to understand how module dependencies are
> calculated. However, I would think the dependency should be *somewhere*
> and so modprobe -r i915 should work.

It's the opposite: "modprobe -r i915" will remove drm.ko,
drm_kms_helper, i2c-algo-bit and video, but won't remove snd_hda_intel
(and if you don't remove it first, it will complain that i915.ko is
still in use).

>
> Don't read that as a nak. Rather, a wtf is with the existing stuff?

I think the dependency is magically tracked by depmod when it sees the
Audio driver uses a symbol exported by i915.ko. But a "modinfo
snd_hda_intel" doesn't show i915.ko on the "depends" section, which is
strange. I'm also not an expert on this area...

>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH igt] module_reload: remove snd_hda_intel

[Jani Nikula]

On Wed, 23 Oct 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/10/23 Ben Widawsky <ben@bwidawsk.net>:
>> On Mon, Oct 21, 2013 at 11:56:09AM -0200, Paulo Zanoni wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>
>>> The audio driver uses the power well provided by our driver, so on
>>> Haswell we can't "rmmod i915" if we don't "rmmod snd_hda_intel" first.
>>>
>>> The problem with removing snd_hda_intel is that we also need to kill
>>> its users. On the specific machine I tested, the only user seem to be
>>> alsactl, but on other machines this may change. IMHO we should leave
>>> the "kill user space" step to whoever is running the script, but
>>> Daniel asked me to put it here so we have a better chance of Just
>>> Working on QA's machines.
>>>
>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> ---
>>>  tests/module_reload | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/tests/module_reload b/tests/module_reload
>>> index 3d8cb85..c1fd395 100755
>>> --- a/tests/module_reload
>>> +++ b/tests/module_reload
>>> @@ -18,6 +18,10 @@ if ! echo 0 > /sys/class/vtconsole/vtcon1/bind ; then
>>>       exit 77
>>>  fi
>>>
>>> +# The sound driver uses our power well
>>> +pkill alsactl
>>> +rmmod snd_hda_intel &> /dev/null
>>> +
>>>  #ignore errors in ips - gen5 only
>>>  rmmod intel_ips &> /dev/null
>>>  rmmod i915
>>> @@ -38,6 +42,8 @@ fi
>>>  modprobe i915
>>>  echo 1 > /sys/class/vtconsole/vtcon1/bind
>>>
>>> +modprobe snd_hda_intel
>>> +
>>>  # try to run something
>>>  $SOURCE_DIR/gem_exec_nop > /dev/null && echo "module successfully loaded again"
>>>
>>
>> I don't really pretend to understand how module dependencies are
>> calculated. However, I would think the dependency should be *somewhere*
>> and so modprobe -r i915 should work.
>
> It's the opposite: "modprobe -r i915" will remove drm.ko,
> drm_kms_helper, i2c-algo-bit and video, but won't remove snd_hda_intel
> (and if you don't remove it first, it will complain that i915.ko is
> still in use).
>
>>
>> Don't read that as a nak. Rather, a wtf is with the existing stuff?
>
> I think the dependency is magically tracked by depmod when it sees the
> Audio driver uses a symbol exported by i915.ko. But a "modinfo
> snd_hda_intel" doesn't show i915.ko on the "depends" section, which is
> strange. I'm also not an expert on this area...

This is because snd_hda_intel does not depend on i915, to make it
possible to use snd_hda_intel without i915. Which makes sense. Instead,
it uses symbol_request() to be able to call a couple of i915 functions
when available.

I don't know if there's a way to check this runtime dependency from
userspace. But obviously you can't remove i915 while snd_hda_intel holds
function pointers to i915. (In fact, intel_ips is the same.)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH igt] module_reload: remove snd_hda_intel

[Ben Widawsky]

On Wed, Oct 23, 2013 at 09:42:06PM +0300, Jani Nikula wrote:
> On Wed, 23 Oct 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> > 2013/10/23 Ben Widawsky <ben@bwidawsk.net>:
> >> On Mon, Oct 21, 2013 at 11:56:09AM -0200, Paulo Zanoni wrote:
> >>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>>
> >>> The audio driver uses the power well provided by our driver, so on
> >>> Haswell we can't "rmmod i915" if we don't "rmmod snd_hda_intel" first.
> >>>
> >>> The problem with removing snd_hda_intel is that we also need to kill
> >>> its users. On the specific machine I tested, the only user seem to be
> >>> alsactl, but on other machines this may change. IMHO we should leave
> >>> the "kill user space" step to whoever is running the script, but
> >>> Daniel asked me to put it here so we have a better chance of Just
> >>> Working on QA's machines.
> >>>
> >>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>> ---
> >>>  tests/module_reload | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/tests/module_reload b/tests/module_reload
> >>> index 3d8cb85..c1fd395 100755
> >>> --- a/tests/module_reload
> >>> +++ b/tests/module_reload
> >>> @@ -18,6 +18,10 @@ if ! echo 0 > /sys/class/vtconsole/vtcon1/bind ; then
> >>>       exit 77
> >>>  fi
> >>>
> >>> +# The sound driver uses our power well
> >>> +pkill alsactl
> >>> +rmmod snd_hda_intel &> /dev/null
> >>> +
> >>>  #ignore errors in ips - gen5 only
> >>>  rmmod intel_ips &> /dev/null
> >>>  rmmod i915
> >>> @@ -38,6 +42,8 @@ fi
> >>>  modprobe i915
> >>>  echo 1 > /sys/class/vtconsole/vtcon1/bind
> >>>
> >>> +modprobe snd_hda_intel
> >>> +
> >>>  # try to run something
> >>>  $SOURCE_DIR/gem_exec_nop > /dev/null && echo "module successfully loaded again"
> >>>
> >>
> >> I don't really pretend to understand how module dependencies are
> >> calculated. However, I would think the dependency should be *somewhere*
> >> and so modprobe -r i915 should work.
> >
> > It's the opposite: "modprobe -r i915" will remove drm.ko,
> > drm_kms_helper, i2c-algo-bit and video, but won't remove snd_hda_intel
> > (and if you don't remove it first, it will complain that i915.ko is
> > still in use).
> >
> >>
> >> Don't read that as a nak. Rather, a wtf is with the existing stuff?
> >
> > I think the dependency is magically tracked by depmod when it sees the
> > Audio driver uses a symbol exported by i915.ko. But a "modinfo
> > snd_hda_intel" doesn't show i915.ko on the "depends" section, which is
> > strange. I'm also not an expert on this area...
> 
> This is because snd_hda_intel does not depend on i915, to make it
> possible to use snd_hda_intel without i915. Which makes sense. Instead,
> it uses symbol_request() to be able to call a couple of i915 functions
> when available.
> 
> I don't know if there's a way to check this runtime dependency from
> userspace. But obviously you can't remove i915 while snd_hda_intel holds
> function pointers to i915. (In fact, intel_ips is the same.)
> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

Looking around it seems a fairly common idiom is to do the
symbol_(get|request) when you need to call the function, and release it
when you're no longer using it. Since these functions we're talking
about require waking the GPU, I think speed shouldn't be a concern.

As you say, it makes sense to be able to load audio without i915 -
however looking at the code, it seems like audio will fail if i915 is
not loaded.

Also, it looks like there should be a way to make userspace aware of the
dependency since it calls back in to userspace (I find it funny that the
name of the function uses 4 underscores: ____call_usermodehelper).


-- 
Ben Widawsky, Intel Open Source Technology Center