linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] media: cec-notifier: debounce reporing of invalid phys addr
@ 2019-09-01 19:45 Jonas Karlman
  2019-09-02 11:33 ` Hans Verkuil
  2019-10-01 20:52 ` [PATCH v2] media: cec-adap: add debounce support when setting an " Jonas Karlman
  0 siblings, 2 replies; 4+ messages in thread
From: Jonas Karlman @ 2019-09-01 19:45 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Jonas Karlman

When EDID is refreshed, HDMI cable is unplugged/replugged or
an AVR is power cycled the CEC phys addr gets invalidated.

This can cause some disruption of CEC communication when
adapter is being reconfigured.

Add a debounce option that can be used to debounce setting
an invalid phys addr.

Power off AVR (debounce = 0):
[  101.536866] cec-dw_hdmi: new physical address f.f.f.f
[  102.495686] cec-dw_hdmi: new physical address 2.1.0.0
[  102.495913] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
[  102.628574] cec-dw_hdmi: config: la 1 pa 2.1.0.0
[  105.130115] cec-dw_hdmi: new physical address f.f.f.f
[  106.979705] cec-dw_hdmi: new physical address 2.1.0.0
[  106.979872] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
[  107.112399] cec-dw_hdmi: config: la 1 pa 2.1.0.0
[  108.979408] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
[  109.205386] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11

Power on AVR (debounce = 0):
[  158.398447] cec-dw_hdmi: new physical address f.f.f.f
[  161.977714] cec-dw_hdmi: new physical address 2.1.0.0
[  161.978766] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
[  162.115624] cec-dw_hdmi: config: la 1 pa 2.1.0.0
[  162.402750] cec-dw_hdmi: new physical address f.f.f.f
[  162.403389] cec-dw_hdmi: cec_transmit_msg_fh: adapter is unconfigured
[  162.886757] cec-dw_hdmi: new physical address 2.1.0.0
[  162.886964] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
[  163.510725] cec-dw_hdmi: config: la 1 pa 2.1.0.0
[  173.034200] cec-dw_hdmi: message 10 89 02 05 timed out

Power off AVR (debounce = 5000):
[  251.720471] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
[  251.922432] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11

Power on AVR (debounce = 5000):
[  291.154262] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
[  291.296199] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11

Using a debounce of 5000 ms reconfiguring can be avoided.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/media/cec/cec-core.c     |  4 ++++
 drivers/media/cec/cec-notifier.c | 23 ++++++++++++++++++++++-
 drivers/media/cec/cec-priv.h     |  1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 9c610e1e99b8..c5094fd9b664 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -28,6 +28,10 @@ static bool debug_phys_addr;
 module_param(debug_phys_addr, bool, 0644);
 MODULE_PARM_DESC(debug_phys_addr, "add CEC_CAP_PHYS_ADDR if set");
 
+int cec_debounce;
+module_param_named(debounce, cec_debounce, int, 0644);
+MODULE_PARM_DESC(debounce, "debounce invalid phys addr");
+
 static dev_t cec_dev_t;
 
 /* Active devices */
diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
index 4d82a5522072..0157d468cfe4 100644
--- a/drivers/media/cec/cec-notifier.c
+++ b/drivers/media/cec/cec-notifier.c
@@ -12,11 +12,14 @@
 #include <linux/list.h>
 #include <linux/kref.h>
 #include <linux/of_platform.h>
+#include <linux/workqueue.h>
 
 #include <media/cec.h>
 #include <media/cec-notifier.h>
 #include <drm/drm_edid.h>
 
