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

[<kuro.chung@ite.com.tw>]

Hi Pin-yen Lin, 
	
	Please see my reply below, Thank you.

BR Kuro Chung
-----Original Message-----
From: Pin-yen Lin <treapking@chromium.org>
Sent: Wednesday, February 21, 2024 7:02 PM
To: Kuro Chung (鐘仕廷) <kuro.chung@ite.com.tw>
Cc: Allen Chen <allen.chen@ite.com.tw>; Kenneth Hung (洪家倫) <Kenneth.Hung@ite.com.tw>; Allen Chen <allen.chen@ite.corp-partner.google.com>; Andrzej Hajda <a.hajda@samsung.com>; Neil Armstrong <narmstrong@baylibre.com>; Robert Foss <robert.foss@linaro.org>; Laurent Pinchart <Laurent.pinchart@ideasonboard.com>; Jonas Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; David Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; open list:DRM DRIVERS <dri-devel@lists.freedesktop.org>; open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] drm/bridge: it6505: fix hibernate to resume no display issue

Hi Kuro,

On Wed, Feb 21, 2024 at 3:53 PM kuro <kuro.chung@ite.com.tw> wrote:
>
> From: kuro chung <kuro.chung@ite.com.tw>
>
> 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@ite.corp-partner.google.com>

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
-> For customer urgent issue, currently use Allen account, next time will be my account, my account still in processed.

>
> BUG=None
> TEST=None
Please remove these two lines for upstream review.
-> I will update in v3

>
> ---
>  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?
-> We could keep it for the log because all INT log shown in functions, 
-> we still need the log the debug if needed

>
>  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)
-> No, if(variable), this variable if not 0 all could be passed, if this variable = 2 then return 2 is not expected, keep it for readable and better for modify.

> +
> +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?
->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->INT recursively) After consideration ITE have decided to handle those two bit in one 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]);

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.
-> The INT INT_STATUS_03 is about input video FIFO error, link train fail, in this step is only consider input stable, so it's fine to clear this INT.

> +
> +               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?
-> 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 cant 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 +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