Re: [PATCH] drm/bridge: it6505: fix hibernate to resume no display issue

From: Pin-yen Lin
Date: Wed Feb 21 2024 - 06:03:01 EST

Hi Kuro,

On Wed, Feb 21, 2024 at 3:53 PM kuro <kuro.chung@xxxxxxxxxx> wrote:
>
> From: kuro chung <kuro.chung@xxxxxxxxxx>
>
> 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 <allen.chen@xxxxxxxxxxxxxxxxxxxxxxxxxxx>

IIUC you should also sign this off with your own account, and don't
include Allen if he is not involved in the patch development.corp
account here

>
> BUG=None
> TEST=None
Please remove these two lines for upstream review.
>
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 53 ++++++++++++++++++++++++-----
> 1 file changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index b53da9bb65a16..86277968fab93 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);
> }

Do we need to keep these two functions if they do nothing other than logging?

>
> static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
> @@ -2502,6 +2496,45 @@ 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;
> +}

Maybe just:
return it6505_test_bit(BIT_INT_VID_FIFO_ERROR, (unsigned int
*)int_status) || it6505_test_bit(BIT_INT_IO_FIFO_OVERFLOW, (unsigned
int *)int_status)

> +
> +static void it6505_irq_video_error_handler(struct it6505 *it6505)
> +{
> + struct device *dev = &it6505->client->dev;
> + int int_status[3] = {0};
> + int reg_0d;
> +
> + 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...");
> +

Can you add some code comments here to explain why we need to clear
the interrupt bits here?

> + 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]);

If we clear all interrupts like this, won't we risk missing other
interrupts here? E.g., if an HPD interrupt is fired here, it will be
cleared without being handled.

> +
> + 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);
> + }
> + }
> +}

Why do we need a for-loop here, and why 10?

> +
> static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> {
> struct it6505 *it6505 = data;
> @@ -2522,7 +2555,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 +2583,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