On Mon, Feb 06, 2017 at 11:29:43AM +0100, Hans Verkuil wrote: > From: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > 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@xxxxxxxxx> > Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> > + * 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
