Re: [PATCH v6 06/11] cec: add HDMI CEC framework
Hi Sean,
I'm taking over this patch series from Kamil for the time being with his
permission (he's switching jobs and moving house so he can't spend any time
on this for a while).
On 05/13/15 13:10, Sean Young wrote:
On Mon, May 04, 2015 at 07:32:59PM +0200, Kamil Debski wrote:
From: Hans Verkuil hansv...@cisco.com
The added HDMI CEC framework provides a generic kernel interface for
HDMI CEC devices.
Signed-off-by: Hans Verkuil hansv...@cisco.com
-snip-
+int cec_create_adapter(struct cec_adapter *adap, const char *name, u32 caps)
+{
+int res = 0;
+
+adap-state = CEC_ADAP_STATE_DISABLED;
+adap-name = name;
+adap-phys_addr = 0x;
+adap-capabilities = caps;
+adap-version = CEC_VERSION_1_4;
+adap-sequence = 0;
+mutex_init(adap-lock);
+adap-kthread = kthread_run(cec_thread_func, adap, name);
+init_waitqueue_head(adap-kthread_waitq);
+init_waitqueue_head(adap-waitq);
+if (IS_ERR(adap-kthread)) {
+pr_err(cec-%s: kernel_thread() failed\n, name);
+return PTR_ERR(adap-kthread);
+}
+if (caps) {
+res = cec_devnode_register(adap-devnode, adap-owner);
+if (res)
+kthread_stop(adap-kthread);
+}
+adap-recv_notifier = cec_receive_notify;
+
+/* Prepare the RC input device */
+adap-rc = rc_allocate_device();
+if (!adap-rc) {
+pr_err(cec-%s: failed to allocate memory for rc_dev\n, name);
+cec_devnode_unregister(adap-devnode);
+kthread_stop(adap-kthread);
+return -ENOMEM;
+}
+
+snprintf(adap-input_name, sizeof(adap-input_name), RC for %s, name);
+snprintf(adap-input_phys, sizeof(adap-input_phys), %s/input0, name);
+strncpy(adap-input_drv, name, sizeof(adap-input_drv));
+
+adap-rc-input_name = adap-input_name;
+adap-rc-input_phys = adap-input_phys;
+adap-rc-dev.parent = adap-devnode.dev;
+adap-rc-driver_name = adap-input_drv;
+adap-rc-driver_type = RC_DRIVER_CEC;
+adap-rc-allowed_protocols = RC_BIT_CEC;
+adap-rc-priv = adap;
+adap-rc-map_name = RC_MAP_CEC;
+adap-rc-timeout = MS_TO_NS(100);
+
rc-input_id is not populated. It would be nice if input_phys has some
resemblance to a physical path (like the output of usb_make_path() if it
is a usb device).
I've added a BUS_CEC type, the version field can probably get the CEC version
used, but the vendor/product IDs are difficult: there isn't a product ID in
the CEC protocol, but there is a 24-bit vendor ID. I'm wondering whether I
should just put the top 8 bits of the vendor ID in the vendor field and the
remaining 16 in the product field. That way the combination of the two will be
unique.
What do you think?
+res = rc_register_device(adap-rc);
+
+if (res) {
+pr_err(cec-%s: failed to prepare input device\n, name);
+cec_devnode_unregister(adap-devnode);
+rc_free_device(adap-rc);
+kthread_stop(adap-kthread);
+}
+
+return res;
+}
+EXPORT_SYMBOL_GPL(cec_create_adapter);
+
+void cec_delete_adapter(struct cec_adapter *adap)
+{
+if (adap-kthread == NULL)
+return;
+kthread_stop(adap-kthread);
+if (adap-kthread_config)
+kthread_stop(adap-kthread_config);
+adap-state = CEC_ADAP_STATE_DISABLED;
+if (cec_devnode_is_registered(adap-devnode))
+cec_devnode_unregister(adap-devnode);
I think you're missing a rc_unregister_device() here.
Yes indeed. Added.
Regards,
Hans
+}
+EXPORT_SYMBOL_GPL(cec_delete_adapter);
Sean
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 06/11] cec: add HDMI CEC framework
On Mon, May 04, 2015 at 07:32:59PM +0200, Kamil Debski wrote:
From: Hans Verkuil hansv...@cisco.com
The added HDMI CEC framework provides a generic kernel interface for
HDMI CEC devices.
Signed-off-by: Hans Verkuil hansv...@cisco.com
-snip-
+int cec_create_adapter(struct cec_adapter *adap, const char *name, u32 caps)
+{
+ int res = 0;
+
+ adap-state = CEC_ADAP_STATE_DISABLED;
+ adap-name = name;
+ adap-phys_addr = 0x;
+ adap-capabilities = caps;
+ adap-version = CEC_VERSION_1_4;
+ adap-sequence = 0;
+ mutex_init(adap-lock);
+ adap-kthread = kthread_run(cec_thread_func, adap, name);
+ init_waitqueue_head(adap-kthread_waitq);
+ init_waitqueue_head(adap-waitq);
+ if (IS_ERR(adap-kthread)) {
+ pr_err(cec-%s: kernel_thread() failed\n, name);
+ return PTR_ERR(adap-kthread);
+ }
+ if (caps) {
+ res = cec_devnode_register(adap-devnode, adap-owner);
+ if (res)
+ kthread_stop(adap-kthread);
+ }
+ adap-recv_notifier = cec_receive_notify;
+
+ /* Prepare the RC input device */
+ adap-rc = rc_allocate_device();
+ if (!adap-rc) {
+ pr_err(cec-%s: failed to allocate memory for rc_dev\n, name);
+ cec_devnode_unregister(adap-devnode);
+ kthread_stop(adap-kthread);
+ return -ENOMEM;
+ }
+
+ snprintf(adap-input_name, sizeof(adap-input_name), RC for %s, name);
+ snprintf(adap-input_phys, sizeof(adap-input_phys), %s/input0, name);
+ strncpy(adap-input_drv, name, sizeof(adap-input_drv));
+
+ adap-rc-input_name = adap-input_name;
+ adap-rc-input_phys = adap-input_phys;
+ adap-rc-dev.parent = adap-devnode.dev;
+ adap-rc-driver_name = adap-input_drv;
+ adap-rc-driver_type = RC_DRIVER_CEC;
+ adap-rc-allowed_protocols = RC_BIT_CEC;
+ adap-rc-priv = adap;
+ adap-rc-map_name = RC_MAP_CEC;
+ adap-rc-timeout = MS_TO_NS(100);
+
rc-input_id is not populated. It would be nice if input_phys has some
resemblance to a physical path (like the output of usb_make_path() if it
is a usb device).
+ res = rc_register_device(adap-rc);
+
+ if (res) {
+ pr_err(cec-%s: failed to prepare input device\n, name);
+ cec_devnode_unregister(adap-devnode);
+ rc_free_device(adap-rc);
+ kthread_stop(adap-kthread);
+ }
+
+ return res;
+}
+EXPORT_SYMBOL_GPL(cec_create_adapter);
+
+void cec_delete_adapter(struct cec_adapter *adap)
+{
+ if (adap-kthread == NULL)
+ return;
+ kthread_stop(adap-kthread);
+ if (adap-kthread_config)
+ kthread_stop(adap-kthread_config);
+ adap-state = CEC_ADAP_STATE_DISABLED;
+ if (cec_devnode_is_registered(adap-devnode))
+ cec_devnode_unregister(adap-devnode);
I think you're missing a rc_unregister_device() here.
+}
+EXPORT_SYMBOL_GPL(cec_delete_adapter);
Sean
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 06/11] cec: add HDMI CEC framework
Typo and question:
On 05/04/15 19:32, Kamil Debski wrote:
+static long cec_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+ struct cec_devnode *cecdev = cec_devnode_data(filp);
+ struct cec_adapter *adap = to_cec_adapter(cecdev);
+ void __user *parg = (void __user *)arg;
+ int err;
+
+ if (!cec_devnode_is_registered(cecdev))
+ return -EIO;
+
+ switch (cmd) {
snip
+ case CEC_G_ADAP_STATE: {
+ u32 state = adap-state != CEC_ADAP_STATE_DISABLED;
+
+ if (copy_to_user(parg, state, sizeof(state)))
+ return -EFAULT;
+ break;
+ }
+
+ case CEC_S_ADAP_STATE: {
+ u32 state;
+
+ if (!(adap-capabilities CEC_CAP_STATE))
+ return -ENOTTY;
+ if (copy_from_user(state, parg, sizeof(state)))
+ return -EFAULT;
+ if (!state adap-state == CEC_ADAP_STATE_DISABLED)
+ return 0;
+ if (state adap-state != CEC_ADAP_STATE_DISABLED)
+ return 0;
+ cec_enable(adap, !!state);
+ break;
+ }
+
+ case CEC_G_ADAP_PHYS_ADDR:
+ if (copy_to_user(parg, adap-phys_addr,
+ sizeof(adap-phys_addr)))
+ return -EFAULT;
If the adapter requires that userspace sets up the phys addr, then what
should this return if no such address has been set up?
I see two options: either 0x (which should be used if the HDMI cable
is disconnected), or return an error (perhaps ENODATA).
I think 0x might be best. This will still allow the unregistered
logical address.
Note that the comment in uapi/linux/cec.h for G_ADAP_LOG_ADDRS says that it
will return an error if the physical address is not set. That's not true
as far as I can tell, and if we go for 0x as the default in a case like
that, then it isn't necessary either to return an error.
cec_create_adapter already initialized the physical address to 0x, so
that looks good. But it should be documented in cec-ioc-g-adap-phys-addr.xml.
+ break;
+
+ case CEC_S_ADAP_PHYS_ADDR: {
+ u16 phys_addr;
+
+ if (!(adap-capabilities CEC_CAP_PHYS_ADDR))
+ return -ENOTTY;
+ if (copy_from_user(phys_addr, parg, sizeof(phys_addr)))
+ return -EFAULT;
+ adap-phys_addr = phys_addr;
+ break;
+ }
+
+ case CEC_G_ADAP_LOG_ADDRS: {
+ struct cec_log_addrs log_addrs;
+
+ log_addrs.cec_version = adap-version;
+ log_addrs.num_log_addrs = adap-num_log_addrs;
+ memcpy(log_addrs.primary_device_type, adap-prim_device,
+ CEC_MAX_LOG_ADDRS);
+ memcpy(log_addrs.log_addr_type, adap-log_addr_type,
+ CEC_MAX_LOG_ADDRS);
+ memcpy(log_addrs.log_addr, adap-log_addr,
+ CEC_MAX_LOG_ADDRS);
+
+ if (copy_to_user(parg, log_addrs, sizeof(log_addrs)))
+ return -EFAULT;
+ break;
+ }
+
+ case CEC_S_ADAP_LOG_ADDRS: {
+ struct cec_log_addrs log_addrs;
+
+ if (!(adap-capabilities CEC_CAP_LOG_ADDRS))
+ return -ENOTTY;
+ if (copy_from_user(log_addrs, parg, sizeof(log_addrs)))
+ return -EFAULT;
+ err = cec_claim_log_addrs(adap, log_addrs,
+ !(filp-f_flags O_NONBLOCK));
+ if (err)
+ return err;
+
+ if (copy_to_user(parg, log_addrs, sizeof(log_addrs)))
+ return -EFAULT;
+ break;
+ }
+
+ case CEC_G_VENDOR_ID:
+ if (copy_to_user(parg, adap-vendor_id,
+ sizeof(adap-vendor_id)))
+ return -EFAULT;
I've been reading up on this. If I understand it correctly, then this is
optional (only if a device supports vendor commands does it have to implement
this).
So if the VENDOR capability is set, then userspace *may* change it. If it is
left undefined, then no vendor commands are allowed.
I think this should be redesigned:
One CEC_CAP_VENDOR_CMDS: if set, then vendor commands are allowed.
One CEC_CAP_VENDOR_ID: userspace may set the Vendor ID. No vendor commands are
allowed as long as no vendor ID was set.
So if VENDOR_CMDS is set and VENDOR_ID isn't, then that means that the driver
will have set the vendor ID and the application can retrieve it with
G_VENDOR_ID.
If both are set, then userspace has to provide a vendor ID before vendor
commands
will be allowed.
That leaves the problem of determining that no vendor ID was set.
Re: [PATCH v6 06/11] cec: add HDMI CEC framework
Hi Kamil, Here is the first cec-compliance bug report: CEC_G_CAPS doesn't zero the reserved field! cec.c needs a memset there. I think this is missing in cec.c for all structs with a reserved field in them. Only G_EVENT looks to be OK. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 06/11] cec: add HDMI CEC framework
Hi Kamil,
I've started work on a cec-compliance utility and while doing that I
noticed a confusing name:
On 05/04/15 19:32, Kamil Debski wrote:
+struct cec_caps {
+ __u32 available_log_addrs;
+ __u32 capabilities;
+ __u32 vendor_id;
+ __u8 version;
+ __u8 reserved[35];
+};
I think 'version' should be renamed to 'cec_version' to indicate that we
are talking about the CEC version that the adapter supports, and not about
the driver version.
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 06/11] cec: add HDMI CEC framework
Hi Kamil,
Just two tiny issues, and after that you can add my:
Reviewed-by: Hans Verkuil hans.verk...@ciso.com
to this.
On 05/04/2015 07:32 PM, Kamil Debski wrote:
diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
new file mode 100644
index 000..67b0049
--- /dev/null
+++ b/include/uapi/linux/cec.h
@@ -0,0 +1,332 @@
+#ifndef _CEC_H
+#define _CEC_H
+
+#include linux/types.h
+
+struct cec_msg {
+ __u64 ts;
+ __u32 len;
+ __u32 status;
+ __u32 timeout;
+ /* timeout (in ms) is used to timeout CEC_RECEIVE.
+Set to 0 if you want to wait forever. */
+ __u8 msg[16];
+ __u8 reply;
+ /* If non-zero, then wait for a reply with this opcode.
+If there was an error when sending the msg or FeatureAbort
+was returned, then reply is set to 0.
+If reply is non-zero upon return, then len/msg are set to
+the received message.
+If reply is zero upon return and status has the
+CEC_TX_STATUS_FEATURE_ABORT bit set, then len/msg are set to the
+received feature abort message.
+If reply is zero upon return and status has the
+CEC_TX_STATUS_REPLY_TIMEOUT
+bit set, then no reply was seen at all.
+This field is ignored with CEC_RECEIVE.
+If reply is non-zero for CEC_TRANSMIT and the message is a broadcast,
+then -EINVAL is returned.
+if reply is non-zero, then timeout is set to 1000 (the required
+maximum response time).
+ */
+ __u32 sequence;
+ /* The framework assigns a sequence number to messages that are sent.
+ * This can be used to track replies to previously sent messages.
+ */
+ __u8 reserved[35];
+};
It is confusing in struct cec_msg that the comments come *after* the field
they belong to instead of just before. Can you change this?
+
+#define CEC_G_EVENT _IOWR('a', 9, struct cec_event)
This can be __IOR since we never write anything.
+/*
+ Read and set the vendor ID of the CEC adapter.
+ */
+#define CEC_G_VENDOR_ID _IOR('a', 10, __u32)
+#define CEC_S_VENDOR_ID _IOW('a', 11, __u32)
+/*
+ Enable/disable the passthrough mode
+ */
+#define CEC_G_PASSTHROUGH_IOR('a', 12, __u32)
+#define CEC_S_PASSTHROUGH_IOW('a', 13, __u32)
+
+#endif
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 06/11] cec: add HDMI CEC framework
From: Hans Verkuil hansv...@cisco.com
The added HDMI CEC framework provides a generic kernel interface for
HDMI CEC devices.
Signed-off-by: Hans Verkuil hansv...@cisco.com
[k.deb...@samsung.com: Merged CEC Updates commit by Hans Verkuil]
[k.deb...@samsung.com: Merged Update author commit by Hans Verkuil]
[k.deb...@samsung.com: change kthread handling when setting logical
address]
[k.deb...@samsung.com: code cleanup and fixes]
[k.deb...@samsung.com: add missing CEC commands to match spec]
[k.deb...@samsung.com: add RC framework support]
[k.deb...@samsung.com: move and edit documentation]
[k.deb...@samsung.com: add vendor id reporting]
[k.deb...@samsung.com: add possibility to clear assigned logical
addresses]
[k.deb...@samsung.com: documentation fixes, clenaup and expansion]
[k.deb...@samsung.com: reorder of API structs and add reserved fields]
[k.deb...@samsung.com: fix handling of events and fix 32/64bit timespec
problem]
[k.deb...@samsung.com: add cec.h to include/uapi/linux/Kbuild]
[k.deb...@samsung.com: add sequence number handling]
[k.deb...@samsung.com: add passthrough mode]
[k.deb...@samsung.com: fix CEC defines, add missing CEC 2.0 commands]
[k.deb...@samsung.com: add DocBook documentation by Hans Verkuil, with
minor additions]
Signed-off-by: Kamil Debski k.deb...@samsung.com
---
Documentation/cec.txt | 165 +++
drivers/media/Kconfig |6 +
drivers/media/Makefile|2 +
drivers/media/cec.c | 1191 +
include/media/cec.h | 142 ++
include/uapi/linux/Kbuild |1 +
include/uapi/linux/cec.h | 332 +
7 files changed, 1839 insertions(+)
create mode 100644 Documentation/cec.txt
create mode 100644 drivers/media/cec.c
create mode 100644 include/media/cec.h
create mode 100644 include/uapi/linux/cec.h
diff --git a/Documentation/cec.txt b/Documentation/cec.txt
new file mode 100644
index 000..a52017c2
--- /dev/null
+++ b/Documentation/cec.txt
@@ -0,0 +1,165 @@
+CEC Kernel Support
+==
+
+The CEC framework provides a unified kernel interface for use with HDMI CEC
+hardware. It is designed to handle a multiple variants of hardware. Adding to
+the flexibility of the framework it enables to set which parts of the CEC
+protocol processing is handled by the hardware, by the driver and by the
+userspace application.
+
+
+The CEC Protocol
+
+
+The CEC protocol enables consumer electronic devices to communicate with each
+other through the HDMI connection. The protocol uses logical addresses in the
+communication. The logical address is strictly connected with the functionality
+provided by the device. The TV acting as the communication hub is always
+assigned address 0. The physical address is determined by the physical
+connection between devices.
+
+The protocol enables control of compatible devices with a single remote.
+Synchronous power on/standby, instant playback with changing the content source
+on the TV.
+
+The Kernel Interface
+
+
+CEC Adapter
+---
+
+#define CEC_LOG_ADDR_INVALID 0xff
+
+/* The maximum number of logical addresses one device can be assigned to.
+ * The CEC 2.0 spec allows for only 2 logical addresses at the moment. The
+ * Analog Devices CEC hardware supports 3. So let's go wild and go for 4. */
+#define CEC_MAX_LOG_ADDRS 4
+
+/* The Primary Device Type */
+#define CEC_PRIM_DEVTYPE_TV0
+#define CEC_PRIM_DEVTYPE_RECORD1
+#define CEC_PRIM_DEVTYPE_TUNER 3
+#define CEC_PRIM_DEVTYPE_PLAYBACK 4
+#define CEC_PRIM_DEVTYPE_AUDIOSYSTEM 5
+#define CEC_PRIM_DEVTYPE_SWITCH6
+#define CEC_PRIM_DEVTYPE_VIDEOPROC 7
+
+/* The All Device Types flags (CEC 2.0) */
+#define CEC_FL_ALL_DEVTYPE_TV (1 7)
+#define CEC_FL_ALL_DEVTYPE_RECORD (1 6)
+#define CEC_FL_ALL_DEVTYPE_TUNER (1 5)
+#define CEC_FL_ALL_DEVTYPE_PLAYBACK(1 4)
+#define CEC_FL_ALL_DEVTYPE_AUDIOSYSTEM (1 3)
+#define CEC_FL_ALL_DEVTYPE_SWITCH (1 2)
+/* And if you wondering what happened to VIDEOPROC devices: those should
+ * be mapped to a SWITCH. */
+
+/* The logical address types that the CEC device wants to claim */
+#define CEC_LOG_ADDR_TYPE_TV 0
+#define CEC_LOG_ADDR_TYPE_RECORD 1
+#define CEC_LOG_ADDR_TYPE_TUNER2
+#define CEC_LOG_ADDR_TYPE_PLAYBACK 3
+#define CEC_LOG_ADDR_TYPE_AUDIOSYSTEM 4
+#define CEC_LOG_ADDR_TYPE_SPECIFIC 5
+#define CEC_LOG_ADDR_TYPE_UNREGISTERED 6
+/* Switches should use UNREGISTERED.
+ * Video processors should use SPECIFIC. */
+
+/* The CEC version */
+#define CEC_VERSION_1_4B 5
+#define CEC_VERSION_2_06
+
+struct cec_adapter {
+ /* internal fields removed */
+
+ u16 phys_addr;
+ u32 capabilities;
+ u8 version;
+ u8 num_log_addrs;
+ u8 prim_device[CEC_MAX_LOG_ADDRS];
+ u8 log_addr_type[CEC_MAX_LOG_ADDRS];
+ u8
