RE: [PATCH v13] drm/bridge: it6505: fix hibernate to resume no display issue
-Original Message- From: Robert Foss Sent: Wednesday, May 22, 2024 8:13 PM To: Maxime Ripard ; Thomas Zimmermann ; Jonas Karlman ; Daniel Vetter ; AngeloGioacchino Del Regno ; Allen Chen ; Neil Armstrong ; Laurent Pinchart ; Maarten Lankhorst ; David Airlie ; Kuro Chung (鐘仕廷) ; Kenneth Hung (洪家倫) ; open list:DRM DRIVERS ; Pin-yen Lin ; Andrzej Hajda ; Jernej Skrabec ; open list ; Hermes Wu (吳佳宏) Subject: Re: [PATCH v13] drm/bridge: it6505: fix hibernate to resume no display issue On Wed, 22 May 2024 14:55:28 +0800, kuro wrote: > From: Kuro Chung > > When the system power resumes, the TTL input of IT6505 may experience > some noise before the video signal stabilizes, necessitating a video > reset. This patch is implemented to prevent a loop of video error > interrupts, which can occur when a video reset in the video FIFO error > interrupt triggers another such interrupt. The patch processes the > SCDT and FIFO error interrupts simultaneously and ignores any video > FIFO error interrupts caused by a video reset. > > [...] Applied, thanks! > thanks everyone for your help!, I really appreciate it. [1/1] drm/bridge: it6505: fix hibernate to resume no display issue https://cgit.freedesktop.org/drm/drm-misc/commit/?id=484436ec5c2b Rob
RE: [PATCH v7 1/1] drm/bridge: it6505: fix hibernate to resume no display issue
-Original Message-
From: Kuro Chung (鐘仕廷)
Sent: Tuesday, May 14, 2024 11:21 AM
To: 'Robert Foss'
Cc: Allen Chen ; Pin-yen Lin ;
Kenneth Hung (洪家倫) ; Kuro Chung
; Andrzej Hajda
; Neil Armstrong ; Laurent
Pinchart ; Jonas Karlman ;
Jernej Skrabec ; Maarten Lankhorst
; Maxime Ripard ; Thomas
Zimmermann ; David Airlie ; Daniel
Vetter ; open list:DRM DRIVERS
; open list
Subject: RE: [PATCH v7 1/1] drm/bridge: it6505: fix hibernate to resume no
display issue
-Original Message-
From: Robert Foss
Sent: Tuesday, May 14, 2024 1:45 AM
To: Kuro Chung (鐘仕廷)
Cc: Allen Chen ; Pin-yen Lin ;
Kenneth Hung (洪家倫) ; Kuro Chung
; Andrzej Hajda
; Neil Armstrong ; Laurent
Pinchart ; Jonas Karlman ;
Jernej Skrabec ; Maarten Lankhorst
; Maxime Ripard ; Thomas
Zimmermann ; David Airlie ; Daniel
Vetter ; open list:DRM DRIVERS
; open list
Subject: Re: [PATCH v7 1/1] drm/bridge: it6505: fix hibernate to resume no
display issue
On Mon, May 13, 2024 at 7:42 PM Robert Foss wrote:
>
> On Mon, May 6, 2024 at 11:36 AM kuro wrote:
> >
> > From: Kuro
> >
> > 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
> >
> > ---
> > drivers/gpu/drm/bridge/ite-it6505.c | 73
> > +++--
> > 1 file changed, 49 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> > b/drivers/gpu/drm/bridge/ite-it6505.c
> > index b53da9bb65a16..64e2706e3d0c3 100644
> > --- a/drivers/gpu/drm/bridge/ite-it6505.c
> > +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> > @@ -1317,9 +1317,15 @@ static void it6505_video_reset(struct it6505 *it6505)
> > it6505_link_reset_step_train(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,
> > TX_FIFO_RESET);
> > + 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, VIDEO_RESET);
> > + usleep_range(1000, 2000);
> > it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, 0x00);
> > }
> >
> > @@ -2249,12 +2255,11 @@ static void it6505_link_training_work(struct
> > work_struct *work)
> > if (ret) {
> > it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> > it6505_link_train_ok(it6505);
> > - return;
> > } else {
> > it6505->auto_train_retry--;
> > + it6505_dump(it6505);
> > }
> >
> > - it6505_dump(it6505);
> > }
> >
> > static void it6505_plugged_status_to_codec(struct it6505 *it6505)
> > @@ -2475,31 +2480,53 @@ static void it6505_irq_link_train_fail(struct
> > it6505 *it6505)
> > schedule_work(>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 = >client->dev;
> > -
> > - DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
> > - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> > - flush_work(>link_works);
> > - it6505_stop_hdcp(it6505);
> > - it6505_video_reset(it6505);
> > + return 1 & (addr[bit / BITS_PER_BYTE] >> (bit %
> > + BITS_PER_BYTE));
> > }
> >
> > -static void it6505_irq_io_latch_fifo_overflow(struct it6505
> > *it6505)
> > +static void it6505_irq_video_handler(struct it6505 *it6505, const
> > +int *int_status)
> > {
> > struct device *dev = >client->dev;
> > + int reg_0d, reg_int03;
> >
> > - DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
> > - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> > - flush_work(>link_works);
> > - it6505_stop_hdcp(it6505);
> > - it6505_video_reset(it6505);
> > -}
> > + /*
> > +* When video SCDT change with video not stable,
> > +* Or video FIFO error, need video reset
> > +*/
> >
> > -static bool it6505_test_bit(unsigned int bit, const unsigned int
> > *addr) -{
> > - return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
> > + if
RE: [PATCH v7 1/1] drm/bridge: it6505: fix hibernate to resume no display issue
-Original Message-
From: Robert Foss
Sent: Tuesday, May 14, 2024 1:45 AM
To: Kuro Chung (鐘仕廷)
Cc: Allen Chen ; Pin-yen Lin ;
Kenneth Hung (洪家倫) ; Kuro Chung
; Andrzej Hajda
; Neil Armstrong ; Laurent
Pinchart ; Jonas Karlman ;
Jernej Skrabec ; Maarten Lankhorst
; Maxime Ripard ; Thomas
Zimmermann ; David Airlie ; Daniel
Vetter ; open list:DRM DRIVERS
; open list
Subject: Re: [PATCH v7 1/1] drm/bridge: it6505: fix hibernate to resume no
display issue
On Mon, May 13, 2024 at 7:42 PM Robert Foss wrote:
>
> On Mon, May 6, 2024 at 11:36 AM kuro wrote:
> >
> > From: Kuro
> >
> > 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
> >
> > ---
> > drivers/gpu/drm/bridge/ite-it6505.c | 73
> > +++--
> > 1 file changed, 49 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> > b/drivers/gpu/drm/bridge/ite-it6505.c
> > index b53da9bb65a16..64e2706e3d0c3 100644
> > --- a/drivers/gpu/drm/bridge/ite-it6505.c
> > +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> > @@ -1317,9 +1317,15 @@ static void it6505_video_reset(struct it6505 *it6505)
> > it6505_link_reset_step_train(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,
> > TX_FIFO_RESET);
> > + 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, VIDEO_RESET);
> > + usleep_range(1000, 2000);
> > it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, 0x00);
> > }
> >
> > @@ -2249,12 +2255,11 @@ static void it6505_link_training_work(struct
> > work_struct *work)
> > if (ret) {
> > it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> > it6505_link_train_ok(it6505);
> > - return;
> > } else {
> > it6505->auto_train_retry--;
> > + it6505_dump(it6505);
> > }
> >
> > - it6505_dump(it6505);
> > }
> >
> > static void it6505_plugged_status_to_codec(struct it6505 *it6505)
> > @@ -2475,31 +2480,53 @@ static void it6505_irq_link_train_fail(struct
> > it6505 *it6505)
> > schedule_work(>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 = >client->dev;
> > -
> > - DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
> > - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> > - flush_work(>link_works);
> > - it6505_stop_hdcp(it6505);
> > - it6505_video_reset(it6505);
> > + return 1 & (addr[bit / BITS_PER_BYTE] >> (bit %
> > + BITS_PER_BYTE));
> > }
> >
> > -static void it6505_irq_io_latch_fifo_overflow(struct it6505
> > *it6505)
> > +static void it6505_irq_video_handler(struct it6505 *it6505, const
> > +int *int_status)
> > {
> > struct device *dev = >client->dev;
> > + int reg_0d, reg_int03;
> >
> > - DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
> > - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> > - flush_work(>link_works);
> > - it6505_stop_hdcp(it6505);
> > - it6505_video_reset(it6505);
> > -}
> > + /*
> > +* When video SCDT change with video not stable,
> > +* Or video FIFO error, need video reset
> > +*/
> >
> > -static bool it6505_test_bit(unsigned int bit, const unsigned int
> > *addr) -{
> > - return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
> > + if ((!it6505_get_video_status(it6505) &&
> > + (it6505_test_bit(INT_SCDT_CHANGE, (unsigned int *)
> > int_status))) ||
> > + (it6505_test_bit(BIT_INT_IO_FIFO_OVERFLOW, (unsigned int *)
> > int_status)) ||
> > + (it6505_test_bit(BIT_INT_VID_FIFO_ERROR, (unsigned
> > + int *) int_status))) {
> > +
> > + it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> > + flush_work(>link_works);
> > + it6505_stop_hdcp(it6505);
> > +
RE: [PATCH v6 1/1] drm/bridge: it6505: fix hibernate to resume no display issue
-Original Message-
From: Robert Foss
Sent: Tuesday, April 23, 2024 9:50 PM
To: Kuro Chung (鐘仕廷)
Cc: Allen Chen ; Pin-yen Lin ;
Kenneth Hung (洪家倫) ; Kuro Chung
; Andrzej Hajda
; Neil Armstrong ; Laurent
Pinchart ; Jonas Karlman ;
Jernej Skrabec ; Maarten Lankhorst
; Maxime Ripard ; Thomas
Zimmermann ; David Airlie ; Daniel
Vetter ; open list:DRM DRIVERS
; open list
Subject: Re: [PATCH v6 1/1] drm/bridge: it6505: fix hibernate to resume no
display issue
On Tue, Apr 23, 2024 at 10:16 AM kuro wrote:
>
> From: Kuro
>
> 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
>
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 73
> +++--
> 1 file changed, 49 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> b/drivers/gpu/drm/bridge/ite-it6505.c
> index b53da9bb65a16..ae7f4c7ec6dd0 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -1317,9 +1317,15 @@ static void it6505_video_reset(struct it6505 *it6505)
> it6505_link_reset_step_train(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, VIDEO_RESET);
> + usleep_range(1000, 2000);
> it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, 0x00);
Can any of these magic values be defined as macros?
-> I will modify the code 0x02 to TX_FIFO_RESET
> }
>
> @@ -2249,12 +2255,11 @@ static void it6505_link_training_work(struct
> work_struct *work)
> if (ret) {
> it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> it6505_link_train_ok(it6505);
> - return;
> } else {
> it6505->auto_train_retry--;
> + it6505_dump(it6505);
> }
>
> - it6505_dump(it6505);
> }
>
> static void it6505_plugged_status_to_codec(struct it6505 *it6505) @@
> -2475,31 +2480,53 @@ static void it6505_irq_link_train_fail(struct it6505
> *it6505)
> schedule_work(>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 = >client->dev;
> -
> - DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(>link_works);
> - it6505_stop_hdcp(it6505);
> - it6505_video_reset(it6505);
> + return 1 & (addr[bit / BITS_PER_BYTE] >> (bit %
> + BITS_PER_BYTE));
> }
>
> -static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
> +static void it6505_irq_video_handler(struct it6505 *it6505, const int
> +*int_status)
> {
> struct device *dev = >client->dev;
> + int reg_0d, reg_int03;
>
> - DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(>link_works);
> - it6505_stop_hdcp(it6505);
> - it6505_video_reset(it6505);
> -}
> + /*
> +* When video SCDT change with video not stable,
> +* Or video FIFO error, need video reset
> +*/
>
> -static bool it6505_test_bit(unsigned int bit, const unsigned int
> *addr) -{
> - return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
> + if ((!it6505_get_video_status(it6505) &&
> + (it6505_test_bit(INT_SCDT_CHANGE, (unsigned int *)
> int_status))) ||
> + (it6505_test_bit(BIT_INT_IO_FIFO_OVERFLOW, (unsigned int *)
> int_status)) ||
> + (it6505_test_bit(BIT_INT_VID_FIFO_ERROR, (unsigned int
> + *) int_status))) {
> +
> + it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> + flush_work(>link_works);
> + it6505_stop_hdcp(it6505);
> + it6505_video_reset(it6505);
> +
> + usleep_range(1, 11000);
> +
> + /*
> +* Clear FIFO error IRQ to prevent fifo error -> reset loop
>
RE: [PATCH v4 1/1] drm/bridge: it6505: fix hibernate to resume no display issue
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
Sent: Saturday, March 9, 2024 3:12 PM
To: Kuro Chung (鐘仕廷)
Cc: Allen Chen ; Kenneth Hung (洪家倫)
; Kuro Chung ;
Andrzej Hajda ; Neil Armstrong
; Robert Foss ; Laurent Pinchart
; Jonas Karlman ; Jernej
Skrabec ; Maarten Lankhorst
; Maxime Ripard ; Thomas
Zimmermann ; David Airlie ; Daniel
Vetter ; open list:DRM DRIVERS
; open list
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 wrote:
>
> From: Kuro
>
> 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
>
> ---
> 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(>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 = >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(>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 = >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(>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));
> +
RE: [PATCH v3 1/1] UPSTREAM: drm/bridge: it6505: fix hibernate to resume no display issue
Hi Pin-Yen,
Please read my comment as blow, thank you very much.
-Original Message-
From: Pin-yen Lin
Sent: Thursday, March 7, 2024 3:37 PM
To: Kuro Chung (鐘仕廷)
Cc: Allen Chen ; Kenneth Hung (洪家倫)
; Allen Chen ;
Andrzej Hajda ; Neil Armstrong
; Robert Foss ; Laurent Pinchart
; Jonas Karlman ; Jernej
Skrabec ; Maarten Lankhorst
; Maxime Ripard ; Thomas
Zimmermann ; David Airlie ; Daniel
Vetter ; open list:DRM DRIVERS
; open list
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 wrote:
>
> From: kuro chung
>
> 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
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 = >client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(>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 = >client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(>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 = >client->dev;
> + int int_status[3] = {0};
> + int reg_0d;
> + int i;
> +
> + it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> + flush_work(>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(1, 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)) {
> +
RE: [PATCH] drm/bridge: it6505: fix hibernate to resume no display issue
Hi Pin-yen Lin,
Please see my reply below, Thank you.
BR Kuro Chung
-Original Message-
From: Pin-yen Lin
Sent: Wednesday, February 21, 2024 7:02 PM
To: Kuro Chung (鐘仕廷)
Cc: Allen Chen ; Kenneth Hung (洪家倫)
; Allen Chen ;
Andrzej Hajda ; Neil Armstrong ;
Robert Foss ; Laurent Pinchart
; Jonas Karlman ; Jernej
Skrabec ; David Airlie ; Daniel
Vetter ; open list:DRM DRIVERS
; open list
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 wrote:
>
> From: kuro chung
>
> 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
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 = >client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(>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 = >client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(>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 = >client->dev;
> + int int_status[3] = {0};
> + int reg_0d;
> +
> + it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> + flush_work(>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
RE: [PATCH] UPSTREAM: drm/bridge: it6505: fix hibernate to resume no display issue
Hi Pin-yen,
I want to upload the patch to let the patch could be review on
website, https://chromium-review.googlesource.com/dashboard/self
But the message ' No Contributor Agreement on file for user
Allen Chen ' seems no longer have Permissions for this file, right?
It have an issue need to update the ite-it6505.c, Could
you give me some advising?
Thank you very much.
BR Kuro Chung
ite@ite-XPS-13-9360:~/project/it6505/google/source/cros/src/third_party/kernel/v5.15$
git push cros HEAD:refs/for/chromeos-5.15
remote: PERMISSION_DENIED: The caller does not have permission
remote: [type.googleapis.com/google.rpc.LocalizedMessage]
remote: locale: "en-US"
remote: message: "No Contributor Agreement on file for user Allen Chen
(id=1321515)"
remote:
remote: [type.googleapis.com/google.rpc.RequestInfo]
remote: request_id: "3c171f2fe78e458ebbb2990bab93cfb3"
remote:
remote: [type.googleapis.com/google.rpc.RequestInfo]
remote: request_id: "c5662672e00f42e1b6562149270c7864"
fatal: unable to access
'https://chromium.googlesource.com/chromiumos/third_party/kernel/': The
requested URL returned error: 403
BR Kuro Chung
-Original Message-
From: Kuro Chung (鐘仕廷)
Sent: Tuesday, February 6, 2024 11:19 AM
Cc: Allen Chen ; Pin-yen Lin ;
Kuro Chung (鐘仕廷) ; Kenneth Hung (洪家倫)
; Andrzej Hajda ; Neil Armstrong
; Robert Foss ; Laurent
Pinchart ; Jonas Karlman ;
Jernej Skrabec ; David Airlie ;
Daniel Vetter ; open list:DRM DRIVERS
; open list
Subject: [PATCH] UPSTREAM: drm/bridge: it6505: fix hibernate to resume no
display issue
From: allen chen
Change-Id: Iaa3cd9da92a625496f579d87d0ab74ca9c4937c4
---
drivers/gpu/drm/bridge/ite-it6505.c | 42 ++---
1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
b/drivers/gpu/drm/bridge/ite-it6505.c
index b53da9bb65a1..07883001e6ca 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 = >client->dev;
DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
- it6505->auto_train_retry = AUTO_TRAIN_RETRY;
- flush_work(>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 = >client->dev;
DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
- it6505->auto_train_retry = AUTO_TRAIN_RETRY;
- flush_work(>link_works);
- it6505_stop_hdcp(it6505);
- it6505_video_reset(it6505);
}
static bool it6505_test_bit(unsigned int bit, const unsigned int *addr) @@
-2522,7 +2516,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 +2544,36 @@ 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_test_bit(irq_vec[9].bit, (unsigned int
*)int_status)) ||
+ (it6505_test_bit(irq_vec[10].bit, (unsigned int
*)int_status))) {
+ it6505->auto_train_retry = AUTO_TRAIN_RETRY;
+ flush_work(>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(1, 11000);
+ int_status[2] = it6505_read(it6505,
INT_STATUS_03);
+ reg_0d
