[PATCH 1/2] drm/radeon: Add HD-audio component notifier support
Takashi Iwai
tiwai at suse.de
Mon Jul 22 16:00:07 UTC 2019
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 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