[PATCH 0/2] Audio component support for radeon and nouveau

[PATCH 0/2] Audio component support for radeon and nouveau

[Takashi Iwai]

Hi,

this is a patchset to add the support for HDMI/DP audio notification
via drm_audio_component framework on radeon and nouveau drivers.
The notification mechanism works for long time on i915, and now AMDGPU
patch is floating around.  This fills the rest gap, the support for
remaining radeon and nouveau drivers.

The implementation is rather simple, and it just adds the hook for
notification and ELD retrieval.  The patch for radeon is basically a
respin of my previous RFC patch.

The counterpart change in HD-audio side has been already merged in
sound.git tree for 5.4 kernel.  The persistent branch,
topic/hda-acomp-base, is provided there, so that you can pull into
yours freely:
  git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git topic/hda-acomp-base

This patchset itself is almost harmless without the HD-audio side
changes (IOW, it doesn't help anything else, either :)


thanks,

Takashi

===

Takashi Iwai (2):
  drm/radeon: Add HD-audio component notifier support
  drm/nouveau: Add HD-audio component notifier support

 drivers/gpu/drm/Kconfig                 |   1 +
 drivers/gpu/drm/nouveau/Kconfig         |   1 +
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 111 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_drv.h   |   7 ++
 drivers/gpu/drm/radeon/radeon.h         |   3 +
 drivers/gpu/drm/radeon/radeon_audio.c   |  92 ++++++++++++++++++++++++++
 6 files changed, 215 insertions(+)

-- 
2.16.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[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.

The bind and unbind callbacks handle the device-link so that it
assures the PM call order.

Acked-by: Jim Qu <Jim.Qu@amd.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 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)
-- 
2.16.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/nouveau: Add HD-audio component notifier support

[Takashi Iwai]

This patch adds the support for the notification of HD-audio hotplug
via the already existing drm_audio_component framework.  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: nouveau driver provides the
get_eld ops for HD-audio, and it notifies the audio hotplug via
pin_eld_notify callback upon each nv50_audio_enable() and _disable()
call.  As the HD-audio pin assignment seems corresponding to the CRTC,
the crtc->index number is passed directly as the zero-based port
number.

The bind and unbind callbacks handle the device-link so that it
assures the PM call order.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/nouveau/Kconfig         |   1 +
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 111 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_drv.h   |   7 ++
 3 files changed, 119 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 96b9814e6d06..33ccf11bd70d 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -16,6 +16,7 @@ config DRM_NOUVEAU
 	select INPUT if ACPI && X86
 	select THERMAL if ACPI && X86
 	select ACPI_VIDEO if ACPI && X86
+	select SND_HDA_COMPONENT if SND_HDA_CORE
 	help
 	  Choose this option for open-source NVIDIA support.
 
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 8497768f1b41..919f3d3db161 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -29,6 +29,7 @@
 
 #include <linux/dma-mapping.h>
 #include <linux/hdmi.h>
+#include <linux/component.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
@@ -466,12 +467,113 @@ nv50_dac_create(struct drm_connector *connector, struct dcb_output *dcbe)
 	return 0;
 }
 
+/*
+ * audio component binding for ELD notification
+ */
+static void
+nv50_audio_component_eld_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 int
+nv50_audio_component_get_eld(struct device *kdev, int port, int pipe,
+			     bool *enabled, unsigned char *buf, int max_bytes)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(kdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+	struct drm_encoder *encoder;
+	struct nouveau_encoder *nv_encoder;
+	struct nouveau_connector *nv_connector;
+	struct nouveau_crtc *nv_crtc;
+	int ret = 0;
+
+	*enabled = false;
+	drm_for_each_encoder(encoder, drm->dev) {
+		nv_encoder = nouveau_encoder(encoder);
+		nv_connector = nouveau_encoder_connector_get(nv_encoder);
+		nv_crtc = nouveau_crtc(encoder->crtc);
+		if (!nv_connector || !nv_crtc || nv_crtc->index != port)
+			continue;
+		*enabled = drm_detect_monitor_audio(nv_connector->edid);
+		if (*enabled) {
+			ret = drm_eld_size(nv_connector->base.eld);
+			memcpy(buf, nv_connector->base.eld,
+			       min(max_bytes, ret));
+		}
+		break;
+	}
+	return ret;
+}
+
+static const struct drm_audio_component_ops nv50_audio_component_ops = {
+	.get_eld = nv50_audio_component_get_eld,
+};
+
+static int
+nv50_audio_component_bind(struct device *kdev, struct device *hda_kdev,
+			  void *data)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(kdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+	struct drm_audio_component *acomp = data;
+
+	if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
+		return -ENOMEM;
+
+	drm_modeset_lock_all(drm_dev);
+	acomp->ops = &nv50_audio_component_ops;
+	acomp->dev = kdev;
+	drm->audio.component = acomp;
+	drm_modeset_unlock_all(drm_dev);
+	return 0;
+}
+
+static void
+nv50_audio_component_unbind(struct device *kdev, struct device *hda_kdev,
+			    void *data)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(kdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+	struct drm_audio_component *acomp = data;
+
+	drm_modeset_lock_all(drm_dev);
+	drm->audio.component = NULL;
+	acomp->ops = NULL;
+	acomp->dev = NULL;
+	drm_modeset_unlock_all(drm_dev);
+}
+
+static const struct component_ops nv50_audio_component_bind_ops = {
+	.bind   = nv50_audio_component_bind,
+	.unbind = nv50_audio_component_unbind,
+};
+
+static void
+nv50_audio_component_init(struct nouveau_drm *drm)
+{
+	if (!component_add(drm->dev->dev, &nv50_audio_component_bind_ops))
+		drm->audio.component_registered = true;
+}
+
+static void
+nv50_audio_component_fini(struct nouveau_drm *drm)
+{
+	if (drm->audio.component_registered) {
+		component_del(drm->dev->dev, &nv50_audio_component_bind_ops);
+		drm->audio.component_registered = false;
+	}
+}
+
 /******************************************************************************
  * Audio
  *****************************************************************************/
 static void
 nv50_audio_disable(struct drm_encoder *encoder, struct nouveau_crtc *nv_crtc)
 {
+	struct nouveau_drm *drm = nouveau_drm(encoder->dev);
 	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
 	struct nv50_disp *disp = nv50_disp(encoder->dev);
 	struct {
@@ -486,11 +588,14 @@ nv50_audio_disable(struct drm_encoder *encoder, struct nouveau_crtc *nv_crtc)
 	};
 
 	nvif_mthd(&disp->disp->object, 0, &args, sizeof(args));
+
+	nv50_audio_component_eld_notify(drm->audio.component, nv_crtc->index);
 }
 
 static void
 nv50_audio_enable(struct drm_encoder *encoder, struct drm_display_mode *mode)
 {
+	struct nouveau_drm *drm = nouveau_drm(encoder->dev);
 	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
 	struct nouveau_crtc *nv_crtc = nouveau_crtc(encoder->crtc);
 	struct nouveau_connector *nv_connector;
@@ -517,6 +622,8 @@ nv50_audio_enable(struct drm_encoder *encoder, struct drm_display_mode *mode)
 
 	nvif_mthd(&disp->disp->object, 0, &args,
 		  sizeof(args.base) + drm_eld_size(args.data));
+
+	nv50_audio_component_eld_notify(drm->audio.component, nv_crtc->index);
 }
 
 /******************************************************************************
@@ -2281,6 +2388,8 @@ nv50_display_destroy(struct drm_device *dev)
 {
 	struct nv50_disp *disp = nv50_disp(dev);
 
+	nv50_audio_component_fini(nouveau_drm(dev));
+
 	nv50_core_del(&disp->core);
 
 	nouveau_bo_unmap(disp->sync);
@@ -2401,6 +2510,8 @@ nv50_display_create(struct drm_device *dev)
 	/* Disable vblank irqs aggressively for power-saving, safe on nv50+ */
 	dev->vblank_disable_immediate = true;
 
+	nv50_audio_component_init(drm);
+
 out:
 	if (ret)
 		nv50_display_destroy(dev);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index aae035816383..15e4f2aa19bf 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -55,6 +55,8 @@
 #include <drm/ttm/ttm_module.h>
 #include <drm/ttm/ttm_page_alloc.h>
 
+#include <drm/drm_audio_component.h>
+
 #include "uapi/drm/nouveau_drm.h"
 
 struct nouveau_channel;
@@ -212,6 +214,11 @@ struct nouveau_drm {
 	struct nouveau_svm *svm;
 
 	struct nouveau_dmem *dmem;
+
+	struct {
+		struct drm_audio_component *component;
+		bool component_registered;
+	} audio;
 };
 
 static inline struct nouveau_drm *
-- 
2.16.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[Koenig, Christian]

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?

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.

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@amd.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[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?

> 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.


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@amd.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/nouveau: Add HD-audio component notifier support

[Ilia Mirkin]

On Mon, Jul 22, 2019 at 10:38 AM Takashi Iwai <tiwai@suse.de> wrote:
> +static void
> +nv50_audio_component_init(struct nouveau_drm *drm)
> +{
> +       if (!component_add(drm->dev->dev, &nv50_audio_component_bind_ops))
> +               drm->audio.component_registered = true;
> +}

Pardon my ignorance on this topic ... but are there any ill effects
from always doing this? For example, the board in question might not
have ELD support at all (the original G80, covered by dispnv50), or it
might not have any HDMI/DP ports, or it may have them but just nothing
is plugged in.

Could you confirm that this is all OK?

Thanks,

  -ilia
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[Koenig, Christian]

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.

>> 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.

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.

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.

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 :)

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@amd.com>
>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>> ---
>>>    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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/nouveau: Add HD-audio component notifier support

[Takashi Iwai]

On Mon, 22 Jul 2019 17:00:15 +0200,
Ilia Mirkin wrote:
> 
> On Mon, Jul 22, 2019 at 10:38 AM Takashi Iwai <tiwai@suse.de> wrote:
> > +static void
> > +nv50_audio_component_init(struct nouveau_drm *drm)
> > +{
> > +       if (!component_add(drm->dev->dev, &nv50_audio_component_bind_ops))
> > +               drm->audio.component_registered = true;
> > +}
> 
> Pardon my ignorance on this topic ... but are there any ill effects
> from always doing this? For example, the board in question might not
> have ELD support at all (the original G80, covered by dispnv50), or it
> might not have any HDMI/DP ports, or it may have them but just nothing
> is plugged in.

In general, this change shouldn't do anything worse than what we have
currently.

If the board has no ELD support, the HDMI audio won't work at all, and
HD-audio side would know that it's an invalid ELD.

If the nvidia chips has no audio support and it doesn't have the
corresponding HD-audio audio driver exposed as a PCI device, this
component registration is also harmless, just a placeholder until it's
bound.

> Could you confirm that this is all OK?

At least I tested other way round -- the nvidia component implemented
but without HD-audio master component.


Thanks!

Takashi

> 
> Thanks,
> 
>   -ilia
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[Takashi Iwai]

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...?

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.

> 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@amd.com>
> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>> ---
> >>>    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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[Alex Deucher]

On Mon, Jul 22, 2019 at 11:21 AM Takashi Iwai <tiwai@suse.de> 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

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.

>
> 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.

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@amd.com>
> > >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > >>> ---
> > >>>    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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[Koenig, Christian]

Am 22.07.19 um 17:21 schrieb Takashi Iwai:
> 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:
>>>> [SNIP]
>> 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.

At least the last time I looked that worked perfectly fine.

ELD is only optional since it was introduced way later than the first 
HDMI audio capable devices.

If ELD became mandatory at some point then that could have potentially 
broken the hardware support and that is a really no-go.

>>>> 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...?

EDID is read to get the video information for the given connector, but 
as far as I know radeon never called drm_edid_to_eld() and doesn't 
provide the ELD information in any way to the audio codec.

> 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.

What partial information do you mean here?

It's about a decade ago that I wrote that code, but IIRC radeon never 
passed anything to the audio codec.

>> 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.

Well that's why I tried to explain. The hardware doesn't care about the 
HD-audio pin in any way as far as I know.

>> 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.

Thanks, yeah without intensive testing I wouldn't want to commit this.

And the problem is really that there aren't that many AGP boards to test 
old hardware around :)

> For AMDGPU, you guys seem working on it, and I'd just wait for the final patches.

Well the problem is ELD is actually something more or less Intel 
specific. I'm not sure if we ever started to fully support that even in 
today's codec.

> The nouveau is still in active deployment and development, so I'd like
> to have it done there, though.

Yeah, and naturally I really don't care about nouveau :)

Regards,
Christian.

>
>
> 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@amd.com>
>>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>>>> ---
>>>>>     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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[Takashi Iwai]

On Mon, 22 Jul 2019 17:35:09 +0200,
Alex Deucher wrote:
> 
> On Mon, Jul 22, 2019 at 11:21 AM Takashi Iwai <tiwai@suse.de> 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@amd.com>
> > > >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > >>> ---
> > > >>>    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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[Takashi Iwai]

On Mon, 22 Jul 2019 17:38:36 +0200,
Koenig, Christian wrote:
> 
> Am 22.07.19 um 17:21 schrieb Takashi Iwai:
> > 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:
> >>>> [SNIP]
> >> 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.
> 
> At least the last time I looked that worked perfectly fine.
> 
> ELD is only optional since it was introduced way later than the first 
> HDMI audio capable devices.
> 
> If ELD became mandatory at some point then that could have potentially 
> broken the hardware support and that is a really no-go.

Hrm, are you sure?  The ELD bits are assembled in HD-audio side for
AMD chips, and ELD has bee always a mandatory requirement.  I don't
mean about the direct ELD passing but the ELD information exposed from
the audio driver.

> >>>> 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...?
> 
> EDID is read to get the video information for the given connector, but 
> as far as I know radeon never called drm_edid_to_eld() and doesn't 
> provide the ELD information in any way to the audio codec.
> 
> > 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.
> 
> What partial information do you mean here?

The speaker allocation, audio descriptor, audio video delay, sink info
index, etc, that are obtained via the vendor-specific HD-audio codec
verbs.

> It's about a decade ago that I wrote that code, but IIRC radeon never 
> passed anything to the audio codec.

As far as I read correctly, the whole radeon_audio_write_xxx() calls
update the corresponding registers, and these are taken from
connector's EDID bits.  HD-audio driver reads these values via the
shadowed HD-audio verbs over HD-audio bus in turn, and builds up the
ELD by itself.  With the eld_notification callback, we can skip this
decompose / compose step.


> >> 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.
> 
> Well that's why I tried to explain. The hardware doesn't care about the 
> HD-audio pin in any way as far as I know.

So essentially it implies that such chips do have only a single
HD-audio pin.  In that case, it's easy to treat in HD-audio side --
just ignore the pin index.

> >> 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.
> 
> Thanks, yeah without intensive testing I wouldn't want to commit this.
> 
> And the problem is really that there aren't that many AGP boards to test 
> old hardware around :)

OK.

> > For AMDGPU, you guys seem working on it, and I'd just wait for the final patches.
> 
> Well the problem is ELD is actually something more or less Intel 
> specific. I'm not sure if we ever started to fully support that even in 
> today's codec.

The DRM core parses and stores ELD at parsing EDID in general, so we
have it there already as gratis.  The only remaining question is how
to pass it, and the audio component notification makes it much easier
than the compose/decompose via hda verbs.

> > The nouveau is still in active deployment and development, so I'd like
> > to have it done there, though.
> 
> Yeah, and naturally I really don't care about nouveau :)

Lucky you! :)


thanks,

Takashi


> 
> Regards,
> Christian.
> 
> >
> >
> > 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@amd.com>
> >>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>>>> ---
> >>>>>     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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[Alex Deucher]

