Re: [PATCH 1/7] video: add HDMI state notifier support

From: Hans Verkuil
Date: Mon Jun 03 2019 - 05:09:26 EST

Next message: Sudeep Holla: "Re: [PATCH v6 2/7] dt-binding: cpu-topology: Move cpu-map to a common binding."
Previous message: Marco Elver: "Re: [PATCH 2/3] x86: Move CPU feature test out of uaccess region"
In reply to: Daniel Vetter: "Re: [PATCH 1/7] video: add HDMI state notifier support"
Next in thread: Daniel Vetter: "Re: [PATCH 1/7] video: add HDMI state notifier support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 6/3/19 10:09 AM, Daniel Vetter wrote:
> On Mon, Jun 03, 2019 at 09:45:49AM +0200, Hans Verkuil wrote:
>> On 6/3/19 6:32 AM, Cheng-Yi Chiang wrote:
>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>
>>> Add support for HDMI hotplug and EDID notifiers, which is used to convey
>>> information from HDMI drivers to their CEC and audio counterparts.
>>>
>>> Based on an earlier version from Russell King:
>>>
>>> https://patchwork.kernel.org/patch/9277043/
>>>
>>> The hdmi_notifier is a reference counted object containing the HDMI state
>>> of an HDMI device.
>>>
>>> When a new notifier is registered the current state will be reported to
>>> that notifier at registration time.
>>>
>>> Based on Hans Verkuil's patch:
>>>
>>> https://patchwork.kernel.org/patch/9472521/
>>
>> Erm, you are aware that this patch morphed into a CEC-specific notifier
>> found in drivers/media/cec/cec-notifier.c?
>>
>> I don't think it makes sense to have two notifier implementations in the kernel.
>> The original intention was to have the notifier deal with both CEC and ASoC
>> notifications, but there was not enough interest for the ASoC bits at the time
>> and it was dropped.
>>
>> I am planning changes to the cec-notifier API, I hope to work on that this
>> week. I'll CC you when I post those. Those might be a good starting point
>> to convert the cec-notifier to an hdmi-notifier as was originally intended.
>>
>> I've added your colleague Dariusz Marcinkiewicz to the CC list since he's been
>> working on some nice cec-notifier improvements as well.
>
> We also have some interfaces for drm/alsa interactions around hdmi
> already in drm/drm_audio_component.h, but it's not used by anything
> outside of i915. Imo we should extend that, not reinvent a new wheel.

If that can be used instead of this hdmi-notifier, then that's fine by me.

> Another note: notifiers considered evil, imo. Gets the job done for one
> case, as soon as you have multiple devices and need to make sure you get
> the update for the right one it all comes crashing down. Please create an
> api which registers for updates from a specific device only, plus
> something that has real callbacks (like the drm_audio_component.h thing we
> started already).

For CEC the notifier works very well. But CEC has some special requirements
that ASoC doesn't have:

- The cec-notifier can be used by both HDMI transmitters and receivers (so
has to work with two different subsystems).

- There may be multiple CEC devices connected to one HDMI transmitter: one
that is used when the system is in Standby, and a more capable CEC device
used when the system is powered up. This isn't supported yet, but it is likely
that we'll need this.

- HDMI and CEC devices are often completely independent and one or the other
(or both) can be unbound at any time. A real-world example is when an FPGA
containing the HDMI and/or CEC support is unloaded to save power when in standby.

- In some cases you want to register a CEC device via a notifier to an HDMI
connector based on userspace information. E.g. the popular USB Pulse-Eight CEC
device can be connected to any HDMI output by the user, there is no way to know
this in the kernel. An application that knows about the Pulse-Eight currently
has to parse the EDID and set the Physical Address of the Pulse-Eight accordingly.
I want to make it possible that the user can just tell the Pulse-Eight which HDMI
output is used and have it connect to that output using the notifier. I have a
proof-of-concept, but this needs Dariusz' series to make it work.

Regards,

Hans

> -Daniel
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>> Modified by Cheng-Yi Chiang:
>>> - Add a section in MAINTAINER.
>>> - Changes connected and has_eld to bitfield of unsigned int.
>>> - Other minor fixes to pass checkpatch.pl --strict checks.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>> Acked-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>>> Signed-off-by: Cheng-Yi Chiang <cychiang@xxxxxxxxxxxx>
>>> ---
>>> The original patch is at
>>> https://lore.kernel.org/linux-arm-kernel/20161213150813.37966-2-hverkuil@xxxxxxxxx
>>>
>>> MAINTAINERS | 6 ++
>>> drivers/video/Kconfig | 3 +
>>> drivers/video/Makefile | 1 +
>>> drivers/video/hdmi-notifier.c | 145 ++++++++++++++++++++++++++++++++++
>>> include/linux/hdmi-notifier.h | 112 ++++++++++++++++++++++++++
>>> 5 files changed, 267 insertions(+)
>>> create mode 100644 drivers/video/hdmi-notifier.c
>>> create mode 100644 include/linux/hdmi-notifier.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 5cfbea4ce575..ffb7376f9509 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -16676,6 +16676,12 @@ W: https://linuxtv.org
>>> S: Maintained
>>> F: drivers/media/platform/vicodec/*
>>>
>>> +VIDEO FRAMEWORK
>>> +M: Hans Verkuil <hverkuil@xxxxxxxxx>
>>> +L: linux-media@xxxxxxxxxxxxxxx
>>> +F: drivers/video/hdmi-notifier.*
>>> +S: Maintained
>>> +
>>> VIDEO MULTIPLEXER DRIVER
>>> M: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>>> L: linux-media@xxxxxxxxxxxxxxx
>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>>> index 83d3d271ca15..000ba9bc0ae7 100644
>>> --- a/drivers/video/Kconfig
>>> +++ b/drivers/video/Kconfig
>>> @@ -34,6 +34,9 @@ config VIDEOMODE_HELPERS
>>> config HDMI
>>> bool
>>>
>>> +config HDMI_NOTIFIERS
>>> + bool
>>> +
>>> endif # HAS_IOMEM
>>>
>>> if VT
>>> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
>>> index df7650adede9..eff4736102ca 100644
>>> --- a/drivers/video/Makefile
>>> +++ b/drivers/video/Makefile
>>> @@ -1,6 +1,7 @@
>>> # SPDX-License-Identifier: GPL-2.0
>>> obj-$(CONFIG_VGASTATE) += vgastate.o
>>> obj-$(CONFIG_HDMI) += hdmi.o
>>> +obj-$(CONFIG_HDMI_NOTIFIERS) += hdmi-notifier.o
>>>
>>> obj-$(CONFIG_VT) += console/
>>> obj-$(CONFIG_FB_STI) += console/
>>> diff --git a/drivers/video/hdmi-notifier.c b/drivers/video/hdmi-notifier.c
>>> new file mode 100644
>>> index 000000000000..d1eedf661648
>>> --- /dev/null
>>> +++ b/drivers/video/hdmi-notifier.c
>>> @@ -0,0 +1,145 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* hdmi-notifier.c - 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.
>>> + */
>>> +
>>> +#include <linux/export.h>
>>> +#include <linux/hdmi-notifier.h>
>>> +#include <linux/string.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/list.h>
>>> +
>>> +static LIST_HEAD(hdmi_notifiers);
>>> +static DEFINE_MUTEX(hdmi_notifiers_lock);
>>> +
>>> +struct hdmi_notifier *hdmi_notifier_get(struct device *dev)
>>> +{
>>> + struct hdmi_notifier *n;
>>> +
>>> + mutex_lock(&hdmi_notifiers_lock);
>>> + list_for_each_entry(n, &hdmi_notifiers, head) {
>>> + if (n->dev == dev) {
>>> + mutex_unlock(&hdmi_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, &hdmi_notifiers);
>>> +unlock:
>>> + mutex_unlock(&hdmi_notifiers_lock);
>>> + return n;
>>> +}
>>> +EXPORT_SYMBOL_GPL(hdmi_notifier_get);
>>> +
>>> +static void hdmi_notifier_release(struct kref *kref)
>>> +{
>>> + struct hdmi_notifier *n =
>>> + container_of(kref, struct hdmi_notifier, kref);
>>> +
>>> + mutex_lock(&hdmi_notifiers_lock);
>>> + list_del(&n->head);
>>> + mutex_unlock(&hdmi_notifiers_lock);
>>> + kfree(n->edid);
>>> + kfree(n);
>>> +}
>>> +
>>> +void hdmi_notifier_put(struct hdmi_notifier *n)
>>> +{
>>> + kref_put(&n->kref, hdmi_notifier_release);
>>> +}
>>> +EXPORT_SYMBOL_GPL(hdmi_notifier_put);
>>> +
>>> +int hdmi_notifier_register(struct hdmi_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, HDMI_CONNECTED, n);
>>> + if (n->edid_size)
>>> + blocking_notifier_call_chain(&n->notifiers,
>>> + HDMI_NEW_EDID, n);
>>> + if (n->has_eld)
>>> + blocking_notifier_call_chain(&n->notifiers,
>>> + HDMI_NEW_ELD, n);
>>> + }
>>> + mutex_unlock(&n->lock);
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(hdmi_notifier_register);
>>> +
>>> +int hdmi_notifier_unregister(struct hdmi_notifier *n, struct notifier_block *nb)
>>> +{
>>> + int ret = blocking_notifier_chain_unregister(&n->notifiers, nb);
>>> +
>>> + if (ret == 0)
>>> + hdmi_notifier_put(n);
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(hdmi_notifier_unregister);
>>> +
>>> +void hdmi_event_connect(struct hdmi_notifier *n)
>>> +{
>>> + mutex_lock(&n->lock);
>>> + n->connected = true;
>>> + blocking_notifier_call_chain(&n->notifiers, HDMI_CONNECTED, n);
>>> + mutex_unlock(&n->lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(hdmi_event_connect);
>>> +
>>> +void hdmi_event_disconnect(struct hdmi_notifier *n)
>>> +{
>>> + mutex_lock(&n->lock);
>>> + n->connected = false;
>>> + n->has_eld = false;
>>> + n->edid_size = 0;
>>> + blocking_notifier_call_chain(&n->notifiers, HDMI_DISCONNECTED, n);
>>> + mutex_unlock(&n->lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(hdmi_event_disconnect);
>>> +
>>> +int hdmi_event_new_edid(struct hdmi_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) {
>>> + 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, HDMI_NEW_EDID, n);
>>> + mutex_unlock(&n->lock);
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(hdmi_event_new_edid);
>>> +
>>> +void hdmi_event_new_eld(struct hdmi_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, HDMI_NEW_ELD, n);
>>> + mutex_unlock(&n->lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(hdmi_event_new_eld);
>>> diff --git a/include/linux/hdmi-notifier.h b/include/linux/hdmi-notifier.h
>>> new file mode 100644
>>> index 000000000000..c8f35110e3e3
>>> --- /dev/null
>>> +++ b/include/linux/hdmi-notifier.h
>>> @@ -0,0 +1,112 @@
>>> +/* SPDX-License-Identifier: GPL-2.0
>>> + * hdmi-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_HDMI_NOTIFIER_H
>>> +#define LINUX_HDMI_NOTIFIER_H
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/notifier.h>
>>> +#include <linux/kref.h>
>>> +
>>> +enum {
>>> + HDMI_CONNECTED,
>>> + HDMI_DISCONNECTED,
>>> + HDMI_NEW_EDID,
>>> + HDMI_NEW_ELD,
>>> +};
>>> +
>>> +struct device;
>>> +
>>> +struct hdmi_notifier {
>>> + /* Lock to protect callback registration and notification. */
>>> + struct mutex lock;
>>> + struct list_head head;
>>> + struct kref kref;
>>> + struct blocking_notifier_head notifiers;
>>> + struct device *dev;
>>> +
>>> + /* Current state */
>>> + unsigned int connected : 1;
>>> + unsigned int has_eld : 1;
>>> + unsigned char eld[128];
>>> + void *edid;
>>> + size_t edid_size;
>>> + size_t edid_allocated_size;
>>> +};
>>> +
>>> +/**
>>> + * hdmi_notifier_get - find or create a new hdmi_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 hdmi_notifier *hdmi_notifier_get(struct device *dev);
>>> +
>>> +/**
>>> + * hdmi_notifier_put - decrease refcount and delete when the refcount reaches 0.
>>> + * @n: notifier
>>> + */
>>> +void hdmi_notifier_put(struct hdmi_notifier *n);
>>> +
>>> +/**
>>> + * hdmi_notifier_register - register the notifier with the notifier_block.
>>> + * @n: the HDMI notifier
>>> + * @nb: the notifier_block
>>> + */
>>> +int hdmi_notifier_register(struct hdmi_notifier *n, struct notifier_block *nb);
>>> +
>>> +/**
>>> + * hdmi_notifier_unregister - unregister the notifier with the notifier_block.
>>> + * @n: the HDMI notifier
>>> + * @nb: the notifier_block
>>> + */
>>> +int hdmi_notifier_unregister(struct hdmi_notifier *n,
>>> + struct notifier_block *nb);
>>> +
>>> +/**
>>> + * hdmi_event_connect - send a connect event.
>>> + * @n: the HDMI notifier
>>> + *
>>> + * Send an HDMI_CONNECTED event to any registered parties.
>>> + */
>>> +void hdmi_event_connect(struct hdmi_notifier *n);
>>> +
>>> +/**
>>> + * hdmi_event_disconnect - send a disconnect event.
>>> + * @n: the HDMI notifier
>>> + *
>>> + * Send an HDMI_DISCONNECTED event to any registered parties.
>>> + */
>>> +void hdmi_event_disconnect(struct hdmi_notifier *n);
>>> +
>>> +/**
>>> + * hdmi_event_new_edid - send a new EDID event.
>>> + * @n: the HDMI notifier
>>> + *
>>> + * Send an HDMI_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 hdmi_event_new_edid(struct hdmi_notifier *n, const void *edid, size_t size);
>>> +
>>> +/**
>>> + * hdmi_event_new_eld - send a new ELD event.
>>> + * @n: the HDMI notifier
>>> + *
>>> + * Send an HDMI_NEW_ELD event to any registered parties.
>>> + */
>>> +void hdmi_event_new_eld(struct hdmi_notifier *n, const u8 eld[128]);
>>> +
>>> +#endif
>>>
>>
>

Next message: Sudeep Holla: "Re: [PATCH v6 2/7] dt-binding: cpu-topology: Move cpu-map to a common binding."
Previous message: Marco Elver: "Re: [PATCH 2/3] x86: Move CPU feature test out of uaccess region"
In reply to: Daniel Vetter: "Re: [PATCH 1/7] video: add HDMI state notifier support"
Next in thread: Daniel Vetter: "Re: [PATCH 1/7] video: add HDMI state notifier support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]