| Message ID | 20180723145030.25133-1-tiwai@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm_audio_component support for AMD/ATI HDMI codecs | expand |
On Mon, Jul 23, 2018 at 10:50 AM, Takashi Iwai <tiwai@suse.de> wrote: > Hi, > > this is a patch set to add the support of drm_audio_component for > AMD/ATI HDMI codecs. With the drm_audio_component, the HDMI/DP audio > hotplug and ELD read-out can be achieved directly without the hardware > access. The best point by that is that it makes the hotplug > notification working even during runtime suspend. > > The support is totally optional and dynamic, hence it still works even > if either HD-audio or DRM side isn't patched, and it'll fall back to > the existing method. I'm still getting my head around how the new callbacks work so bear with me. It seems like we'd want to set the ELD and report whether the display is attached in when we detect the displays or fetch the EDID rather than at modeset time when we enable the audio stream. Alex > > The current patch supports only radeon and a part of amdgpu; the DC > support isn't included yet. > > > Takashi > > === > > Takashi Iwai (4): > ALSA: hda/hdmi: Use single mutex unlock in error paths > ALSA: hda/hdmi: Allow audio component for AMD/ATI HDMI > drm/radeon: Add audio component support > drm/amdgpu: Add audio component support > > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 + > drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c | 97 ++++++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 + > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 + > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 + > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 6 + > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 + > drivers/gpu/drm/radeon/radeon.h | 3 + > drivers/gpu/drm/radeon/radeon_audio.c | 79 ++++++++ > sound/pci/hda/patch_hdmi.c | 209 +++++++++++++++++----- > 12 files changed, 374 insertions(+), 49 deletions(-) > create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c > > -- > 2.18.0 > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Mon, 23 Jul 2018 22:53:08 +0200, Alex Deucher wrote: > > On Mon, Jul 23, 2018 at 10:50 AM, Takashi Iwai <tiwai@suse.de> wrote: > > Hi, > > > > this is a patch set to add the support of drm_audio_component for > > AMD/ATI HDMI codecs. With the drm_audio_component, the HDMI/DP audio > > hotplug and ELD read-out can be achieved directly without the hardware > > access. The best point by that is that it makes the hotplug > > notification working even during runtime suspend. > > > > The support is totally optional and dynamic, hence it still works even > > if either HD-audio or DRM side isn't patched, and it'll fall back to > > the existing method. > > I'm still getting my head around how the new callbacks work so bear > with me. It seems like we'd want to set the ELD and report whether > the display is attached in when we detect the displays or fetch the > EDID rather than at modeset time when we enable the audio stream. Basically the callbacks are just replacements of the existing mechanism with the direct calls. In the traditional model, from GPU to HD-audio, we trigger a hotplug event via writing a dedicated GPU register. In HD-audio side, it's transmitted as an unsolicited event via HD-audio bus, and HD-audio driver receives it. And for passing ELD, we write some bytes to GPU registers in DRM driver. These are read by HD-audio driver in hotplug handler after receiving the event. In the callback model, GPU calls audio_ops.pin_eld_notify() with the pin index. This callback is set by HD-audio. Then HD-audio reads back ELD bytes in return by calling ops.get_eld() with the given pin index. This callback is set by DRM. For registration and de-registration, DRM gives the component bind / unbind to set / clear its ops. thanks, Takashi > > Alex > > > > > > The current patch supports only radeon and a part of amdgpu; the DC > > support isn't included yet. > > > > > > Takashi > > > > === > > > > Takashi Iwai (4): > > ALSA: hda/hdmi: Use single mutex unlock in error paths > > ALSA: hda/hdmi: Allow audio component for AMD/ATI HDMI > > drm/radeon: Add audio component support > > drm/amdgpu: Add audio component support > > > > drivers/gpu/drm/Kconfig | 2 + > > drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 + > > drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c | 97 ++++++++++ > > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 + > > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 + > > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 + > > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 6 + > > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 + > > drivers/gpu/drm/radeon/radeon.h | 3 + > > drivers/gpu/drm/radeon/radeon_audio.c | 79 ++++++++ > > sound/pci/hda/patch_hdmi.c | 209 +++++++++++++++++----- > > 12 files changed, 374 insertions(+), 49 deletions(-) > > create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c > > > > -- > > 2.18.0 > > > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx >
On Wed, 25 Jul 2018 05:32:52 +0200, Qu, Jim wrote: > @@ -269,6 +271,10 @@ static void radeon_audio_enable(struct radeon_device *rdev, > > if (rdev->audio.funcs->enable) > rdev->audio.funcs->enable(rdev, pin, enable_mask); > + > + if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) > + acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, > + pin->id, -1); > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > Jim: radeon_audio_enable() can enable audios and also disable them, so eld noitfy callback should be called when enable_mask is true. It's intentional. The notifier needs to be called at disablement time, too. The audio driver has to follow the HDMI audio disablement, and notifier receives both on and off case. The actual state is inquired via get_eld call by HD-audio followed after the notification. thanks, Takashi
On 2018年07月25日 13:28, Takashi Iwai wrote: > On Wed, 25 Jul 2018 05:32:52 +0200, > Qu, Jim wrote: >> @@ -269,6 +271,10 @@ static void radeon_audio_enable(struct radeon_device *rdev, >> >> if (rdev->audio.funcs->enable) >> rdev->audio.funcs->enable(rdev, pin, enable_mask); >> + >> + if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) >> + acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, >> + pin->id, -1); >> >> Jim: radeon_audio_enable() can enable audios and also disable them, so eld noitfy callback should be called when enable_mask is true. > It's intentional. The notifier needs to be called at disablement > time, too. The audio driver has to follow the HDMI audio disablement, > and notifier receives both on and off case. > > The actual state is inquired via get_eld call by HD-audio followed > after the notification. > > > thanks, > > Takashi OK, I got your point. Thanks JimQu
On Wed, 25 Jul 2018 07:38:37 +0200, Qu, Jim wrote: > > Jim: Just like Alex said, we want driver can get eld info when hotplug in new device. amdgpu driver is a bit difference from radeon driver, it is not a suitable place to call notify() function in *_audio_enable() , since they are not in the hotplug process context like them in radeon driver, but the mode setting. > > IMO, the right place to call notify() function is also in the amdgpu_connector_xxxx_detect() in amdgpu_connector.c. Hm, but by the modesetting, it actually enables / disables the audio as well, no? If so, the notifier is exactly for that purpose. The audio driver needs to know not only about the physical connection but whether the audio can be actually driven. That is, even if the monitor is connected, the audio won't come out if the mode is off. That is equivalent with the unplugged state for the audio driver. The i915 driver deals with the notifier just like the above, so the behavior is intentional. thanks, Takashi > With None-DC driver, the detect() of connectors will to be preformed in drm_helper_hpd_irq_event() when there is device hotplug. However, amdgpu_connector_xxxx_detect() maybe need to be modified to fit your change. > > Thanks > JimQu > ************************************************************************************************************ > --- > drivers/gpu/drm/Kconfig | 1 + > drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 + > drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c | 97 +++++++++++++++++++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 + > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++ > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++ > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 6 ++ > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++ > 9 files changed, 130 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 2c7112ddfed4..fbe7216c5c56 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -193,6 +193,7 @@ config DRM_AMDGPU > select BACKLIGHT_LCD_SUPPORT > select INTERVAL_TREE > select CHASH > + select SND_HDA_COMPONENT if SND_HDA_CORE > help > Choose this option if you have a recent AMD Radeon graphics card. > > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile > index bfd332c95b61..9c26facddb17 100644 > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > @@ -52,7 +52,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \ > amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \ > amdgpu_gtt_mgr.o amdgpu_vram_mgr.o amdgpu_virt.o amdgpu_atomfirmware.o \ > amdgpu_queue_mgr.o amdgpu_vf_error.o amdgpu_sched.o amdgpu_debugfs.o \ > - amdgpu_ids.o > + amdgpu_ids.o amdgpu_audio.o > > # add asic specific block > amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o kv_smc.o kv_dpm.o \ > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index a59c07590cee..203d2584c989 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -1957,5 +1957,9 @@ int amdgpu_dm_display_resume(struct amdgpu_device *adev ); > static inline int amdgpu_dm_display_resume(struct amdgpu_device *adev) { return 0; } > #endif > > +int amdgpu_audio_component_init(struct amdgpu_device *adev); > +void amdgpu_audio_component_fini(struct amdgpu_device *adev); > +void amdgpu_audio_eld_notify(struct amdgpu_device *adev, int pin); > + > #include "amdgpu_object.h" > #endif > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c > new file mode 100644 > index 000000000000..39256e2f84b3 > --- /dev/null > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c > @@ -0,0 +1,97 @@ > +// SPDX-License-Identifier: MIT > + > +#include <linux/component.h> > +#include "amdgpu.h" > + > +static int amdgpu_audio_component_get_eld(struct device *kdev, int port, > + int pipe, bool *enabled, > + unsigned char *buf, int max_bytes) > +{ > + struct drm_device *dev = dev_get_drvdata(kdev); > + struct drm_encoder *encoder; > + struct amdgpu_encoder *amdgpu_encoder; > + struct amdgpu_encoder_atom_dig *dig; > + struct drm_connector *connector; > + int ret = 0; > + > + *enabled = 0; > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { > + amdgpu_encoder = to_amdgpu_encoder(encoder); > + dig = amdgpu_encoder->enc_priv; > + if (!dig || !dig->afmt || !dig->afmt->enabled) > + continue; > + if (!dig->afmt->pin || dig->afmt->pin->id != port) > + continue; > + connector = amdgpu_get_connector_for_encoder(encoder); > + *enabled = !!connector; > + if (connector) { > + ret = drm_eld_size(connector->eld); > + memcpy(buf, connector->eld, min(max_bytes, ret)); > + break; > + } > + } > + > + return ret; > +} > + > +static const struct drm_audio_component_ops amdgpu_audio_component_ops = { > + .get_eld = amdgpu_audio_component_get_eld, > +}; > + > +static int amdgpu_audio_component_bind(struct device *kdev, > + struct device *hda_kdev, void *data) > +{ > + struct drm_device *dev = dev_get_drvdata(kdev); > + struct amdgpu_device *adev = dev->dev_private; > + struct drm_audio_component *acomp = data; > + > + acomp->ops = &amdgpu_audio_component_ops; > + acomp->dev = kdev; > + adev->mode_info.audio.component = acomp; > + return 0; > +} > + > +static void amdgpu_audio_component_unbind(struct device *kdev, > + struct device *hda_kdev, void *data) > +{ > + struct drm_device *dev = dev_get_drvdata(kdev); > + struct amdgpu_device *adev = dev->dev_private; > + struct drm_audio_component *acomp = data; > + > + acomp->ops = NULL; > + acomp->dev = NULL; > + adev->mode_info.audio.component = NULL; > +} > + > +static const struct component_ops amdgpu_audio_component_bind_ops = { > + .bind = amdgpu_audio_component_bind, > + .unbind = amdgpu_audio_component_unbind, > +}; > + > +int amdgpu_audio_component_init(struct amdgpu_device *adev) > +{ > + int err; > + > + err = component_add(adev->dev, &amdgpu_audio_component_bind_ops); > + if (err < 0) > + return err; > + adev->mode_info.audio.component_registered = true; > + return 0; > +} > + > +void amdgpu_audio_component_fini(struct amdgpu_device *adev) > +{ > + if (adev->mode_info.audio.component_registered) { > + component_del(adev->dev, &amdgpu_audio_component_bind_ops); > + adev->mode_info.audio.component_registered = false; > + } > +} > + > +void amdgpu_audio_eld_notify(struct amdgpu_device *adev, int pin) > +{ > + struct drm_audio_component *acomp = adev->mode_info.audio.component; > + > + if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) > + acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, > + pin, -1); > +} > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > index b9e9e8b02fb7..7cb1aa97e522 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > @@ -39,6 +39,7 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_plane_helper.h> > #include <drm/drm_fb_helper.h> > +#include <drm/drm_audio_component.h> > #include <linux/i2c.h> > #include <linux/i2c-algo-bit.h> > #include <linux/hrtimer.h> > @@ -260,6 +261,8 @@ struct amdgpu_audio { > bool enabled; > struct amdgpu_audio_pin pin[AMDGPU_MAX_AFMT_BLOCKS]; > int num_pins; > + bool component_registered; > + struct drm_audio_component *component; > }; > > struct amdgpu_display_funcs { > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > index ada241bfeee9..c8471a59930f 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > @@ -1392,6 +1392,8 @@ static void dce_v10_0_audio_enable(struct amdgpu_device *adev, > > WREG32_AUDIO_ENDPT(pin->offset, ixAZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL, > enable ? AZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL__AUDIO_ENABLED_MASK : 0); > + > + amdgpu_audio_eld_notify(adev, pin->id); > } > > static const u32 pin_offsets[] = > @@ -1430,6 +1432,8 @@ static int dce_v10_0_audio_init(struct amdgpu_device *adev) > dce_v10_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); > } > > + amdgpu_audio_component_init(adev); > + > return 0; > } > > @@ -1443,6 +1447,8 @@ static void dce_v10_0_audio_fini(struct amdgpu_device *adev) > if (!adev->mode_info.audio.enabled) > return; > > + amdgpu_audio_component_fini(adev); > + > for (i = 0; i < adev->mode_info.audio.num_pins; i++) > dce_v10_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); > > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > index a5b96eac3033..49edb74725a7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > @@ -1418,6 +1418,8 @@ static void dce_v11_0_audio_enable(struct amdgpu_device *adev, > > WREG32_AUDIO_ENDPT(pin->offset, ixAZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL, > enable ? AZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL__AUDIO_ENABLED_MASK : 0); > + > + amdgpu_audio_eld_notify(adev, pin->id); > } > > static const u32 pin_offsets[] = > @@ -1472,6 +1474,8 @@ static int dce_v11_0_audio_init(struct amdgpu_device *adev) > dce_v11_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); > } > > + amdgpu_audio_component_init(adev); > + > return 0; > } > > @@ -1485,6 +1489,8 @@ static void dce_v11_0_audio_fini(struct amdgpu_device *adev) > if (!adev->mode_info.audio.enabled) > return; > > + amdgpu_audio_component_fini(adev); > + > for (i = 0; i < adev->mode_info.audio.num_pins; i++) > dce_v11_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); > > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > index 394cc1e8fe20..2b7468eaacca 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > @@ -1297,6 +1297,8 @@ static void dce_v6_0_audio_enable(struct amdgpu_device *adev, > > WREG32_AUDIO_ENDPT(pin->offset, ixAZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL, > enable ? AZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL__AUDIO_ENABLED_MASK : 0); > + > + amdgpu_audio_eld_notify(adev, pin->id); > } > > static const u32 pin_offsets[7] = > @@ -1343,6 +1345,8 @@ static int dce_v6_0_audio_init(struct amdgpu_device *adev) > dce_v6_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); > } > > + amdgpu_audio_component_init(adev); > + > return 0; > } > > @@ -1356,6 +1360,8 @@ static void dce_v6_0_audio_fini(struct amdgpu_device *adev) > if (!adev->mode_info.audio.enabled) > return; > > + amdgpu_audio_component_fini(adev); > + > for (i = 0; i < adev->mode_info.audio.num_pins; i++) > dce_v6_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); > > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > index c9b9ab8f1b05..c2370ea1f64f 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > @@ -1348,6 +1348,8 @@ static void dce_v8_0_audio_enable(struct amdgpu_device *adev, > > WREG32_AUDIO_ENDPT(pin->offset, ixAZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL, > enable ? AZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL__AUDIO_ENABLED_MASK : 0); > + > + amdgpu_audio_eld_notify(adev, pin->id); > } > > static const u32 pin_offsets[7] = > @@ -1395,6 +1397,8 @@ static int dce_v8_0_audio_init(struct amdgpu_device *adev) > dce_v8_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); > } > > + amdgpu_audio_component_init(adev); > + > return 0; > } > > @@ -1408,6 +1412,8 @@ static void dce_v8_0_audio_fini(struct amdgpu_device *adev) > if (!adev->mode_info.audio.enabled) > return; > > + amdgpu_audio_component_fini(adev); > + > for (i = 0; i < adev->mode_info.audio.num_pins; i++) > dce_v8_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); > > -- > 2.18.0 > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 2018年07月25日 13:46, Takashi Iwai wrote: > On Wed, 25 Jul 2018 07:38:37 +0200, > Qu, Jim wrote: >> Jim: Just like Alex said, we want driver can get eld info when hotplug in new device. amdgpu driver is a bit difference from radeon driver, it is not a suitable place to call notify() function in *_audio_enable() , since they are not in the hotplug process context like them in radeon driver, but the mode setting. >> >> IMO, the right place to call notify() function is also in the amdgpu_connector_xxxx_detect() in amdgpu_connector.c. > Hm, but by the modesetting, it actually enables / disables the audio > as well, no? If so, the notifier is exactly for that purpose. The > audio driver needs to know not only about the physical connection but > whether the audio can be actually driven. > > That is, even if the monitor is connected, the audio won't come out if > the mode is off. That is equivalent with the unplugged state for the > audio driver. > > The i915 driver deals with the notifier just like the above, so the > behavior is intentional. > > > thanks, > > Takashi I am afraid if device hotplug out, how is audio state if it follow up eld info? Since the modesetting is never performed for the display which is plugged out, so there is no notify() call on it. Thanks JimQu >> With None-DC driver, the detect() of connectors will to be preformed in drm_helper_hpd_irq_event() when there is device hotplug. However, amdgpu_connector_xxxx_detect() maybe need to be modified to fit your change. >> >> Thanks >> JimQu >> ************************************************************************************************************ >> --- >> drivers/gpu/drm/Kconfig | 1 + >> drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- >> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 + >> drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c | 97 +++++++++++++++++++++++ >> drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 + >> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 ++ >> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 ++ >> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 6 ++ >> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++ >> 9 files changed, 130 insertions(+), 1 deletion(-) >> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c >> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >> index 2c7112ddfed4..fbe7216c5c56 100644 >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -193,6 +193,7 @@ config DRM_AMDGPU >> select BACKLIGHT_LCD_SUPPORT >> select INTERVAL_TREE >> select CHASH >> + select SND_HDA_COMPONENT if SND_HDA_CORE >> help >> Choose this option if you have a recent AMD Radeon graphics card. >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile >> index bfd332c95b61..9c26facddb17 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/Makefile >> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile >> @@ -52,7 +52,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \ >> amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \ >> amdgpu_gtt_mgr.o amdgpu_vram_mgr.o amdgpu_virt.o amdgpu_atomfirmware.o \ >> amdgpu_queue_mgr.o amdgpu_vf_error.o amdgpu_sched.o amdgpu_debugfs.o \ >> - amdgpu_ids.o >> + amdgpu_ids.o amdgpu_audio.o >> >> # add asic specific block >> amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o kv_smc.o kv_dpm.o \ >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> index a59c07590cee..203d2584c989 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> @@ -1957,5 +1957,9 @@ int amdgpu_dm_display_resume(struct amdgpu_device *adev ); >> static inline int amdgpu_dm_display_resume(struct amdgpu_device *adev) { return 0; } >> #endif >> >> +int amdgpu_audio_component_init(struct amdgpu_device *adev); >> +void amdgpu_audio_component_fini(struct amdgpu_device *adev); >> +void amdgpu_audio_eld_notify(struct amdgpu_device *adev, int pin); >> + >> #include "amdgpu_object.h" >> #endif >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c >> new file mode 100644 >> index 000000000000..39256e2f84b3 >> --- /dev/null >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c >> @@ -0,0 +1,97 @@ >> +// SPDX-License-Identifier: MIT >> + >> +#include <linux/component.h> >> +#include "amdgpu.h" >> + >> +static int amdgpu_audio_component_get_eld(struct device *kdev, int port, >> + int pipe, bool *enabled, >> + unsigned char *buf, int max_bytes) >> +{ >> + struct drm_device *dev = dev_get_drvdata(kdev); >> + struct drm_encoder *encoder; >> + struct amdgpu_encoder *amdgpu_encoder; >> + struct amdgpu_encoder_atom_dig *dig; >> + struct drm_connector *connector; >> + int ret = 0; >> + >> + *enabled = 0; >> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { >> + amdgpu_encoder = to_amdgpu_encoder(encoder); >> + dig = amdgpu_encoder->enc_priv; >> + if (!dig || !dig->afmt || !dig->afmt->enabled) >> + continue; >> + if (!dig->afmt->pin || dig->afmt->pin->id != port) >> + continue; >> + connector = amdgpu_get_connector_for_encoder(encoder); >> + *enabled = !!connector; >> + if (connector) { >> + ret = drm_eld_size(connector->eld); >> + memcpy(buf, connector->eld, min(max_bytes, ret)); >> + break; >> + } >> + } >> + >> + return ret; >> +} >> + >> +static const struct drm_audio_component_ops amdgpu_audio_component_ops = { >> + .get_eld = amdgpu_audio_component_get_eld, >> +}; >> + >> +static int amdgpu_audio_component_bind(struct device *kdev, >> + struct device *hda_kdev, void *data) >> +{ >> + struct drm_device *dev = dev_get_drvdata(kdev); >> + struct amdgpu_device *adev = dev->dev_private; >> + struct drm_audio_component *acomp = data; >> + >> + acomp->ops = &amdgpu_audio_component_ops; >> + acomp->dev = kdev; >> + adev->mode_info.audio.component = acomp; >> + return 0; >> +} >> + >> +static void amdgpu_audio_component_unbind(struct device *kdev, >> + struct device *hda_kdev, void *data) >> +{ >> + struct drm_device *dev = dev_get_drvdata(kdev); >> + struct amdgpu_device *adev = dev->dev_private; >> + struct drm_audio_component *acomp = data; >> + >> + acomp->ops = NULL; >> + acomp->dev = NULL; >> + adev->mode_info.audio.component = NULL; >> +} >> + >> +static const struct component_ops amdgpu_audio_component_bind_ops = { >> + .bind = amdgpu_audio_component_bind, >> + .unbind = amdgpu_audio_component_unbind, >> +}; >> + >> +int amdgpu_audio_component_init(struct amdgpu_device *adev) >> +{ >> + int err; >> + >> + err = component_add(adev->dev, &amdgpu_audio_component_bind_ops); >> + if (err < 0) >> + return err; >> + adev->mode_info.audio.component_registered = true; >> + return 0; >> +} >> + >> +void amdgpu_audio_component_fini(struct amdgpu_device *adev) >> +{ >> + if (adev->mode_info.audio.component_registered) { >> + component_del(adev->dev, &amdgpu_audio_component_bind_ops); >> + adev->mode_info.audio.component_registered = false; >> + } >> +} >> + >> +void amdgpu_audio_eld_notify(struct amdgpu_device *adev, int pin) >> +{ >> + struct drm_audio_component *acomp = adev->mode_info.audio.component; >> + >> + if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) >> + acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, >> + pin, -1); >> +} >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h >> index b9e9e8b02fb7..7cb1aa97e522 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h >> @@ -39,6 +39,7 @@ >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_plane_helper.h> >> #include <drm/drm_fb_helper.h> >> +#include <drm/drm_audio_component.h> >> #include <linux/i2c.h> >> #include <linux/i2c-algo-bit.h> >> #include <linux/hrtimer.h> >> @@ -260,6 +261,8 @@ struct amdgpu_audio { >> bool enabled; >> struct amdgpu_audio_pin pin[AMDGPU_MAX_AFMT_BLOCKS]; >> int num_pins; >> + bool component_registered; >> + struct drm_audio_component *component; >> }; >> >> struct amdgpu_display_funcs { >> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c >> index ada241bfeee9..c8471a59930f 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c >> @@ -1392,6 +1392,8 @@ static void dce_v10_0_audio_enable(struct amdgpu_device *adev, >> >> WREG32_AUDIO_ENDPT(pin->offset, ixAZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL, >> enable ? AZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL__AUDIO_ENABLED_MASK : 0); >> + >> + amdgpu_audio_eld_notify(adev, pin->id); >> } >> >> static const u32 pin_offsets[] = >> @@ -1430,6 +1432,8 @@ static int dce_v10_0_audio_init(struct amdgpu_device *adev) >> dce_v10_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); >> } >> >> + amdgpu_audio_component_init(adev); >> + >> return 0; >> } >> >> @@ -1443,6 +1447,8 @@ static void dce_v10_0_audio_fini(struct amdgpu_device *adev) >> if (!adev->mode_info.audio.enabled) >> return; >> >> + amdgpu_audio_component_fini(adev); >> + >> for (i = 0; i < adev->mode_info.audio.num_pins; i++) >> dce_v10_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c >> index a5b96eac3033..49edb74725a7 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c >> @@ -1418,6 +1418,8 @@ static void dce_v11_0_audio_enable(struct amdgpu_device *adev, >> >> WREG32_AUDIO_ENDPT(pin->offset, ixAZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL, >> enable ? AZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL__AUDIO_ENABLED_MASK : 0); >> + >> + amdgpu_audio_eld_notify(adev, pin->id); >> } >> >> static const u32 pin_offsets[] = >> @@ -1472,6 +1474,8 @@ static int dce_v11_0_audio_init(struct amdgpu_device *adev) >> dce_v11_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); >> } >> >> + amdgpu_audio_component_init(adev); >> + >> return 0; >> } >> >> @@ -1485,6 +1489,8 @@ static void dce_v11_0_audio_fini(struct amdgpu_device *adev) >> if (!adev->mode_info.audio.enabled) >> return; >> >> + amdgpu_audio_component_fini(adev); >> + >> for (i = 0; i < adev->mode_info.audio.num_pins; i++) >> dce_v11_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> index 394cc1e8fe20..2b7468eaacca 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> @@ -1297,6 +1297,8 @@ static void dce_v6_0_audio_enable(struct amdgpu_device *adev, >> >> WREG32_AUDIO_ENDPT(pin->offset, ixAZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL, >> enable ? AZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL__AUDIO_ENABLED_MASK : 0); >> + >> + amdgpu_audio_eld_notify(adev, pin->id); >> } >> >> static const u32 pin_offsets[7] = >> @@ -1343,6 +1345,8 @@ static int dce_v6_0_audio_init(struct amdgpu_device *adev) >> dce_v6_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); >> } >> >> + amdgpu_audio_component_init(adev); >> + >> return 0; >> } >> >> @@ -1356,6 +1360,8 @@ static void dce_v6_0_audio_fini(struct amdgpu_device *adev) >> if (!adev->mode_info.audio.enabled) >> return; >> >> + amdgpu_audio_component_fini(adev); >> + >> for (i = 0; i < adev->mode_info.audio.num_pins; i++) >> dce_v6_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c >> index c9b9ab8f1b05..c2370ea1f64f 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c >> @@ -1348,6 +1348,8 @@ static void dce_v8_0_audio_enable(struct amdgpu_device *adev, >> >> WREG32_AUDIO_ENDPT(pin->offset, ixAZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL, >> enable ? AZALIA_F0_CODEC_PIN_CONTROL_HOT_PLUG_CONTROL__AUDIO_ENABLED_MASK : 0); >> + >> + amdgpu_audio_eld_notify(adev, pin->id); >> } >> >> static const u32 pin_offsets[7] = >> @@ -1395,6 +1397,8 @@ static int dce_v8_0_audio_init(struct amdgpu_device *adev) >> dce_v8_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); >> } >> >> + amdgpu_audio_component_init(adev); >> + >> return 0; >> } >> >> @@ -1408,6 +1412,8 @@ static void dce_v8_0_audio_fini(struct amdgpu_device *adev) >> if (!adev->mode_info.audio.enabled) >> return; >> >> + amdgpu_audio_component_fini(adev); >> + >> for (i = 0; i < adev->mode_info.audio.num_pins; i++) >> dce_v8_0_audio_enable(adev, &adev->mode_info.audio.pin[i], false); >> >> -- >> 2.18.0 >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Wed, 25 Jul 2018 10:02:34 +0200, jimqu wrote: > > > > On 2018年07月25日 13:46, Takashi Iwai wrote: > > On Wed, 25 Jul 2018 07:38:37 +0200, > > Qu, Jim wrote: > >> Jim: Just like Alex said, we want driver can get eld info when hotplug in new device. amdgpu driver is a bit difference from radeon driver, it is not a suitable place to call notify() function in *_audio_enable() , since they are not in the hotplug process context like them in radeon driver, but the mode setting. > >> > >> IMO, the right place to call notify() function is also in the amdgpu_connector_xxxx_detect() in amdgpu_connector.c. > > Hm, but by the modesetting, it actually enables / disables the audio > > as well, no? If so, the notifier is exactly for that purpose. The > > audio driver needs to know not only about the physical connection but > > whether the audio can be actually driven. > > > > That is, even if the monitor is connected, the audio won't come out if > > the mode is off. That is equivalent with the unplugged state for the > > audio driver. > > > > The i915 driver deals with the notifier just like the above, so the > > behavior is intentional. > > > > > > thanks, > > > > Takashi > > I am afraid if device hotplug out, how is audio state if it follow up > eld info? Since the modesetting is never performed for the display > which is plugged out, so there is no notify() call on it. In principle, the HDMI audio just needs to follows the video state, and it doesn't need to care actual physical connections. As long as video can go out, it's fine, audio can, too. When video is disabled (even if connected), audio can't be used as well, so it must follow to off, too. The notifier is used to follow this video state change. Practically seen, the user-space shall switch off the video accordingly upon hot unplug, then the audio notifier is sent, and the audio gets off, too. thanks, Takashi
On 2018年07月25日 16:20, Takashi Iwai wrote: > On Wed, 25 Jul 2018 10:02:34 +0200, > jimqu wrote: >> >> >> On 2018年07月25日 13:46, Takashi Iwai wrote: >>> On Wed, 25 Jul 2018 07:38:37 +0200, >>> Qu, Jim wrote: >>>> Jim: Just like Alex said, we want driver can get eld info when hotplug in new device. amdgpu driver is a bit difference from radeon driver, it is not a suitable place to call notify() function in *_audio_enable() , since they are not in the hotplug process context like them in radeon driver, but the mode setting. >>>> >>>> IMO, the right place to call notify() function is also in the amdgpu_connector_xxxx_detect() in amdgpu_connector.c. >>> Hm, but by the modesetting, it actually enables / disables the audio >>> as well, no? If so, the notifier is exactly for that purpose. The >>> audio driver needs to know not only about the physical connection but >>> whether the audio can be actually driven. >>> >>> That is, even if the monitor is connected, the audio won't come out if >>> the mode is off. That is equivalent with the unplugged state for the >>> audio driver. >>> >>> The i915 driver deals with the notifier just like the above, so the >>> behavior is intentional. >>> >>> >>> thanks, >>> >>> Takashi >> I am afraid if device hotplug out, how is audio state if it follow up >> eld info? Since the modesetting is never performed for the display >> which is plugged out, so there is no notify() call on it. > In principle, the HDMI audio just needs to follows the video state, > and it doesn't need to care actual physical connections. As long as > video can go out, it's fine, audio can, too. When video is disabled > (even if connected), audio can't be used as well, so it must follow to > off, too. The notifier is used to follow this video state change. > > Practically seen, the user-space shall switch off the video > accordingly upon hot unplug, then the audio notifier is sent, and the > audio gets off, too. > > > thanks, > > Takashi Ok, that means there are other code pathes to update audio state. Anyway, for patch 3/4 Acked-by: Jim Qu <Jim.Qu@amd.com> There may be other concerns from Alex/Christian. Thanks JimQu