On Mon, 22 Jul 2019 17:35:09 +0200, Alex Deucher wrote: > > On Mon, Jul 22, 2019 at 11:21 AM Takashi Iwai <tiwai@xxxxxxx> wrote: > > > > On Mon, 22 Jul 2019 17:07:09 +0200, > > Koenig, Christian wrote: > > > > > > Am 22.07.19 um 16:53 schrieb Takashi Iwai: > > > > On Mon, 22 Jul 2019 16:44:06 +0200, > > > > Koenig, Christian wrote: > > > >> Am 22.07.19 um 16:38 schrieb Takashi Iwai: > > > >>> This patch adds the support for the notification of HD-audio hotplug > > > >>> via the already existing drm_audio_component framework to radeon > > > >>> driver. This allows us more reliable hotplug notification and ELD > > > >>> transfer without accessing HD-audio bus; it's more efficient, and more > > > >>> importantly, it works without waking up the runtime PM. > > > >>> > > > >>> The implementation is rather simplistic: radeon driver provides the > > > >>> get_eld ops for HD-audio, and it notifies the audio hotplug via > > > >>> pin_eld_notify callback upon each radeon_audio_enable() call. > > > >>> The pin->id is referred as the port number passed to the notifier > > > >>> callback, and the corresponding connector is looked through the > > > >>> encoder list in the get_eld callback in turn. > > > >> On most older Radeon parts you only got one audio codec which can be > > > >> routed to multiple connectors. > > > >> > > > >> As far as I can see this is not correctly supported with this framework > > > >> here since we only grab the first ELD. Is that correct? > > > > Hrm, how does it work currently (without the patch) in the hardware > > > > level? I mean, the unsolicited event is still triggered via HD-audio > > > > bus as well as the partial ELD pass-through via HD-audio. Isn't the > > > > reported pin ID corresponding to that connector? > > > > > > The hardware we are talking about here doesn't have ELD support at all. > > > > > > So no ELD pass-through or unsolicited event when something is connected. > > > > > > You basically have to setup the capabilities in the applications manually. > > > > Well, if ELD isn't passed, the HDMI audio doesn't work at all -- even > > for the driver as of today. So we're talking about a stuff that is > > already broken now, if any. > > > > > >> Additional to that I'm not sure if that is the right design for AMD > > > >> hardware in general since we don't really support ELD in the first place. > > > > ELD is a kind of mandatory and we've assembled the ELD from the > > > > partial information taken via HD-audio bus like speaker allocation and > > > > other bits, so far. I thought the very same information is found in > > > > connector->edid that is always parsed at EDID parsing time. > > > > > > > > Let me know if I'm obviously overlooking something. > > > > > > The hardware we are talking about existed way before the ELD standard. > > > So nothing from the ELD standard is supported here. > > > > > > I mean it would be great to get this cleaned up, but you need to take > > > care all those nasty corner cases not supported by ELD. > > > > That's not clear to me: the ELD information bits are filled in > > drm_edid_to_eld() generically for the given EDID. Do you mean that > > this isn't applied to the old radeon chip? IOW, doesn't such a chip > > read EDID at all...? > > No AMD audio hw uses ELD. The original hw design for display auto > pre-dated ELD. the display driver updates the the hw state and that > state is updated in the audio hw as well. So there is an internal hw > interface between the two drivers. The display driver updates via GPU > MMIO, and the audio driver can interact with that state via AMD > specific VERBs. See this document: > http://developer.amd.com/wordpress/media/2013/10/AMD_HDA_verbs_v2.pdf Yes, that's what I mentioned as "assembling ELD in HD-audio side". The HD-audio driver has an AMD-specific code for building up the ELD from these partial bits. But my question is whether these AMD chips do read EDID and process via the standard EDID helper (drm_add_edid_modes(), etc). If yes, the ELD is filled up there, and we can use the same information from there as done in my patch. > On older AMD chips (everything prior to SI chips IIRC), had a single > audio engine which could be routed to one or more display connectors. And does really appear as multiple HD-audio pins? Unless that happens, the picking up one connector should work. > > I know that the old radeon chip doesn't pass the ELD information via > > HD-audio bus unlike others. For such chips, we've assembled the ELD > > from the partial information, as I already mentioned. > > > > > Like one audio codec routed to multiple physical connectors. In theory > > > you even have the ability to route the left and right channel to > > > different connectors. > > > > That's a question how currently it's exposed in the hardware level at > > all to HD-audio side. Again, my patchset tries to simplify this > > communication -- between gfx and HD-audio codec chip. If the > > translation of connector / pin can't be done in the way I implemented, > > I'd like to hear how the hardware actually decides the HD-audio pin > > index from the given configuration. > > On AMD hw the ELD is technically not required. If everything has > power, the internal interface works as expected. The only case where > we have a problem is runtime pm on HG platforms where I think we end > up missing updates due to timing of powering up the relevant hw. To clarify: the ELD meant in my comments is the ELD information exposed from HD-audio driver. This is mandatory for making the audio working, so far, and currently HD-audio driver even disables if ELD isn't available. Again, for AMD, the ELD is assembled from other information sources. I hope that, with my patch, that complex path can be avoided as well by passing the already existing data (already parsed from EDID). thanks, Takashi > > Alex > > > > > > I think nouveau has an even nastier case where you have an audio codec > > > which can be attached both to a normal audio plug as well as to a > > > display device to route stuff over HDMI. > > > > The nouveau looks even more straightforward :) > > > > > You won't find any of that on modern hardware, so I'm not sure if it's > > > worth putting to much effort into it :) > > > > Heh, I'm fine to drop the radeon part, as it's becoming dead > > nowadays, if you think it being too risky. For AMDGPU, you guys seem > > working on it, and I'd just wait for the final patches. > > > > The nouveau is still in active deployment and development, so I'd like > > to have it done there, though. > > > > > > thanks, > > > > Takashi > > > > > > > > Regards, > > > Christian. > > > > > > > > > > > > > > > Thanks! > > > > > > > > Takashi > > > > > > > > > > > >> Regards, > > > >> Christian. > > > >> > > > >>> The bind and unbind callbacks handle the device-link so that it > > > >>> assures the PM call order. > > > >>> > > > >>> Acked-by: Jim Qu <Jim.Qu@xxxxxxx> > > > >>> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > > > >>> --- > > > >>> drivers/gpu/drm/Kconfig | 1 + > > > >>> drivers/gpu/drm/radeon/radeon.h | 3 ++ > > > >>> drivers/gpu/drm/radeon/radeon_audio.c | 92 +++++++++++++++++++++++++++++++++++ > > > >>> 3 files changed, 96 insertions(+) > > > >>> > > > >>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > > >>> index 1d80222587ad..8b44cc458830 100644 > > > >>> --- a/drivers/gpu/drm/Kconfig > > > >>> +++ b/drivers/gpu/drm/Kconfig > > > >>> @@ -209,6 +209,7 @@ config DRM_RADEON > > > >>> select HWMON > > > >>> select BACKLIGHT_CLASS_DEVICE > > > >>> select INTERVAL_TREE > > > >>> + select SND_HDA_COMPONENT if SND_HDA_CORE > > > >>> help > > > >>> Choose this option if you have an ATI Radeon graphics card. There > > > >>> are both PCI and AGP versions. You don't need to choose this to > > > >>> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h > > > >>> index 32808e50be12..2973284b7961 100644 > > > >>> --- a/drivers/gpu/drm/radeon/radeon.h > > > >>> +++ b/drivers/gpu/drm/radeon/radeon.h > > > >>> @@ -75,6 +75,7 @@ > > > >>> #include <drm/ttm/ttm_execbuf_util.h> > > > >>> > > > >>> #include <drm/drm_gem.h> > > > >>> +#include <drm/drm_audio_component.h> > > > >>> > > > >>> #include "radeon_family.h" > > > >>> #include "radeon_mode.h" > > > >>> @@ -1761,6 +1762,8 @@ struct r600_audio { > > > >>> struct radeon_audio_funcs *hdmi_funcs; > > > >>> struct radeon_audio_funcs *dp_funcs; > > > >>> struct radeon_audio_basic_funcs *funcs; > > > >>> + struct drm_audio_component *component; > > > >>> + bool component_registered; > > > >>> }; > > > >>> > > > >>> /* > > > >>> diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c > > > >>> index b9aea5776d3d..976502e31c43 100644 > > > >>> --- a/drivers/gpu/drm/radeon/radeon_audio.c > > > >>> +++ b/drivers/gpu/drm/radeon/radeon_audio.c > > > >>> @@ -23,6 +23,7 @@ > > > >>> */ > > > >>> > > > >>> #include <linux/gcd.h> > > > >>> +#include <linux/component.h> > > > >>> > > > >>> #include <drm/drm_crtc.h> > > > >>> #include "radeon.h" > > > >>> @@ -242,6 +243,14 @@ static struct radeon_audio_funcs dce6_dp_funcs = { > > > >>> .dpms = evergreen_dp_enable, > > > >>> }; > > > >>> > > > >>> +static void radeon_audio_component_notify(struct drm_audio_component *acomp, > > > >>> + int port) > > > >>> +{ > > > >>> + if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) > > > >>> + acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, > > > >>> + port, -1); > > > >>> +} > > > >>> + > > > >>> static void radeon_audio_enable(struct radeon_device *rdev, > > > >>> struct r600_audio_pin *pin, u8 enable_mask) > > > >>> { > > > >>> @@ -269,6 +278,8 @@ static void radeon_audio_enable(struct radeon_device *rdev, > > > >>> > > > >>> if (rdev->audio.funcs->enable) > > > >>> rdev->audio.funcs->enable(rdev, pin, enable_mask); > > > >>> + > > > >>> + radeon_audio_component_notify(rdev->audio.component, pin->id); > > > >>> } > > > >>> > > > >>> static void radeon_audio_interface_init(struct radeon_device *rdev) > > > >>> @@ -292,6 +303,79 @@ static void radeon_audio_interface_init(struct radeon_device *rdev) > > > >>> } > > > >>> } > > > >>> > > > >>> +static int radeon_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 radeon_encoder *radeon_encoder; > > > >>> + struct radeon_encoder_atom_dig *dig; > > > >>> + struct drm_connector *connector; > > > >>> + int ret = 0; > > > >>> + > > > >>> + *enabled = false; > > > >>> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { > > > >>> + if (!radeon_encoder_is_digital(encoder)) > > > >>> + continue; > > > >>> + radeon_encoder = to_radeon_encoder(encoder); > > > >>> + dig = radeon_encoder->enc_priv; > > > >>> + if (!dig->pin || dig->pin->id != port) > > > >>> + continue; > > > >>> + connector = radeon_get_connector_for_encoder(encoder); > > > >>> + if (connector) { > > > >>> + *enabled = true; > > > >>> + ret = drm_eld_size(connector->eld); > > > >>> + memcpy(buf, connector->eld, min(max_bytes, ret)); > > > >>> + break; > > > >>> + } > > > >>> + } > > > >>> + > > > >>> + return ret; > > > >>> +} > > > >>> + > > > >>> +static const struct drm_audio_component_ops radeon_audio_component_ops = { > > > >>> + .get_eld = radeon_audio_component_get_eld, > > > >>> +}; > > > >>> + > > > >>> +static int radeon_audio_component_bind(struct device *kdev, > > > >>> + struct device *hda_kdev, void *data) > > > >>> +{ > > > >>> + struct drm_device *dev = dev_get_drvdata(kdev); > > > >>> + struct radeon_device *rdev = dev->dev_private; > > > >>> + struct drm_audio_component *acomp = data; > > > >>> + > > > >>> + if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS))) > > > >>> + return -ENOMEM; > > > >>> + > > > >>> + drm_modeset_lock_all(dev); > > > >>> + acomp->ops = &radeon_audio_component_ops; > > > >>> + acomp->dev = kdev; > > > >>> + rdev->audio.component = acomp; > > > >>> + drm_modeset_unlock_all(dev); > > > >>> + > > > >>> + return 0; > > > >>> +} > > > >>> + > > > >>> +static void radeon_audio_component_unbind(struct device *kdev, > > > >>> + struct device *hda_kdev, void *data) > > > >>> +{ > > > >>> + struct drm_device *dev = dev_get_drvdata(kdev); > > > >>> + struct radeon_device *rdev = dev->dev_private; > > > >>> + struct drm_audio_component *acomp = data; > > > >>> + > > > >>> + drm_modeset_lock_all(dev); > > > >>> + rdev->audio.component = NULL; > > > >>> + acomp->ops = NULL; > > > >>> + acomp->dev = NULL; > > > >>> + drm_modeset_unlock_all(dev); > > > >>> +} > > > >>> + > > > >>> +static const struct component_ops radeon_audio_component_bind_ops = { > > > >>> + .bind = radeon_audio_component_bind, > > > >>> + .unbind = radeon_audio_component_unbind, > > > >>> +}; > > > >>> + > > > >>> static int radeon_audio_chipset_supported(struct radeon_device *rdev) > > > >>> { > > > >>> return ASIC_IS_DCE2(rdev) && !ASIC_IS_NODCE(rdev); > > > >>> @@ -338,6 +422,9 @@ int radeon_audio_init(struct radeon_device *rdev) > > > >>> for (i = 0; i < rdev->audio.num_pins; i++) > > > >>> radeon_audio_enable(rdev, &rdev->audio.pin[i], 0); > > > >>> > > > >>> + if (!component_add(rdev->dev, &radeon_audio_component_bind_ops)) > > > >>> + rdev->audio.component_registered = true; > > > >>> + > > > >>> return 0; > > > >>> } > > > >>> > > > >>> @@ -490,6 +577,11 @@ void radeon_audio_fini(struct radeon_device *rdev) > > > >>> radeon_audio_enable(rdev, &rdev->audio.pin[i], 0); > > > >>> > > > >>> rdev->audio.enabled = false; > > > >>> + > > > >>> + if (rdev->audio.component_registered) { > > > >>> + component_del(rdev->dev, &radeon_audio_component_bind_ops); > > > >>> + rdev->audio.component_registered = false; > > > >>> + } > > > >>> } > > > >>> > > > >>> static void radeon_audio_set_dto(struct drm_encoder *encoder, unsigned int clock) > > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