+#include "cec-priv.h"
+
 struct cec_notifier {
 	struct mutex lock;
 	struct list_head head;
@@ -28,11 +31,25 @@ struct cec_notifier {
 	void (*callback)(struct cec_adapter *adap, u16 pa);
 
 	u16 phys_addr;
+	struct delayed_work work;
 };
 
 static LIST_HEAD(cec_notifiers);
 static DEFINE_MUTEX(cec_notifiers_lock);
 
+static void cec_notifier_delayed_work(struct work_struct *work)
+{
+	struct cec_notifier *n =
+		container_of(to_delayed_work(work), struct cec_notifier, work);
+
+	mutex_lock(&n->lock);
+	if (n->callback)
+		n->callback(n->cec_adap, n->phys_addr);
+	else if (n->cec_adap)
+		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);
+	mutex_unlock(&n->lock);
+}
+
 struct cec_notifier *
 cec_notifier_get_conn(struct device *hdmi_dev, const char *conn_name)
 {
@@ -62,6 +79,7 @@ cec_notifier_get_conn(struct device *hdmi_dev, const char *conn_name)
 	}
 	n->phys_addr = CEC_PHYS_ADDR_INVALID;
 
+	INIT_DELAYED_WORK(&n->work, cec_notifier_delayed_work);
 	mutex_init(&n->lock);
 	kref_init(&n->kref);
 	list_add_tail(&n->head, &cec_notifiers);
@@ -172,9 +190,12 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
 	if (n == NULL)
 		return;
 
+	cancel_delayed_work_sync(&n->work);
 	mutex_lock(&n->lock);
 	n->phys_addr = pa;
-	if (n->callback)
+	if (cec_debounce > 0 && pa == CEC_PHYS_ADDR_INVALID)
+		schedule_delayed_work(&n->work, msecs_to_jiffies(cec_debounce));
+	else if (n->callback)
 		n->callback(n->cec_adap, n->phys_addr);
 	else if (n->cec_adap)
 		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);
diff --git a/drivers/media/cec/cec-priv.h b/drivers/media/cec/cec-priv.h
index 7bdf855aaecd..65176294fcf0 100644
--- a/drivers/media/cec/cec-priv.h
+++ b/drivers/media/cec/cec-priv.h
@@ -27,6 +27,7 @@ static inline bool msg_is_raw(const struct cec_msg *msg)
 
 /* cec-core.c */
 extern int cec_debug;
+extern int cec_debounce;
 int cec_get_device(struct cec_devnode *devnode);
 void cec_put_device(struct cec_devnode *devnode);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] media: cec-notifier: debounce reporing of invalid phys addr
  2019-09-01 19:45 [PATCH] media: cec-notifier: debounce reporing of invalid phys addr Jonas Karlman
@ 2019-09-02 11:33 ` Hans Verkuil
  2019-10-01 20:52 ` [PATCH v2] media: cec-adap: add debounce support when setting an " Jonas Karlman
  1 sibling, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2019-09-02 11:33 UTC (permalink / raw)
  To: Jonas Karlman; +Cc: Mauro Carvalho Chehab, linux-media, linux-kernel

Hi Jonas,

On 9/1/19 9:45 PM, Jonas Karlman wrote:
> When EDID is refreshed, HDMI cable is unplugged/replugged or
> an AVR is power cycled the CEC phys addr gets invalidated.
> 
> This can cause some disruption of CEC communication when
> adapter is being reconfigured.
> 
> Add a debounce option that can be used to debounce setting
> an invalid phys addr.

Debouncing can be dangerous since there are scenarios where there
is a quick unplug/replug such as in HDMI splitters.

So by default this should be off: I have yet to see issues with
just assuming the sink was unplugged and a potentially different
sink was plugged in.

That said, it would be nice to have a debounce module option, but
this doesn't belong in cec-notifier. Instead, this should be done
in the cec_s_phys_addr() function itself.

Regards,

	Hans

