| Message ID | 20221208154358.3848764-2-kai.vehmanen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda/hdmi: i915 keepalive fixes | expand |
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 <kai.vehmanen@linux.intel.com> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > 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+ */
Hi, On Thu, 8 Dec 2022, Amadeusz Sławiński wrote: > On 12/8/2022 4:43 PM, Kai Vehmanen wrote: > > @@ -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. that's a good point. I think we are safe with current code as setup_stream ops is only called from generic_hdmi_playback_pcm_prepare() and spec->ops.setup_stream() there is only called with a valid pin. But this leaves room for future errors, and passing negative index to get_pin() is pretty bad. Let me send a V2 later today. Thanks for the review! Br, Kai
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); + 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+ */
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 <kai.vehmanen@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- sound/pci/hda/patch_hdmi.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)