[PATCH 0/5] cec/adv/omap: fixes and new status flags

[PATCH 0/5] cec/adv/omap: fixes and new status flags

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The first patch adds new status flags to indicate when a pending
message is aborted because the CEC adapter is unconfigured, and when
a transmit times out (this indicates a driver bug).

The second and third patch fix a minor issue with the adv HDMI receivers:
if the EDID goes away, then the physical address also becomes invalid.

The fourth patch fixes a race condition in the omap4 CEC driver that
causes a transmit time out. The final patch drops the code in the omap4
CEC driver that attempts to set the number of retransmits: those register
bits are read-only, so the code is pointless.

There are no dependencies between these patches, although the first
and fourth patch relate to the same problem. With the new transmit
TIMEOUT status I hope that it will be easier to catch driver bugs like
that earlier since this bug remained hidden for too long.

Regards,

	Hans

Hans Verkuil (5):
  cec: add new tx/rx status bits to detect aborts/timeouts
  adv7604: when the EDID is cleared, unconfigure CEC as well
  adv7842: when the EDID is cleared, unconfigure CEC as well
  omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done
  omapdrm/dss/hdmi4_cec.c: don't set the retransmit count

 .../media/uapi/cec/cec-ioc-receive.rst        | 25 ++++++-
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c       | 38 +++++------
 drivers/media/cec/cec-adap.c                  | 66 +++++--------------
 drivers/media/i2c/adv7604.c                   |  4 +-
 drivers/media/i2c/adv7842.c                   |  4 +-
 include/uapi/linux/cec.h                      |  3 +
 6 files changed, 67 insertions(+), 73 deletions(-)

-- 
2.18.0

[PATCH 0/5] cec/adv/omap: fixes and new status flags

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The first patch adds new status flags to indicate when a pending
message is aborted because the CEC adapter is unconfigured, and when
a transmit times out (this indicates a driver bug).

The second and third patch fix a minor issue with the adv HDMI receivers:
if the EDID goes away, then the physical address also becomes invalid.

The fourth patch fixes a race condition in the omap4 CEC driver that
causes a transmit time out. The final patch drops the code in the omap4
CEC driver that attempts to set the number of retransmits: those register
bits are read-only, so the code is pointless.

There are no dependencies between these patches, although the first
and fourth patch relate to the same problem. With the new transmit
TIMEOUT status I hope that it will be easier to catch driver bugs like
that earlier since this bug remained hidden for too long.

Regards,

	Hans

Hans Verkuil (5):
  cec: add new tx/rx status bits to detect aborts/timeouts
  adv7604: when the EDID is cleared, unconfigure CEC as well
  adv7842: when the EDID is cleared, unconfigure CEC as well
  omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done
  omapdrm/dss/hdmi4_cec.c: don't set the retransmit count

 .../media/uapi/cec/cec-ioc-receive.rst        | 25 ++++++-
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c       | 38 +++++------
 drivers/media/cec/cec-adap.c                  | 66 +++++--------------
 drivers/media/i2c/adv7604.c                   |  4 +-
 drivers/media/i2c/adv7842.c                   |  4 +-
 include/uapi/linux/cec.h                      |  3 +
 6 files changed, 67 insertions(+), 73 deletions(-)

-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] cec: add new tx/rx status bits to detect aborts/timeouts

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If the HDMI cable is disconnected or the CEC adapter is manually
unconfigured, then all pending transmits and wait-for-replies are
aborted. Signal this with new status bits (CEC_RX/TX_STATUS_ABORTED).

If due to (usually) a driver bug a transmit never ends (i.e. the
transmit_done was never called by the driver), then when this times
out the message is marked with CEC_TX_STATUS_TIMEOUT.

This should not happen and is an indication of a driver bug.

Without a separate status bit for this it was impossible to detect
this from userspace.

The 'transmit timed out' kernel message is now a warning, so this
should be more prominent in the kernel log as well.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../media/uapi/cec/cec-ioc-receive.rst        | 25 ++++++-
 drivers/media/cec/cec-adap.c                  | 66 +++++--------------
 include/uapi/linux/cec.h                      |  3 +
 3 files changed, 44 insertions(+), 50 deletions(-)

diff --git a/Documentation/media/uapi/cec/cec-ioc-receive.rst b/Documentation/media/uapi/cec/cec-ioc-receive.rst
index e964074cd15b..b25e48afaa08 100644
--- a/Documentation/media/uapi/cec/cec-ioc-receive.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-receive.rst
@@ -16,10 +16,10 @@ CEC_RECEIVE, CEC_TRANSMIT - Receive or transmit a CEC message
 Synopsis
 ========
 
-.. c:function:: int ioctl( int fd, CEC_RECEIVE, struct cec_msg *argp )
+.. c:function:: int ioctl( int fd, CEC_RECEIVE, struct cec_msg \*argp )
     :name: CEC_RECEIVE
 
-.. c:function:: int ioctl( int fd, CEC_TRANSMIT, struct cec_msg *argp )
+.. c:function:: int ioctl( int fd, CEC_TRANSMIT, struct cec_msg \*argp )
     :name: CEC_TRANSMIT
 
 Arguments
@@ -272,6 +272,19 @@ View On' messages from initiator 0xf ('Unregistered') to destination 0 ('TV').
       - The transmit failed after one or more retries. This status bit is
 	mutually exclusive with :ref:`CEC_TX_STATUS_OK <CEC-TX-STATUS-OK>`.
 	Other bits can still be set to explain which failures were seen.
+    * .. _`CEC-TX-STATUS-ABORTED`:
+
+      - ``CEC_TX_STATUS_ABORTED``
+      - 0x40
+      - The transmit was aborted due to an HDMI disconnect, or the adapter
+        was unconfigured, or a transmit was interrupted, or the driver
+	returned an error when attempting to start a transmit.
+    * .. _`CEC-TX-STATUS-TIMEOUT`:
+
+      - ``CEC_TX_STATUS_TIMEOUT``
+      - 0x80
+      - The transmit timed out. This should not normally happen and this
+	indicates a driver problem.
 
 
 .. tabularcolumns:: |p{5.6cm}|p{0.9cm}|p{11.0cm}|
@@ -300,6 +313,14 @@ View On' messages from initiator 0xf ('Unregistered') to destination 0 ('TV').
       - The message was received successfully but the reply was
 	``CEC_MSG_FEATURE_ABORT``. This status is only set if this message
 	was the reply to an earlier transmitted message.
+    * .. _`CEC-RX-STATUS-ABORTED`:
+
+      - ``CEC_RX_STATUS_ABORTED``
+      - 0x08
+      - The wait for a reply to an earlier transmitted message was aborted
+        because the HDMI cable was disconnected, the adapter was unconfigured
+	or the :ref:`CEC_TRANSMIT <CEC_RECEIVE>` that waited for a
+	reply was interrupted.
 
 
 
diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 030b2602faf0..2ebb53fd4800 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -341,7 +341,7 @@ static void cec_data_completed(struct cec_data *data)
  *
  * This function is called with adap->lock held.
  */
