Re: [Intel-gfx] [PATCH 1/3] ALSA: hda/hdmi: fix i915 silent stream programming flow

[Amadeusz Sławiński]

On 12/8/2022 4:43 PM, Kai Vehmanen wrote:

The i915 display codec may not successfully transition to
normal audio streaming mode, if the stream id is programmed
while codec is actively transmitting data. This can happen
when silent stream is enabled in KAE mode.

Fix the issue by implementing a i915 specific programming
flow, where the silent streaming is temporarily stopped,
a small delay is applied to ensure display codec becomes
idle, and then proceed with reprogramming the stream ID.

Fixes: 15175a4f2bbb ("ALSA: hda/hdmi: add keep-alive support for ADL-P and DG2")
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/7353
Signed-off-by: Kai Vehmanen 
Reviewed-by: Pierre-Louis Bossart 
Tested-by: Rodrigo Vivi 
---
  sound/pci/hda/patch_hdmi.c | 23 +--
  1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 7a40ddfd695a..a0ba24165ae2 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2879,9 +2879,28 @@ static int i915_hsw_setup_stream(struct hda_codec 
*codec, hda_nid_t cvt_nid,
 hda_nid_t pin_nid, int dev_id, u32 stream_tag,
 int format)
  {
+   struct hdmi_spec *spec = codec->spec;
+   int pin_idx = pin_id_to_pin_index(codec, pin_nid, dev_id);


Shouldn't return value from pin_id_to_pin_index() be checked? It seems 
that it can return -EINVAL.



+   struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
+   int res;
+
haswell_verify_D0(codec, cvt_nid, pin_nid);
-   return hdmi_setup_stream(codec, cvt_nid, pin_nid, dev_id,
-stream_tag, format);
+
+   if (spec->silent_stream_type == SILENT_STREAM_KAE && per_pin && 
per_pin->silent_stream) {
+   silent_stream_set_kae(codec, per_pin, false);
+   /* wait for pending transfers in codec to clear */
+   usleep_range(100, 200);
+   }
+
+   res = hdmi_setup_stream(codec, cvt_nid, pin_nid, dev_id,
+   stream_tag, format);
+
+   if (spec->silent_stream_type == SILENT_STREAM_KAE && per_pin && 
per_pin->silent_stream) {
+   usleep_range(100, 200);
+   silent_stream_set_kae(codec, per_pin, true);
+   }
+
+   return res;
  }
  
  /* pin_cvt_fixup ops override for HSW+ and VLV+ */

Re: [Intel-gfx] [PATCH] ALSA: hda/i915 - avoid hung task timeout in i915 wait

[Amadeusz Sławiński]

On 3/8/2022 3:11 PM, Kai Vehmanen wrote:

If kernel is built with hung task detection enabled and
CONFIG_DEFAULT_HUNG_TASK_TIMEOUT set to less than 60 seconds,
snd_hdac_i915_init() will trigger the hung task timeout in case i915 is
not available and taint the kernel.

Split the 60sec wait into a loop of smaller waits to avoid this.

Cc: Lucas De Marchi 
Co-developed-by: Ramalingam C 
Signed-off-by: Ramalingam C 
Signed-off-by: Kai Vehmanen 
---
  sound/hda/hdac_i915.c | 10 ++
  1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 454474ac5716..6219de739b56 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -143,7 +143,8 @@ static bool i915_gfx_present(void)
  int snd_hdac_i915_init(struct hdac_bus *bus)
  {
struct drm_audio_component *acomp;
-   int err;
+   unsigned long wait = 0;


I'm not sure if it is best name for variable that is set to 0 ("false"), 
maybe `done` would be better? Especially looking at condition in the 
following for loop.



+   int err, i;
  
  	if (!i915_gfx_present())

return -ENODEV;
@@ -159,9 +160,10 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
if (!acomp->ops) {
if (!IS_ENABLED(CONFIG_MODULES) ||
!request_module("i915")) {
-   /* 60s timeout */
-   
wait_for_completion_timeout(>master_bind_complete,
-   msecs_to_jiffies(60 * 
1000));
+   /* max 60s timeout */
+   for (i = 0; !wait && i < 60; i++)
+   wait = 
wait_for_completion_timeout(>master_bind_complete,
+  
msecs_to_jiffies(1000));
}
}
if (!acomp->ops) {

base-commit: fd7698cf0858f8c5e659b655109cd93c2f15cdd3