Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/amdgpu: dce_v*_0_afmt_setmode() disables shared audio pin when !has_audio --
commit 82b015798aa29dad02e55c15ad68c627d16b7df9 Author: renpanpan <[email protected]> drm/amdgpu: Only enable HDMI audio if display supports it This commit gates the final audio_enable(adev, dig->afmt->pin, true) call at the end of dce_v6_0_afmt_setmode(), dce_v8_0_afmt_setmode(), and dce_v10_0_afmt_setmode() on connector->display_info.has_audio or the AMDGPU_AUDIO_ENABLE override. > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > index 7de7adc8f225c..a692227ddfefa 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > @@ -1705,8 +1705,10 @@ static void dce_v10_0_afmt_setmode(struct drm_encoder > *encoder, > WREG32(mmAFMT_RAMP_CONTROL2 + dig->afmt->offset, 0x00000001); > WREG32(mmAFMT_RAMP_CONTROL3 + dig->afmt->offset, 0x00000001); > > - /* enable audio after to setting up hw */ > - dce_v10_0_audio_enable(adev, dig->afmt->pin, true); > + /* Only enable audio if the display supports it or is forced on */ > + if (connector->display_info.has_audio || > + to_amdgpu_connector(connector)->audio == AMDGPU_AUDIO_ENABLE) > + dce_v10_0_audio_enable(adev, dig->afmt->pin, true); > } > > static void dce_v10_0_afmt_enable(struct drm_encoder *encoder, bool enable) > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > index 3c1d6901127d1..d21efce004f56 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > @@ -1748,8 +1748,10 @@ static void dce_v6_0_afmt_setmode(struct drm_encoder > *encoder, > dce_v6_0_audio_dp_enable(encoder, 1); > } > > - /* enable audio after setting up hw */ > - dce_v6_0_audio_enable(adev, dig->afmt->pin, true); > + /* Only enable audio if the display supports it or is forced on */ > + if (connector->display_info.has_audio || > + amdgpu_connector->audio == AMDGPU_AUDIO_ENABLE) > + dce_v6_0_audio_enable(adev, dig->afmt->pin, true); > } > > static void dce_v6_0_afmt_enable(struct drm_encoder *encoder, bool enable) > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > index a17944d5e8434..22c0d91c3041c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > @@ -1653,8 +1653,10 @@ static void dce_v8_0_afmt_setmode(struct drm_encoder > *encoder, > WREG32(mmAFMT_RAMP_CONTROL2 + offset, 0x00000001); > WREG32(mmAFMT_RAMP_CONTROL3 + offset, 0x00000001); > > - /* enable audio after setting up hw */ > - dce_v8_0_audio_enable(adev, dig->afmt->pin, true); > + /* Only enable audio if the display supports it or is forced on */ > + if (connector->display_info.has_audio || > + to_amdgpu_connector(connector)->audio == AMDGPU_AUDIO_ENABLE) > + dce_v8_0_audio_enable(adev, dig->afmt->pin, true); > } [Severity: Medium] Can gating only the final audio_enable(..., true) call in dce_v6_0_afmt_setmode(), dce_v8_0_afmt_setmode(), and dce_v10_0_afmt_setmode() disable active audio on another connected display? In dce_v6_0.c, dce_v8_0.c, and dce_v10_0.c, the audio_get_pin() helpers do not track per-encoder pin ownership and simply return the first connected pin in adev->mode_info.audio.pin[]. At the start of dce_v10_0_afmt_setmode() (and similarly in dce_v6_0_afmt_setmode() and dce_v8_0_afmt_setmode()), the function unconditionally grabs and disables that pin before configuring the hardware: /* disable audio prior to setting up hw */ dig->afmt->pin = dce_v10_0_audio_get_pin(adev); dce_v10_0_audio_enable(adev, dig->afmt->pin, false); It then programs the DTO, pin endpoint registers (speaker allocation, SAD, lipsync), and enables HDMI_AUDIO_INFO_SEND and AFMT_AUDIO_SAMPLE_SEND. When a non-audio HDMI monitor (or a non-audio DP monitor on DCE6) sets mode while another encoder is driving an audio-capable display, afmt_setmode() on the non-audio encoder will grab the same audio pin, disable it via audio_enable(adev, dig->afmt->pin, false), overwrite its endpoint and DTO state, and then skip re-enabling it at the end of the function. Furthermore, because dig->afmt->pin remains assigned on the non-audio encoder, a subsequent call to dce_v6_0_afmt_enable(), dce_v8_0_afmt_enable(), or dce_v10_0_afmt_enable() with enable == false on the non-audio encoder will also call audio_enable(adev, dig->afmt->pin, false) and shut off the shared pin: if (!enable && dig->afmt->pin) { dce_v10_0_audio_enable(adev, dig->afmt->pin, false); dig->afmt->pin = NULL; } Should the audio capability check happen before acquiring and disabling dig->afmt->pin (or in the caller before enabling AFMT audio), similar to how radeon checks drm_detect_monitor_audio() before setting up audio? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
