* [PATCH igt] module_reload: remove snd_hda_intel @ 2013-10-21 13:56 Paulo Zanoni 2013-10-23 5:19 ` Ben Widawsky 0 siblings, 1 reply; 5+ messages in thread From: Paulo Zanoni @ 2013-10-21 13:56 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH igt] module_reload: remove snd_hda_intel 2013-10-21 13:56 [PATCH igt] module_reload: remove snd_hda_intel Paulo Zanoni @ 2013-10-23 5:19 ` Ben Widawsky 2013-10-23 16:57 ` Paulo Zanoni 0 siblings, 1 reply; 5+ messages in thread From: Ben Widawsky @ 2013-10-23 5:19 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH igt] module_reload: remove snd_hda_intel 2013-10-23 5:19 ` Ben Widawsky @ 2013-10-23 16:57 ` Paulo Zanoni 2013-10-23 18:42 ` Jani Nikula 0 siblings, 1 reply; 5+ messages in thread From: Paulo Zanoni @ 2013-10-23 16:57 UTC (permalink / raw) To: Ben Widawsky; +Cc: Intel Graphics Development, 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH igt] module_reload: remove snd_hda_intel 2013-10-23 16:57 ` Paulo Zanoni @ 2013-10-23 18:42 ` Jani Nikula 2013-10-24 22:41 ` Ben Widawsky 0 siblings, 1 reply; 5+ messages in thread From: Jani Nikula @ 2013-10-23 18:42 UTC (permalink / raw) To: Paulo Zanoni, Ben Widawsky; +Cc: Intel Graphics Development, Paulo Zanoni 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH igt] module_reload: remove snd_hda_intel 2013-10-23 18:42 ` Jani Nikula @ 2013-10-24 22:41 ` Ben Widawsky 0 siblings, 0 replies; 5+ messages in thread From: Ben Widawsky @ 2013-10-24 22:41 UTC (permalink / raw) To: Jani Nikula; +Cc: Intel Graphics Development, Paulo Zanoni 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-24 22:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-21 13:56 [PATCH igt] module_reload: remove snd_hda_intel Paulo Zanoni 2013-10-23 5:19 ` Ben Widawsky 2013-10-23 16:57 ` Paulo Zanoni 2013-10-23 18:42 ` Jani Nikula 2013-10-24 22:41 ` Ben Widawsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox