[PATCH 1/2] drm/radeon: Add HD-audio component notifier support
Koenig, Christian
Christian.Koenig at amd.com
Mon Jul 22 15:38:36 UTC 2019
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 at amd.com>
>>>>> Signed-off-by: Takashi Iwai <tiwai at 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)
More information about the dri-devel
mailing list