On Mon, Jul 22, 2019 at 11:47 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Mon, 22 Jul 2019 17:35:09 +0200,
> Alex Deucher wrote:
> >
> > On Mon, Jul 22, 2019 at 11:21 AM Takashi Iwai <tiwai@suse.de> 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.
>

Yes, we do use the standard helpers for fetching EDID and parsing the
audio related extensions.  IIRC, we used to call the drm ELD helpers
directly to populate everything even before they moved into the core
drm EDID helpers.

> > 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 don't remember exactly how it's exposed on the audio side.  I think
it's a single pin.

>
>
> > > 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).
>

RIght, I agree.  I think it makes it more robust by handling the
corner cases that don't work with the hw only design.

Alex

>
> 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@amd.com>
> > > > >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > >>> ---
> > > > >>>    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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/nouveau: Add HD-audio component notifier support

[Takashi Iwai]

On Mon, 22 Jul 2019 16:38:15 +0200,
Takashi Iwai wrote:
> 
> This patch adds the support for the notification of HD-audio hotplug
> via the already existing drm_audio_component framework.  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: nouveau driver provides the
> get_eld ops for HD-audio, and it notifies the audio hotplug via
> pin_eld_notify callback upon each nv50_audio_enable() and _disable()
> call.  As the HD-audio pin assignment seems corresponding to the CRTC,
> the crtc->index number is passed directly as the zero-based port
> number.
> 
> The bind and unbind callbacks handle the device-link so that it
> assures the PM call order.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Any review / reaction to this patch?

I don't mind dropping the patch for radeon, but nouveau has more
potential benefit by this.


thanks,

Takashi

> ---
>  drivers/gpu/drm/nouveau/Kconfig         |   1 +
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 111 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/nouveau/nouveau_drv.h   |   7 ++
>  3 files changed, 119 insertions(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> index 96b9814e6d06..33ccf11bd70d 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -16,6 +16,7 @@ config DRM_NOUVEAU
>  	select INPUT if ACPI && X86
>  	select THERMAL if ACPI && X86
>  	select ACPI_VIDEO if ACPI && X86
> +	select SND_HDA_COMPONENT if SND_HDA_CORE
>  	help
>  	  Choose this option for open-source NVIDIA support.
>  
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 8497768f1b41..919f3d3db161 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -29,6 +29,7 @@
>  
>  #include <linux/dma-mapping.h>
>  #include <linux/hdmi.h>
> +#include <linux/component.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -466,12 +467,113 @@ nv50_dac_create(struct drm_connector *connector, struct dcb_output *dcbe)
>  	return 0;
>  }
>  
> +/*
> + * audio component binding for ELD notification
> + */
> +static void
> +nv50_audio_component_eld_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 int
> +nv50_audio_component_get_eld(struct device *kdev, int port, int pipe,
> +			     bool *enabled, unsigned char *buf, int max_bytes)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(kdev);
> +	struct nouveau_drm *drm = nouveau_drm(drm_dev);
> +	struct drm_encoder *encoder;
> +	struct nouveau_encoder *nv_encoder;
> +	struct nouveau_connector *nv_connector;
> +	struct nouveau_crtc *nv_crtc;
> +	int ret = 0;
> +
> +	*enabled = false;
> +	drm_for_each_encoder(encoder, drm->dev) {
> +		nv_encoder = nouveau_encoder(encoder);
> +		nv_connector = nouveau_encoder_connector_get(nv_encoder);
> +		nv_crtc = nouveau_crtc(encoder->crtc);
> +		if (!nv_connector || !nv_crtc || nv_crtc->index != port)
> +			continue;
> +		*enabled = drm_detect_monitor_audio(nv_connector->edid);
> +		if (*enabled) {
> +			ret = drm_eld_size(nv_connector->base.eld);
> +			memcpy(buf, nv_connector->base.eld,
> +			       min(max_bytes, ret));
> +		}
> +		break;
> +	}
> +	return ret;
> +}
> +
> +static const struct drm_audio_component_ops nv50_audio_component_ops = {
> +	.get_eld = nv50_audio_component_get_eld,
> +};
> +
> +static int
> +nv50_audio_component_bind(struct device *kdev, struct device *hda_kdev,
> +			  void *data)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(kdev);
> +	struct nouveau_drm *drm = nouveau_drm(drm_dev);
> +	struct drm_audio_component *acomp = data;
> +
> +	if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
> +		return -ENOMEM;
> +
> +	drm_modeset_lock_all(drm_dev);
> +	acomp->ops = &nv50_audio_component_ops;
> +	acomp->dev = kdev;
> +	drm->audio.component = acomp;
> +	drm_modeset_unlock_all(drm_dev);
> +	return 0;
> +}
> +
> +static void
> +nv50_audio_component_unbind(struct device *kdev, struct device *hda_kdev,
> +			    void *data)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(kdev);
> +	struct nouveau_drm *drm = nouveau_drm(drm_dev);
> +	struct drm_audio_component *acomp = data;
> +
> +	drm_modeset_lock_all(drm_dev);
> +	drm->audio.component = NULL;
> +	acomp->ops = NULL;
> +	acomp->dev = NULL;
> +	drm_modeset_unlock_all(drm_dev);
> +}
> +
> +static const struct component_ops nv50_audio_component_bind_ops = {
> +	.bind   = nv50_audio_component_bind,
> +	.unbind = nv50_audio_component_unbind,
> +};
> +
> +static void
> +nv50_audio_component_init(struct nouveau_drm *drm)
> +{
> +	if (!component_add(drm->dev->dev, &nv50_audio_component_bind_ops))
> +		drm->audio.component_registered = true;
> +}
> +
> +static void
> +nv50_audio_component_fini(struct nouveau_drm *drm)
> +{
> +	if (drm->audio.component_registered) {
> +		component_del(drm->dev->dev, &nv50_audio_component_bind_ops);
> +		drm->audio.component_registered = false;
> +	}
> +}
> +
>  /******************************************************************************
>   * Audio
>   *****************************************************************************/
>  static void
>  nv50_audio_disable(struct drm_encoder *encoder, struct nouveau_crtc *nv_crtc)
>  {
> +	struct nouveau_drm *drm = nouveau_drm(encoder->dev);
>  	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
>  	struct nv50_disp *disp = nv50_disp(encoder->dev);
>  	struct {
> @@ -486,11 +588,14 @@ nv50_audio_disable(struct drm_encoder *encoder, struct nouveau_crtc *nv_crtc)
>  	};
>  
>  	nvif_mthd(&disp->disp->object, 0, &args, sizeof(args));
> +
> +	nv50_audio_component_eld_notify(drm->audio.component, nv_crtc->index);
>  }
>  
>  static void
>  nv50_audio_enable(struct drm_encoder *encoder, struct drm_display_mode *mode)
>  {
> +	struct nouveau_drm *drm = nouveau_drm(encoder->dev);
>  	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
>  	struct nouveau_crtc *nv_crtc = nouveau_crtc(encoder->crtc);
>  	struct nouveau_connector *nv_connector;
> @@ -517,6 +622,8 @@ nv50_audio_enable(struct drm_encoder *encoder, struct drm_display_mode *mode)
>  
>  	nvif_mthd(&disp->disp->object, 0, &args,
>  		  sizeof(args.base) + drm_eld_size(args.data));
> +
> +	nv50_audio_component_eld_notify(drm->audio.component, nv_crtc->index);
>  }
>  
>  /******************************************************************************
> @@ -2281,6 +2388,8 @@ nv50_display_destroy(struct drm_device *dev)
>  {
>  	struct nv50_disp *disp = nv50_disp(dev);
>  
> +	nv50_audio_component_fini(nouveau_drm(dev));
> +
>  	nv50_core_del(&disp->core);
>  
>  	nouveau_bo_unmap(disp->sync);
> @@ -2401,6 +2510,8 @@ nv50_display_create(struct drm_device *dev)
>  	/* Disable vblank irqs aggressively for power-saving, safe on nv50+ */
>  	dev->vblank_disable_immediate = true;
>  
> +	nv50_audio_component_init(drm);
> +
>  out:
>  	if (ret)
>  		nv50_display_destroy(dev);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index aae035816383..15e4f2aa19bf 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -55,6 +55,8 @@
>  #include <drm/ttm/ttm_module.h>
>  #include <drm/ttm/ttm_page_alloc.h>
>  
> +#include <drm/drm_audio_component.h>
> +
>  #include "uapi/drm/nouveau_drm.h"
>  
>  struct nouveau_channel;
> @@ -212,6 +214,11 @@ struct nouveau_drm {
>  	struct nouveau_svm *svm;
>  
>  	struct nouveau_dmem *dmem;
> +
> +	struct {
> +		struct drm_audio_component *component;
> +		bool component_registered;
> +	} audio;
>  };
>  
>  static inline struct nouveau_drm *
> -- 
> 2.16.4
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support

