Hi Kuro,
On Fri, Mar 8, 2024 at 4:54 PM kuro <[email protected]> wrote:
>
> From: Kuro <[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: Kuro Chung <[email protected]>
>
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 50 ++++++++++++++++++++---------
> 1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index b53da9bb65a16..eff888fe7c2e7 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);
> @@ -2475,31 +2477,49 @@ static void it6505_irq_link_train_fail(struct it6505 *it6505)
> schedule_work(&it6505->link_works);
> }
>
> -static void it6505_irq_video_fifo_error(struct it6505 *it6505)
> +static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
> {
> - struct device *dev = &it6505->client->dev;
> + return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
> +}
>
> - 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 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_io_latch_fifo_overflow(struct it6505 *it6505)
> +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;
>
> - 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)
> -{
> - return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
> + DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video...");
> +
I still don't see any code comment around the following section.
> + 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);
> + }
> + }
Okay let me make the questions more specific:
1. What would happen if we remove the loop and only check the video
error interrupts once? If another video error interrupt comes out, we
handle it in the next interrupt handler. Will this lead to an infinite
loop?
2. Why do we run the loop for 10 times (100ms as you mentioned), but
not 5 times or 20 times? Does this "100ms" come from the hardware spec
or your experience on debugging this issue? I guess it's okay if it's
"I tried it a few times and 100ms seems to be just enough", but I
would prefer you to write that down in the code comments.
> }
>
> static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> @@ -2519,8 +2539,6 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> { BIT_INT_HDCP_KSV_CHECK, it6505_irq_hdcp_ksv_check },
> { BIT_INT_AUDIO_FIFO_ERROR, it6505_irq_audio_fifo_error },
> { BIT_INT_LINK_TRAIN_FAIL, it6505_irq_link_train_fail },
> - { 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;
>
> @@ -2550,6 +2568,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
>
Regards,
Pin-yen
Hi Pin-yen Lin,
1. What would happen if we remove the loop and only check the video error interrupts once? If another video error interrupt comes out, we handle it in the next interrupt handler. Will this lead to an infinite loop?
2. Why do we run the loop for 10 times (100ms as you mentioned), but not 5 times or 20 times? Does this "100ms" come from the hardware spec or your experience on debugging this issue? I guess it's okay if it's "I tried it a few times and 100ms seems to be just enough", but I would prefer you to write that down in the code comments.
-> This video error interrupt loop issue happen when system sleep ->resume and SOC turn on DPI signal.
The video signal might be stable immediately, but in some case, 6505 will went into video error loop
And not only video FIFO error interrupt, but also SCDT interrupt happen(SCTD on/off).
The SCD interrupt will also trigger link training, and this will cause more "no display issue" when connect certain TYEP-C DP alt mode docking.
When testing at the platform which have video loop issue, the video error loop happen in about 1/350(sleep/resume loop)
And 90% can stable in 100ms, 10% need 150~200ms. so we wait 100ms in loop.
-----Original Message-----
From: Pin-yen Lin <[email protected]>
Sent: Saturday, March 9, 2024 3:12 PM
To: Kuro Chung (鐘仕廷) <[email protected]>
Cc: Allen Chen <[email protected]>; Kenneth Hung (洪家倫) <[email protected]>; Kuro Chung <[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 v4 1/1] drm/bridge: it6505: fix hibernate to resume no display issue
Hi Kuro,
On Fri, Mar 8, 2024 at 4:54 PM kuro <[email protected]> wrote:
>
> From: Kuro <[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: Kuro Chung <[email protected]>
>
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 50
> ++++++++++++++++++++---------
> 1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> b/drivers/gpu/drm/bridge/ite-it6505.c
> index b53da9bb65a16..eff888fe7c2e7 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); @@
> -2475,31 +2477,49 @@ static void it6505_irq_link_train_fail(struct it6505 *it6505)
> schedule_work(&it6505->link_works);
> }
>
> -static void it6505_irq_video_fifo_error(struct it6505 *it6505)
> +static bool it6505_test_bit(unsigned int bit, const unsigned int
> +*addr)
> {
> - struct device *dev = &it6505->client->dev;
> + return 1 & (addr[bit / BITS_PER_BYTE] >> (bit %
> +BITS_PER_BYTE)); }
>
> - 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 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_io_latch_fifo_overflow(struct it6505 *it6505)
> +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;
>
> - 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) -{
> - return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
> + DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video...");
> +
I still don't see any code comment around the following section.
> + 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);
> + }
> + }
Okay let me make the questions more specific:
1. What would happen if we remove the loop and only check the video error interrupts once? If another video error interrupt comes out, we handle it in the next interrupt handler. Will this lead to an infinite loop?
2. Why do we run the loop for 10 times (100ms as you mentioned), but not 5 times or 20 times? Does this "100ms" come from the hardware spec or your experience on debugging this issue? I guess it's okay if it's "I tried it a few times and 100ms seems to be just enough", but I would prefer you to write that down in the code comments.
> }
>
> static irqreturn_t it6505_int_threaded_handler(int unused, void
> *data) @@ -2519,8 +2539,6 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> { BIT_INT_HDCP_KSV_CHECK, it6505_irq_hdcp_ksv_check },
> { BIT_INT_AUDIO_FIFO_ERROR, it6505_irq_audio_fifo_error },
> { BIT_INT_LINK_TRAIN_FAIL, it6505_irq_link_train_fail },
> - { 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;
>
> @@ -2550,6 +2568,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
>
Regards,
Pin-yen