-static void cec_data_cancel(struct cec_data *data)
+static void cec_data_cancel(struct cec_data *data, u8 tx_status)
 {
 	/*
 	 * It's either the current transmit, or it is a pending
@@ -356,13 +356,11 @@ static void cec_data_cancel(struct cec_data *data)
 	}
 
 	if (data->msg.tx_status & CEC_TX_STATUS_OK) {
-		/* Mark the canceled RX as a timeout */
 		data->msg.rx_ts = ktime_get_ns();
-		data->msg.rx_status = CEC_RX_STATUS_TIMEOUT;
+		data->msg.rx_status = CEC_RX_STATUS_ABORTED;
 	} else {
-		/* Mark the canceled TX as an error */
 		data->msg.tx_ts = ktime_get_ns();
-		data->msg.tx_status |= CEC_TX_STATUS_ERROR |
+		data->msg.tx_status |= tx_status |
 				       CEC_TX_STATUS_MAX_RETRIES;
 		data->msg.tx_error_cnt++;
 		data->attempts = 0;
@@ -390,15 +388,15 @@ static void cec_flush(struct cec_adapter *adap)
 	while (!list_empty(&adap->transmit_queue)) {
 		data = list_first_entry(&adap->transmit_queue,
 					struct cec_data, list);
-		cec_data_cancel(data);
+		cec_data_cancel(data, CEC_TX_STATUS_ABORTED);
 	}
 	if (adap->transmitting)
-		cec_data_cancel(adap->transmitting);
+		cec_data_cancel(adap->transmitting, CEC_TX_STATUS_ABORTED);
 
 	/* Cancel the pending timeout work. */
 	list_for_each_entry_safe(data, n, &adap->wait_queue, list) {
 		if (cancel_delayed_work(&data->work))
-			cec_data_cancel(data);
+			cec_data_cancel(data, CEC_TX_STATUS_OK);
 		/*
 		 * If cancel_delayed_work returned false, then
 		 * the cec_wait_timeout function is running,
@@ -474,12 +472,13 @@ int cec_thread_func(void *_adap)
 			 * so much traffic on the bus that the adapter was
 			 * unable to transmit for CEC_XFER_TIMEOUT_MS (2.1s).
 			 */
-			dprintk(1, "%s: message %*ph timed out\n", __func__,
+			pr_warn("cec-%s: message %*ph timed out\n", adap->name,
 				adap->transmitting->msg.len,
 				adap->transmitting->msg.msg);
 			adap->tx_timeouts++;
 			/* Just give up on this. */
-			cec_data_cancel(adap->transmitting);
+			cec_data_cancel(adap->transmitting,
+					CEC_TX_STATUS_TIMEOUT);
 			goto unlock;
 		}
 
@@ -530,7 +529,7 @@ int cec_thread_func(void *_adap)
 		/* Tell the adapter to transmit, cancel on error */
 		if (adap->ops->adap_transmit(adap, data->attempts,
 					     signal_free_time, &data->msg))
-			cec_data_cancel(data);
+			cec_data_cancel(data, CEC_TX_STATUS_ABORTED);
 
 unlock:
 		mutex_unlock(&adap->lock);
@@ -702,8 +701,6 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 {
 	struct cec_data *data;
 	u8 last_initiator = 0xff;
-	unsigned int timeout;
-	int res = 0;
 
 	msg->rx_ts = 0;
 	msg->tx_ts = 0;
@@ -845,48 +842,21 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 	if (!block)
 		return 0;
 
-	/*
-	 * If we don't get a completion before this time something is really
-	 * wrong and we time out.
-	 */
-	timeout = CEC_XFER_TIMEOUT_MS;
-	/* Add the requested timeout if we have to wait for a reply as well */
-	if (msg->timeout)
-		timeout += msg->timeout;
-
 	/*
 	 * Release the lock and wait, retake the lock afterwards.
 	 */
 	mutex_unlock(&adap->lock);
-	res = wait_for_completion_killable_timeout(&data->c,
-						   msecs_to_jiffies(timeout));
+	wait_for_completion_killable(&data->c);
 	mutex_lock(&adap->lock);
 
-	if (data->completed) {
-		/* The transmit completed (possibly with an error) */
-		*msg = data->msg;
-		kfree(data);
-		return 0;
-	}
-	/*
-	 * The wait for completion timed out or was interrupted, so mark this
-	 * as non-blocking and disconnect from the filehandle since it is
-	 * still 'in flight'. When it finally completes it will just drop the
-	 * result silently.
-	 */
-	data->blocking = false;
-	if (data->fh)
-		list_del(&data->xfer_list);
-	data->fh = NULL;
+	/* Cancel the transmit if it was interrupted */
+	if (!data->completed)
+		cec_data_cancel(data, CEC_TX_STATUS_ABORTED);
 
-	if (res == 0) { /* timed out */
-		/* Check if the reply or the transmit failed */
-		if (msg->timeout && (msg->tx_status & CEC_TX_STATUS_OK))
-			msg->rx_status = CEC_RX_STATUS_TIMEOUT;
-		else
-			msg->tx_status = CEC_TX_STATUS_MAX_RETRIES;
-	}
-	return res > 0 ? 0 : res;
+	/* The transmit completed (possibly with an error) */
+	*msg = data->msg;
+	kfree(data);
+	return 0;
 }
 
 /* Helper function to be used by drivers and this framework. */
diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
index 097fcd812471..3094af68b6e7 100644
--- a/include/uapi/linux/cec.h
+++ b/include/uapi/linux/cec.h
@@ -152,10 +152,13 @@ static inline void cec_msg_set_reply_to(struct cec_msg *msg,
 #define CEC_TX_STATUS_LOW_DRIVE		(1 << 3)
 #define CEC_TX_STATUS_ERROR		(1 << 4)
 #define CEC_TX_STATUS_MAX_RETRIES	(1 << 5)
+#define CEC_TX_STATUS_ABORTED		(1 << 6)
+#define CEC_TX_STATUS_TIMEOUT		(1 << 7)
 
 #define CEC_RX_STATUS_OK		(1 << 0)
 #define CEC_RX_STATUS_TIMEOUT		(1 << 1)
 #define CEC_RX_STATUS_FEATURE_ABORT	(1 << 2)
+#define CEC_RX_STATUS_ABORTED		(1 << 3)
 
 static inline int cec_msg_status_is_ok(const struct cec_msg *msg)
 {
-- 
2.18.0

[PATCH 1/5] cec: add new tx/rx status bits to detect aborts/timeouts

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If the HDMI cable is disconnected or the CEC adapter is manually
unconfigured, then all pending transmits and wait-for-replies are
aborted. Signal this with new status bits (CEC_RX/TX_STATUS_ABORTED).

If due to (usually) a driver bug a transmit never ends (i.e. the
transmit_done was never called by the driver), then when this times
out the message is marked with CEC_TX_STATUS_TIMEOUT.

This should not happen and is an indication of a driver bug.

Without a separate status bit for this it was impossible to detect
this from userspace.

The 'transmit timed out' kernel message is now a warning, so this
should be more prominent in the kernel log as well.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../media/uapi/cec/cec-ioc-receive.rst        | 25 ++++++-
 drivers/media/cec/cec-adap.c                  | 66 +++++--------------
 include/uapi/linux/cec.h                      |  3 +
 3 files changed, 44 insertions(+), 50 deletions(-)

diff --git a/Documentation/media/uapi/cec/cec-ioc-receive.rst b/Documentation/media/uapi/cec/cec-ioc-receive.rst
index e964074cd15b..b25e48afaa08 100644
--- a/Documentation/media/uapi/cec/cec-ioc-receive.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-receive.rst
@@ -16,10 +16,10 @@ CEC_RECEIVE, CEC_TRANSMIT - Receive or transmit a CEC message
 Synopsis
 ========
 
-.. c:function:: int ioctl( int fd, CEC_RECEIVE, struct cec_msg *argp )
+.. c:function:: int ioctl( int fd, CEC_RECEIVE, struct cec_msg \*argp )
     :name: CEC_RECEIVE
 
-.. c:function:: int ioctl( int fd, CEC_TRANSMIT, struct cec_msg *argp )
+.. c:function:: int ioctl( int fd, CEC_TRANSMIT, struct cec_msg \*argp )
     :name: CEC_TRANSMIT
 
 Arguments
@@ -272,6 +272,19 @@ View On' messages from initiator 0xf ('Unregistered') to destination 0 ('TV').
       - The transmit failed after one or more retries. This status bit is
 	mutually exclusive with :ref:`CEC_TX_STATUS_OK <CEC-TX-STATUS-OK>`.
 	Other bits can still be set to explain which failures were seen.
+    * .. _`CEC-TX-STATUS-ABORTED`:
+
+      - ``CEC_TX_STATUS_ABORTED``
+      - 0x40
+      - The transmit was aborted due to an HDMI disconnect, or the adapter
+        was unconfigured, or a transmit was interrupted, or the driver
+	returned an error when attempting to start a transmit.
+    * .. _`CEC-TX-STATUS-TIMEOUT`:
+
+      - ``CEC_TX_STATUS_TIMEOUT``
+      - 0x80
+      - The transmit timed out. This should not normally happen and this
+	indicates a driver problem.
 
 
 .. tabularcolumns:: |p{5.6cm}|p{0.9cm}|p{11.0cm}|
@@ -300,6 +313,14 @@ View On' messages from initiator 0xf ('Unregistered') to destination 0 ('TV').
       - The message was received successfully but the reply was
 	``CEC_MSG_FEATURE_ABORT``. This status is only set if this message
 	was the reply to an earlier transmitted message.
+    * .. _`CEC-RX-STATUS-ABORTED`:
+
+      - ``CEC_RX_STATUS_ABORTED``
+      - 0x08
+      - The wait for a reply to an earlier transmitted message was aborted
+        because the HDMI cable was disconnected, the adapter was unconfigured
+	or the :ref:`CEC_TRANSMIT <CEC_RECEIVE>` that waited for a
+	reply was interrupted.
 
 
 
diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 030b2602faf0..2ebb53fd4800 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -341,7 +341,7 @@ static void cec_data_completed(struct cec_data *data)
  *
  * This function is called with adap->lock held.
  */
-static void cec_data_cancel(struct cec_data *data)
+static void cec_data_cancel(struct cec_data *data, u8 tx_status)
 {
 	/*
 	 * It's either the current transmit, or it is a pending
@@ -356,13 +356,11 @@ static void cec_data_cancel(struct cec_data *data)
 	}
 
 	if (data->msg.tx_status & CEC_TX_STATUS_OK) {
-		/* Mark the canceled RX as a timeout */
 		data->msg.rx_ts = ktime_get_ns();
-		data->msg.rx_status = CEC_RX_STATUS_TIMEOUT;
+		data->msg.rx_status = CEC_RX_STATUS_ABORTED;
 	} else {
-		/* Mark the canceled TX as an error */
 		data->msg.tx_ts = ktime_get_ns();
-		data->msg.tx_status |= CEC_TX_STATUS_ERROR |
+		data->msg.tx_status |= tx_status |
 				       CEC_TX_STATUS_MAX_RETRIES;
 		data->msg.tx_error_cnt++;
 		data->attempts = 0;
@@ -390,15 +388,15 @@ static void cec_flush(struct cec_adapter *adap)
 	while (!list_empty(&adap->transmit_queue)) {
 		data = list_first_entry(&adap->transmit_queue,
 					struct cec_data, list);
-		cec_data_cancel(data);
+		cec_data_cancel(data, CEC_TX_STATUS_ABORTED);
 	}
 	if (adap->transmitting)
-		cec_data_cancel(adap->transmitting);
+		cec_data_cancel(adap->transmitting, CEC_TX_STATUS_ABORTED);
 
 	/* Cancel the pending timeout work. */
 	list_for_each_entry_safe(data, n, &adap->wait_queue, list) {
 		if (cancel_delayed_work(&data->work))
-			cec_data_cancel(data);
+			cec_data_cancel(data, CEC_TX_STATUS_OK);
 		/*
 		 * If cancel_delayed_work returned false, then
 		 * the cec_wait_timeout function is running,
@@ -474,12 +472,13 @@ int cec_thread_func(void *_adap)
 			 * so much traffic on the bus that the adapter was
 			 * unable to transmit for CEC_XFER_TIMEOUT_MS (2.1s).
 			 */
-			dprintk(1, "%s: message %*ph timed out\n", __func__,
+			pr_warn("cec-%s: message %*ph timed out\n", adap->name,
 				adap->transmitting->msg.len,
 				adap->transmitting->msg.msg);
 			adap->tx_timeouts++;
 			/* Just give up on this. */
-			cec_data_cancel(adap->transmitting);
+			cec_data_cancel(adap->transmitting,
+					CEC_TX_STATUS_TIMEOUT);
 			goto unlock;
 		}
 
@@ -530,7 +529,7 @@ int cec_thread_func(void *_adap)
 		/* Tell the adapter to transmit, cancel on error */
 		if (adap->ops->adap_transmit(adap, data->attempts,
 					     signal_free_time, &data->msg))
-			cec_data_cancel(data);
+			cec_data_cancel(data, CEC_TX_STATUS_ABORTED);
 
 unlock:
 		mutex_unlock(&adap->lock);
@@ -702,8 +701,6 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 {
 	struct cec_data *data;
 	u8 last_initiator = 0xff;
-	unsigned int timeout;
-	int res = 0;
 
 	msg->rx_ts = 0;
 	msg->tx_ts = 0;
@@ -845,48 +842,21 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 	if (!block)
 		return 0;
 
-	/*
-	 * If we don't get a completion before this time something is really
-	 * wrong and we time out.
-	 */
-	timeout = CEC_XFER_TIMEOUT_MS;
-	/* Add the requested timeout if we have to wait for a reply as well */
-	if (msg->timeout)
-		timeout += msg->timeout;
-
 	/*
 	 * Release the lock and wait, retake the lock afterwards.
 	 */
 	mutex_unlock(&adap->lock);
-	res = wait_for_completion_killable_timeout(&data->c,
-						   msecs_to_jiffies(timeout));
+	wait_for_completion_killable(&data->c);
 	mutex_lock(&adap->lock);
 
-	if (data->completed) {
-		/* The transmit completed (possibly with an error) */
-		*msg = data->msg;
-		kfree(data);
-		return 0;
-	}
-	/*
-	 * The wait for completion timed out or was interrupted, so mark this
-	 * as non-blocking and disconnect from the filehandle since it is
-	 * still 'in flight'. When it finally completes it will just drop the
-	 * result silently.
-	 */
-	data->blocking = false;
-	if (data->fh)
-		list_del(&data->xfer_list);
-	data->fh = NULL;
+	/* Cancel the transmit if it was interrupted */
+	if (!data->completed)
+		cec_data_cancel(data, CEC_TX_STATUS_ABORTED);
 
-	if (res == 0) { /* timed out */
-		/* Check if the reply or the transmit failed */
-		if (msg->timeout && (msg->tx_status & CEC_TX_STATUS_OK))
-			msg->rx_status = CEC_RX_STATUS_TIMEOUT;
-		else
-			msg->tx_status = CEC_TX_STATUS_MAX_RETRIES;
-	}
-	return res > 0 ? 0 : res;
+	/* The transmit completed (possibly with an error) */
+	*msg = data->msg;
+	kfree(data);
+	return 0;
 }
 
 /* Helper function to be used by drivers and this framework. */
diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
index 097fcd812471..3094af68b6e7 100644
--- a/include/uapi/linux/cec.h
+++ b/include/uapi/linux/cec.h
@@ -152,10 +152,13 @@ static inline void cec_msg_set_reply_to(struct cec_msg *msg,
 #define CEC_TX_STATUS_LOW_DRIVE		(1 << 3)
 #define CEC_TX_STATUS_ERROR		(1 << 4)
 #define CEC_TX_STATUS_MAX_RETRIES	(1 << 5)
+#define CEC_TX_STATUS_ABORTED		(1 << 6)
+#define CEC_TX_STATUS_TIMEOUT		(1 << 7)
 
 #define CEC_RX_STATUS_OK		(1 << 0)
 #define CEC_RX_STATUS_TIMEOUT		(1 << 1)
 #define CEC_RX_STATUS_FEATURE_ABORT	(1 << 2)
+#define CEC_RX_STATUS_ABORTED		(1 << 3)
 
 static inline int cec_msg_status_is_ok(const struct cec_msg *msg)
 {
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] adv7604: when the EDID is cleared, unconfigure CEC as well

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When there is no EDID the CEC adapter should be unconfigured as
well. So call cec_phys_addr_invalidate() when this happens.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/adv7604.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 668be2bca57a..3376d5cb05d5 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2284,8 +2284,10 @@ static int adv76xx_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
 		state->aspect_ratio.numerator = 16;
 		state->aspect_ratio.denominator = 9;
 
-		if (!state->edid.present)
+		if (!state->edid.present) {
 			state->edid.blocks = 0;
+			cec_phys_addr_invalidate(state->cec_adap);
+		}
 
 		v4l2_dbg(2, debug, sd, "%s: clear EDID pad %d, edid.present = 0x%x\n",
 				__func__, edid->pad, state->edid.present);
-- 
2.18.0

[PATCH 2/5] adv7604: when the EDID is cleared, unconfigure CEC as well

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When there is no EDID the CEC adapter should be unconfigured as
well. So call cec_phys_addr_invalidate() when this happens.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/adv7604.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 668be2bca57a..3376d5cb05d5 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2284,8 +2284,10 @@ static int adv76xx_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
 		state->aspect_ratio.numerator = 16;
 		state->aspect_ratio.denominator = 9;
 
-		if (!state->edid.present)
+		if (!state->edid.present) {
 			state->edid.blocks = 0;
+			cec_phys_addr_invalidate(state->cec_adap);
+		}
 
 		v4l2_dbg(2, debug, sd, "%s: clear EDID pad %d, edid.present = 0x%x\n",
 				__func__, edid->pad, state->edid.present);
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] adv7842: when the EDID is cleared, unconfigure CEC as well

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When there is no EDID the CEC adapter should be unconfigured as
well. So call cec_phys_addr_invalidate() when this happens.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/adv7842.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index 4f8fbdd00e35..71fe56565f75 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -786,8 +786,10 @@ static int edid_write_hdmi_segment(struct v4l2_subdev *sd, u8 port)
 	/* Disable I2C access to internal EDID ram from HDMI DDC ports */
 	rep_write_and_or(sd, 0x77, 0xf3, 0x00);
 
-	if (!state->hdmi_edid.present)
+	if (!state->hdmi_edid.present) {
+		cec_phys_addr_invalidate(state->cec_adap);
 		return 0;
+	}
 
 	pa = cec_get_edid_phys_addr(edid, 256, &spa_loc);
 	err = cec_phys_addr_validate(pa, &pa, NULL);
-- 
2.18.0

[PATCH 3/5] adv7842: when the EDID is cleared, unconfigure CEC as well

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When there is no EDID the CEC adapter should be unconfigured as
well. So call cec_phys_addr_invalidate() when this happens.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/adv7842.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index 4f8fbdd00e35..71fe56565f75 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -786,8 +786,10 @@ static int edid_write_hdmi_segment(struct v4l2_subdev *sd, u8 port)
 	/* Disable I2C access to internal EDID ram from HDMI DDC ports */
 	rep_write_and_or(sd, 0x77, 0xf3, 0x00);
 
-	if (!state->hdmi_edid.present)
+	if (!state->hdmi_edid.present) {
+		cec_phys_addr_invalidate(state->cec_adap);
 		return 0;
+	}
 
 	pa = cec_get_edid_phys_addr(edid, 256, &spa_loc);
 	err = cec_phys_addr_validate(pa, &pa, NULL);
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The TX FIFO has to be cleared if the transmit failed due to e.g.
a NACK condition, otherwise the hardware will keep trying to
transmit the message.

An attempt was made to do this, but it was done after the call to
cec_transmit_done, which can cause a race condition since the call
to cec_transmit_done can cause a new transmit to be issued, and
then attempting to clear the TX FIFO will actually clear the new
transmit instead of the old transmit and the new transmit simply
never happens.

By clearing the FIFO before transmit_done is called this race
is fixed.

Note that there is no reason to clear the FIFO if the transmit
was successful, so the attempt to clear the FIFO in that case
was dropped.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
index 340383150fb9..dee66a5101b5 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
@@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
 	}
 }
 
+static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
+{
+	struct hdmi_core_data *core = cec_get_drvdata(adap);
+	int retry = HDMI_CORE_CEC_RETRY;
+	int temp;
+
+	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
+	while (retry) {
+		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
+		if (FLD_GET(temp, 7, 7) == 0)
+			break;
+		retry--;
+	}
+	return retry != 0;
+}
+
 void hdmi4_cec_irq(struct hdmi_core_data *core)
 {
 	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
@@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
 	if (stat0 & 0x20) {
 		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
 				  0, 0, 0, 0);
-		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
 	} else if (stat1 & 0x02) {
 		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
 
+		hdmi_cec_clear_tx_fifo(core->adap);
 		cec_transmit_done(core->adap,
 				  CEC_TX_STATUS_NACK |
 				  CEC_TX_STATUS_MAX_RETRIES,
 				  0, (dbg3 >> 4) & 7, 0, 0);
-		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
 	}
 	if (stat0 & 0x02)
 		hdmi_cec_received_msg(core);
 }
 
-static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
-{
-	struct hdmi_core_data *core = cec_get_drvdata(adap);
-	int retry = HDMI_CORE_CEC_RETRY;
-	int temp;
-
-	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
-	while (retry) {
-		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
-		if (FLD_GET(temp, 7, 7) == 0)
-			break;
-		retry--;
-	}
-	return retry != 0;
-}
-
 static bool hdmi_cec_clear_rx_fifo(struct cec_adapter *adap)
 {
 	struct hdmi_core_data *core = cec_get_drvdata(adap);
-- 
2.18.0

[PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The TX FIFO has to be cleared if the transmit failed due to e.g.
a NACK condition, otherwise the hardware will keep trying to
transmit the message.

An attempt was made to do this, but it was done after the call to
cec_transmit_done, which can cause a race condition since the call
to cec_transmit_done can cause a new transmit to be issued, and
then attempting to clear the TX FIFO will actually clear the new
transmit instead of the old transmit and the new transmit simply
never happens.

By clearing the FIFO before transmit_done is called this race
is fixed.

Note that there is no reason to clear the FIFO if the transmit
was successful, so the attempt to clear the FIFO in that case
was dropped.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
index 340383150fb9..dee66a5101b5 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
@@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
 	}
 }
 
+static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
+{
+	struct hdmi_core_data *core = cec_get_drvdata(adap);
+	int retry = HDMI_CORE_CEC_RETRY;
+	int temp;
+
+	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
+	while (retry) {
+		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
+		if (FLD_GET(temp, 7, 7) == 0)
+			break;
+		retry--;
+	}
+	return retry != 0;
+}
+
 void hdmi4_cec_irq(struct hdmi_core_data *core)
 {
 	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
@@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
 	if (stat0 & 0x20) {
 		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
 				  0, 0, 0, 0);
-		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
 	} else if (stat1 & 0x02) {
 		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
 
+		hdmi_cec_clear_tx_fifo(core->adap);
 		cec_transmit_done(core->adap,
 				  CEC_TX_STATUS_NACK |
 				  CEC_TX_STATUS_MAX_RETRIES,
 				  0, (dbg3 >> 4) & 7, 0, 0);
-		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
 	}
 	if (stat0 & 0x02)
 		hdmi_cec_received_msg(core);
 }
 
-static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
-{
-	struct hdmi_core_data *core = cec_get_drvdata(adap);
-	int retry = HDMI_CORE_CEC_RETRY;
-	int temp;
-
-	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
-	while (retry) {
-		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
-		if (FLD_GET(temp, 7, 7) == 0)
-			break;
-		retry--;
-	}
-	return retry != 0;
-}
-
 static bool hdmi_cec_clear_rx_fifo(struct cec_adapter *adap)
 {
 	struct hdmi_core_data *core = cec_get_drvdata(adap);
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] omapdrm/dss/hdmi4_cec.c: don't set the retransmit count

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The HDMI_CEC_DBG_3 register does have a retransmit count, but you
can't write to it, those bits are read-only.

So drop the attempt to set the retransmit count, since it doesn't
work.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
index dee66a5101b5..00407f1995a8 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
@@ -280,9 +280,6 @@ static int hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	hdmi_write_reg(core->base, HDMI_CEC_INT_STATUS_1,
 		       HDMI_CEC_RETRANSMIT_CNT_INT_MASK);
 
-	/* Set the retry count */
-	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, attempts - 1, 6, 4);
-
 	/* Set the initiator addresses */
 	hdmi_write_reg(core->base, HDMI_CEC_TX_INIT, cec_msg_initiator(msg));
 
-- 
2.18.0

[PATCH 5/5] omapdrm/dss/hdmi4_cec.c: don't set the retransmit count

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The HDMI_CEC_DBG_3 register does have a retransmit count, but you
can't write to it, those bits are read-only.

So drop the attempt to set the retransmit count, since it doesn't
work.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
index dee66a5101b5..00407f1995a8 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
@@ -280,9 +280,6 @@ static int hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	hdmi_write_reg(core->base, HDMI_CEC_INT_STATUS_1,
 		       HDMI_CEC_RETRANSMIT_CNT_INT_MASK);
 
-	/* Set the retry count */
-	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, attempts - 1, 6, 4);
-
 	/* Set the initiator addresses */
 	hdmi_write_reg(core->base, HDMI_CEC_TX_INIT, cec_msg_initiator(msg));
 
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Hans Verkuil]