[Alex Deucher]

On Mon, Jul 22, 2019 at 10:38 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> 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.
>
> The bind and unbind callbacks handle the device-link so that it
> assures the PM call order.
>
> Acked-by: Jim Qu <Jim.Qu@amd.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  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)
> --
> 2.16.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/nouveau: Add HD-audio component notifier support

[Lyude Paul]

Got shown this patch at work and realized it still needed review, so I went
ahead and did that :)

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Mon, 2019-07-22 at 16:38 +0200, Takashi Iwai wrote:
> This patch adds the support for the notification of HD-audio hotplug
> via the already existing drm_audio_component framework.  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: nouveau driver provides the
> get_eld ops for HD-audio, and it notifies the audio hotplug via
> pin_eld_notify callback upon each nv50_audio_enable() and _disable()
> call.  As the HD-audio pin assignment seems corresponding to the CRTC,
> the crtc->index number is passed directly as the zero-based port
> number.
> 
> The bind and unbind callbacks handle the device-link so that it
> assures the PM call order.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/gpu/drm/nouveau/Kconfig         |   1 +
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 111
> ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/nouveau/nouveau_drv.h   |   7 ++
>  3 files changed, 119 insertions(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/Kconfig
> b/drivers/gpu/drm/nouveau/Kconfig
> index 96b9814e6d06..33ccf11bd70d 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -16,6 +16,7 @@ config DRM_NOUVEAU
>  	select INPUT if ACPI && X86
>  	select THERMAL if ACPI && X86
>  	select ACPI_VIDEO if ACPI && X86
> +	select SND_HDA_COMPONENT if SND_HDA_CORE
>  	help
>  	  Choose this option for open-source NVIDIA support.
>  
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 8497768f1b41..919f3d3db161 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -29,6 +29,7 @@
>  
>  #include <linux/dma-mapping.h>
>  #include <linux/hdmi.h>
> +#include <linux/component.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -466,12 +467,113 @@ nv50_dac_create(struct drm_connector *connector,
> struct dcb_output *dcbe)
>  	return 0;
>  }
>  
> +/*
> + * audio component binding for ELD notification
> + */
> +static void
> +nv50_audio_component_eld_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 int
> +nv50_audio_component_get_eld(struct device *kdev, int port, int pipe,
> +			     bool *enabled, unsigned char *buf, int max_bytes)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(kdev);
> +	struct nouveau_drm *drm = nouveau_drm(drm_dev);
> +	struct drm_encoder *encoder;
> +	struct nouveau_encoder *nv_encoder;
> +	struct nouveau_connector *nv_connector;
> +	struct nouveau_crtc *nv_crtc;
> +	int ret = 0;
> +
> +	*enabled = false;
> +	drm_for_each_encoder(encoder, drm->dev) {
> +		nv_encoder = nouveau_encoder(encoder);
> +		nv_connector = nouveau_encoder_connector_get(nv_encoder);
> +		nv_crtc = nouveau_crtc(encoder->crtc);
> +		if (!nv_connector || !nv_crtc || nv_crtc->index != port)
> +			continue;
> +		*enabled = drm_detect_monitor_audio(nv_connector->edid);
> +		if (*enabled) {
> +			ret = drm_eld_size(nv_connector->base.eld);
> +			memcpy(buf, nv_connector->base.eld,
> +			       min(max_bytes, ret));
> +		}
> +		break;
> +	}
> +	return ret;
> +}
> +
> +static const struct drm_audio_component_ops nv50_audio_component_ops = {
> +	.get_eld = nv50_audio_component_get_eld,
> +};
> +
> +static int
> +nv50_audio_component_bind(struct device *kdev, struct device *hda_kdev,
> +			  void *data)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(kdev);
> +	struct nouveau_drm *drm = nouveau_drm(drm_dev);
> +	struct drm_audio_component *acomp = data;
> +
> +	if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
> +		return -ENOMEM;
> +
> +	drm_modeset_lock_all(drm_dev);
> +	acomp->ops = &nv50_audio_component_ops;
> +	acomp->dev = kdev;
> +	drm->audio.component = acomp;
> +	drm_modeset_unlock_all(drm_dev);
> +	return 0;
> +}
> +
> +static void
> +nv50_audio_component_unbind(struct device *kdev, struct device *hda_kdev,
> +			    void *data)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(kdev);
> +	struct nouveau_drm *drm = nouveau_drm(drm_dev);
> +	struct drm_audio_component *acomp = data;
> +
> +	drm_modeset_lock_all(drm_dev);
> +	drm->audio.component = NULL;
> +	acomp->ops = NULL;
> +	acomp->dev = NULL;
> +	drm_modeset_unlock_all(drm_dev);
> +}
> +
> +static const struct component_ops nv50_audio_component_bind_ops = {
> +	.bind   = nv50_audio_component_bind,
> +	.unbind = nv50_audio_component_unbind,
> +};
> +
> +static void
> +nv50_audio_component_init(struct nouveau_drm *drm)
> +{
> +	if (!component_add(drm->dev->dev, &nv50_audio_component_bind_ops))
> +		drm->audio.component_registered = true;
> +}
> +
> +static void
> +nv50_audio_component_fini(struct nouveau_drm *drm)
> +{
> +	if (drm->audio.component_registered) {
> +		component_del(drm->dev->dev, &nv50_audio_component_bind_ops);
> +		drm->audio.component_registered = false;
> +	}
> +}
> +
>  /**************************************************************************
> ****
>   * Audio
>  
> ****************************************************************************
> */
>  static void
>  nv50_audio_disable(struct drm_encoder *encoder, struct nouveau_crtc
> *nv_crtc)
>  {
> +	struct nouveau_drm *drm = nouveau_drm(encoder->dev);
>  	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
>  	struct nv50_disp *disp = nv50_disp(encoder->dev);
>  	struct {
> @@ -486,11 +588,14 @@ nv50_audio_disable(struct drm_encoder *encoder, struct
> nouveau_crtc *nv_crtc)
>  	};
>  
>  	nvif_mthd(&disp->disp->object, 0, &args, sizeof(args));
> +
> +	nv50_audio_component_eld_notify(drm->audio.component, nv_crtc->index);
>  }
>  
>  static void
>  nv50_audio_enable(struct drm_encoder *encoder, struct drm_display_mode
> *mode)
>  {
> +	struct nouveau_drm *drm = nouveau_drm(encoder->dev);
>  	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
>  	struct nouveau_crtc *nv_crtc = nouveau_crtc(encoder->crtc);
>  	struct nouveau_connector *nv_connector;
> @@ -517,6 +622,8 @@ nv50_audio_enable(struct drm_encoder *encoder, struct
> drm_display_mode *mode)
>  
>  	nvif_mthd(&disp->disp->object, 0, &args,
>  		  sizeof(args.base) + drm_eld_size(args.data));
> +
> +	nv50_audio_component_eld_notify(drm->audio.component, nv_crtc->index);
>  }
>  
>  /**************************************************************************
> ****
> @@ -2281,6 +2388,8 @@ nv50_display_destroy(struct drm_device *dev)
>  {
>  	struct nv50_disp *disp = nv50_disp(dev);
>  
> +	nv50_audio_component_fini(nouveau_drm(dev));
> +
>  	nv50_core_del(&disp->core);
>  
>  	nouveau_bo_unmap(disp->sync);
> @@ -2401,6 +2510,8 @@ nv50_display_create(struct drm_device *dev)
>  	/* Disable vblank irqs aggressively for power-saving, safe on nv50+ */
>  	dev->vblank_disable_immediate = true;
>  
> +	nv50_audio_component_init(drm);
> +
>  out:
>  	if (ret)
>  		nv50_display_destroy(dev);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index aae035816383..15e4f2aa19bf 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -55,6 +55,8 @@
>  #include <drm/ttm/ttm_module.h>
>  #include <drm/ttm/ttm_page_alloc.h>
>  
> +#include <drm/drm_audio_component.h>
> +
>  #include "uapi/drm/nouveau_drm.h"
>  
>  struct nouveau_channel;
> @@ -212,6 +214,11 @@ struct nouveau_drm {
>  	struct nouveau_svm *svm;
>  
>  	struct nouveau_dmem *dmem;
> +
> +	struct {
> +		struct drm_audio_component *component;
> +		bool component_registered;
> +	} audio;
>  };
>  
>  static inline struct nouveau_drm *
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/nouveau: Add HD-audio component notifier support

[Takashi Iwai]

On Sat, 04 Jan 2020 00:01:14 +0100,
Lyude Paul wrote:
> 
> Got shown this patch at work and realized it still needed review, so I went
> ahead and did that :)
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>

Thanks!

Let me know if the submission of the patch is needed.


Takashi

> 
> On Mon, 2019-07-22 at 16:38 +0200, Takashi Iwai wrote:
> > This patch adds the support for the notification of HD-audio hotplug
> > via the already existing drm_audio_component framework.  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: nouveau driver provides the
> > get_eld ops for HD-audio, and it notifies the audio hotplug via
> > pin_eld_notify callback upon each nv50_audio_enable() and _disable()
> > call.  As the HD-audio pin assignment seems corresponding to the CRTC,
> > the crtc->index number is passed directly as the zero-based port
> > number.
> > 
> > The bind and unbind callbacks handle the device-link so that it
> > assures the PM call order.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/gpu/drm/nouveau/Kconfig         |   1 +
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c | 111
> > ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/nouveau/nouveau_drv.h   |   7 ++
> >  3 files changed, 119 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/Kconfig
> > b/drivers/gpu/drm/nouveau/Kconfig
> > index 96b9814e6d06..33ccf11bd70d 100644
> > --- a/drivers/gpu/drm/nouveau/Kconfig
> > +++ b/drivers/gpu/drm/nouveau/Kconfig
> > @@ -16,6 +16,7 @@ config DRM_NOUVEAU
> >  	select INPUT if ACPI && X86
> >  	select THERMAL if ACPI && X86
> >  	select ACPI_VIDEO if ACPI && X86
> > +	select SND_HDA_COMPONENT if SND_HDA_CORE
> >  	help
> >  	  Choose this option for open-source NVIDIA support.
> >  
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index 8497768f1b41..919f3d3db161 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -29,6 +29,7 @@
> >  
> >  #include <linux/dma-mapping.h>
> >  #include <linux/hdmi.h>
> > +#include <linux/component.h>
> >  
> >  #include <drm/drmP.h>
> >  #include <drm/drm_atomic_helper.h>
> > @@ -466,12 +467,113 @@ nv50_dac_create(struct drm_connector *connector,
> > struct dcb_output *dcbe)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * audio component binding for ELD notification
> > + */
> > +static void
> > +nv50_audio_component_eld_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 int
> > +nv50_audio_component_get_eld(struct device *kdev, int port, int pipe,
> > +			     bool *enabled, unsigned char *buf, int max_bytes)
> > +{
> > +	struct drm_device *drm_dev = dev_get_drvdata(kdev);
> > +	struct nouveau_drm *drm = nouveau_drm(drm_dev);
> > +	struct drm_encoder *encoder;
> > +	struct nouveau_encoder *nv_encoder;
> > +	struct nouveau_connector *nv_connector;
> > +	struct nouveau_crtc *nv_crtc;
> > +	int ret = 0;
> > +
> > +	*enabled = false;
> > +	drm_for_each_encoder(encoder, drm->dev) {
> > +		nv_encoder = nouveau_encoder(encoder);
> > +		nv_connector = nouveau_encoder_connector_get(nv_encoder);
> > +		nv_crtc = nouveau_crtc(encoder->crtc);
> > +		if (!nv_connector || !nv_crtc || nv_crtc->index != port)
> > +			continue;
> > +		*enabled = drm_detect_monitor_audio(nv_connector->edid);
> > +		if (*enabled) {
> > +			ret = drm_eld_size(nv_connector->base.eld);
> > +			memcpy(buf, nv_connector->base.eld,
> > +			       min(max_bytes, ret));
> > +		}
> > +		break;
> > +	}
> > +	return ret;
> > +}
> > +
> > +static const struct drm_audio_component_ops nv50_audio_component_ops = {
> > +	.get_eld = nv50_audio_component_get_eld,
> > +};
> > +
> > +static int
> > +nv50_audio_component_bind(struct device *kdev, struct device *hda_kdev,
> > +			  void *data)
> > +{
> > +	struct drm_device *drm_dev = dev_get_drvdata(kdev);
> > +	struct nouveau_drm *drm = nouveau_drm(drm_dev);
> > +	struct drm_audio_component *acomp = data;
> > +
> > +	if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
> > +		return -ENOMEM;
> > +
> > +	drm_modeset_lock_all(drm_dev);
> > +	acomp->ops = &nv50_audio_component_ops;
> > +	acomp->dev = kdev;
> > +	drm->audio.component = acomp;
> > +	drm_modeset_unlock_all(drm_dev);
> > +	return 0;
> > +}
> > +
> > +static void
> > +nv50_audio_component_unbind(struct device *kdev, struct device *hda_kdev,
> > +			    void *data)
> > +{
> > +	struct drm_device *drm_dev = dev_get_drvdata(kdev);
> > +	struct nouveau_drm *drm = nouveau_drm(drm_dev);
> > +	struct drm_audio_component *acomp = data;
> > +
> > +	drm_modeset_lock_all(drm_dev);
> > +	drm->audio.component = NULL;
> > +	acomp->ops = NULL;
> > +	acomp->dev = NULL;
> > +	drm_modeset_unlock_all(drm_dev);
> > +}
> > +
> > +static const struct component_ops nv50_audio_component_bind_ops = {
> > +	.bind   = nv50_audio_component_bind,
> > +	.unbind = nv50_audio_component_unbind,
> > +};
> > +
> > +static void
> > +nv50_audio_component_init(struct nouveau_drm *drm)
> > +{
> > +	if (!component_add(drm->dev->dev, &nv50_audio_component_bind_ops))
> > +		drm->audio.component_registered = true;
> > +}
> > +
> > +static void
> > +nv50_audio_component_fini(struct nouveau_drm *drm)
> > +{
> > +	if (drm->audio.component_registered) {
> > +		component_del(drm->dev->dev, &nv50_audio_component_bind_ops);
> > +		drm->audio.component_registered = false;
> > +	}
> > +}
> > +
> >  /**************************************************************************
> > ****
> >   * Audio
> >  
> > ****************************************************************************
> > */
> >  static void
> >  nv50_audio_disable(struct drm_encoder *encoder, struct nouveau_crtc
> > *nv_crtc)
> >  {
> > +	struct nouveau_drm *drm = nouveau_drm(encoder->dev);
> >  	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
> >  	struct nv50_disp *disp = nv50_disp(encoder->dev);
> >  	struct {
> > @@ -486,11 +588,14 @@ nv50_audio_disable(struct drm_encoder *encoder, struct
> > nouveau_crtc *nv_crtc)
> >  	};
> >  
> >  	nvif_mthd(&disp->disp->object, 0, &args, sizeof(args));
> > +
> > +	nv50_audio_component_eld_notify(drm->audio.component, nv_crtc->index);
> >  }
> >  
> >  static void
> >  nv50_audio_enable(struct drm_encoder *encoder, struct drm_display_mode
> > *mode)
> >  {
> > +	struct nouveau_drm *drm = nouveau_drm(encoder->dev);
> >  	struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
> >  	struct nouveau_crtc *nv_crtc = nouveau_crtc(encoder->crtc);
> >  	struct nouveau_connector *nv_connector;
> > @@ -517,6 +622,8 @@ nv50_audio_enable(struct drm_encoder *encoder, struct
> > drm_display_mode *mode)
> >  
> >  	nvif_mthd(&disp->disp->object, 0, &args,
> >  		  sizeof(args.base) + drm_eld_size(args.data));
> > +
> > +	nv50_audio_component_eld_notify(drm->audio.component, nv_crtc->index);
> >  }
> >  
> >  /**************************************************************************
> > ****
> > @@ -2281,6 +2388,8 @@ nv50_display_destroy(struct drm_device *dev)
> >  {
> >  	struct nv50_disp *disp = nv50_disp(dev);
> >  
> > +	nv50_audio_component_fini(nouveau_drm(dev));
> > +
> >  	nv50_core_del(&disp->core);
> >  
> >  	nouveau_bo_unmap(disp->sync);
> > @@ -2401,6 +2510,8 @@ nv50_display_create(struct drm_device *dev)
> >  	/* Disable vblank irqs aggressively for power-saving, safe on nv50+ */
> >  	dev->vblank_disable_immediate = true;
> >  
> > +	nv50_audio_component_init(drm);
> > +
> >  out:
> >  	if (ret)
> >  		nv50_display_destroy(dev);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > index aae035816383..15e4f2aa19bf 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > @@ -55,6 +55,8 @@
> >  #include <drm/ttm/ttm_module.h>
> >  #include <drm/ttm/ttm_page_alloc.h>
> >  
> > +#include <drm/drm_audio_component.h>
> > +
> >  #include "uapi/drm/nouveau_drm.h"
> >  
> >  struct nouveau_channel;
> > @@ -212,6 +214,11 @@ struct nouveau_drm {
> >  	struct nouveau_svm *svm;
> >  
> >  	struct nouveau_dmem *dmem;
> > +
> > +	struct {
> > +		struct drm_audio_component *component;
> > +		bool component_registered;
> > +	} audio;
> >  };
> >  
> >  static inline struct nouveau_drm *
> -- 
> Cheers,
> 	Lyude Paul
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel