| Message ID | 1470907227-899-2-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 11, 2016 at 11:20:23AM +0200, Philipp Zabel wrote: > This is Russell's HDMI notification prototype [1], currently waiting > for the HDMI CEC situation to resolve. > > The use case for the notifications on MediaTek MT8173 is to let the > (dis)connection notifications control an ALSA jack object. > > No Signed-off-by since this is not my code, and still up for discussion. Well, I have two drivers (both CEC drivers) which use this, and I still don't see any alternative solution to the problem that this patch is solving. I don't think it's really a CEC problem - there's three bits to HDMI that need to track each others state - the video, audio and CEC paths. While the video and audio paths may be one block, the CEC path may actually be a separate block. For example, the TDA998x devices integrate the HDMI video/audio block along with a TDA9950 on the same device - the TDA9950 being a CEC engine. The TDA9950 is also available as a separate device, and even when integrated with HDMI, it appears on the I2C bus as a seperate device. So, splitting the functionality is definitely the right model. We just need some way to keep each blocks state in sync. What's provided in this patch is the simple solution which seems to work for the use cases we have. I think, in light of no comments against this approach, and no other approach being available, that this is good enough justification to merge this, especially as it is blocking other work. So... if people want to give me reviewed-by/acked-bys, I'll add them to my patch, and I'll post that and the dw-hdmi and tda9950 CEC drivers.
On Thu, Aug 11, 2016 at 12:30:03PM +0200, Hans Verkuil wrote: > Hi Russell, > > I like this approach and it is something I will need for other CEC drivers > (not yet merged) as well. > > Just a few comments: > > On 08/11/16 11:20, Philipp Zabel wrote: > > This is Russell's HDMI notification prototype [1], currently waiting > > for the HDMI CEC situation to resolve. > > > > The use case for the notifications on MediaTek MT8173 is to let the > > (dis)connection notifications control an ALSA jack object. > > > > No Signed-off-by since this is not my code, and still up for discussion. > > > > [1] https://patchwork.kernel.org/patch/8351501/ > > --- > > drivers/video/Kconfig | 3 +++ > > drivers/video/Makefile | 1 + > > drivers/video/hdmi-notifier.c | 61 +++++++++++++++++++++++++++++++++++++++++++ > > include/linux/hdmi-notifier.h | 44 +++++++++++++++++++++++++++++++ > > 4 files changed, 109 insertions(+) > > create mode 100644 drivers/video/hdmi-notifier.c > > create mode 100644 include/linux/hdmi-notifier.h > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig > > index 3c20af9..1ee7b9f 100644 > > --- a/drivers/video/Kconfig > > +++ b/drivers/video/Kconfig > > @@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS > > config HDMI > > bool > > > > +config HDMI_NOTIFIERS > > + bool > > + > > if VT > > source "drivers/video/console/Kconfig" > > endif > > diff --git a/drivers/video/Makefile b/drivers/video/Makefile > > index 9ad3c17..65f5649 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_HDMI_NOTIFIERS) += hdmi-notifier.o > > > > obj-$(CONFIG_VT) += console/ > > obj-$(CONFIG_LOGO) += logo/ > > diff --git a/drivers/video/hdmi-notifier.c b/drivers/video/hdmi-notifier.c > > new file mode 100644 > > index 0000000..a233359 > > --- /dev/null > > +++ b/drivers/video/hdmi-notifier.c > > @@ -0,0 +1,61 @@ > > +#include <linux/export.h> > > +#include <linux/hdmi-notifier.h> > > +#include <linux/notifier.h> > > +#include <linux/string.h> > > + > > +static BLOCKING_NOTIFIER_HEAD(hdmi_notifier); > > + > > +int hdmi_register_notifier(struct notifier_block *nb) > > +{ > > + return blocking_notifier_chain_register(&hdmi_notifier, nb); > > +} > > +EXPORT_SYMBOL_GPL(hdmi_register_notifier); > > + > > +int hdmi_unregister_notifier(struct notifier_block *nb) > > +{ > > + return blocking_notifier_chain_unregister(&hdmi_notifier, nb); > > +} > > +EXPORT_SYMBOL_GPL(hdmi_unregister_notifier); > > + > > +void hdmi_event_connect(struct device *dev) > > +{ > > + struct hdmi_event_base base; > > + > > + base.source = dev; > > + > > + blocking_notifier_call_chain(&hdmi_notifier, HDMI_CONNECTED, &base); > > +} > > +EXPORT_SYMBOL_GPL(hdmi_event_connect); > > + > > +void hdmi_event_disconnect(struct device *dev) > > +{ > > + struct hdmi_event_base base; > > + > > + base.source = dev; > > + > > + blocking_notifier_call_chain(&hdmi_notifier, HDMI_DISCONNECTED, &base); > > +} > > +EXPORT_SYMBOL_GPL(hdmi_event_disconnect); > > + > > +void hdmi_event_new_edid(struct device *dev, const void *edid, size_t size) > > I would prefer const u8 *edid. It is after all just a u8 array and not some > opaque data structure. In DRM, the edid is of type "struct edid" (defined in include/drm/drm_edid.h). So, making it "const u8 *" means that all DRM drivers will have to explicitly cast it - not something I'm in favour of. > > +{ > > + struct hdmi_event_new_edid new_edid; > > + > > + new_edid.base.source = dev; > > + new_edid.edid = edid; > > + new_edid.size = size; > > + > > + blocking_notifier_call_chain(&hdmi_notifier, HDMI_NEW_EDID, &new_edid); > > +} > > +EXPORT_SYMBOL_GPL(hdmi_event_new_edid); > > + > > +void hdmi_event_new_eld(struct device *dev, const void *eld) > > Stupid question: what does ELD stand for? I've no idea... > > And is void the right choice here or should it also be u8? The ELD stands for EDID-like data. See information on HDA039. It's a method of conveying the EDID data specific to audio drivers to them without having the complexities of parsing the full EDID each time. It's also exported to userland so that userland can determine the capabilities of the audio path, again, without having to have full EDID parsers and exporting the full EDID data block through audio drivers.
On Thu, Aug 11, 2016 at 12:49:21PM +0200, Hans Verkuil wrote: > On 08/11/16 12:39, Russell King - ARM Linux wrote: > > On Thu, Aug 11, 2016 at 12:30:03PM +0200, Hans Verkuil wrote: > >> Hi Russell, > >> > >> I like this approach and it is something I will need for other CEC drivers > >> (not yet merged) as well. > >> > >> Just a few comments: > >> > >> On 08/11/16 11:20, Philipp Zabel wrote: > >>> This is Russell's HDMI notification prototype [1], currently waiting > >>> for the HDMI CEC situation to resolve. > >>> > >>> The use case for the notifications on MediaTek MT8173 is to let the > >>> (dis)connection notifications control an ALSA jack object. > >>> > >>> No Signed-off-by since this is not my code, and still up for discussion. > >>> > >>> [1] https://patchwork.kernel.org/patch/8351501/ > >>> --- > >>> drivers/video/Kconfig | 3 +++ > >>> drivers/video/Makefile | 1 + > >>> drivers/video/hdmi-notifier.c | 61 +++++++++++++++++++++++++++++++++++++++++++ > >>> include/linux/hdmi-notifier.h | 44 +++++++++++++++++++++++++++++++ > >>> 4 files changed, 109 insertions(+) > >>> create mode 100644 drivers/video/hdmi-notifier.c > >>> create mode 100644 include/linux/hdmi-notifier.h > >>> > >>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig > >>> index 3c20af9..1ee7b9f 100644 > >>> --- a/drivers/video/Kconfig > >>> +++ b/drivers/video/Kconfig > >>> @@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS > >>> config HDMI > >>> bool > >>> > >>> +config HDMI_NOTIFIERS > >>> + bool > >>> + > >>> if VT > >>> source "drivers/video/console/Kconfig" > >>> endif > >>> diff --git a/drivers/video/Makefile b/drivers/video/Makefile > >>> index 9ad3c17..65f5649 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_HDMI_NOTIFIERS) += hdmi-notifier.o > >>> > >>> obj-$(CONFIG_VT) += console/ > >>> obj-$(CONFIG_LOGO) += logo/ > >>> diff --git a/drivers/video/hdmi-notifier.c b/drivers/video/hdmi-notifier.c > >>> new file mode 100644 > >>> index 0000000..a233359 > >>> --- /dev/null > >>> +++ b/drivers/video/hdmi-notifier.c > >>> @@ -0,0 +1,61 @@ > >>> +#include <linux/export.h> > >>> +#include <linux/hdmi-notifier.h> > >>> +#include <linux/notifier.h> > >>> +#include <linux/string.h> > >>> + > >>> +static BLOCKING_NOTIFIER_HEAD(hdmi_notifier); > >>> + > >>> +int hdmi_register_notifier(struct notifier_block *nb) > >>> +{ > >>> + return blocking_notifier_chain_register(&hdmi_notifier, nb); > >>> +} > >>> +EXPORT_SYMBOL_GPL(hdmi_register_notifier); > >>> + > >>> +int hdmi_unregister_notifier(struct notifier_block *nb) > >>> +{ > >>> + return blocking_notifier_chain_unregister(&hdmi_notifier, nb); > >>> +} > >>> +EXPORT_SYMBOL_GPL(hdmi_unregister_notifier); > >>> + > >>> +void hdmi_event_connect(struct device *dev) > >>> +{ > >>> + struct hdmi_event_base base; > >>> + > >>> + base.source = dev; > >>> + > >>> + blocking_notifier_call_chain(&hdmi_notifier, HDMI_CONNECTED, &base); > >>> +} > >>> +EXPORT_SYMBOL_GPL(hdmi_event_connect); > >>> + > >>> +void hdmi_event_disconnect(struct device *dev) > >>> +{ > >>> + struct hdmi_event_base base; > >>> + > >>> + base.source = dev; > >>> + > >>> + blocking_notifier_call_chain(&hdmi_notifier, HDMI_DISCONNECTED, &base); > >>> +} > >>> +EXPORT_SYMBOL_GPL(hdmi_event_disconnect); > >>> + > >>> +void hdmi_event_new_edid(struct device *dev, const void *edid, size_t size) > >> > >> I would prefer const u8 *edid. It is after all just a u8 array and not some > >> opaque data structure. > > > > In DRM, the edid is of type "struct edid" (defined in > > include/drm/drm_edid.h). So, making it "const u8 *" means that all > > DRM drivers will have to explicitly cast it - not something I'm in > > favour of. > > I thought this was the raw EDID data. But if you pass a struct edid around, > then why not make this const struct edid *edid? I fail to see why you would > want to use a void pointer here. struct edid is a DRM thing - it's not generic. Using struct edid here would force everyone to use the DRM structure, whereas other subsystems use, eg, unsigned char. > Also struct edid is for the first EDID block only, whereas CEC drivers need > the CEA-861 block to get the physical address. That may be, but DRM parses the HDMI vendor block too while still using struct edid to represent it. See drm_find_edid_extension() in drivers/gpu/drm/drm_edid.c This is just how DRM is... I'm not sure what the design decisions were that came up with this, but it's what we have, and I'm not sure whether DRM would be open to changing it. > > The ELD stands for EDID-like data. See information on HDA039. It's a > > method of conveying the EDID data specific to audio drivers to them > > without having the complexities of parsing the full EDID each time. > > It's also exported to userland so that userland can determine the > > capabilities of the audio path, again, without having to have full > > EDID parsers and exporting the full EDID data block through audio > > drivers. > > > > OK. Since eld is an unsigned char array I would suggest changing void to > unsigned char here. > > Or perhaps const unsigned char eld[128]. Yes, I'll make that change.
Am Donnerstag, den 11.08.2016, 11:18 +0100 schrieb Russell King - ARM Linux: > On Thu, Aug 11, 2016 at 11:20:23AM +0200, Philipp Zabel wrote: > > This is Russell's HDMI notification prototype [1], currently waiting > > for the HDMI CEC situation to resolve. > > > > The use case for the notifications on MediaTek MT8173 is to let the > > (dis)connection notifications control an ALSA jack object. > > > > No Signed-off-by since this is not my code, and still up for discussion. > > Well, I have two drivers (both CEC drivers) which use this, and I still > don't see any alternative solution to the problem that this patch is > solving. > > I don't think it's really a CEC problem - there's three bits to HDMI > that need to track each others state - the video, audio and CEC paths. > > While the video and audio paths may be one block, the CEC path may > actually be a separate block. For example, the TDA998x devices > integrate the HDMI video/audio block along with a TDA9950 on the > same device - the TDA9950 being a CEC engine. The TDA9950 is also > available as a separate device, and even when integrated with HDMI, > it appears on the I2C bus as a seperate device. > > So, splitting the functionality is definitely the right model. We > just need some way to keep each blocks state in sync. What's provided > in this patch is the simple solution which seems to work for the use > cases we have. Yes, it works fine for the MT8173 use case. > I think, in light of no comments against this approach, and no other > approach being available, that this is good enough justification to > merge this, especially as it is blocking other work. > > So... if people want to give me reviewed-by/acked-bys, I'll add them > to my patch, and I'll post that and the dw-hdmi and tda9950 CEC drivers. Acked-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
On Thu, Aug 11, 2016 at 04:49:28PM +0200, Hans Verkuil wrote: > On 08/11/16 16:16, Russell King - ARM Linux wrote: > > On Thu, Aug 11, 2016 at 12:49:21PM +0200, Hans Verkuil wrote: > >> I thought this was the raw EDID data. But if you pass a struct edid around, > >> then why not make this const struct edid *edid? I fail to see why you would > >> want to use a void pointer here. > > > > struct edid is a DRM thing - it's not generic. Using struct edid here > > would force everyone to use the DRM structure, whereas other subsystems > > use, eg, unsigned char. > > So how the edid pointer should be interpreted depends on which subsystem > sends it? That doesn't sound right. It makes it really hard for the drivers > receiving the notifications. No, that's wrong... and I really don't see why you're trying to make a meal of this. In both cases, it's the EDID as received from the device. One subsystem happens to represent it internally as a "struct edid" in its drivers, another subsystem uses "unsigned char []". In order to _cleanly_ accept both styles, without having to resort to driver authors needing to litter their code with lots of idiotic casts, I'm accepting a void * pointer, so that we can accept _both_ a struct edid and an unsigned char array. Both are compatible with each other. Both have the same bytes at the same offset. There is no difference between them other than the C types that are used by each subsystem. > But I understand that currently the main purpose of the hdmi_event_new_edid > is to get the physical address from the EDID for CEC? Or do you have other > uses as well? That is one use, but if an audio driver wants to parse the raw EDID instead of the ELD, it can. What I want to avoid is having the video driver parse lots of different bits from the EDID and then send lots of events "just in case" something wants some of the parsed information - it's much more efficient to give drivers the raw information and allow them to parse what they need from the EDID. I'm not convinced that parsing out the physical address, and then passing that from the video side is a good idea. The reason I made an exception for the ELD is because (a) it's already passed between video and audio drivers, (b) the code we have present to convert an EDID to ELD is in DRM, and requires DRM data structures which won't be available in an audio driver. If you want a mechanism to pass just the physical address, we could add it, and require CEC drivers to make use of both the EDID or physical address events. That would mean situations where the physical address is known to be fixed before hand can be easily catered for.
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 3c20af9..1ee7b9f 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS config HDMI bool +config HDMI_NOTIFIERS + bool + if VT source "drivers/video/console/Kconfig" endif diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 9ad3c17..65f5649 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_HDMI_NOTIFIERS) += hdmi-notifier.o obj-$(CONFIG_VT) += console/ obj-$(CONFIG_LOGO) += logo/ diff --git a/drivers/video/hdmi-notifier.c b/drivers/video/hdmi-notifier.c new file mode 100644 index 0000000..a233359 --- /dev/null +++ b/drivers/video/hdmi-notifier.c @@ -0,0 +1,61 @@ +#include <linux/export.h> +#include <linux/hdmi-notifier.h> +#include <linux/notifier.h> +#include <linux/string.h> + +static BLOCKING_NOTIFIER_HEAD(hdmi_notifier); + +int hdmi_register_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&hdmi_notifier, nb); +} +EXPORT_SYMBOL_GPL(hdmi_register_notifier); + +int hdmi_unregister_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&hdmi_notifier, nb); +} +EXPORT_SYMBOL_GPL(hdmi_unregister_notifier); + +void hdmi_event_connect(struct device *dev) +{ + struct hdmi_event_base base; + + base.source = dev; + + blocking_notifier_call_chain(&hdmi_notifier, HDMI_CONNECTED, &base); +} +EXPORT_SYMBOL_GPL(hdmi_event_connect); + +void hdmi_event_disconnect(struct device *dev) +{ + struct hdmi_event_base base; + + base.source = dev; + + blocking_notifier_call_chain(&hdmi_notifier, HDMI_DISCONNECTED, &base); +} +EXPORT_SYMBOL_GPL(hdmi_event_disconnect); + +void hdmi_event_new_edid(struct device *dev, const void *edid, size_t size) +{ + struct hdmi_event_new_edid new_edid; + + new_edid.base.source = dev; + new_edid.edid = edid; + new_edid.size = size; + + blocking_notifier_call_chain(&hdmi_notifier, HDMI_NEW_EDID, &new_edid); +} +EXPORT_SYMBOL_GPL(hdmi_event_new_edid); + +void hdmi_event_new_eld(struct device *dev, const void *eld) +{ + struct hdmi_event_new_eld new_eld; + + new_eld.base.source = dev; + memcpy(new_eld.eld, eld, sizeof(new_eld.eld)); + + blocking_notifier_call_chain(&hdmi_notifier, HDMI_NEW_ELD, &new_eld); +} +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 0000000..9c5ad49 --- /dev/null +++ b/include/linux/hdmi-notifier.h @@ -0,0 +1,44 @@ +#ifndef LINUX_HDMI_NOTIFIER_H +#define LINUX_HDMI_NOTIFIER_H + +#include <linux/types.h> + +enum { + HDMI_CONNECTED, + HDMI_DISCONNECTED, + HDMI_NEW_EDID, + HDMI_NEW_ELD, +}; + +struct hdmi_event_base { + struct device *source; +}; + +struct hdmi_event_new_edid { + struct hdmi_event_base base; + const void *edid; + size_t size; +}; + +struct hdmi_event_new_eld { + struct hdmi_event_base base; + unsigned char eld[128]; +}; + +union hdmi_event { + struct hdmi_event_base base; + struct hdmi_event_new_edid edid; + struct hdmi_event_new_eld eld; +}; + +struct notifier_block; + +int hdmi_register_notifier(struct notifier_block *nb); +int hdmi_unregister_notifier(struct notifier_block *nb); + +void hdmi_event_connect(struct device *dev); +void hdmi_event_disconnect(struct device *dev); +void hdmi_event_new_edid(struct device *dev, const void *edid, size_t size); +void hdmi_event_new_eld(struct device *dev, const void *eld); + +#endif