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

["Koenig, Christian" <Christian.Koenig@amd.com>]

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