| Message ID | 20170206102951.12623-2-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06.02.2017 11:29, Hans Verkuil wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > Add support for video hotplug detect and EDID/ELD notifiers, which is used > to convey information from video drivers to their CEC and audio counterparts. > > Based on an earlier version from Russell King: > > https://patchwork.kernel.org/patch/9277043/ > > The hpd_notifier is a reference counted object containing the HPD/EDID/ELD state > of a video device. > > When a new notifier is registered the current state will be reported to > that notifier at registration time. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> For patches 1-6: Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej
On Mon, Feb 06, 2017 at 11:29:43AM +0100, Hans Verkuil wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > Add support for video hotplug detect and EDID/ELD notifiers, which is used > to convey information from video drivers to their CEC and audio counterparts. > > Based on an earlier version from Russell King: > > https://patchwork.kernel.org/patch/9277043/ > > The hpd_notifier is a reference counted object containing the HPD/EDID/ELD state > of a video device. > > When a new notifier is registered the current state will be reported to > that notifier at registration time. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> So I'm super late to the party because I kinda ignored all things CEC thus far. Ḯ'm not sure this is a great design, with two main concerns: - notifiers have a tendency to make locking utter painful. By design notifiers use their own lock to make sure any callbacks don't disappear unduly, but then on the other hand the locking order at register/unregister time is inverted. Or anything where you need to go the other way. For simple things it works, but my experience is that sooner or later things stop being simple, and then you're in complete pain. Viz fbdev notifier. I much more prefer if we have a direct interface, where we can define the lifetime rules and locking rules seperately, and if needed separately for each interface. Especially for something which is meant to connect different drivers from different subsystems so widely. You could object that this is the only interaction, and therefore there can't be loops, but we have dma-buf, reservation_obj and dma_fence sharing going on between lots of drivers already, and lots more will follow, so there's plenty of other cross-subsystem interactions going on to help complete the loop. - The other concern I have is that we already have interfaces for ELD and hotplug notification. Atm their only restricted for use between gfx and snd, and e.g. i915 rolls 2 different kinds of its own, but there is a semi-standard one: include/sound/hdmi-codec.h That also deals with ELD and stuff, would be great if we don't force drivers to signal ELD changes in multiple different ways. Also, CEC should only exist with a HDMI output, so same thing. - Afaiui we'll always should have a 1:1 mapping between drm HDMI connector and a given CEC pin. Notifiers are for n:m, how is this supposed to work if you have multiple HDMI ports around? I see that you look up by struct device, but it's a bit unclear what kind of device is supposed to be used as the key. If we extend the hdmi-codec stuff from sound and make it a notch more generic, then we'd already have the arbiter object to ties things together. Just some thoughts on this. And again my apologies for being late with my input. Thanks, Daniel > --- > drivers/video/Kconfig | 3 + > drivers/video/Makefile | 1 + > drivers/video/hpd-notifier.c | 134 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/hpd-notifier.h | 109 +++++++++++++++++++++++++++++++++++ > 4 files changed, 247 insertions(+) > create mode 100644 drivers/video/hpd-notifier.c > create mode 100644 include/linux/hpd-notifier.h > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig > index 3c20af999893..a3a58d8481e9 100644 > --- a/drivers/video/Kconfig > +++ b/drivers/video/Kconfig > @@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS > config HDMI > bool > > +config HPD_NOTIFIER > + bool > + > if VT > source "drivers/video/console/Kconfig" > endif > diff --git a/drivers/video/Makefile b/drivers/video/Makefile > index 9ad3c17d6456..2697ae5c4166 100644 > --- a/drivers/video/Makefile > +++ b/drivers/video/Makefile > @@ -1,5 +1,6 @@ > obj-$(CONFIG_VGASTATE) += vgastate.o > obj-$(CONFIG_HDMI) += hdmi.o > +obj-$(CONFIG_HPD_NOTIFIER) += hpd-notifier.o > > obj-$(CONFIG_VT) += console/ > obj-$(CONFIG_LOGO) += logo/ > diff --git a/drivers/video/hpd-notifier.c b/drivers/video/hpd-notifier.c > new file mode 100644 > index 000000000000..971e823ead00 > --- /dev/null > +++ b/drivers/video/hpd-notifier.c > @@ -0,0 +1,134 @@ > +#include <linux/export.h> > +#include <linux/hpd-notifier.h> > +#include <linux/string.h> > +#include <linux/slab.h> > +#include <linux/list.h> > + > +static LIST_HEAD(hpd_notifiers); > +static DEFINE_MUTEX(hpd_notifiers_lock); > + > +struct hpd_notifier *hpd_notifier_get(struct device *dev) > +{ > + struct hpd_notifier *n; > + > + mutex_lock(&hpd_notifiers_lock); > + list_for_each_entry(n, &hpd_notifiers, head) { > + if (n->dev == dev) { > + mutex_unlock(&hpd_notifiers_lock); > + kref_get(&n->kref); > + return n; > + } > + } > + n = kzalloc(sizeof(*n), GFP_KERNEL); > + if (!n) > + goto unlock; > + n->dev = dev; > + mutex_init(&n->lock); > + BLOCKING_INIT_NOTIFIER_HEAD(&n->notifiers); > + kref_init(&n->kref); > + list_add_tail(&n->head, &hpd_notifiers); > +unlock: > + mutex_unlock(&hpd_notifiers_lock); > + return n; > +} > +EXPORT_SYMBOL_GPL(hpd_notifier_get); > + > +static void hpd_notifier_release(struct kref *kref) > +{ > + struct hpd_notifier *n = > + container_of(kref, struct hpd_notifier, kref); > + > + list_del(&n->head); > + kfree(n->edid); > + kfree(n); > +} > + > +void hpd_notifier_put(struct hpd_notifier *n) > +{ > + mutex_lock(&hpd_notifiers_lock); > + kref_put(&n->kref, hpd_notifier_release); > + mutex_unlock(&hpd_notifiers_lock); > +} > +EXPORT_SYMBOL_GPL(hpd_notifier_put); > + > +int hpd_notifier_register(struct hpd_notifier *n, struct notifier_block *nb) > +{ > + int ret = blocking_notifier_chain_register(&n->notifiers, nb); > + > + if (ret) > + return ret; > + kref_get(&n->kref); > + mutex_lock(&n->lock); > + if (n->connected) { > + blocking_notifier_call_chain(&n->notifiers, HPD_CONNECTED, n); > + if (n->edid_size) > + blocking_notifier_call_chain(&n->notifiers, HPD_NEW_EDID, n); > + if (n->has_eld) > + blocking_notifier_call_chain(&n->notifiers, HPD_NEW_ELD, n); > + } > + mutex_unlock(&n->lock); > + return 0; > +} > +EXPORT_SYMBOL_GPL(hpd_notifier_register); > + > +int hpd_notifier_unregister(struct hpd_notifier *n, struct notifier_block *nb) > +{ > + int ret = blocking_notifier_chain_unregister(&n->notifiers, nb); > + > + if (ret == 0) > + hpd_notifier_put(n); > + return ret; > +} > +EXPORT_SYMBOL_GPL(hpd_notifier_unregister); > + > +void hpd_event_connect(struct hpd_notifier *n) > +{ > + mutex_lock(&n->lock); > + n->connected = true; > + blocking_notifier_call_chain(&n->notifiers, HPD_CONNECTED, n); > + mutex_unlock(&n->lock); > +} > +EXPORT_SYMBOL_GPL(hpd_event_connect); > + > +void hpd_event_disconnect(struct hpd_notifier *n) > +{ > + mutex_lock(&n->lock); > + n->connected = false; > + n->has_eld = false; > + n->edid_size = 0; > + blocking_notifier_call_chain(&n->notifiers, HPD_DISCONNECTED, n); > + mutex_unlock(&n->lock); > +} > +EXPORT_SYMBOL_GPL(hpd_event_disconnect); > + > +int hpd_event_new_edid(struct hpd_notifier *n, const void *edid, size_t size) > +{ > + mutex_lock(&n->lock); > + if (n->edid_allocated_size < size) { > + void *p = kmalloc(size, GFP_KERNEL); > + > + if (p == NULL) { > + mutex_unlock(&n->lock); > + return -ENOMEM; > + } > + kfree(n->edid); > + n->edid = p; > + n->edid_allocated_size = size; > + } > + memcpy(n->edid, edid, size); > + n->edid_size = size; > + blocking_notifier_call_chain(&n->notifiers, HPD_NEW_EDID, n); > + mutex_unlock(&n->lock); > + return 0; > +} > +EXPORT_SYMBOL_GPL(hpd_event_new_edid); > + > +void hpd_event_new_eld(struct hpd_notifier *n, const u8 eld[128]) > +{ > + mutex_lock(&n->lock); > + memcpy(n->eld, eld, sizeof(n->eld)); > + n->has_eld = true; > + blocking_notifier_call_chain(&n->notifiers, HPD_NEW_ELD, n); > + mutex_unlock(&n->lock); > +} > +EXPORT_SYMBOL_GPL(hpd_event_new_eld); > diff --git a/include/linux/hpd-notifier.h b/include/linux/hpd-notifier.h > new file mode 100644 > index 000000000000..4dcb4515d2b2 > --- /dev/null > +++ b/include/linux/hpd-notifier.h > @@ -0,0 +1,109 @@ > +/* > + * hpd-notifier.h - notify interested parties of (dis)connect and EDID > + * events > + * > + * Copyright 2016 Russell King <rmk+kernel@arm.linux.org.uk> > + * Copyright 2016 Cisco Systems, Inc. and/or its affiliates. All rights reserved. > + */ > + > +#ifndef LINUX_HPD_NOTIFIER_H > +#define LINUX_HPD_NOTIFIER_H > + > +#include <linux/types.h> > +#include <linux/notifier.h> > +#include <linux/kref.h> > + > +enum { > + HPD_CONNECTED, > + HPD_DISCONNECTED, > + HPD_NEW_EDID, > + HPD_NEW_ELD, > +}; > + > +struct device; > + > +struct hpd_notifier { > + struct mutex lock; > + struct list_head head; > + struct kref kref; > + struct blocking_notifier_head notifiers; > + struct device *dev; > + > + /* Current state */ > + bool connected; > + bool has_eld; > + unsigned char eld[128]; > + void *edid; > + size_t edid_size; > + size_t edid_allocated_size; > +}; > + > +/** > + * hpd_notifier_get - find or create a new hpd_notifier for the given device. > + * @dev: device that sends the events. > + * > + * If a notifier for device @dev already exists, then increase the refcount > + * and return that notifier. > + * > + * If it doesn't exist, then allocate a new notifier struct and return a > + * pointer to that new struct. > + * > + * Return NULL if the memory could not be allocated. > + */ > +struct hpd_notifier *hpd_notifier_get(struct device *dev); > + > +/** > + * hpd_notifier_put - decrease refcount and delete when the refcount reaches 0. > + * @n: notifier > + */ > +void hpd_notifier_put(struct hpd_notifier *n); > + > +/** > + * hpd_notifier_register - register the notifier with the notifier_block. > + * @n: the HPD notifier > + * @nb: the notifier_block > + */ > +int hpd_notifier_register(struct hpd_notifier *n, struct notifier_block *nb); > + > +/** > + * hpd_notifier_unregister - unregister the notifier with the notifier_block. > + * @n: the HPD notifier > + * @nb: the notifier_block > + */ > +int hpd_notifier_unregister(struct hpd_notifier *n, struct notifier_block *nb); > + > +/** > + * hpd_event_connect - send a connect event. > + * @n: the HPD notifier > + * > + * Send an HPD_CONNECTED event to any registered parties. > + */ > +void hpd_event_connect(struct hpd_notifier *n); > + > +/** > + * hpd_event_disconnect - send a disconnect event. > + * @n: the HPD notifier > + * > + * Send an HPD_DISCONNECTED event to any registered parties. > + */ > +void hpd_event_disconnect(struct hpd_notifier *n); > + > +/** > + * hpd_event_new_edid - send a new EDID event. > + * @n: the HPD notifier > + * > + * Send an HPD_NEW_EDID event to any registered parties. > + * This function will make a copy the EDID so it can return -ENOMEM if > + * no memory could be allocated. > + */ > +int hpd_event_new_edid(struct hpd_notifier *n, const void *edid, size_t size); > + > +/** > + * hpd_event_new_eld - send a new ELD event. > + * @n: the HPD notifier > + * > + * Send an HPD_NEW_ELD event to any registered parties. > + */ > +void hpd_event_new_eld(struct hpd_notifier *n, const u8 eld[128]); > + > +#endif > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Feb 27, 2017 at 05:08:41PM +0100, Daniel Vetter wrote: > On Mon, Feb 06, 2017 at 11:29:43AM +0100, Hans Verkuil wrote: > > From: Hans Verkuil <hans.verkuil@cisco.com> > > > > Add support for video hotplug detect and EDID/ELD notifiers, which is used > > to convey information from video drivers to their CEC and audio counterparts. > > > > Based on an earlier version from Russell King: > > > > https://patchwork.kernel.org/patch/9277043/ > > > > The hpd_notifier is a reference counted object containing the HPD/EDID/ELD state > > of a video device. > > > > When a new notifier is registered the current state will be reported to > > that notifier at registration time. > > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > > So I'm super late to the party because I kinda ignored all things CEC thus > far. Ḯ'm not sure this is a great design, with two main concerns: I'm afraid that I walked away from this after it became clear that there was little hope for any forward progress being made in a timely manner for multiple reasons (mainly the core CEC code being out of mainline.) The original notifier was created in August 2015, before there was any "hdmi codec" support or anything of the like. At some point (I'm not sure when) Philipp gave his ack on it, and I definitely know it was subsequently posted for RFC in August 2016. We're now 1.5 years after its creation, 7 months after it was definitely publically posted to dri-devel, and you've only just said that you don't like the approach... Anyway, the hdmi-codec header you point at is only relevant when you have a driver using ASoC and you have the codec part tightly integrated with your HDMI interface. That generally works fine there, because generally they are on the same device, and are very dependent (due to the need to know the HDMI bus clock.) The same is not true of CEC though - for example, the TDA998x is actually two devices - the HDMI bridge, and an entirely separate TDA9950 CEC device. They may be in the same package, but the TDA9950 was available as an entirely separate device. The reason that is the case is because they are entirely separate entities as far as functionality goes: nothing on the CEC communication side electrically depends on the HDMI bus itself. The only common thing in common is the connector. From the protocol point of view, CEC requires the "physical address" of a device, and that is part of the EDID information from the HDMI device - so CEC needs to have access to the EDID. CEC also needs to know when if/when the EDID information is updated, or when connection/ disconnection events occur so that it can re-negotiate its "logical address", and update for any physical address changes. For example, if you have a CEC device connected to an AV receiver, which is in turn connected to a TV, and the TV is powered down but the AV receiver is powered up, then the AV receiver will give all devices connected to it a physical address to the best of its knowledge. Turn the TV on, and the physical address will change (especially so if the AV receiver has been moved between different inputs on the TV.) This all needs the HDMI driver to _notify_ the CEC part of these state changes - you can't get away from the need to _notify_ these events. So, what we need is: (a) some way for CEC to be _notified_ of all HPD change events (b) some way for CEC to query the EDID in a race free manner w.r.t. HPD (a) pretty much involves some kind of notification system. It doesn't matter whether it's a real notifier, or a struct of function pointers, the effect is going to be the same no matter what - the basic requirement is that we run some code in the CEC side when a HPD state change occurs. Given that, what you seem to be objecting to (wrt locking on this) is against the fundamental requirement that CEC needs to track the HPD state. (b) can be done in other ways, but I'd suggest reversing the design (iow, having CEC explicitly query the HDMI part for the current EDID) is more racy than having the HDMI part notify CEC - you have the situation where CEC could be querying the EDID on one CPU while HDMI on another CPU is saying that the HPD changed state. The query approach also carries with it a whole new set of locking issues, because we can get into this situation: HDMI CEC --- HPD insert ---> <--- EDID read ---- The problem then is that if HDMI holds a lock while sending the HPD insert message, and it tries to take the same lock when supplying the EDID back to CEC, you have an immediate deadlock. So, given that HDMI needs to notify CEC about HPD changes, it also makes sense to keep the overall flow of data the same for everything - avoid back-queries, and have HDMI notify CEC of the new EDID. It also avoids the problem where we may see HPD assert, but it may take some time for the EDID to become available from HDMI (eg, in the case of TDA998x, we have to wait a while before even attempting to read the EDID.) The last point on EDID is one about the source of the EDID (eg, firmware- loaded EDID from disk). That won't work for CEC, since loading a fixed EDID off disk will not give the correct physical address, and so HDMI routing will break. That could be worked around by having userspace modify the loaded EDID, but that sounds like making the job unnecessarily hard, when the correct information is only available in the sink's EDID. When I created the notifier, the obvious problem was how does a driver receiving a notify message know that it should process that message - and I chose to supply the source "struct device" with each message. This would be the HDMI interface itself, and using "struct device" gives a firmware/no-firmware independent way of identifying the source. For cases like the TDA998x and dw-hdmi, firmware doesn't get involved with this: in both cases, the drivers declare their CEC device as a platform device, so the relationship between the two struct device's is known. In the case of a stand-alone TDA9950, then the struct device for the HDMI side needs to be indicated by firmware, and it's possible to get the struct device corresponding with a firmware node. For setups like i915, I would not expect i915 to want to use this, as I would imagine that (if they did support CEC) CEC would be tightly integrated, following the pattern of ultra-tight integration of everything in i915 hardware (it seems to program everything through the GPU stream.) If you can think of a better approach, then I'm sure there's lots of people who'd be willing to do the coding for it... if not, I'm not sure where we go from here (apart from keeping code in private/vendor trees.)
On 02/27/2017 06:04 PM, Russell King - ARM Linux wrote: > On Mon, Feb 27, 2017 at 05:08:41PM +0100, Daniel Vetter wrote: >> On Mon, Feb 06, 2017 at 11:29:43AM +0100, Hans Verkuil wrote: >>> From: Hans Verkuil <hans.verkuil@cisco.com> >>> >>> Add support for video hotplug detect and EDID/ELD notifiers, which is used >>> to convey information from video drivers to their CEC and audio counterparts. >>> >>> Based on an earlier version from Russell King: >>> >>> https://patchwork.kernel.org/patch/9277043/ >>> >>> The hpd_notifier is a reference counted object containing the HPD/EDID/ELD state >>> of a video device. >>> >>> When a new notifier is registered the current state will be reported to >>> that notifier at registration time. >>> >>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> >> >> So I'm super late to the party because I kinda ignored all things CEC thus >> far. Ḯ'm not sure this is a great design, with two main concerns: > > I'm afraid that I walked away from this after it became clear that there > was little hope for any forward progress being made in a timely manner > for multiple reasons (mainly the core CEC code being out of mainline.) In case you missed it: the core CEC code was moved out of staging and into mainline in 4.10. > > The original notifier was created in August 2015, before there was any > "hdmi codec" support or anything of the like. At some point (I'm not > sure when) Philipp gave his ack on it, and I definitely know it was > subsequently posted for RFC in August 2016. We're now 1.5 years after > its creation, 7 months after it was definitely publically posted to > dri-devel, and you've only just said that you don't like the approach... > > Anyway, the hdmi-codec header you point at is only relevant when you > have a driver using ASoC and you have the codec part tightly integrated > with your HDMI interface. That generally works fine there, because > generally they are on the same device, and are very dependent (due to > the need to know the HDMI bus clock.) > > The same is not true of CEC though - for example, the TDA998x is > actually two devices - the HDMI bridge, and an entirely separate > TDA9950 CEC device. They may be in the same package, but the TDA9950 > was available as an entirely separate device. The reason that is the > case is because they are entirely separate entities as far as > functionality goes: nothing on the CEC communication side electrically > depends on the HDMI bus itself. The only common thing in common is > the connector. > > From the protocol point of view, CEC requires the "physical address" > of a device, and that is part of the EDID information from the HDMI > device - so CEC needs to have access to the EDID. CEC also needs to > know when if/when the EDID information is updated, or when connection/ > disconnection events occur so that it can re-negotiate its "logical > address", and update for any physical address changes. > > For example, if you have a CEC device connected to an AV receiver, > which is in turn connected to a TV, and the TV is powered down but > the AV receiver is powered up, then the AV receiver will give all > devices connected to it a physical address to the best of its > knowledge. Turn the TV on, and the physical address will change > (especially so if the AV receiver has been moved between different > inputs on the TV.) > > This all needs the HDMI driver to _notify_ the CEC part of these state > changes - you can't get away from the need to _notify_ these events. > > So, what we need is: > > (a) some way for CEC to be _notified_ of all HPD change events > (b) some way for CEC to query the EDID in a race free manner w.r.t. HPD > > (a) pretty much involves some kind of notification system. It doesn't > matter whether it's a real notifier, or a struct of function pointers, > the effect is going to be the same no matter what - the basic requirement > is that we run some code in the CEC side when a HPD state change occurs. > Given that, what you seem to be objecting to (wrt locking on this) is > against the fundamental requirement that CEC needs to track the HPD > state. > > (b) can be done in other ways, but I'd suggest reversing the design (iow, > having CEC explicitly query the HDMI part for the current EDID) is more > racy than having the HDMI part notify CEC - you have the situation where > CEC could be querying the EDID on one CPU while HDMI on another CPU is > saying that the HPD changed state. > > The query approach also carries with it a whole new set of locking issues, > because we can get into this situation: > > HDMI CEC > --- HPD insert ---> > <--- EDID read ---- > > The problem then is that if HDMI holds a lock while sending the HPD insert > message, and it tries to take the same lock when supplying the EDID back > to CEC, you have an immediate deadlock. > > So, given that HDMI needs to notify CEC about HPD changes, it also makes > sense to keep the overall flow of data the same for everything - avoid > back-queries, and have HDMI notify CEC of the new EDID. > > It also avoids the problem where we may see HPD assert, but it may take > some time for the EDID to become available from HDMI (eg, in the case > of TDA998x, we have to wait a while before even attempting to read the > EDID.) > > The last point on EDID is one about the source of the EDID (eg, firmware- > loaded EDID from disk). That won't work for CEC, since loading a fixed > EDID off disk will not give the correct physical address, and so HDMI > routing will break. That could be worked around by having userspace > modify the loaded EDID, but that sounds like making the job unnecessarily > hard, when the correct information is only available in the sink's EDID. > > When I created the notifier, the obvious problem was how does a driver > receiving a notify message know that it should process that message - > and I chose to supply the source "struct device" with each message. > This would be the HDMI interface itself, and using "struct device" > gives a firmware/no-firmware independent way of identifying the source. > > For cases like the TDA998x and dw-hdmi, firmware doesn't get involved > with this: in both cases, the drivers declare their CEC device as a > platform device, so the relationship between the two struct device's > is known. In the case of a stand-alone TDA9950, then the struct > device for the HDMI side needs to be indicated by firmware, and it's > possible to get the struct device corresponding with a firmware node. > > For setups like i915, I would not expect i915 to want to use this, as > I would imagine that (if they did support CEC) CEC would be tightly > integrated, following the pattern of ultra-tight integration of > everything in i915 hardware (it seems to program everything through > the GPU stream.) Absolutely true. From what I've seen there are two types of CEC hardware implementations: either tightly integrated in the HDMI IP (in which case you would call the CEC framework directly, without requiring a notifier framework), or it is entirely decoupled in which case a notifier is IMHO the best approach. > If you can think of a better approach, then I'm sure there's lots of > people who'd be willing to do the coding for it... if not, I'm not > sure where we go from here (apart from keeping code in private/vendor > trees.) For CEC there are just two things that it needs: the physical address (contained in the EDID) and it needs to be informed when the EDID disappears (typically due to a disconnect), in which case the physical address becomes invalid (f.f.f.f). Russell, do you have pending code that needs the ELD support in the notifier? CEC doesn't need it, so from my perspective it can be dropped in the first version. It is also possible to have the notifier parse and store the physical address rather than the whole EDID to slim it down further. Personally I have no preference. All I need is the physical address :-) Regards, Hans
On Mon, Feb 27, 2017 at 06:21:05PM +0100, Hans Verkuil wrote: > On 02/27/2017 06:04 PM, Russell King - ARM Linux wrote: > > I'm afraid that I walked away from this after it became clear that there > > was little hope for any forward progress being made in a timely manner > > for multiple reasons (mainly the core CEC code being out of mainline.) > > In case you missed it: the core CEC code was moved out of staging and into > mainline in 4.10. I was aware (even though I've not been publishing anything, I do keep dw-hdmi-cec and tda9950/tda998x up to date with every final kernel release.) > > If you can think of a better approach, then I'm sure there's lots of > > people who'd be willing to do the coding for it... if not, I'm not > > sure where we go from here (apart from keeping code in private/vendor > > trees.) > > For CEC there are just two things that it needs: the physical address > (contained in the EDID) and it needs to be informed when the EDID disappears > (typically due to a disconnect), in which case the physical address > becomes invalid (f.f.f.f). Yep. CEC really only needs to know "have new phys address" and "disconnect" provided that CEC drivers understand that they may receive a new phys address with no intervening disconnect. (Consider the case where EDID changes, but the HDMI driver failed to spot the HPD signal pulse - unfortunately, there's hardware out there where HPD is next to useless.) > Russell, do you have pending code that needs the ELD support in the > notifier? CEC doesn't need it, so from my perspective it can be > dropped in the first version. I was looking for that while writing the previous mail, and I think it's time to drop it - I had thought dw-hdmi-*audio would use it, or the ASoC people, but it's still got no users, so I think it's time to drop it. I have seen some patch sets go by which are making use of the notifier, but I haven't paid close attention to how they're using it or what they're using it for... as I sort of implied in my previous mail, I had lost interest in mainline wrt CEC stuff due to the glacial rate of progress. (That's not a criticism.)
On Mon, Feb 27, 2017 at 05:46:51PM +0000, Russell King - ARM Linux wrote: > On Mon, Feb 27, 2017 at 06:21:05PM +0100, Hans Verkuil wrote: > > On 02/27/2017 06:04 PM, Russell King - ARM Linux wrote: > > > I'm afraid that I walked away from this after it became clear that there > > > was little hope for any forward progress being made in a timely manner > > > for multiple reasons (mainly the core CEC code being out of mainline.) > > > > In case you missed it: the core CEC code was moved out of staging and into > > mainline in 4.10. > > I was aware (even though I've not been publishing anything, I do keep > dw-hdmi-cec and tda9950/tda998x up to date with every final kernel > release.) > > > > If you can think of a better approach, then I'm sure there's lots of > > > people who'd be willing to do the coding for it... if not, I'm not > > > sure where we go from here (apart from keeping code in private/vendor > > > trees.) > > > > For CEC there are just two things that it needs: the physical address > > (contained in the EDID) and it needs to be informed when the EDID disappears > > (typically due to a disconnect), in which case the physical address > > becomes invalid (f.f.f.f). > > Yep. CEC really only needs to know "have new phys address" and > "disconnect" provided that CEC drivers understand that they may receive > a new phys address with no intervening disconnect. (Consider the case > where EDID changes, but the HDMI driver failed to spot the HPD signal > pulse - unfortunately, there's hardware out there where HPD is next to > useless.) Ok, simplifying the CEC stuff like that would be a lot better I think, to avoid overlap with other in-kernel interfaces. I still have some questions, but this might be my misunderstanding of how CEC works: I thought that CEC is driven through a special separate wire in the HDMI bus, with the receiver in the TV. Which means that we'd have a 1:1 relationship between HDMI connector and CEC port. That's at least the use-case I've heard of for Intel boards. Are there other use-cases where we do not have a 1:1 relationship between HDMI connector and CEC port? Imo notifier really only makes sense as a quick&dirty hack, or if you really have n:m for at least one of n,m != 1. Otherwise some very specific callback service which just provides the information the CEC side needs seems like a much better idea to me. > > Russell, do you have pending code that needs the ELD support in the > > notifier? CEC doesn't need it, so from my perspective it can be > > dropped in the first version. > > I was looking for that while writing the previous mail, and I think > it's time to drop it - I had thought dw-hdmi-*audio would use it, or > the ASoC people, but it's still got no users, so I think it's time > to drop it. For ELD I'd definitely say let's please use the hdmi-codec.h. It's what's in tree, so better to converge on one solution instead of proliferating even more. The entire sad story of multiple people inventing similar wheels without talking with each another is water down the river, can't fix that anymore :( > I have seen some patch sets go by which are making use of the notifier, > but I haven't paid close attention to how they're using it or what > they're using it for... as I sort of implied in my previous mail, I > had lost interest in mainline wrt CEC stuff due to the glacial rate > of progress. (That's not a criticism.) Maybe some docs that would describe the flow we want to achieve here would help? Doesn't need to be more than a few lines, but reconstructing that from the various driver patches later on is indeed hard. -Daniel
On 02/28/17 09:51, Daniel Vetter wrote: > On Mon, Feb 27, 2017 at 05:46:51PM +0000, Russell King - ARM Linux wrote: >> On Mon, Feb 27, 2017 at 06:21:05PM +0100, Hans Verkuil wrote: >>> On 02/27/2017 06:04 PM, Russell King - ARM Linux wrote: >>>> I'm afraid that I walked away from this after it became clear that there >>>> was little hope for any forward progress being made in a timely manner >>>> for multiple reasons (mainly the core CEC code being out of mainline.) >>> >>> In case you missed it: the core CEC code was moved out of staging and into >>> mainline in 4.10. >> >> I was aware (even though I've not been publishing anything, I do keep >> dw-hdmi-cec and tda9950/tda998x up to date with every final kernel >> release.) >> >>>> If you can think of a better approach, then I'm sure there's lots of >>>> people who'd be willing to do the coding for it... if not, I'm not >>>> sure where we go from here (apart from keeping code in private/vendor >>>> trees.) >>> >>> For CEC there are just two things that it needs: the physical address >>> (contained in the EDID) and it needs to be informed when the EDID disappears >>> (typically due to a disconnect), in which case the physical address >>> becomes invalid (f.f.f.f). >> >> Yep. CEC really only needs to know "have new phys address" and >> "disconnect" provided that CEC drivers understand that they may receive >> a new phys address with no intervening disconnect. (Consider the case >> where EDID changes, but the HDMI driver failed to spot the HPD signal >> pulse - unfortunately, there's hardware out there where HPD is next to >> useless.) > > Ok, simplifying the CEC stuff like that would be a lot better I think, to > avoid overlap with other in-kernel interfaces. I still have some > questions, but this might be my misunderstanding of how CEC works: > > I thought that CEC is driven through a special separate wire in the HDMI > bus, with the receiver in the TV. Which means that we'd have a 1:1 > relationship between HDMI connector and CEC port. That's at least the > use-case I've heard of for Intel boards. Are there other use-cases where > we do not have a 1:1 relationship between HDMI connector and CEC port? Imo > notifier really only makes sense as a quick&dirty hack, or if you really > have n:m for at least one of n,m != 1. Otherwise some very specific > callback service which just provides the information the CEC side needs > seems like a much better idea to me. For the current set of CEC drivers it is 1:1. I am fairly certain you can get n:1 situations where multiple HDMI connectors use a single CEC adapter. I'm thinking primarily about HDMI switches here. Also TVs with multiple inputs (basically a switch as well). I do not support such hardware yet, though. Or to be more specific: I've never tested this, so I am not sure if this would work in the current framework, or if I need to do some more work for that. That said, each CEC adapter would have only 0 or 1 HDMI outputs and 0 or more HDMI inputs. More than one HDMI outputs is AFAICT not possible. > >>> Russell, do you have pending code that needs the ELD support in the >>> notifier? CEC doesn't need it, so from my perspective it can be >>> dropped in the first version. >> >> I was looking for that while writing the previous mail, and I think >> it's time to drop it - I had thought dw-hdmi-*audio would use it, or >> the ASoC people, but it's still got no users, so I think it's time >> to drop it. > > For ELD I'd definitely say let's please use the hdmi-codec.h. It's what's > in tree, so better to converge on one solution instead of proliferating > even more. The entire sad story of multiple people inventing similar > wheels without talking with each another is water down the river, can't > fix that anymore :( I'll drop that in my next patch series. > >> I have seen some patch sets go by which are making use of the notifier, >> but I haven't paid close attention to how they're using it or what >> they're using it for... as I sort of implied in my previous mail, I >> had lost interest in mainline wrt CEC stuff due to the glacial rate >> of progress. (That's not a criticism.) > > Maybe some docs that would describe the flow we want to achieve here would > help? Doesn't need to be more than a few lines, but reconstructing that > from the various driver patches later on is indeed hard. I'll add some more documentation for the next version. But in a nutshell: each HDMI connector (in or out) has to notify the CEC driver when the physical address changes (when an EDID is read or set, or when the HPD goes down). It also needs to provide the current physical address when the CEC driver is first loaded. This latter requirement is handled by the notifier framework which remembers the EDID and will create a notify event as soon as the CEC driver registers itself. Regards, Hans
On Tue, Feb 28, 2017 at 10:23:57AM +0100, Hans Verkuil wrote: > On 02/28/17 09:51, Daniel Vetter wrote: > > On Mon, Feb 27, 2017 at 05:46:51PM +0000, Russell King - ARM Linux wrote: > > > On Mon, Feb 27, 2017 at 06:21:05PM +0100, Hans Verkuil wrote: > > > > On 02/27/2017 06:04 PM, Russell King - ARM Linux wrote: > > > > > I'm afraid that I walked away from this after it became clear that there > > > > > was little hope for any forward progress being made in a timely manner > > > > > for multiple reasons (mainly the core CEC code being out of mainline.) > > > > > > > > In case you missed it: the core CEC code was moved out of staging and into > > > > mainline in 4.10. > > > > > > I was aware (even though I've not been publishing anything, I do keep > > > dw-hdmi-cec and tda9950/tda998x up to date with every final kernel > > > release.) > > > > > > > > If you can think of a better approach, then I'm sure there's lots of > > > > > people who'd be willing to do the coding for it... if not, I'm not > > > > > sure where we go from here (apart from keeping code in private/vendor > > > > > trees.) > > > > > > > > For CEC there are just two things that it needs: the physical address > > > > (contained in the EDID) and it needs to be informed when the EDID disappears > > > > (typically due to a disconnect), in which case the physical address > > > > becomes invalid (f.f.f.f). > > > > > > Yep. CEC really only needs to know "have new phys address" and > > > "disconnect" provided that CEC drivers understand that they may receive > > > a new phys address with no intervening disconnect. (Consider the case > > > where EDID changes, but the HDMI driver failed to spot the HPD signal > > > pulse - unfortunately, there's hardware out there where HPD is next to > > > useless.) > > > > Ok, simplifying the CEC stuff like that would be a lot better I think, to > > avoid overlap with other in-kernel interfaces. I still have some > > questions, but this might be my misunderstanding of how CEC works: > > > > I thought that CEC is driven through a special separate wire in the HDMI > > bus, with the receiver in the TV. Which means that we'd have a 1:1 > > relationship between HDMI connector and CEC port. That's at least the > > use-case I've heard of for Intel boards. Are there other use-cases where > > we do not have a 1:1 relationship between HDMI connector and CEC port? Imo > > notifier really only makes sense as a quick&dirty hack, or if you really > > have n:m for at least one of n,m != 1. Otherwise some very specific > > callback service which just provides the information the CEC side needs > > seems like a much better idea to me. > > For the current set of CEC drivers it is 1:1. > > I am fairly certain you can get n:1 situations where multiple HDMI connectors > use a single CEC adapter. I'm thinking primarily about HDMI switches here. Also > TVs with multiple inputs (basically a switch as well). > > I do not support such hardware yet, though. Or to be more specific: I've never > tested this, so I am not sure if this would work in the current framework, or > if I need to do some more work for that. > > That said, each CEC adapter would have only 0 or 1 HDMI outputs and 0 or more > HDMI inputs. More than one HDMI outputs is AFAICT not possible. > > > > > > > Russell, do you have pending code that needs the ELD support in the > > > > notifier? CEC doesn't need it, so from my perspective it can be > > > > dropped in the first version. > > > > > > I was looking for that while writing the previous mail, and I think > > > it's time to drop it - I had thought dw-hdmi-*audio would use it, or > > > the ASoC people, but it's still got no users, so I think it's time > > > to drop it. > > > > For ELD I'd definitely say let's please use the hdmi-codec.h. It's what's > > in tree, so better to converge on one solution instead of proliferating > > even more. The entire sad story of multiple people inventing similar > > wheels without talking with each another is water down the river, can't > > fix that anymore :( > > I'll drop that in my next patch series. > > > > > > I have seen some patch sets go by which are making use of the notifier, > > > but I haven't paid close attention to how they're using it or what > > > they're using it for... as I sort of implied in my previous mail, I > > > had lost interest in mainline wrt CEC stuff due to the glacial rate > > > of progress. (That's not a criticism.) > > > > Maybe some docs that would describe the flow we want to achieve here would > > help? Doesn't need to be more than a few lines, but reconstructing that > > from the various driver patches later on is indeed hard. > > I'll add some more documentation for the next version. > > But in a nutshell: each HDMI connector (in or out) has to notify the CEC > driver when the physical address changes (when an EDID is read or set, or > when the HPD goes down). It also needs to provide the current physical > address when the CEC driver is first loaded. This latter requirement is > handled by the notifier framework which remembers the EDID and will create > a notify event as soon as the CEC driver registers itself. [one reply for all blocks] Ok, if this is all we need, then I think we should do a minimal interface just for that. I think we should also have an opaque struct to mediate the 1:1 relationship, maybe struct cec_pin or similar? I guess reusing hdmi_codec is indeed not a perfect fit, since both CEC and snd could be integrate, or just one, or neither. If we go through an opaque struct we could also provide platform-specific magic to look them up (like DT/of_). Internally it could still use notifiers for the first implementation, but I think it'd be good if we don't expose the n:m semantics to users. So super rough sketch without looking at anything: - on the display side (v4l or drm): cec_(un)register_pin(struct cec_pin *pin); Registers an unregisters a pin with the CEC subsystem. We'd need platform data or maybe an entire struct device embedded. cec_set_address(struct cec_pin *pin, struct cec_address *address); ... or whatever exactly is needed. - on the CEC driver side: cec_(un)register_callbacks(struct cec_pin *pin, struct cec_pin_callbacks* cbs); There wouldn't be any way to get at a cec_pin, that would be done by platform-specific of_*. Or maybe a fallback using something else, like we have with gpios or similar. This would also grab refcounts on the device underlying the cec_pin, to make sure stuff doesn't disappear untimely. The only callback would be one that gives you the new address. How much garbage is this? :-) Cheers, Daniel
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 3c20af999893..a3a58d8481e9 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS config HDMI bool +config HPD_NOTIFIER + bool + if VT source "drivers/video/console/Kconfig" endif diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 9ad3c17d6456..2697ae5c4166 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -1,5 +1,6 @@ obj-$(CONFIG_VGASTATE) += vgastate.o obj-$(CONFIG_HDMI) += hdmi.o +obj-$(CONFIG_HPD_NOTIFIER) += hpd-notifier.o obj-$(CONFIG_VT) += console/ obj-$(CONFIG_LOGO) += logo/ diff --git a/drivers/video/hpd-notifier.c b/drivers/video/hpd-notifier.c new file mode 100644 index 000000000000..971e823ead00 --- /dev/null +++ b/drivers/video/hpd-notifier.c @@ -0,0 +1,134 @@ +#include <linux/export.h> +#include <linux/hpd-notifier.h> +#include <linux/string.h> +#include <linux/slab.h> +#include <linux/list.h> + +static LIST_HEAD(hpd_notifiers); +static DEFINE_MUTEX(hpd_notifiers_lock); + +struct hpd_notifier *hpd_notifier_get(struct device *dev) +{ + struct hpd_notifier *n; + + mutex_lock(&hpd_notifiers_lock); + list_for_each_entry(n, &hpd_notifiers, head) { + if (n->dev == dev) { + mutex_unlock(&hpd_notifiers_lock); + kref_get(&n->kref); + return n; + } + } + n = kzalloc(sizeof(*n), GFP_KERNEL); + if (!n) + goto unlock; + n->dev = dev; + mutex_init(&n->lock); + BLOCKING_INIT_NOTIFIER_HEAD(&n->notifiers); + kref_init(&n->kref); + list_add_tail(&n->head, &hpd_notifiers); +unlock: + mutex_unlock(&hpd_notifiers_lock); + return n; +} +EXPORT_SYMBOL_GPL(hpd_notifier_get); + +static void hpd_notifier_release(struct kref *kref) +{ + struct hpd_notifier *n = + container_of(kref, struct hpd_notifier, kref); + + list_del(&n->head); + kfree(n->edid); + kfree(n); +} + +void hpd_notifier_put(struct hpd_notifier *n) +{ + mutex_lock(&hpd_notifiers_lock); + kref_put(&n->kref, hpd_notifier_release); + mutex_unlock(&hpd_notifiers_lock); +} +EXPORT_SYMBOL_GPL(hpd_notifier_put); + +int hpd_notifier_register(struct hpd_notifier *n, struct notifier_block *nb) +{ + int ret = blocking_notifier_chain_register(&n->notifiers, nb); + + if (ret) + return ret; + kref_get(&n->kref); + mutex_lock(&n->lock); + if (n->connected) { + blocking_notifier_call_chain(&n->notifiers, HPD_CONNECTED, n); + if (n->edid_size) + blocking_notifier_call_chain(&n->notifiers, HPD_NEW_EDID, n); + if (n->has_eld) + blocking_notifier_call_chain(&n->notifiers, HPD_NEW_ELD, n); + } + mutex_unlock(&n->lock); + return 0; +} +EXPORT_SYMBOL_GPL(hpd_notifier_register); + +int hpd_notifier_unregister(struct hpd_notifier *n, struct notifier_block *nb) +{ + int ret = blocking_notifier_chain_unregister(&n->notifiers, nb); + + if (ret == 0) + hpd_notifier_put(n); + return ret; +} +EXPORT_SYMBOL_GPL(hpd_notifier_unregister); + +void hpd_event_connect(struct hpd_notifier *n) +{ + mutex_lock(&n->lock); + n->connected = true; + blocking_notifier_call_chain(&n->notifiers, HPD_CONNECTED, n); + mutex_unlock(&n->lock); +} +EXPORT_SYMBOL_GPL(hpd_event_connect); + +void hpd_event_disconnect(struct hpd_notifier *n) +{ + mutex_lock(&n->lock); + n->connected = false; + n->has_eld = false; + n->edid_size = 0; + blocking_notifier_call_chain(&n->notifiers, HPD_DISCONNECTED, n); + mutex_unlock(&n->lock); +} +EXPORT_SYMBOL_GPL(hpd_event_disconnect); + +int hpd_event_new_edid(struct hpd_notifier *n, const void *edid, size_t size) +{ + mutex_lock(&n->lock); + if (n->edid_allocated_size < size) { + void *p = kmalloc(size, GFP_KERNEL); + + if (p == NULL) { + mutex_unlock(&n->lock); + return -ENOMEM; + } + kfree(n->edid); + n->edid = p; + n->edid_allocated_size = size; + } + memcpy(n->edid, edid, size); + n->edid_size = size; + blocking_notifier_call_chain(&n->notifiers, HPD_NEW_EDID, n); + mutex_unlock(&n->lock); + return 0; +} +EXPORT_SYMBOL_GPL(hpd_event_new_edid); + +void hpd_event_new_eld(struct hpd_notifier *n, const u8 eld[128]) +{ + mutex_lock(&n->lock); + memcpy(n->eld, eld, sizeof(n->eld)); + n->has_eld = true; + blocking_notifier_call_chain(&n->notifiers, HPD_NEW_ELD, n); + mutex_unlock(&n->lock); +} +EXPORT_SYMBOL_GPL(hpd_event_new_eld); diff --git a/include/linux/hpd-notifier.h b/include/linux/hpd-notifier.h new file mode 100644 index 000000000000..4dcb4515d2b2 --- /dev/null +++ b/include/linux/hpd-notifier.h @@ -0,0 +1,109 @@ +/* + * hpd-notifier.h - notify interested parties of (dis)connect and EDID + * events + * + * Copyright 2016 Russell King <rmk+kernel@arm.linux.org.uk> + * Copyright 2016 Cisco Systems, Inc. and/or its affiliates. All rights reserved. + */ + +#ifndef LINUX_HPD_NOTIFIER_H +#define LINUX_HPD_NOTIFIER_H + +#include <linux/types.h> +#include <linux/notifier.h> +#include <linux/kref.h> + +enum { + HPD_CONNECTED, + HPD_DISCONNECTED, + HPD_NEW_EDID, + HPD_NEW_ELD, +}; + +struct device; + +struct hpd_notifier { + struct mutex lock; + struct list_head head; + struct kref kref; + struct blocking_notifier_head notifiers; + struct device *dev; + + /* Current state */ + bool connected; + bool has_eld; + unsigned char eld[128]; + void *edid; + size_t edid_size; + size_t edid_allocated_size; +}; + +/** + * hpd_notifier_get - find or create a new hpd_notifier for the given device. + * @dev: device that sends the events. + * + * If a notifier for device @dev already exists, then increase the refcount + * and return that notifier. + * + * If it doesn't exist, then allocate a new notifier struct and return a + * pointer to that new struct. + * + * Return NULL if the memory could not be allocated. + */ +struct hpd_notifier *hpd_notifier_get(struct device *dev); + +/** + * hpd_notifier_put - decrease refcount and delete when the refcount reaches 0. + * @n: notifier + */ +void hpd_notifier_put(struct hpd_notifier *n); + +/** + * hpd_notifier_register - register the notifier with the notifier_block. + * @n: the HPD notifier + * @nb: the notifier_block + */ +int hpd_notifier_register(struct hpd_notifier *n, struct notifier_block *nb); + +/** + * hpd_notifier_unregister - unregister the notifier with the notifier_block. + * @n: the HPD notifier + * @nb: the notifier_block + */ +int hpd_notifier_unregister(struct hpd_notifier *n, struct notifier_block *nb); + +/** + * hpd_event_connect - send a connect event. + * @n: the HPD notifier + * + * Send an HPD_CONNECTED event to any registered parties. + */ +void hpd_event_connect(struct hpd_notifier *n); + +/** + * hpd_event_disconnect - send a disconnect event. + * @n: the HPD notifier + * + * Send an HPD_DISCONNECTED event to any registered parties. + */ +void hpd_event_disconnect(struct hpd_notifier *n); + +/** + * hpd_event_new_edid - send a new EDID event. + * @n: the HPD notifier + * + * Send an HPD_NEW_EDID event to any registered parties. + * This function will make a copy the EDID so it can return -ENOMEM if + * no memory could be allocated. + */ +int hpd_event_new_edid(struct hpd_notifier *n, const void *edid, size_t size); + +/** + * hpd_event_new_eld - send a new ELD event. + * @n: the HPD notifier + * + * Send an HPD_NEW_ELD event to any registered parties. + */ +void hpd_event_new_eld(struct hpd_notifier *n, const u8 eld[128]); + +#endif