| Message ID | 1510224592-14732-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Mika Kahola (2017-11-09 10:49:52) > At least in Coffee Lake it happens that we start initiliazing audio when > no display is connected. This was discovered by CI when running IGT test > case > > drv_module_reload --r basic-no-display > > The issue here is that the 'intel_device_info_runtime_init()' sets > num_pipes to 0 but before this happens the audio part has already started > to initialize itself. Later on the num_pipes is updated to 0 in > intel_device_info_runtime_init() and we hit that when audio part is digging > out ELD. This causes a warning in dmesg. To fix this issue, let's check the > number of available pipes when trying to read out ELD. dev_info_runtime_init() is too late. It depends on mmio being enabled to probe the HW and nothing else; so move it to i915_driver_init_mmio()? -Chris
On Thu, 2017-11-09 at 11:01 +0000, Chris Wilson wrote: > Quoting Mika Kahola (2017-11-09 10:49:52) > > > > At least in Coffee Lake it happens that we start initiliazing audio > > when > > no display is connected. This was discovered by CI when running IGT > > test > > case > > > > drv_module_reload --r basic-no-display > > > > The issue here is that the 'intel_device_info_runtime_init()' sets > > num_pipes to 0 but before this happens the audio part has already > > started > > to initialize itself. Later on the num_pipes is updated to 0 in > > intel_device_info_runtime_init() and we hit that when audio part is > > digging > > out ELD. This causes a warning in dmesg. To fix this issue, let's > > check the > > number of available pipes when trying to read out ELD. > dev_info_runtime_init() is too late. It depends on mmio being enabled > to > probe the HW and nothing else; so move it to i915_driver_init_mmio()? Ok. I could try that. I was also thinking that is there a way to postpone audio initialization? > -Chris
On Thu, Nov 09, 2017 at 01:11:05PM +0200, Mika Kahola wrote: > On Thu, 2017-11-09 at 11:01 +0000, Chris Wilson wrote: > > Quoting Mika Kahola (2017-11-09 10:49:52) > > > > > > At least in Coffee Lake it happens that we start initiliazing audio > > > when > > > no display is connected. This was discovered by CI when running IGT > > > test > > > case > > > > > > drv_module_reload --r basic-no-display > > > > > > The issue here is that the 'intel_device_info_runtime_init()' sets > > > num_pipes to 0 but before this happens the audio part has already > > > started > > > to initialize itself. Later on the num_pipes is updated to 0 in > > > intel_device_info_runtime_init() and we hit that when audio part is > > > digging > > > out ELD. This causes a warning in dmesg. To fix this issue, let's > > > check the > > > number of available pipes when trying to read out ELD. > > dev_info_runtime_init() is too late. It depends on mmio being enabled > > to > > probe the HW and nothing else; so move it to i915_driver_init_mmio()? > Ok. I could try that. I was also thinking that is there a way to > postpone audio initialization? We probably shouldn't be registering the audio thing until we've actually set up the outputs.
On Thu, 2017-11-09 at 15:15 +0200, Ville Syrjälä wrote: > On Thu, Nov 09, 2017 at 01:11:05PM +0200, Mika Kahola wrote: > > > > On Thu, 2017-11-09 at 11:01 +0000, Chris Wilson wrote: > > > > > > Quoting Mika Kahola (2017-11-09 10:49:52) > > > > > > > > > > > > At least in Coffee Lake it happens that we start initiliazing > > > > audio > > > > when > > > > no display is connected. This was discovered by CI when running > > > > IGT > > > > test > > > > case > > > > > > > > drv_module_reload --r basic-no-display > > > > > > > > The issue here is that the 'intel_device_info_runtime_init()' > > > > sets > > > > num_pipes to 0 but before this happens the audio part has > > > > already > > > > started > > > > to initialize itself. Later on the num_pipes is updated to 0 in > > > > intel_device_info_runtime_init() and we hit that when audio > > > > part is > > > > digging > > > > out ELD. This causes a warning in dmesg. To fix this issue, > > > > let's > > > > check the > > > > number of available pipes when trying to read out ELD. > > > dev_info_runtime_init() is too late. It depends on mmio being > > > enabled > > > to > > > probe the HW and nothing else; so move it to > > > i915_driver_init_mmio()? > > Ok. I could try that. I was also thinking that is there a way to > > postpone audio initialization? > We probably shouldn't be registering the audio thing until we've > actually set up the outputs. I tried couple of versions. One with Chris's idea to move intel_device_info_runtime_init() to i915_driver_init_mmio() didn't turn out to be a success. https://patchwork.freedesktop.org/series/33514/ I also tried to set num_pipes to 0 in case of display disable already in i915_driver_init_early(). That approach didn't turn out to be a success either. https://patchwork.freedesktop.org/series/33522/ I'll give it a go for a patch that doesn't register audio in case of disabled display. >
On Fri, 10 Nov 2017, Mika Kahola <mika.kahola@intel.com> wrote: > On Thu, 2017-11-09 at 15:15 +0200, Ville Syrjälä wrote: >> On Thu, Nov 09, 2017 at 01:11:05PM +0200, Mika Kahola wrote: >> > >> > On Thu, 2017-11-09 at 11:01 +0000, Chris Wilson wrote: >> > > >> > > Quoting Mika Kahola (2017-11-09 10:49:52) >> > > > >> > > > >> > > > At least in Coffee Lake it happens that we start initiliazing >> > > > audio >> > > > when >> > > > no display is connected. This was discovered by CI when running >> > > > IGT >> > > > test >> > > > case >> > > > >> > > > drv_module_reload --r basic-no-display >> > > > >> > > > The issue here is that the 'intel_device_info_runtime_init()' >> > > > sets >> > > > num_pipes to 0 but before this happens the audio part has >> > > > already >> > > > started >> > > > to initialize itself. Later on the num_pipes is updated to 0 in >> > > > intel_device_info_runtime_init() and we hit that when audio >> > > > part is >> > > > digging >> > > > out ELD. This causes a warning in dmesg. To fix this issue, >> > > > let's >> > > > check the >> > > > number of available pipes when trying to read out ELD. >> > > dev_info_runtime_init() is too late. It depends on mmio being >> > > enabled >> > > to >> > > probe the HW and nothing else; so move it to >> > > i915_driver_init_mmio()? >> > Ok. I could try that. I was also thinking that is there a way to >> > postpone audio initialization? >> We probably shouldn't be registering the audio thing until we've >> actually set up the outputs. > I tried couple of versions. One with Chris's idea to move > intel_device_info_runtime_init() to i915_driver_init_mmio() didn't turn > out to be a success. > > https://patchwork.freedesktop.org/series/33514/ > > I also tried to set num_pipes to 0 in case of display disable already > in i915_driver_init_early(). That approach didn't turn out to be a > success either. > > https://patchwork.freedesktop.org/series/33522/ > > I'll give it a go for a patch that doesn't register audio in case of > disabled display. That seems like the way to go. Display disable on the command line should be no different from num_pipes == 0. BR, Jani.
On Fri, 2017-11-10 at 15:22 +0200, Jani Nikula wrote: > On Fri, 10 Nov 2017, Mika Kahola <mika.kahola@intel.com> wrote: > > > > On Thu, 2017-11-09 at 15:15 +0200, Ville Syrjälä wrote: > > > > > > On Thu, Nov 09, 2017 at 01:11:05PM +0200, Mika Kahola wrote: > > > > > > > > > > > > On Thu, 2017-11-09 at 11:01 +0000, Chris Wilson wrote: > > > > > > > > > > > > > > > Quoting Mika Kahola (2017-11-09 10:49:52) > > > > > > > > > > > > > > > > > > > > > > > > At least in Coffee Lake it happens that we start > > > > > > initiliazing > > > > > > audio > > > > > > when > > > > > > no display is connected. This was discovered by CI when > > > > > > running > > > > > > IGT > > > > > > test > > > > > > case > > > > > > > > > > > > drv_module_reload --r basic-no-display > > > > > > > > > > > > The issue here is that the > > > > > > 'intel_device_info_runtime_init()' > > > > > > sets > > > > > > num_pipes to 0 but before this happens the audio part has > > > > > > already > > > > > > started > > > > > > to initialize itself. Later on the num_pipes is updated to > > > > > > 0 in > > > > > > intel_device_info_runtime_init() and we hit that when audio > > > > > > part is > > > > > > digging > > > > > > out ELD. This causes a warning in dmesg. To fix this issue, > > > > > > let's > > > > > > check the > > > > > > number of available pipes when trying to read out ELD. > > > > > dev_info_runtime_init() is too late. It depends on mmio being > > > > > enabled > > > > > to > > > > > probe the HW and nothing else; so move it to > > > > > i915_driver_init_mmio()? > > > > Ok. I could try that. I was also thinking that is there a way > > > > to > > > > postpone audio initialization? > > > We probably shouldn't be registering the audio thing until we've > > > actually set up the outputs. > > I tried couple of versions. One with Chris's idea to move > > intel_device_info_runtime_init() to i915_driver_init_mmio() didn't > > turn > > out to be a success. > > > > https://patchwork.freedesktop.org/series/33514/ > > > > I also tried to set num_pipes to 0 in case of display disable > > already > > in i915_driver_init_early(). That approach didn't turn out to be a > > success either. > > > > https://patchwork.freedesktop.org/series/33522/ > > > > I'll give it a go for a patch that doesn't register audio in case > > of > > disabled display. > That seems like the way to go. Display disable on the command line > should be no different from num_pipes == 0. Yeah, let's skip this patch for this. I tested another patch that doesn't register the audio in case of disabled display. At least the trybot run for this patch was clean. https://patchwork.freedesktop.org/series/33601/ > > BR, > Jani. >
HI, > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > Mika Kahola > Sent: perjantai 10. marraskuuta 2017 15.35 > To: Jani Nikula <jani.nikula@linux.intel.com>; Ville Syrjälä > <ville.syrjala@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Return early when pipes are not > available > > > > https://patchwork.freedesktop.org/series/33522/ > > > > > > I'll give it a go for a patch that doesn't register audio in case of > > > disabled display. > > That seems like the way to go. Display disable on the command line > > should be no different from num_pipes == 0. > Yeah, let's skip this patch for this. I tested another patch that doesn't register > the audio in case of disabled display. At least the trybot run for this patch was > clean. Well, no if you look all.html. same issues seen. > > https://patchwork.freedesktop.org/series/33601/ > > > > > > BR, > > Jani. > > > -- > Mika Kahola - Intel OTC > Jani Saarinen Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 4705194..5e367b1 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -859,6 +859,9 @@ static int i915_audio_component_get_eld(struct device *kdev, int port, const u8 *eld; int ret = -EINVAL; + if (INTEL_INFO(dev_priv)->num_pipes == 0) + return ret; + mutex_lock(&dev_priv->av_mutex); intel_encoder = get_saved_enc(dev_priv, port, pipe);
At least in Coffee Lake it happens that we start initiliazing audio when no display is connected. This was discovered by CI when running IGT test case drv_module_reload --r basic-no-display The issue here is that the 'intel_device_info_runtime_init()' sets num_pipes to 0 but before this happens the audio part has already started to initialize itself. Later on the num_pipes is updated to 0 in intel_device_info_runtime_init() and we hit that when audio part is digging out ELD. This causes a warning in dmesg. To fix this issue, let's check the number of available pipes when trying to read out ELD. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206 Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/intel_audio.c | 3 +++ 1 file changed, 3 insertions(+)