Tomi,

Can you review this patch and the next? They should go to 4.20.
This patch in particular is a nasty one, hard to reproduce.

This patch should also be Cc-ed to stable for 4.15 and up.

Tracking down randomly disappearing CEC transmits was no fun :-(

Regards,

	Hans

On 10/04/18 11:08, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The TX FIFO has to be cleared if the transmit failed due to e.g.
> a NACK condition, otherwise the hardware will keep trying to
> transmit the message.
> 
> An attempt was made to do this, but it was done after the call to
> cec_transmit_done, which can cause a race condition since the call
> to cec_transmit_done can cause a new transmit to be issued, and
> then attempting to clear the TX FIFO will actually clear the new
> transmit instead of the old transmit and the new transmit simply
> never happens.
> 
> By clearing the FIFO before transmit_done is called this race
> is fixed.
> 
> Note that there is no reason to clear the FIFO if the transmit
> was successful, so the attempt to clear the FIFO in that case
> was dropped.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> index 340383150fb9..dee66a5101b5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
>  	}
>  }
>  
> +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
> +{
> +	struct hdmi_core_data *core = cec_get_drvdata(adap);
> +	int retry = HDMI_CORE_CEC_RETRY;
> +	int temp;
> +
> +	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
> +	while (retry) {
> +		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
> +		if (FLD_GET(temp, 7, 7) == 0)
> +			break;
> +		retry--;
> +	}
> +	return retry != 0;
> +}
> +
>  void hdmi4_cec_irq(struct hdmi_core_data *core)
>  {
>  	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
> @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
>  	if (stat0 & 0x20) {
>  		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
>  				  0, 0, 0, 0);
> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>  	} else if (stat1 & 0x02) {
>  		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>  
> +		hdmi_cec_clear_tx_fifo(core->adap);
>  		cec_transmit_done(core->adap,
>  				  CEC_TX_STATUS_NACK |
>  				  CEC_TX_STATUS_MAX_RETRIES,
>  				  0, (dbg3 >> 4) & 7, 0, 0);
> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>  	}
>  	if (stat0 & 0x02)
>  		hdmi_cec_received_msg(core);
>  }
>  
> -static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
> -{
> -	struct hdmi_core_data *core = cec_get_drvdata(adap);
> -	int retry = HDMI_CORE_CEC_RETRY;
> -	int temp;
> -
> -	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
> -	while (retry) {
> -		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
> -		if (FLD_GET(temp, 7, 7) == 0)
> -			break;
> -		retry--;
> -	}
> -	return retry != 0;
> -}
> -
>  static bool hdmi_cec_clear_rx_fifo(struct cec_adapter *adap)
>  {
>  	struct hdmi_core_data *core = cec_get_drvdata(adap);
> 

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Hans Verkuil]

Tomi,

Can you review this patch and the next? They should go to 4.20.
This patch in particular is a nasty one, hard to reproduce.

This patch should also be Cc-ed to stable for 4.15 and up.

Tracking down randomly disappearing CEC transmits was no fun :-(

Regards,

	Hans

On 10/04/18 11:08, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The TX FIFO has to be cleared if the transmit failed due to e.g.
> a NACK condition, otherwise the hardware will keep trying to
> transmit the message.
> 
> An attempt was made to do this, but it was done after the call to
> cec_transmit_done, which can cause a race condition since the call
> to cec_transmit_done can cause a new transmit to be issued, and
> then attempting to clear the TX FIFO will actually clear the new
> transmit instead of the old transmit and the new transmit simply
> never happens.
> 
> By clearing the FIFO before transmit_done is called this race
> is fixed.
> 
> Note that there is no reason to clear the FIFO if the transmit
> was successful, so the attempt to clear the FIFO in that case
> was dropped.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> index 340383150fb9..dee66a5101b5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
>  	}
>  }
>  
> +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
> +{
> +	struct hdmi_core_data *core = cec_get_drvdata(adap);
> +	int retry = HDMI_CORE_CEC_RETRY;
> +	int temp;
> +
> +	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
> +	while (retry) {
> +		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
> +		if (FLD_GET(temp, 7, 7) == 0)
> +			break;
> +		retry--;
> +	}
> +	return retry != 0;
> +}
> +
>  void hdmi4_cec_irq(struct hdmi_core_data *core)
>  {
>  	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
> @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
>  	if (stat0 & 0x20) {
>  		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
>  				  0, 0, 0, 0);
> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>  	} else if (stat1 & 0x02) {
>  		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>  
> +		hdmi_cec_clear_tx_fifo(core->adap);
>  		cec_transmit_done(core->adap,
>  				  CEC_TX_STATUS_NACK |
>  				  CEC_TX_STATUS_MAX_RETRIES,
>  				  0, (dbg3 >> 4) & 7, 0, 0);
> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>  	}
>  	if (stat0 & 0x02)
>  		hdmi_cec_received_msg(core);
>  }
>  
> -static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
> -{
> -	struct hdmi_core_data *core = cec_get_drvdata(adap);
> -	int retry = HDMI_CORE_CEC_RETRY;
> -	int temp;
> -
> -	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
> -	while (retry) {
> -		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
> -		if (FLD_GET(temp, 7, 7) == 0)
> -			break;
> -		retry--;
> -	}
> -	return retry != 0;
> -}
> -
>  static bool hdmi_cec_clear_rx_fifo(struct cec_adapter *adap)
>  {
>  	struct hdmi_core_data *core = cec_get_drvdata(adap);
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Tomi Valkeinen]

Hi Hans,

On 04/10/18 12:08, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The TX FIFO has to be cleared if the transmit failed due to e.g.
> a NACK condition, otherwise the hardware will keep trying to
> transmit the message.
> 
> An attempt was made to do this, but it was done after the call to
> cec_transmit_done, which can cause a race condition since the call
> to cec_transmit_done can cause a new transmit to be issued, and
> then attempting to clear the TX FIFO will actually clear the new
> transmit instead of the old transmit and the new transmit simply
> never happens.
> 
> By clearing the FIFO before transmit_done is called this race
> is fixed.
> 
> Note that there is no reason to clear the FIFO if the transmit
> was successful, so the attempt to clear the FIFO in that case
> was dropped.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> index 340383150fb9..dee66a5101b5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
>  	}
>  }
>  
> +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
> +{
> +	struct hdmi_core_data *core = cec_get_drvdata(adap);
> +	int retry = HDMI_CORE_CEC_RETRY;
> +	int temp;
> +
> +	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
> +	while (retry) {
> +		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
> +		if (FLD_GET(temp, 7, 7) == 0)
> +			break;

This is fine, but as you're using the helper macros already, there's
REG_GET:

REG_GET(core->base, HDMI_CEC_DBG_3, 7, 7)

which removes the need for temp. Are you sure this works reliably?
Usually when polling a register bit, I like to measure real-world-time
in some way to ensure I actually poll for a certain amount of time.

And just a matter of opinion, but I would've written:

while (retry) {
	if (!REG_GET(..))
		return true;
	retry--;
}

return false;

> +		retry--;
> +	}
> +	return retry != 0;
> +}
> +
>  void hdmi4_cec_irq(struct hdmi_core_data *core)
>  {
>  	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
> @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
>  	if (stat0 & 0x20) {
>  		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
>  				  0, 0, 0, 0);
> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>  	} else if (stat1 & 0x02) {
>  		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>  
> +		hdmi_cec_clear_tx_fifo(core->adap);

Would a dev_err be ok here?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Tomi Valkeinen]

