| Message ID | 20221012104936.30911-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Wed, Oct 12, 2022 at 01:49:36PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > On HSW/BDW the hardware ELD buffer does not work if the controller > is suspended. I'm not 100% which thing in there is needed to make it > work (at least just forcing the controller into D0 with setpci is > not enough). But a full runtime resume seems to do the trick here > at least, and so far it looks like this doesn't even deadlock or > anything. > > Cc: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Cc: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_audio.c | 38 ++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index 328c47719fd8..467f3f260969 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -23,6 +23,7 @@ > > #include <linux/component.h> > #include <linux/kernel.h> > +#include <linux/pm_runtime.h> > > #include <drm/drm_edid.h> > #include <drm/i915_component.h> > @@ -480,6 +481,16 @@ hsw_audio_config_update(struct intel_encoder *encoder, > hsw_hdmi_audio_config_update(encoder, crtc_state); > } > > +static struct pci_dev *hsw_hda_controller(struct drm_i915_private *i915) > +{ > + int domain = pci_domain_nr(to_pci_dev(i915->drm.dev)->bus); > + > + if (!IS_HASWELL(i915) && !IS_BROADWELL(i915)) > + return NULL; > + > + return pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(3, 0)); > +} > + > static int hsw_eld_buffer_size(struct drm_i915_private *i915, > enum transcoder cpu_transcoder) > { > @@ -497,8 +508,14 @@ static void hsw_audio_codec_get_config(struct intel_encoder *encoder, > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > u32 *eld = (u32 *)crtc_state->eld; > int eld_buffer_size, len, i; > + struct pci_dev *hsw_hdac; > u32 tmp; > > + /* on HSW/BDW ELD access doesn't work if the HDA controller is supended */ > + hsw_hdac = hsw_hda_controller(i915); > + if (hsw_hdac) > + pm_runtime_get_sync(&hsw_hdac->dev); > + > tmp = intel_de_read(i915, HSW_AUD_PIN_ELD_CP_VLD); > if ((tmp & AUDIO_ELD_VALID(cpu_transcoder)) == 0) > return; > @@ -511,6 +528,9 @@ static void hsw_audio_codec_get_config(struct intel_encoder *encoder, > > for (i = 0; i < len; i++) > eld[i] = intel_de_read(i915, HSW_AUD_EDID_DATA(cpu_transcoder)); > + > + if (hsw_hdac) > + pm_runtime_put(&hsw_hdac->dev); Maybe these should be put_autosuspend()? Shrug. The runtime pm api remains as impenetrable as ever.
Hi, On Wed, 12 Oct 2022, Ville Syrjala wrote: > On HSW/BDW the hardware ELD buffer does not work if the controller > is suspended. I'm not 100% which thing in there is needed to make it > work (at least just forcing the controller into D0 with setpci is how does the error show up? I'm puzzled how this could have gone unreported for so long. I know many distros have had runtime PM disabled for the HDA controller driver, so that could perhaps explain some of this, but still seems a bit amazing. We started SOF testing with upstream around GLK, so we never covered HSW/BDW, but surely in SOF we always have runtime suspend by default enabled on the audio side. I'll need some more time for review. Will follow up later. Br, Kai
Hi, On Wed, 12 Oct 2022, Kai Vehmanen wrote: > On Wed, 12 Oct 2022, Ville Syrjala wrote: > > > On HSW/BDW the hardware ELD buffer does not work if the controller > > is suspended. I'm not 100% which thing in there is needed to make it > > work (at least just forcing the controller into D0 with setpci is > > how does the error show up? I'm puzzled how this could have gone > unreported for so long. I know many distros have had runtime PM disabled > for the HDA controller driver, so that could perhaps explain some of this, > but still seems a bit amazing. > > We started SOF testing with upstream around GLK, so we never covered > HSW/BDW, but surely in SOF we always have runtime suspend by default > enabled on the audio side. > > I'll need some more time for review. Will follow up later. adding Cezary to the mail thread as well (I forwarded you some context separately. Br, Kai
On Wed, Oct 12, 2022 at 01:49:36PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > On HSW/BDW the hardware ELD buffer does not work if the controller > is suspended. I'm not 100% which thing in there is needed to make it > work (at least just forcing the controller into D0 with setpci is > not enough). But a full runtime resume seems to do the trick here > at least, and so far it looks like this doesn't even deadlock or > anything. So this apparently works for evrything else except module reload, where the ELD buffer isn't ready by the time we do the first modeset. Strangely the same thing works fine at boot time when we first load the drivers. Not sure what the difference is here. I also tried to disable runtime pm for all the hda stuff via sysfs (echo on > power/control) before unloading the modules, but that doesn't help either. And I didn't immediately spot any explicit stuff to power things off when unloading the hda driver. But maybe I missed something? > > Cc: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Cc: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_audio.c | 38 ++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index 328c47719fd8..467f3f260969 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -23,6 +23,7 @@ > > #include <linux/component.h> > #include <linux/kernel.h> > +#include <linux/pm_runtime.h> > > #include <drm/drm_edid.h> > #include <drm/i915_component.h> > @@ -480,6 +481,16 @@ hsw_audio_config_update(struct intel_encoder *encoder, > hsw_hdmi_audio_config_update(encoder, crtc_state); > } > > +static struct pci_dev *hsw_hda_controller(struct drm_i915_private *i915) > +{ > + int domain = pci_domain_nr(to_pci_dev(i915->drm.dev)->bus); > + > + if (!IS_HASWELL(i915) && !IS_BROADWELL(i915)) > + return NULL; > + > + return pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(3, 0)); > +} > + > static int hsw_eld_buffer_size(struct drm_i915_private *i915, > enum transcoder cpu_transcoder) > { > @@ -497,8 +508,14 @@ static void hsw_audio_codec_get_config(struct intel_encoder *encoder, > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > u32 *eld = (u32 *)crtc_state->eld; > int eld_buffer_size, len, i; > + struct pci_dev *hsw_hdac; > u32 tmp; > > + /* on HSW/BDW ELD access doesn't work if the HDA controller is supended */ > + hsw_hdac = hsw_hda_controller(i915); > + if (hsw_hdac) > + pm_runtime_get_sync(&hsw_hdac->dev); > + > tmp = intel_de_read(i915, HSW_AUD_PIN_ELD_CP_VLD); > if ((tmp & AUDIO_ELD_VALID(cpu_transcoder)) == 0) > return; > @@ -511,6 +528,9 @@ static void hsw_audio_codec_get_config(struct intel_encoder *encoder, > > for (i = 0; i < len; i++) > eld[i] = intel_de_read(i915, HSW_AUD_EDID_DATA(cpu_transcoder)); > + > + if (hsw_hdac) > + pm_runtime_put(&hsw_hdac->dev); > } > > static void hsw_audio_codec_disable(struct intel_encoder *encoder, > @@ -520,6 +540,12 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder, > struct drm_i915_private *i915 = to_i915(encoder->base.dev); > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); > enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder; > + struct pci_dev *hsw_hdac; > + > + /* on HSW/BDW ELD access doesn't work if the HDA controller is supended */ > + hsw_hdac = hsw_hda_controller(i915); > + if (hsw_hdac) > + pm_runtime_get_sync(&hsw_hdac->dev); > > mutex_lock(&i915->display.audio.mutex); > > @@ -544,6 +570,9 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder, > AUDIO_OUTPUT_ENABLE(cpu_transcoder), 0); > > mutex_unlock(&i915->display.audio.mutex); > + > + if (hsw_hdac) > + pm_runtime_put(&hsw_hdac->dev); > } > > static unsigned int calc_hblank_early_prog(struct intel_encoder *encoder, > @@ -664,6 +693,12 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder, > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > const u32 *eld = (const u32 *)crtc_state->eld; > int eld_buffer_size, len, i; > + struct pci_dev *hsw_hdac; > + > + /* on HSW/BDW ELD access doesn't work if the HDA controller is supended */ > + hsw_hdac = hsw_hda_controller(i915); > + if (hsw_hdac) > + pm_runtime_get_sync(&hsw_hdac->dev); > > mutex_lock(&i915->display.audio.mutex); > > @@ -705,6 +740,9 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder, > hsw_audio_config_update(encoder, crtc_state); > > mutex_unlock(&i915->display.audio.mutex); > + > + if (hsw_hdac) > + pm_runtime_put(&hsw_hdac->dev); > } > > struct ilk_audio_regs { > -- > 2.35.1
Hi, On Wed, 12 Oct 2022, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > On HSW/BDW the hardware ELD buffer does not work if the controller > is suspended. I'm not 100% which thing in there is needed to make it > work (at least just forcing the controller into D0 with setpci is > not enough). But a full runtime resume seems to do the trick here > at least, and so far it looks like this doesn't even deadlock or > anything. excuse my lack of history information/context, but I also wonder how important writing this to hw AUD_EDID_DATA is anymore. All platforms since Sandy/Ivy Bridge have used the DRM component interface to query ELD (via direct kernel call i915_audio_component_get_eld()). So I don't see any usage of querying the ELD data via "legacy" AC_VERB_GET_HDMI_ELDD method (as that does require powering on the audio controller and codec). At least based on quick browse, I don't see this register having impact to other things than said HDA verb implementation in hardware. May explain why the issue has not been reported before. The patches in the series look good otherwise: Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > + > + if (hsw_hdac) > + pm_runtime_put(&hsw_hdac->dev); I think this is ok. Br, Kai
On Wed, Oct 12, 2022 at 05:24:56PM +0300, Ville Syrjälä wrote: > On Wed, Oct 12, 2022 at 01:49:36PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > On HSW/BDW the hardware ELD buffer does not work if the controller > > is suspended. I'm not 100% which thing in there is needed to make it > > work (at least just forcing the controller into D0 with setpci is > > not enough). But a full runtime resume seems to do the trick here > > at least, and so far it looks like this doesn't even deadlock or > > anything. > > So this apparently works for evrything else except module reload, > where the ELD buffer isn't ready by the time we do the first modeset. > Strangely the same thing works fine at boot time when we first load > the drivers. Not sure what the difference is here. I think the difference was that during boot I had an enabled displays so we never turned off the power well. But during reload the power well gets disabled briefly and some state gets lost.
On Fri, Oct 14, 2022 at 01:51:47PM +0300, Kai Vehmanen wrote: > Hi, > > On Wed, 12 Oct 2022, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > On HSW/BDW the hardware ELD buffer does not work if the controller > > is suspended. I'm not 100% which thing in there is needed to make it > > work (at least just forcing the controller into D0 with setpci is > > not enough). But a full runtime resume seems to do the trick here > > at least, and so far it looks like this doesn't even deadlock or > > anything. > > excuse my lack of history information/context, but I also wonder how > important writing this to hw AUD_EDID_DATA is anymore. All platforms since > Sandy/Ivy Bridge have used the DRM component interface to query ELD (via > direct kernel call i915_audio_component_get_eld()). So I don't see any > usage of querying the ELD data via "legacy" AC_VERB_GET_HDMI_ELDD method > (as that does require powering on the audio controller and codec). At > least based on quick browse, I don't see this register having impact to > other things than said HDA verb implementation in hardware. May explain > why the issue has not been reported before. I guess just trying to not write it and seeing what happens is the best we can do. Do all the platforms that use the software get_eld() stuff totally ignore the hw buffer already in the audio driver? Or do they still respond somehow when we toggle the valid bit? If it's all getting ignored already then I'd like to stop using the buffer for all ilk+. Just need to double check that is where the split is also on the audio driver side. Of if it's not that clear cut on the audio driver side (and not easy to fix), then maybe we need to do the cutoff at hsw+. g4x I'd perhaps like to leave to use the hw buffer since I think it can still take SDVO ADD2 cards (not sure any ilk+ can), so there is at least some kind of chance of someone plugging in a HDMI ADD2 card (rare as those are). And since SDVO depends on the hw buffer still we need to depend on it for the native HDMI too, or else we'll have to convert absolutely everything away from the hw buffer. That might be too much hassle. Anyways, I guess I'll be spooling up a few olders systems and testing how they look w/o the buffer written at all.
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c index 328c47719fd8..467f3f260969 100644 --- a/drivers/gpu/drm/i915/display/intel_audio.c +++ b/drivers/gpu/drm/i915/display/intel_audio.c @@ -23,6 +23,7 @@ #include <linux/component.h> #include <linux/kernel.h> +#include <linux/pm_runtime.h> #include <drm/drm_edid.h> #include <drm/i915_component.h> @@ -480,6 +481,16 @@ hsw_audio_config_update(struct intel_encoder *encoder, hsw_hdmi_audio_config_update(encoder, crtc_state); } +static struct pci_dev *hsw_hda_controller(struct drm_i915_private *i915) +{ + int domain = pci_domain_nr(to_pci_dev(i915->drm.dev)->bus); + + if (!IS_HASWELL(i915) && !IS_BROADWELL(i915)) + return NULL; + + return pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(3, 0)); +} + static int hsw_eld_buffer_size(struct drm_i915_private *i915, enum transcoder cpu_transcoder) { @@ -497,8 +508,14 @@ static void hsw_audio_codec_get_config(struct intel_encoder *encoder, enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; u32 *eld = (u32 *)crtc_state->eld; int eld_buffer_size, len, i; + struct pci_dev *hsw_hdac; u32 tmp; + /* on HSW/BDW ELD access doesn't work if the HDA controller is supended */ + hsw_hdac = hsw_hda_controller(i915); + if (hsw_hdac) + pm_runtime_get_sync(&hsw_hdac->dev); + tmp = intel_de_read(i915, HSW_AUD_PIN_ELD_CP_VLD); if ((tmp & AUDIO_ELD_VALID(cpu_transcoder)) == 0) return; @@ -511,6 +528,9 @@ static void hsw_audio_codec_get_config(struct intel_encoder *encoder, for (i = 0; i < len; i++) eld[i] = intel_de_read(i915, HSW_AUD_EDID_DATA(cpu_transcoder)); + + if (hsw_hdac) + pm_runtime_put(&hsw_hdac->dev); } static void hsw_audio_codec_disable(struct intel_encoder *encoder, @@ -520,6 +540,12 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder, struct drm_i915_private *i915 = to_i915(encoder->base.dev); struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder; + struct pci_dev *hsw_hdac; + + /* on HSW/BDW ELD access doesn't work if the HDA controller is supended */ + hsw_hdac = hsw_hda_controller(i915); + if (hsw_hdac) + pm_runtime_get_sync(&hsw_hdac->dev); mutex_lock(&i915->display.audio.mutex); @@ -544,6 +570,9 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder, AUDIO_OUTPUT_ENABLE(cpu_transcoder), 0); mutex_unlock(&i915->display.audio.mutex); + + if (hsw_hdac) + pm_runtime_put(&hsw_hdac->dev); } static unsigned int calc_hblank_early_prog(struct intel_encoder *encoder, @@ -664,6 +693,12 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder, enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; const u32 *eld = (const u32 *)crtc_state->eld; int eld_buffer_size, len, i; + struct pci_dev *hsw_hdac; + + /* on HSW/BDW ELD access doesn't work if the HDA controller is supended */ + hsw_hdac = hsw_hda_controller(i915); + if (hsw_hdac) + pm_runtime_get_sync(&hsw_hdac->dev); mutex_lock(&i915->display.audio.mutex); @@ -705,6 +740,9 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder, hsw_audio_config_update(encoder, crtc_state); mutex_unlock(&i915->display.audio.mutex); + + if (hsw_hdac) + pm_runtime_put(&hsw_hdac->dev); } struct ilk_audio_regs {