From: kuro chung <[email protected]>
ITE added a FIFO reset bit for input video. When system power resume,
the TTL input of it6505 may get some noise before video signal stable
and the hardware function reset is required.
But the input FIFO reset will also trigger error interrupts of output module rising.
Thus, it6505 have to wait a period can clear those expected error interrupts
caused by manual hardware reset in one interrupt handler calling to avoid interrupt looping.
Signed-off-by: Allen Chen <[email protected]>
---
drivers/gpu/drm/bridge/ite-it6505.c | 54 ++++++++++++++++++++++++-----
1 file changed, 45 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index b53da9bb65a16..e592e14a48578 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -1318,6 +1318,8 @@ static void it6505_video_reset(struct it6505 *it6505)
it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_VID_MUTE, EN_VID_MUTE);
it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_VID_CTRL_PKT, 0x00);
it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, VIDEO_RESET);
+ it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET, 0x02);
+ it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET, 0x00);
it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, RST_501_FIFO);
it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, 0x00);
it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, 0x00);
@@ -2480,10 +2482,6 @@ static void it6505_irq_video_fifo_error(struct it6505 *it6505)
struct device *dev = &it6505->client->dev;
DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
- it6505->auto_train_retry = AUTO_TRAIN_RETRY;
- flush_work(&it6505->link_works);
- it6505_stop_hdcp(it6505);
- it6505_video_reset(it6505);
}
static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
@@ -2491,10 +2489,6 @@ static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
struct device *dev = &it6505->client->dev;
DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
- it6505->auto_train_retry = AUTO_TRAIN_RETRY;
- flush_work(&it6505->link_works);
- it6505_stop_hdcp(it6505);
- it6505_video_reset(it6505);
}
static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
@@ -2502,6 +2496,46 @@ static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
}
+static bool it6505_is_video_error_int(const int *int_status)
+{
+ if ((it6505_test_bit(BIT_INT_VID_FIFO_ERROR, (unsigned int *)int_status)) || (it6505_test_bit(BIT_INT_IO_FIFO_OVERFLOW, (unsigned int *)int_status)))
+ return 1;
+ return 0;
+}
+
+static void it6505_irq_video_error_handler(struct it6505 *it6505)
+{
+ struct device *dev = &it6505->client->dev;
+ int int_status[3] = {0};
+ int reg_0d;
+ int i;
+
+ it6505->auto_train_retry = AUTO_TRAIN_RETRY;
+ flush_work(&it6505->link_works);
+ it6505_stop_hdcp(it6505);
+ it6505_video_reset(it6505);
+
+ DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video...");
+
+ for (i = 0; i < 10; i++) {
+ usleep_range(10000, 11000);
+ int_status[2] = it6505_read(it6505, INT_STATUS_03);
+ reg_0d = it6505_read(it6505, REG_SYSTEM_STS);
+ it6505_write(it6505, INT_STATUS_03, int_status[2]);
+
+ DRM_DEV_DEBUG_DRIVER(dev, "reg08 = 0x%02x", int_status[2]);
+ DRM_DEV_DEBUG_DRIVER(dev, "reg0D = 0x%02x", reg_0d);
+
+ if ((reg_0d & VIDEO_STB) && (reg_0d >= 0))
+ break;
+
+ if (it6505_is_video_error_int(int_status)) {
+ it6505_video_reset(it6505);
+ DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video (%d)", i);
+ }
+ }
+}
+
static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
{
struct it6505 *it6505 = data;
@@ -2522,7 +2556,7 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
{ BIT_INT_VID_FIFO_ERROR, it6505_irq_video_fifo_error },
{ BIT_INT_IO_FIFO_OVERFLOW, it6505_irq_io_latch_fifo_overflow },
};
- int int_status[3], i;
+ int int_status[3], i, reg_0d;
if (it6505->enable_drv_hold || !it6505->powered)
return IRQ_HANDLED;
@@ -2550,6 +2584,8 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
if (it6505_test_bit(irq_vec[i].bit, (unsigned int *)int_status))
irq_vec[i].handler(it6505);
}
+ if (it6505_is_video_error_int(int_status))
+ it6505_irq_video_error_handler(it6505);
}
pm_runtime_put_sync(dev);
--
2.25.1
Hi Kuro,
Following up my comments from v2 [1]:
On Wed, Mar 6, 2024 at 10:09 AM kuro <[email protected]> wrote:
>
> From: kuro chung <[email protected]>
>
> ITE added a FIFO reset bit for input video. When system power resume,
> the TTL input of it6505 may get some noise before video signal stable
> and the hardware function reset is required.
> But the input FIFO reset will also trigger error interrupts of output module rising.
> Thus, it6505 have to wait a period can clear those expected error interrupts
> caused by manual hardware reset in one interrupt handler calling to avoid interrupt looping.
>
> Signed-off-by: Allen Chen <[email protected]>
IIUC you need to sign this off with your name as well. See [2] for more details.
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 54 ++++++++++++++++++++++++-----
> 1 file changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index b53da9bb65a16..e592e14a48578 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -1318,6 +1318,8 @@ static void it6505_video_reset(struct it6505 *it6505)
> it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_VID_MUTE, EN_VID_MUTE);
> it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_VID_CTRL_PKT, 0x00);
> it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, VIDEO_RESET);
> + it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET, 0x02);
> + it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET, 0x00);
> it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, RST_501_FIFO);
> it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, 0x00);
> it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, 0x00);
> @@ -2480,10 +2482,6 @@ static void it6505_irq_video_fifo_error(struct it6505 *it6505)
> struct device *dev = &it6505->client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(&it6505->link_works);
> - it6505_stop_hdcp(it6505);
> - it6505_video_reset(it6505);
> }
>
> static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
> @@ -2491,10 +2489,6 @@ static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
> struct device *dev = &it6505->client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(&it6505->link_works);
> - it6505_stop_hdcp(it6505);
> - it6505_video_reset(it6505);
> }
I don't really like functions that only print one line of debug log,
but I'm not sure what other reviewers think about this.
>
> static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
> @@ -2502,6 +2496,46 @@ static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
> return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
> }
>
> +static bool it6505_is_video_error_int(const int *int_status)
> +{
> + if ((it6505_test_bit(BIT_INT_VID_FIFO_ERROR, (unsigned int *)int_status)) || (it6505_test_bit(BIT_INT_IO_FIFO_OVERFLOW, (unsigned int *)int_status)))
> + return 1;
> + return 0;
> +}
> +
> +static void it6505_irq_video_error_handler(struct it6505 *it6505)
> +{
> + struct device *dev = &it6505->client->dev;
> + int int_status[3] = {0};
> + int reg_0d;
> + int i;
> +
> + it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> + flush_work(&it6505->link_works);
> + it6505_stop_hdcp(it6505);
> + it6505_video_reset(it6505);
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video...");
> +
> + for (i = 0; i < 10; i++) {
> + usleep_range(10000, 11000);
> + int_status[2] = it6505_read(it6505, INT_STATUS_03);
> + reg_0d = it6505_read(it6505, REG_SYSTEM_STS);
> + it6505_write(it6505, INT_STATUS_03, int_status[2]);
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "reg08 = 0x%02x", int_status[2]);
> + DRM_DEV_DEBUG_DRIVER(dev, "reg0D = 0x%02x", reg_0d);
> +
> + if ((reg_0d & VIDEO_STB) && (reg_0d >= 0))
> + break;
> +
> + if (it6505_is_video_error_int(int_status)) {
> + it6505_video_reset(it6505);
> + DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video (%d)", i);
> + }
> + }
Again, I think we need some code comments for this section, and some
of your replies should be included there.
And can you elaborate more about how this speeds up the video
stabilization? What would happen if we only clear the interrupts once
instead of doing a loop?
> +}
> +
> static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> {
> struct it6505 *it6505 = data;
> @@ -2522,7 +2556,7 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> { BIT_INT_VID_FIFO_ERROR, it6505_irq_video_fifo_error },
> { BIT_INT_IO_FIFO_OVERFLOW, it6505_irq_io_latch_fifo_overflow },
> };
> - int int_status[3], i;
> + int int_status[3], i, reg_0d;
>
> if (it6505->enable_drv_hold || !it6505->powered)
> return IRQ_HANDLED;
> @@ -2550,6 +2584,8 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> if (it6505_test_bit(irq_vec[i].bit, (unsigned int *)int_status))
> irq_vec[i].handler(it6505);
> }
> + if (it6505_is_video_error_int(int_status))
> + it6505_irq_video_error_handler(it6505);
> }
>
> pm_runtime_put_sync(dev);
> --
> 2.25.1
>
[1]: https://lore.kernel.org/all/CAEXTbpc6084rcmhFABw51SibU7FVyTWo=teQsETq5vCujGKWng@mail.gmail.com/
[2]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
Regards,
Pin-yen
Hi Pin-Yen,
Please read my comment as blow, thank you very much.
-----Original Message-----
From: Pin-yen Lin <[email protected]>
Sent: Thursday, March 7, 2024 3:37 PM
To: Kuro Chung (鐘仕廷) <[email protected]>
Cc: Allen Chen <[email protected]>; Kenneth Hung (洪家倫) <[email protected]>; Allen Chen <[email protected]>; Andrzej Hajda <[email protected]>; Neil Armstrong <[email protected]>; Robert Foss <[email protected]>; Laurent Pinchart <[email protected]>; Jonas Karlman <[email protected]>; Jernej Skrabec <[email protected]>; Maarten Lankhorst <[email protected]>; Maxime Ripard <[email protected]>; Thomas Zimmermann <[email protected]>; David Airlie <[email protected]>; Daniel Vetter <[email protected]>; open list:DRM DRIVERS <[email protected]>; open list <[email protected]>
Subject: Re: [PATCH v3 1/1] UPSTREAM: drm/bridge: it6505: fix hibernate to resume no display issue
Hi Kuro,
Following up my comments from v2 [1]:
On Wed, Mar 6, 2024 at 10:09 AM kuro <[email protected]> wrote:
>
> From: kuro chung <[email protected]>
>
> ITE added a FIFO reset bit for input video. When system power resume,
> the TTL input of it6505 may get some noise before video signal stable
> and the hardware function reset is required.
> But the input FIFO reset will also trigger error interrupts of output module rising.
> Thus, it6505 have to wait a period can clear those expected error
> interrupts caused by manual hardware reset in one interrupt handler calling to avoid interrupt looping.
>
> Signed-off-by: Allen Chen <[email protected]>
IIUC you need to sign this off with your name as well. See [2] for more details.
-> I update in the last patch v4
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 54
> ++++++++++++++++++++++++-----
> 1 file changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> b/drivers/gpu/drm/bridge/ite-it6505.c
> index b53da9bb65a16..e592e14a48578 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -1318,6 +1318,8 @@ static void it6505_video_reset(struct it6505 *it6505)
> it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_VID_MUTE, EN_VID_MUTE);
> it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_VID_CTRL_PKT, 0x00);
> it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET,
> VIDEO_RESET);
> + it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET, 0x02);
> + it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET,
> + 0x00);
> it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, RST_501_FIFO);
> it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, 0x00);
> it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, 0x00); @@
> -2480,10 +2482,6 @@ static void it6505_irq_video_fifo_error(struct it6505 *it6505)
> struct device *dev = &it6505->client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(&it6505->link_works);
> - it6505_stop_hdcp(it6505);
> - it6505_video_reset(it6505);
> }
>
> static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
> @@ -2491,10 +2489,6 @@ static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
> struct device *dev = &it6505->client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(&it6505->link_works);
> - it6505_stop_hdcp(it6505);
> - it6505_video_reset(it6505);
> }
I don't really like functions that only print one line of debug log, but I'm not sure what other reviewers think about this.
-> I totally remove those two function in the new patch v4.
>
> static bool it6505_test_bit(unsigned int bit, const unsigned int
> *addr) @@ -2502,6 +2496,46 @@ static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
> return 1 & (addr[bit / BITS_PER_BYTE] >> (bit %
> BITS_PER_BYTE)); }
>
> +static bool it6505_is_video_error_int(const int *int_status) {
> + if ((it6505_test_bit(BIT_INT_VID_FIFO_ERROR, (unsigned int *)int_status)) || (it6505_test_bit(BIT_INT_IO_FIFO_OVERFLOW, (unsigned int *)int_status)))
> + return 1;
> + return 0;
> +}
> +
> +static void it6505_irq_video_error_handler(struct it6505 *it6505) {
> + struct device *dev = &it6505->client->dev;
> + int int_status[3] = {0};
> + int reg_0d;
> + int i;
> +
> + it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> + flush_work(&it6505->link_works);
> + it6505_stop_hdcp(it6505);
> + it6505_video_reset(it6505);
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video...");
> +
> + for (i = 0; i < 10; i++) {
> + usleep_range(10000, 11000);
> + int_status[2] = it6505_read(it6505, INT_STATUS_03);
> + reg_0d = it6505_read(it6505, REG_SYSTEM_STS);
> + it6505_write(it6505, INT_STATUS_03, int_status[2]);
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "reg08 = 0x%02x", int_status[2]);
> + DRM_DEV_DEBUG_DRIVER(dev, "reg0D = 0x%02x", reg_0d);
> +
> + if ((reg_0d & VIDEO_STB) && (reg_0d >= 0))
> + break;
> +
> + if (it6505_is_video_error_int(int_status)) {
> + it6505_video_reset(it6505);
> + DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video (%d)", i);
> + }
> + }
Again, I think we need some code comments for this section, and some of your replies should be included there.
And can you elaborate more about how this speeds up the video stabilization? What would happen if we only clear the interrupts once instead of doing a loop?
->
The original design is each INT bit handler by function it6505_irq_video_fifo_error and it6505_irq_io_latch_fifo_overflow.
But the handler flow is influence each other.(It may occur recursively rst->INT->rst)
After consideration ITE have decided to handle those two bit in this section.
The video fifo reset will cause SCDT loss and trigger SCDT INT
SCDT change interrupt also handling the link training handler. We need to avoid SCDT change recursively if create new video handler.
If the FIFO error is trigger by FIFO rest , it will happen within 10ms.
To speed up video stable to link training, do 10 time check in interrupt.
If the video can't stable in 100ms(6 frame in 60hz), the video signal from IO bus may not stable.
> +}
> +
> static irqreturn_t it6505_int_threaded_handler(int unused, void
> *data) {
> struct it6505 *it6505 = data;
> @@ -2522,7 +2556,7 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> { BIT_INT_VID_FIFO_ERROR, it6505_irq_video_fifo_error },
> { BIT_INT_IO_FIFO_OVERFLOW, it6505_irq_io_latch_fifo_overflow },
> };
> - int int_status[3], i;
> + int int_status[3], i, reg_0d;
>
> if (it6505->enable_drv_hold || !it6505->powered)
> return IRQ_HANDLED;
> @@ -2550,6 +2584,8 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> if (it6505_test_bit(irq_vec[i].bit, (unsigned int *)int_status))
> irq_vec[i].handler(it6505);
> }
> + if (it6505_is_video_error_int(int_status))
> + it6505_irq_video_error_handler(it6505);
> }
>
> pm_runtime_put_sync(dev);
> --
> 2.25.1
>
[1]: https://lore.kernel.org/all/CAEXTbpc6084rcmhFABw51SibU7FVyTWo=teQsETq5vCujGKWng@mail.gmail.com/
[2]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
Regards,
Pin-yen