Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm/amdgpu: shared audio pin disabled in dce_v*_0_afmt_setmode() - [Medium] drm/amdgpu: audio packets still enabled when !has_audio in afmt_setmode() - [Medium] drm/amdgpu: dce_v*_0_afmt_setmode() ignores AMDGPU_AUDIO_ENABLE override --
commit f7468a5a289e3e4a150c1f54f3a6dce9ca20fa57 Author: renpanpan <[email protected]> drm/amdgpu: Only enable HDMI audio if display supports it This commit gates the call to dce_v6_0_audio_enable(), dce_v8_0_audio_enable(), and dce_v10_0_audio_enable() at the end of each afmt_setmode() callback on connector->display_info.has_audio so that displays without EDID audio support do not appear as active HDMI audio outputs. > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > index 7de7adc8f225c..05d7da888f492 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > @@ -1705,8 +1705,9 @@ 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 */ > + if (connector->display_info.has_audio) > + 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..f0722bdaeab61 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > @@ -1748,8 +1748,9 @@ 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 */ > + if (connector->display_info.has_audio) > + 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..3f5247314edee 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > @@ -1653,8 +1653,9 @@ 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 */ > + if (connector->display_info.has_audio) > + dce_v8_0_audio_enable(adev, dig->afmt->pin, true); > } [Severity: Medium] Can this disable audio on another active monitor in a multi-display setup? Unlike radeon's dce6_audio_get_pin(), which tracks which pins are already in use by other encoders and only allocates a pin when the display supports audio, dce_v6_0_audio_get_pin(), dce_v8_0_audio_get_pin(), and dce_v10_0_audio_get_pin() always return the first connected pin in adev->mode_info.audio.pin[]. At the start of dce_v6_0_afmt_setmode(), dce_v8_0_afmt_setmode(), and dce_v10_0_afmt_setmode(), the code unconditionally grabs that pin and disables it: dig->afmt->pin = dce_v10_0_audio_get_pin(adev); dce_v10_0_audio_enable(adev, dig->afmt->pin, false); If one encoder is driving a display with audio enabled and a second encoder then runs afmt_setmode() for a display where connector->display_info.has_audio is false (including non-audio DP/eDP displays on DCE6, where dce_v6_0_encoder_mode_set() calls dce_v6_0_afmt_setmode() for both HDMI and DP), the second encoder will grab the same shared pin, disable it at entry, overwrite its endpoint registers, and skip re-enabling it at the end of the function. In addition, because dig->afmt->pin remains non-NULL on the non-audio encoder, later disabling that encoder via dce_v6_0_afmt_enable(), dce_v8_0_afmt_enable(), or dce_v10_0_afmt_enable() with enable == false will also call audio_enable(adev, dig->afmt->pin, false) on the shared pin. [Severity: Medium] When connector->display_info.has_audio is false, should dce_v6_0_afmt_setmode(), dce_v8_0_afmt_setmode(), and dce_v10_0_afmt_setmode() also avoid enabling audio packet and secondary stream transmission in the encoder hardware? With this change, only the final Azalia codec pin enable (AZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL__AUDIO_ENABLED_MASK) is skipped when has_audio is false. Earlier in dce_v8_0_afmt_setmode() and dce_v10_0_afmt_setmode(), the hardware is still programmed to send HDMI Audio InfoFrames, ACR packets, and audio sample packets: tmp = REG_SET_FIELD(tmp, HDMI_INFOFRAME_CONTROL0, HDMI_AUDIO_INFO_SEND, 1); tmp = REG_SET_FIELD(tmp, HDMI_INFOFRAME_CONTROL0, HDMI_AUDIO_INFO_CONT, 1); WREG32(mmHDMI_INFOFRAME_CONTROL0 + dig->afmt->offset, tmp); ... tmp = REG_SET_FIELD(tmp, HDMI_ACR_PACKET_CONTROL, HDMI_ACR_AUTO_SEND, 1); WREG32(mmHDMI_ACR_PACKET_CONTROL + dig->afmt->offset, tmp); ... tmp = REG_SET_FIELD(tmp, AFMT_AUDIO_PACKET_CONTROL, AFMT_AUDIO_SAMPLE_SEND, 1); WREG32(mmAFMT_AUDIO_PACKET_CONTROL + dig->afmt->offset, tmp); Similarly, in dce_v6_0_afmt_setmode(), dce_v6_0_audio_hdmi_enable(encoder, 1) and dce_v6_0_audio_dp_enable(encoder, 1) are still called unconditionally right before the has_audio check, enabling HDMI_AUDIO_INFO_SEND, AFMT_AUDIO_SAMPLE_SEND, and the DP secondary audio stream bits (DP_SEC_ASP_ENABLE, DP_SEC_ATP_ENABLE, DP_SEC_AIP_ENABLE, and DP_SEC_STREAM_ENABLE in mmDP_SEC_CNTL) on sinks that do not support audio. [Severity: Medium] Does checking only connector->display_info.has_audio break the connector audio property override when userspace forces audio on? In drivers/gpu/drm/amd/amdgpu/atombios_encoders.c, amdgpu_atombios_encoder_get_encoder_mode() checks amdgpu_connector->audio: if (amdgpu_audio != 0) { if (amdgpu_connector->audio == AMDGPU_AUDIO_ENABLE) return ATOM_ENCODER_MODE_HDMI; else if (connector->display_info.is_hdmi && (amdgpu_connector->audio == AMDGPU_AUDIO_AUTO)) return ATOM_ENCODER_MODE_HDMI; When a user sets the connector audio property to "on" (AMDGPU_AUDIO_ENABLE) for a monitor whose EDID does not advertise audio (so connector->display_info.has_audio is false), the encoder mode is forced to ATOM_ENCODER_MODE_HDMI and dce_v6_0_afmt_setmode(), dce_v8_0_afmt_setmode(), or dce_v10_0_afmt_setmode() is called, but the final audio_enable(..., true) call will now be skipped because only connector->display_info.has_audio is checked. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
