[PATCH v8 1/5] video: rmk's HDMI notification prototype

Thu Aug 11 07:49:28 PDT 2016

On 08/11/16 16:16, Russell King - ARM Linux wrote:
> 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.

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.

I am aware of only two subsystems that deal with EDIDs: drm and v4l2 (well,
fbdev as well, but I assume we can ignore that one).

drm uses struct edid, v4l2 just a u8 array. Receivers typically do not need
to decode an EDID, so that's why no effort went into decoding the array on
the v4l2 side. This might change, though. Making struct edid usable in v4l2
would be nice going forward.

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?

If it is only the physical address, then I would just make a simple event
that passes that on as a u16 value. This can then be used by both drm and
v4l2.

Note that v4l2 primarily deals with video receivers, and the EDID for
receivers is typically only set once on boot and not changed. So an
hdmi_event_new_phys_addr call would usually only be made once when the
EDID is loaded.

> 
>> 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.
> 

Regards,

	Hans