Hi Hans,

On 04/10/18 12:08, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The TX FIFO has to be cleared if the transmit failed due to e.g.
> a NACK condition, otherwise the hardware will keep trying to
> transmit the message.
> 
> An attempt was made to do this, but it was done after the call to
> cec_transmit_done, which can cause a race condition since the call
> to cec_transmit_done can cause a new transmit to be issued, and
> then attempting to clear the TX FIFO will actually clear the new
> transmit instead of the old transmit and the new transmit simply
> never happens.
> 
> By clearing the FIFO before transmit_done is called this race
> is fixed.
> 
> Note that there is no reason to clear the FIFO if the transmit
> was successful, so the attempt to clear the FIFO in that case
> was dropped.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> index 340383150fb9..dee66a5101b5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
>  	}
>  }
>  
> +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
> +{
> +	struct hdmi_core_data *core = cec_get_drvdata(adap);
> +	int retry = HDMI_CORE_CEC_RETRY;
> +	int temp;
> +
> +	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
> +	while (retry) {
> +		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
> +		if (FLD_GET(temp, 7, 7) == 0)
> +			break;

This is fine, but as you're using the helper macros already, there's
REG_GET:

REG_GET(core->base, HDMI_CEC_DBG_3, 7, 7)

which removes the need for temp. Are you sure this works reliably?
Usually when polling a register bit, I like to measure real-world-time
in some way to ensure I actually poll for a certain amount of time.

And just a matter of opinion, but I would've written:

while (retry) {
	if (!REG_GET(..))
		return true;
	retry--;
}

return false;

> +		retry--;
> +	}
> +	return retry != 0;
> +}
> +
>  void hdmi4_cec_irq(struct hdmi_core_data *core)
>  {
>  	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
> @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
>  	if (stat0 & 0x20) {
>  		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
>  				  0, 0, 0, 0);
> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>  	} else if (stat1 & 0x02) {
>  		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>  
> +		hdmi_cec_clear_tx_fifo(core->adap);

Would a dev_err be ok here?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] omapdrm/dss/hdmi4_cec.c: don't set the retransmit count

[Tomi Valkeinen]

On 04/10/18 12:09, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The HDMI_CEC_DBG_3 register does have a retransmit count, but you
> can't write to it, those bits are read-only.
> 
> So drop the attempt to set the retransmit count, since it doesn't
> work.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> index dee66a5101b5..00407f1995a8 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> @@ -280,9 +280,6 @@ static int hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>  	hdmi_write_reg(core->base, HDMI_CEC_INT_STATUS_1,
>  		       HDMI_CEC_RETRANSMIT_CNT_INT_MASK);
>  
> -	/* Set the retry count */
> -	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, attempts - 1, 6, 4);
> -

I presume there's no harm in having a different retry count in the HW
than what was requested via the API?

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 5/5] omapdrm/dss/hdmi4_cec.c: don't set the retransmit count

[Tomi Valkeinen]

On 04/10/18 12:09, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The HDMI_CEC_DBG_3 register does have a retransmit count, but you
> can't write to it, those bits are read-only.
> 
> So drop the attempt to set the retransmit count, since it doesn't
> work.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> index dee66a5101b5..00407f1995a8 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
> @@ -280,9 +280,6 @@ static int hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>  	hdmi_write_reg(core->base, HDMI_CEC_INT_STATUS_1,
>  		       HDMI_CEC_RETRANSMIT_CNT_INT_MASK);
>  
> -	/* Set the retry count */
> -	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, attempts - 1, 6, 4);
> -

I presume there's no harm in having a different retry count in the HW
than what was requested via the API?

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Tomi Valkeinen]

On 05/10/18 17:13, Hans Verkuil wrote:
> Tomi,
> 
> Can you review this patch and the next? They should go to 4.20.
> This patch in particular is a nasty one, hard to reproduce.
> 
> This patch should also be Cc-ed to stable for 4.15 and up.

Done. There's no dependency from the omapdrm patches to the first patch,
is there? I.e. I can apply these via omapdrm?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Tomi Valkeinen]

On 05/10/18 17:13, Hans Verkuil wrote:
> Tomi,
> 
> Can you review this patch and the next? They should go to 4.20.
> This patch in particular is a nasty one, hard to reproduce.
> 
> This patch should also be Cc-ed to stable for 4.15 and up.

Done. There's no dependency from the omapdrm patches to the first patch,
is there? I.e. I can apply these via omapdrm?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Hans Verkuil]

On 10/08/2018 02:45 PM, Tomi Valkeinen wrote:
> Hi Hans,
> 
> On 04/10/18 12:08, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The TX FIFO has to be cleared if the transmit failed due to e.g.
>> a NACK condition, otherwise the hardware will keep trying to
>> transmit the message.
>>
>> An attempt was made to do this, but it was done after the call to
>> cec_transmit_done, which can cause a race condition since the call
>> to cec_transmit_done can cause a new transmit to be issued, and
>> then attempting to clear the TX FIFO will actually clear the new
>> transmit instead of the old transmit and the new transmit simply
>> never happens.
>>
>> By clearing the FIFO before transmit_done is called this race
>> is fixed.
>>
>> Note that there is no reason to clear the FIFO if the transmit
>> was successful, so the attempt to clear the FIFO in that case
>> was dropped.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
>>  1 file changed, 17 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> index 340383150fb9..dee66a5101b5 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
>>  	}
>>  }
>>  
>> +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
>> +{
>> +	struct hdmi_core_data *core = cec_get_drvdata(adap);
>> +	int retry = HDMI_CORE_CEC_RETRY;
>> +	int temp;
>> +
>> +	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>> +	while (retry) {
>> +		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>> +		if (FLD_GET(temp, 7, 7) == 0)
>> +			break;
> 
> This is fine, but as you're using the helper macros already, there's
> REG_GET:
> 
> REG_GET(core->base, HDMI_CEC_DBG_3, 7, 7)
> 
> which removes the need for temp. Are you sure this works reliably?
> Usually when polling a register bit, I like to measure real-world-time
> in some way to ensure I actually poll for a certain amount of time.

I'll add a bit of debugging to double-check but as far as I remember this
is very fast and adding delays is overkill.

FYI: we (Cisco) use this code in our products and we'd have seen it if this
would fail.

> 
> And just a matter of opinion, but I would've written:
> 
> while (retry) {
> 	if (!REG_GET(..))
> 		return true;
> 	retry--;
> }
> 
> return false;
> 
>> +		retry--;
>> +	}
>> +	return retry != 0;
>> +}
>> +

In this patch I just moved up the hdmi_cec_clear_tx_fifo so I can use it in
hdmi4_cec_irq. I rather not make any changes to that function.

Unless you object I prefer to make a new patch for 4.21 to improve it.

>>  void hdmi4_cec_irq(struct hdmi_core_data *core)
>>  {
>>  	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
>> @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
>>  	if (stat0 & 0x20) {
>>  		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
>>  				  0, 0, 0, 0);
>> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>>  	} else if (stat1 & 0x02) {
>>  		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>>  
>> +		hdmi_cec_clear_tx_fifo(core->adap);
> 
> Would a dev_err be ok here?

Hmm. I would prefer a dev_err_once. Chances are that if this ever fails, it
might fail continuously (as in: something is very seriously wrong), and you
don't want that in an irq function.

Regards,

	Hans

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Hans Verkuil]

On 10/08/2018 02:45 PM, Tomi Valkeinen wrote:
> Hi Hans,
> 
> On 04/10/18 12:08, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The TX FIFO has to be cleared if the transmit failed due to e.g.
>> a NACK condition, otherwise the hardware will keep trying to
>> transmit the message.
>>
>> An attempt was made to do this, but it was done after the call to
>> cec_transmit_done, which can cause a race condition since the call
>> to cec_transmit_done can cause a new transmit to be issued, and
>> then attempting to clear the TX FIFO will actually clear the new
>> transmit instead of the old transmit and the new transmit simply
>> never happens.
>>
>> By clearing the FIFO before transmit_done is called this race
>> is fixed.
>>
>> Note that there is no reason to clear the FIFO if the transmit
>> was successful, so the attempt to clear the FIFO in that case
>> was dropped.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
>>  1 file changed, 17 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> index 340383150fb9..dee66a5101b5 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
>>  	}
>>  }
>>  
>> +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
>> +{
>> +	struct hdmi_core_data *core = cec_get_drvdata(adap);
>> +	int retry = HDMI_CORE_CEC_RETRY;
>> +	int temp;
>> +
>> +	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>> +	while (retry) {
>> +		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>> +		if (FLD_GET(temp, 7, 7) == 0)
>> +			break;
> 
> This is fine, but as you're using the helper macros already, there's
> REG_GET:
> 
> REG_GET(core->base, HDMI_CEC_DBG_3, 7, 7)
> 
> which removes the need for temp. Are you sure this works reliably?
> Usually when polling a register bit, I like to measure real-world-time
> in some way to ensure I actually poll for a certain amount of time.

I'll add a bit of debugging to double-check but as far as I remember this
is very fast and adding delays is overkill.

FYI: we (Cisco) use this code in our products and we'd have seen it if this
would fail.

> 
> And just a matter of opinion, but I would've written:
> 
> while (retry) {
> 	if (!REG_GET(..))
> 		return true;
> 	retry--;
> }
> 
> return false;
> 
>> +		retry--;
>> +	}
>> +	return retry != 0;
>> +}
>> +

In this patch I just moved up the hdmi_cec_clear_tx_fifo so I can use it in
hdmi4_cec_irq. I rather not make any changes to that function.

Unless you object I prefer to make a new patch for 4.21 to improve it.

>>  void hdmi4_cec_irq(struct hdmi_core_data *core)
>>  {
>>  	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
>> @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
>>  	if (stat0 & 0x20) {
>>  		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
>>  				  0, 0, 0, 0);
>> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>>  	} else if (stat1 & 0x02) {
>>  		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>>  
>> +		hdmi_cec_clear_tx_fifo(core->adap);
> 
> Would a dev_err be ok here?

Hmm. I would prefer a dev_err_once. Chances are that if this ever fails, it
might fail continuously (as in: something is very seriously wrong), and you
don't want that in an irq function.

Regards,

	Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Hans Verkuil]

On 10/08/2018 02:52 PM, Tomi Valkeinen wrote:
> 
> On 05/10/18 17:13, Hans Verkuil wrote:
>> Tomi,
>>
>> Can you review this patch and the next? They should go to 4.20.
>> This patch in particular is a nasty one, hard to reproduce.
>>
>> This patch should also be Cc-ed to stable for 4.15 and up.
> 
> Done. There's no dependency from the omapdrm patches to the first patch,
> is there? I.e. I can apply these via omapdrm?

Correct.

Regards,

	Hans

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Hans Verkuil]

On 10/08/2018 02:52 PM, Tomi Valkeinen wrote:
> 
> On 05/10/18 17:13, Hans Verkuil wrote:
>> Tomi,
>>
>> Can you review this patch and the next? They should go to 4.20.
>> This patch in particular is a nasty one, hard to reproduce.
>>
>> This patch should also be Cc-ed to stable for 4.15 and up.
> 
> Done. There's no dependency from the omapdrm patches to the first patch,
> is there? I.e. I can apply these via omapdrm?

Correct.

Regards,

	Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] omapdrm/dss/hdmi4_cec.c: don't set the retransmit count

[Hans Verkuil]

On 10/08/2018 02:47 PM, Tomi Valkeinen wrote:
> On 04/10/18 12:09, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The HDMI_CEC_DBG_3 register does have a retransmit count, but you
>> can't write to it, those bits are read-only.
>>
>> So drop the attempt to set the retransmit count, since it doesn't
>> work.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> index dee66a5101b5..00407f1995a8 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> @@ -280,9 +280,6 @@ static int hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>  	hdmi_write_reg(core->base, HDMI_CEC_INT_STATUS_1,
>>  		       HDMI_CEC_RETRANSMIT_CNT_INT_MASK);
>>  
>> -	/* Set the retry count */
>> -	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, attempts - 1, 6, 4);
>> -
> 
> I presume there's no harm in having a different retry count in the HW
> than what was requested via the API?

Correct.

Some CEC HW implementations expect that the software calculates the retry count,
but the omap4 does this in hardware (not quite optimally, I'm afraid) and those
just ignore the argument.

Regards,

	Hans

> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
>  Tomi
> 

Re: [PATCH 5/5] omapdrm/dss/hdmi4_cec.c: don't set the retransmit count

[Hans Verkuil]

On 10/08/2018 02:47 PM, Tomi Valkeinen wrote:
> On 04/10/18 12:09, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The HDMI_CEC_DBG_3 register does have a retransmit count, but you
>> can't write to it, those bits are read-only.
>>
>> So drop the attempt to set the retransmit count, since it doesn't
>> work.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> index dee66a5101b5..00407f1995a8 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>> @@ -280,9 +280,6 @@ static int hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>  	hdmi_write_reg(core->base, HDMI_CEC_INT_STATUS_1,
>>  		       HDMI_CEC_RETRANSMIT_CNT_INT_MASK);
>>  
>> -	/* Set the retry count */
>> -	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, attempts - 1, 6, 4);
>> -
> 
> I presume there's no harm in having a different retry count in the HW
> than what was requested via the API?

Correct.

Some CEC HW implementations expect that the software calculates the retry count,
but the omap4 does this in hardware (not quite optimally, I'm afraid) and those
just ignore the argument.

Regards,

	Hans

> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
>  Tomi
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Tomi Valkeinen]

On 08/10/18 15:55, Hans Verkuil wrote:
> On 10/08/2018 02:45 PM, Tomi Valkeinen wrote:
>> Hi Hans,
>>
>> On 04/10/18 12:08, Hans Verkuil wrote:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> The TX FIFO has to be cleared if the transmit failed due to e.g.
>>> a NACK condition, otherwise the hardware will keep trying to
>>> transmit the message.
>>>
>>> An attempt was made to do this, but it was done after the call to
>>> cec_transmit_done, which can cause a race condition since the call
>>> to cec_transmit_done can cause a new transmit to be issued, and
>>> then attempting to clear the TX FIFO will actually clear the new
>>> transmit instead of the old transmit and the new transmit simply
>>> never happens.
>>>
>>> By clearing the FIFO before transmit_done is called this race
>>> is fixed.
>>>
>>> Note that there is no reason to clear the FIFO if the transmit
>>> was successful, so the attempt to clear the FIFO in that case
>>> was dropped.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>> ---
>>>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
>>>  1 file changed, 17 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>>> index 340383150fb9..dee66a5101b5 100644
>>> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>>> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>>> @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
>>>  	}
>>>  }
>>>  
>>> +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
>>> +{
>>> +	struct hdmi_core_data *core = cec_get_drvdata(adap);
>>> +	int retry = HDMI_CORE_CEC_RETRY;
>>> +	int temp;
>>> +
>>> +	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>>> +	while (retry) {
>>> +		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>>> +		if (FLD_GET(temp, 7, 7) == 0)
>>> +			break;
>>
>> This is fine, but as you're using the helper macros already, there's
>> REG_GET:
>>
>> REG_GET(core->base, HDMI_CEC_DBG_3, 7, 7)
>>
>> which removes the need for temp. Are you sure this works reliably?
>> Usually when polling a register bit, I like to measure real-world-time
>> in some way to ensure I actually poll for a certain amount of time.
> 
> I'll add a bit of debugging to double-check but as far as I remember this
> is very fast and adding delays is overkill.

I agree, it's very unlikely that this would not work. Loops like this
just make me a bit uneasy =).

As we will catch the error via the return value, I think it's not worth
adding real-time tracking here, unless problems start to show up.

> FYI: we (Cisco) use this code in our products and we'd have seen it if this
> would fail.
> 
>>
>> And just a matter of opinion, but I would've written:
>>
>> while (retry) {
>> 	if (!REG_GET(..))
>> 		return true;
>> 	retry--;
>> }
>>
>> return false;
>>
>>> +		retry--;
>>> +	}
>>> +	return retry != 0;
>>> +}
>>> +
> 
> In this patch I just moved up the hdmi_cec_clear_tx_fifo so I can use it in
> hdmi4_cec_irq. I rather not make any changes to that function.
> 
> Unless you object I prefer to make a new patch for 4.21 to improve it.

Sounds fine.

> 
>>>  void hdmi4_cec_irq(struct hdmi_core_data *core)
>>>  {
>>>  	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
>>> @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
>>>  	if (stat0 & 0x20) {
>>>  		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
>>>  				  0, 0, 0, 0);
>>> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>>>  	} else if (stat1 & 0x02) {
>>>  		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>>>  
>>> +		hdmi_cec_clear_tx_fifo(core->adap);
>>
>> Would a dev_err be ok here?
> 
> Hmm. I would prefer a dev_err_once. Chances are that if this ever fails, it
> might fail continuously (as in: something is very seriously wrong), and you
> don't want that in an irq function.

That's ok. As long as there's some indication that we're having an
issue. Or do you think some other part will break and print errors, and
it's easy to pinpoint to the fifo clearing? Well, I don't even know how
the fifo clearing could break....

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done

[Tomi Valkeinen]

On 08/10/18 15:55, Hans Verkuil wrote:
> On 10/08/2018 02:45 PM, Tomi Valkeinen wrote:
>> Hi Hans,
>>
>> On 04/10/18 12:08, Hans Verkuil wrote:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> The TX FIFO has to be cleared if the transmit failed due to e.g.
>>> a NACK condition, otherwise the hardware will keep trying to
>>> transmit the message.
>>>
>>> An attempt was made to do this, but it was done after the call to
>>> cec_transmit_done, which can cause a race condition since the call
>>> to cec_transmit_done can cause a new transmit to be issued, and
>>> then attempting to clear the TX FIFO will actually clear the new
>>> transmit instead of the old transmit and the new transmit simply
>>> never happens.
>>>
>>> By clearing the FIFO before transmit_done is called this race
>>> is fixed.
>>>
>>> Note that there is no reason to clear the FIFO if the transmit
>>> was successful, so the attempt to clear the FIFO in that case
>>> was dropped.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>> ---
>>>  drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++-------------
>>>  1 file changed, 17 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>>> index 340383150fb9..dee66a5101b5 100644
>>> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>>> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
>>> @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core)
>>>  	}
>>>  }
>>>  
>>> +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap)
>>> +{
>>> +	struct hdmi_core_data *core = cec_get_drvdata(adap);
>>> +	int retry = HDMI_CORE_CEC_RETRY;
>>> +	int temp;
>>> +
>>> +	REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>>> +	while (retry) {
>>> +		temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>>> +		if (FLD_GET(temp, 7, 7) == 0)
>>> +			break;
>>
>> This is fine, but as you're using the helper macros already, there's
>> REG_GET:
>>
>> REG_GET(core->base, HDMI_CEC_DBG_3, 7, 7)
>>
>> which removes the need for temp. Are you sure this works reliably?
>> Usually when polling a register bit, I like to measure real-world-time
>> in some way to ensure I actually poll for a certain amount of time.
> 
> I'll add a bit of debugging to double-check but as far as I remember this
> is very fast and adding delays is overkill.

I agree, it's very unlikely that this would not work. Loops like this
just make me a bit uneasy =).

As we will catch the error via the return value, I think it's not worth
adding real-time tracking here, unless problems start to show up.

> FYI: we (Cisco) use this code in our products and we'd have seen it if this
> would fail.
> 
>>
>> And just a matter of opinion, but I would've written:
>>
>> while (retry) {
>> 	if (!REG_GET(..))
>> 		return true;
>> 	retry--;
>> }
>>
>> return false;
>>
>>> +		retry--;
>>> +	}
>>> +	return retry != 0;
>>> +}
>>> +
> 
> In this patch I just moved up the hdmi_cec_clear_tx_fifo so I can use it in
> hdmi4_cec_irq. I rather not make any changes to that function.
> 
> Unless you object I prefer to make a new patch for 4.21 to improve it.

Sounds fine.

> 
>>>  void hdmi4_cec_irq(struct hdmi_core_data *core)
>>>  {
>>>  	u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0);
>>> @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core)
>>>  	if (stat0 & 0x20) {
>>>  		cec_transmit_done(core->adap, CEC_TX_STATUS_OK,
>>>  				  0, 0, 0, 0);
>>> -		REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7);
>>>  	} else if (stat1 & 0x02) {
>>>  		u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3);
>>>  
>>> +		hdmi_cec_clear_tx_fifo(core->adap);
>>
>> Would a dev_err be ok here?
> 
> Hmm. I would prefer a dev_err_once. Chances are that if this ever fails, it
> might fail continuously (as in: something is very seriously wrong), and you
> don't want that in an irq function.

That's ok. As long as there's some indication that we're having an
issue. Or do you think some other part will break and print errors, and
it's easy to pinpoint to the fifo clearing? Well, I don't even know how
the fifo clearing could break....

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel