Re: [PATCH v2] drm/amdgpu: Only enable HDMI audio if display supports it

[sashiko-bot@kernel.org]

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 <renpanpan@kylinos.cn>

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/20261103033914.21660-1-renpanpan@kylinos.cn?part=1