> 
> Power off AVR (debounce = 0):
> [  101.536866] cec-dw_hdmi: new physical address f.f.f.f
> [  102.495686] cec-dw_hdmi: new physical address 2.1.0.0
> [  102.495913] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
> [  102.628574] cec-dw_hdmi: config: la 1 pa 2.1.0.0
> [  105.130115] cec-dw_hdmi: new physical address f.f.f.f
> [  106.979705] cec-dw_hdmi: new physical address 2.1.0.0
> [  106.979872] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
> [  107.112399] cec-dw_hdmi: config: la 1 pa 2.1.0.0
> [  108.979408] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
> [  109.205386] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11
> 
> Power on AVR (debounce = 0):
> [  158.398447] cec-dw_hdmi: new physical address f.f.f.f
> [  161.977714] cec-dw_hdmi: new physical address 2.1.0.0
> [  161.978766] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
> [  162.115624] cec-dw_hdmi: config: la 1 pa 2.1.0.0
> [  162.402750] cec-dw_hdmi: new physical address f.f.f.f
> [  162.403389] cec-dw_hdmi: cec_transmit_msg_fh: adapter is unconfigured
> [  162.886757] cec-dw_hdmi: new physical address 2.1.0.0
> [  162.886964] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
> [  163.510725] cec-dw_hdmi: config: la 1 pa 2.1.0.0
> [  173.034200] cec-dw_hdmi: message 10 89 02 05 timed out
> 
> Power off AVR (debounce = 5000):
> [  251.720471] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
> [  251.922432] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11
> 
> Power on AVR (debounce = 5000):
> [  291.154262] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
> [  291.296199] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11
> 
> Using a debounce of 5000 ms reconfiguring can be avoided.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/media/cec/cec-core.c     |  4 ++++
>  drivers/media/cec/cec-notifier.c | 23 ++++++++++++++++++++++-
>  drivers/media/cec/cec-priv.h     |  1 +
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index 9c610e1e99b8..c5094fd9b664 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -28,6 +28,10 @@ static bool debug_phys_addr;
>  module_param(debug_phys_addr, bool, 0644);
>  MODULE_PARM_DESC(debug_phys_addr, "add CEC_CAP_PHYS_ADDR if set");
>  
> +int cec_debounce;
> +module_param_named(debounce, cec_debounce, int, 0644);
> +MODULE_PARM_DESC(debounce, "debounce invalid phys addr");
> +
>  static dev_t cec_dev_t;
>  
>  /* Active devices */
> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
> index 4d82a5522072..0157d468cfe4 100644
> --- a/drivers/media/cec/cec-notifier.c
> +++ b/drivers/media/cec/cec-notifier.c
> @@ -12,11 +12,14 @@
>  #include <linux/list.h>
>  #include <linux/kref.h>
>  #include <linux/of_platform.h>
> +#include <linux/workqueue.h>
>  
>  #include <media/cec.h>
>  #include <media/cec-notifier.h>
>  #include <drm/drm_edid.h>
>  
> +#include "cec-priv.h"
> +
>  struct cec_notifier {
>  	struct mutex lock;
>  	struct list_head head;
> @@ -28,11 +31,25 @@ struct cec_notifier {
>  	void (*callback)(struct cec_adapter *adap, u16 pa);
>  
>  	u16 phys_addr;
> +	struct delayed_work work;
>  };
>  
>  static LIST_HEAD(cec_notifiers);
>  static DEFINE_MUTEX(cec_notifiers_lock);
>  
> +static void cec_notifier_delayed_work(struct work_struct *work)
> +{
> +	struct cec_notifier *n =
> +		container_of(to_delayed_work(work), struct cec_notifier, work);
> +
> +	mutex_lock(&n->lock);
> +	if (n->callback)
> +		n->callback(n->cec_adap, n->phys_addr);
> +	else if (n->cec_adap)
> +		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);
> +	mutex_unlock(&n->lock);
> +}
> +
>  struct cec_notifier *
>  cec_notifier_get_conn(struct device *hdmi_dev, const char *conn_name)
>  {
> @@ -62,6 +79,7 @@ cec_notifier_get_conn(struct device *hdmi_dev, const char *conn_name)
>  	}
>  	n->phys_addr = CEC_PHYS_ADDR_INVALID;
>  
> +	INIT_DELAYED_WORK(&n->work, cec_notifier_delayed_work);
>  	mutex_init(&n->lock);
>  	kref_init(&n->kref);
>  	list_add_tail(&n->head, &cec_notifiers);
> @@ -172,9 +190,12 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
>  	if (n == NULL)
>  		return;
>  
> +	cancel_delayed_work_sync(&n->work);
>  	mutex_lock(&n->lock);
>  	n->phys_addr = pa;
> -	if (n->callback)
> +	if (cec_debounce > 0 && pa == CEC_PHYS_ADDR_INVALID)
> +		schedule_delayed_work(&n->work, msecs_to_jiffies(cec_debounce));
> +	else if (n->callback)
>  		n->callback(n->cec_adap, n->phys_addr);
>  	else if (n->cec_adap)
>  		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);
> diff --git a/drivers/media/cec/cec-priv.h b/drivers/media/cec/cec-priv.h
> index 7bdf855aaecd..65176294fcf0 100644
> --- a/drivers/media/cec/cec-priv.h
> +++ b/drivers/media/cec/cec-priv.h
> @@ -27,6 +27,7 @@ static inline bool msg_is_raw(const struct cec_msg *msg)
>  
>  /* cec-core.c */
>  extern int cec_debug;
> +extern int cec_debounce;
>  int cec_get_device(struct cec_devnode *devnode);
>  void cec_put_device(struct cec_devnode *devnode);
>  
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] media: cec-adap: add debounce support when setting an invalid phys addr
  2019-09-01 19:45 [PATCH] media: cec-notifier: debounce reporing of invalid phys addr Jonas Karlman
  2019-09-02 11:33 ` Hans Verkuil
@ 2019-10-01 20:52 ` Jonas Karlman
  2019-10-09 10:37   ` Hans Verkuil
  1 sibling, 1 reply; 4+ messages in thread
From: Jonas Karlman @ 2019-10-01 20:52 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Jonas Karlman

When EDID is refreshed, HDMI cable is unplugged/replugged or
an AVR is power cycled the CEC phys addr gets invalidated.

This can cause some disruption of CEC communication when
adapter is being reconfigured.

Add a debounce module option that can be used to debounce setting
an invalid phys addr. Default is not to use debouncing.

Using a configured debounce of e.g. 5000 ms, cec reconfiguring
could be avoided when AVR was power cycled on my setup.

Power off AVR (default cec.debounce=0):
[  101.536866] cec-dw_hdmi: new physical address f.f.f.f
[  102.495686] cec-dw_hdmi: new physical address 2.1.0.0
[  102.495913] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
[  102.628574] cec-dw_hdmi: config: la 1 pa 2.1.0.0
[  105.130115] cec-dw_hdmi: new physical address f.f.f.f
[  106.979705] cec-dw_hdmi: new physical address 2.1.0.0
[  106.979872] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
[  107.112399] cec-dw_hdmi: config: la 1 pa 2.1.0.0
[  108.979408] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
[  109.205386] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11

Power on AVR (default cec.debounce=0):
[  158.398447] cec-dw_hdmi: new physical address f.f.f.f
[  161.977714] cec-dw_hdmi: new physical address 2.1.0.0
[  161.978766] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
[  162.115624] cec-dw_hdmi: config: la 1 pa 2.1.0.0
[  162.402750] cec-dw_hdmi: new physical address f.f.f.f
[  162.403389] cec-dw_hdmi: cec_transmit_msg_fh: adapter is unconfigured
[  162.886757] cec-dw_hdmi: new physical address 2.1.0.0
[  162.886964] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
[  163.510725] cec-dw_hdmi: config: la 1 pa 2.1.0.0
[  173.034200] cec-dw_hdmi: message 10 89 02 05 timed out

Power off AVR (cec.debounce=5000):
[  251.720471] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
[  251.922432] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11

Power on AVR (cec.debounce=5000):
[  291.154262] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
[  291.296199] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/media/cec/cec-adap.c |  9 ++++++++-
 drivers/media/cec/cec-core.c | 18 ++++++++++++++++++
 drivers/media/cec/cec-priv.h |  1 +
 include/media/cec.h          |  2 ++
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 5ef7daeb8cbd..4c94f6da526b 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -1593,8 +1593,15 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
 	if (IS_ERR_OR_NULL(adap))
 		return;
 
+	cancel_delayed_work_sync(&adap->debounce_work);
+
 	mutex_lock(&adap->lock);
-	__cec_s_phys_addr(adap, phys_addr, block);
+	if (cec_debounce > 0 && !block && phys_addr == CEC_PHYS_ADDR_INVALID &&
+	    adap->phys_addr != phys_addr)
+		schedule_delayed_work(&adap->debounce_work,
+				      msecs_to_jiffies(cec_debounce));
+	else
+		__cec_s_phys_addr(adap, phys_addr, block);
 	mutex_unlock(&adap->lock);
 }
 EXPORT_SYMBOL_GPL(cec_s_phys_addr);
diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 9c610e1e99b8..fa93a724d7aa 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -28,6 +28,10 @@ static bool debug_phys_addr;
 module_param(debug_phys_addr, bool, 0644);
 MODULE_PARM_DESC(debug_phys_addr, "add CEC_CAP_PHYS_ADDR if set");
 
+int cec_debounce;
+module_param_named(debounce, cec_debounce, int, 0644);
+MODULE_PARM_DESC(debounce, "debounce invalid phys addr");
+
 static dev_t cec_dev_t;
 
 /* Active devices */
@@ -174,6 +178,8 @@ static void cec_devnode_unregister(struct cec_adapter *adap)
 	devnode->unregistered = true;
 	mutex_unlock(&devnode->lock);
 
+	cancel_delayed_work_sync(&adap->debounce_work);
+
 	mutex_lock(&adap->lock);
 	__cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false);
 	__cec_s_log_addrs(adap, NULL, false);
@@ -250,6 +256,17 @@ static const struct file_operations cec_error_inj_fops = {
 };
 #endif
 
+static void cec_s_phys_addr_debounce(struct work_struct *work)
+{
+	struct delayed_work *delayed_work = to_delayed_work(work);
+	struct cec_adapter *adap =
+		container_of(delayed_work, struct cec_adapter, debounce_work);
+
+	mutex_lock(&adap->lock);
+	__cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false);
+	mutex_unlock(&adap->lock);
+}
+
 struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
 					 void *priv, const char *name, u32 caps,
 					 u8 available_las)
@@ -293,6 +310,7 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
 	INIT_LIST_HEAD(&adap->transmit_queue);
 	INIT_LIST_HEAD(&adap->wait_queue);
 	init_waitqueue_head(&adap->kthread_waitq);
+	INIT_DELAYED_WORK(&adap->debounce_work, cec_s_phys_addr_debounce);
 
 	/* adap->devnode initialization */
 	INIT_LIST_HEAD(&adap->devnode.fhs);
diff --git a/drivers/media/cec/cec-priv.h b/drivers/media/cec/cec-priv.h
index 7bdf855aaecd..65176294fcf0 100644
--- a/drivers/media/cec/cec-priv.h
+++ b/drivers/media/cec/cec-priv.h
@@ -27,6 +27,7 @@ static inline bool msg_is_raw(const struct cec_msg *msg)
 
 /* cec-core.c */
 extern int cec_debug;
+extern int cec_debounce;
 int cec_get_device(struct cec_devnode *devnode);
 void cec_put_device(struct cec_devnode *devnode);
 
diff --git a/include/media/cec.h b/include/media/cec.h
index 4d59387bc61b..4d96ece01ba2 100644
--- a/include/media/cec.h
+++ b/include/media/cec.h
@@ -195,6 +195,8 @@ struct cec_adapter {
 	wait_queue_head_t kthread_waitq;
 	wait_queue_head_t waitq;
 
+	struct delayed_work debounce_work;
+
 	const struct cec_adap_ops *ops;
 	void *priv;
 	u32 capabilities;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] media: cec-adap: add debounce support when setting an invalid phys addr
  2019-10-01 20:52 ` [PATCH v2] media: cec-adap: add debounce support when setting an " Jonas Karlman
@ 2019-10-09 10:37   ` Hans Verkuil
  0 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2019-10-09 10:37 UTC (permalink / raw)
  To: Jonas Karlman; +Cc: Mauro Carvalho Chehab, linux-media, linux-kernel

Hi Jonas,

On 10/1/19 10:52 PM, Jonas Karlman wrote:
> When EDID is refreshed, HDMI cable is unplugged/replugged or
> an AVR is power cycled the CEC phys addr gets invalidated.
> 
> This can cause some disruption of CEC communication when
> adapter is being reconfigured.
> 
> Add a debounce module option that can be used to debounce setting
> an invalid phys addr. Default is not to use debouncing.
> 
> Using a configured debounce of e.g. 5000 ms, cec reconfiguring
> could be avoided when AVR was power cycled on my setup.
> 
> Power off AVR (default cec.debounce=0):
> [  101.536866] cec-dw_hdmi: new physical address f.f.f.f
> [  102.495686] cec-dw_hdmi: new physical address 2.1.0.0
> [  102.495913] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
> [  102.628574] cec-dw_hdmi: config: la 1 pa 2.1.0.0
> [  105.130115] cec-dw_hdmi: new physical address f.f.f.f
> [  106.979705] cec-dw_hdmi: new physical address 2.1.0.0
> [  106.979872] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
> [  107.112399] cec-dw_hdmi: config: la 1 pa 2.1.0.0
> [  108.979408] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
> [  109.205386] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11
> 
> Power on AVR (default cec.debounce=0):
> [  158.398447] cec-dw_hdmi: new physical address f.f.f.f
> [  161.977714] cec-dw_hdmi: new physical address 2.1.0.0
> [  161.978766] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
> [  162.115624] cec-dw_hdmi: config: la 1 pa 2.1.0.0
> [  162.402750] cec-dw_hdmi: new physical address f.f.f.f
> [  162.403389] cec-dw_hdmi: cec_transmit_msg_fh: adapter is unconfigured
> [  162.886757] cec-dw_hdmi: new physical address 2.1.0.0
> [  162.886964] cec-dw_hdmi: physical address: 2.1.0.0, claim 1 logical addresses
> [  163.510725] cec-dw_hdmi: config: la 1 pa 2.1.0.0
> [  173.034200] cec-dw_hdmi: message 10 89 02 05 timed out
> 
> Power off AVR (cec.debounce=5000):
> [  251.720471] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
> [  251.922432] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11
> 
> Power on AVR (cec.debounce=5000):
> [  291.154262] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 5
> [  291.296199] cec-dw_hdmi: reported physical address 2.0.0.0 for logical address 11
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/media/cec/cec-adap.c |  9 ++++++++-
>  drivers/media/cec/cec-core.c | 18 ++++++++++++++++++
>  drivers/media/cec/cec-priv.h |  1 +
>  include/media/cec.h          |  2 ++
>  4 files changed, 29 insertions(+), 1 deletion(-)

This patch looks pretty good. It's surprisingly little code for a feature like this,
nice!

This module option should be documented in Documentation/media/uapi/cec/cec-intro.rst.
Make a section "1.1 Debouncing" that explains this module option.

I also have one code comment:

> 
> diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
> index 5ef7daeb8cbd..4c94f6da526b 100644
> --- a/drivers/media/cec/cec-adap.c
> +++ b/drivers/media/cec/cec-adap.c
> @@ -1593,8 +1593,15 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
>  	if (IS_ERR_OR_NULL(adap))
>  		return;
>  
> +	cancel_delayed_work_sync(&adap->debounce_work);
> +
>  	mutex_lock(&adap->lock);
> -	__cec_s_phys_addr(adap, phys_addr, block);
> +	if (cec_debounce > 0 && !block && phys_addr == CEC_PHYS_ADDR_INVALID &&
> +	    adap->phys_addr != phys_addr)
> +		schedule_delayed_work(&adap->debounce_work,
> +				      msecs_to_jiffies(cec_debounce));
> +	else
> +		__cec_s_phys_addr(adap, phys_addr, block);
>  	mutex_unlock(&adap->lock);
>  }
>  EXPORT_SYMBOL_GPL(cec_s_phys_addr);
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index 9c610e1e99b8..fa93a724d7aa 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -28,6 +28,10 @@ static bool debug_phys_addr;
>  module_param(debug_phys_addr, bool, 0644);
>  MODULE_PARM_DESC(debug_phys_addr, "add CEC_CAP_PHYS_ADDR if set");
>  
> +int cec_debounce;
> +module_param_named(debounce, cec_debounce, int, 0644);
> +MODULE_PARM_DESC(debounce, "debounce invalid phys addr");

This should say: "invalid physical address debounce time in ms"

I also recommend renaming 'debounce' to 'debounce_ms' to signal that this
is in ms.

Regards,

	Hans

> +
>  static dev_t cec_dev_t;
>  
>  /* Active devices */
> @@ -174,6 +178,8 @@ static void cec_devnode_unregister(struct cec_adapter *adap)
>  	devnode->unregistered = true;
>  	mutex_unlock(&devnode->lock);
>  
> +	cancel_delayed_work_sync(&adap->debounce_work);
> +
>  	mutex_lock(&adap->lock);
>  	__cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false);
>  	__cec_s_log_addrs(adap, NULL, false);
> @@ -250,6 +256,17 @@ static const struct file_operations cec_error_inj_fops = {
>  };
>  #endif
>  
> +static void cec_s_phys_addr_debounce(struct work_struct *work)
> +{
> +	struct delayed_work *delayed_work = to_delayed_work(work);
> +	struct cec_adapter *adap =
> +		container_of(delayed_work, struct cec_adapter, debounce_work);
> +
> +	mutex_lock(&adap->lock);
> +	__cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false);
> +	mutex_unlock(&adap->lock);
> +}
> +
>  struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>  					 void *priv, const char *name, u32 caps,
>  					 u8 available_las)
> @@ -293,6 +310,7 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>  	INIT_LIST_HEAD(&adap->transmit_queue);
>  	INIT_LIST_HEAD(&adap->wait_queue);
>  	init_waitqueue_head(&adap->kthread_waitq);
> +	INIT_DELAYED_WORK(&adap->debounce_work, cec_s_phys_addr_debounce);
>  
>  	/* adap->devnode initialization */
>  	INIT_LIST_HEAD(&adap->devnode.fhs);
> diff --git a/drivers/media/cec/cec-priv.h b/drivers/media/cec/cec-priv.h
> index 7bdf855aaecd..65176294fcf0 100644
> --- a/drivers/media/cec/cec-priv.h
> +++ b/drivers/media/cec/cec-priv.h
> @@ -27,6 +27,7 @@ static inline bool msg_is_raw(const struct cec_msg *msg)
>  
>  /* cec-core.c */
>  extern int cec_debug;
> +extern int cec_debounce;
>  int cec_get_device(struct cec_devnode *devnode);
>  void cec_put_device(struct cec_devnode *devnode);
>  
> diff --git a/include/media/cec.h b/include/media/cec.h
> index 4d59387bc61b..4d96ece01ba2 100644
> --- a/include/media/cec.h
> +++ b/include/media/cec.h
> @@ -195,6 +195,8 @@ struct cec_adapter {
>  	wait_queue_head_t kthread_waitq;
>  	wait_queue_head_t waitq;
>  
> +	struct delayed_work debounce_work;
> +
>  	const struct cec_adap_ops *ops;
>  	void *priv;
>  	u32 capabilities;
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-10-09 10:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-01 19:45 [PATCH] media: cec-notifier: debounce reporing of invalid phys addr Jonas Karlman
2019-09-02 11:33 ` Hans Verkuil
2019-10-01 20:52 ` [PATCH v2] media: cec-adap: add debounce support when setting an " Jonas Karlman
2019-10-09 10:37   ` Hans Verkuil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).