[PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment

[PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment

[Yunlongli]

In the actual tests,  the IT66121 chip sometimes misjudged whether
it had an external screen, so, reference the it66121_user_guid.pdf
about Audio/Video data is stable or not A typical initialization
of HDMI link should be based on interrupt signal and appropriate
register probing. Recommended flow is detailed in IT66121
Programming Guide. Simply put, the microcontroller should monitor
the HPD status first. Upon valid HPD event, move on to check
RxSENDetect register to see if the receiver chip is ready for
further handshaking. When RxSENDetect is asserted, start reading EDID
data through DDC channels and carry on the rest of the handshaking
subsequently.If the micro-controller makes no use of the interrupt
signal as well as the above-mentioned status  registers, the link
establishment might fail. Please do follow the suggested
initialization flow recommended in IT66121 Programming Guide.
So, I add the IT66121_SYS_STATUS_SENDECTECT register status detection.

Signed-off-by: Yunlongli <liyunlonga@uniontech.com>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 2f2a09adb4bc..9ed4fa298d11 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -523,7 +523,7 @@ static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
 	if (regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val))
 		return false;
 
-	return val & IT66121_SYS_STATUS_HPDETECT;
+	return ((val & IT66121_SYS_STATUS_HPDETECT) && (val & IT66121_SYS_STATUS_SENDECTECT));
 }
 
 static int it66121_bridge_attach(struct drm_bridge *bridge,
-- 
2.20.1

Re: [PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment

[Neil Armstrong]

Hi,

On 21/09/2021 09:54, Yunlongli wrote:
> In the actual tests,  the IT66121 chip sometimes misjudged whether
> it had an external screen, so, reference the it66121_user_guid.pdf
> about Audio/Video data is stable or not A typical initialization
> of HDMI link should be based on interrupt signal and appropriate
> register probing. Recommended flow is detailed in IT66121
> Programming Guide. Simply put, the microcontroller should monitor
> the HPD status first. Upon valid HPD event, move on to check
> RxSENDetect register to see if the receiver chip is ready for
> further handshaking. When RxSENDetect is asserted, start reading EDID
> data through DDC channels and carry on the rest of the handshaking
> subsequently.If the micro-controller makes no use of the interrupt
> signal as well as the above-mentioned status  registers, the link
> establishment might fail. Please do follow the suggested
> initialization flow recommended in IT66121 Programming Guide.
> So, I add the IT66121_SYS_STATUS_SENDECTECT register status detection.

Ok, the RxSENDetect is the "rx-sense" detection bit as described in the same doc:

Receiver detection circuit reports the presence or absence of an active termination at the TMDS Clock Channel (RxSENDetect)

The usage of the rx-sense signal in hpd_detect() is not clear because this would break detection of "Fake" EDID dongles or idle monitors.

The dw-hdmi handles the rx-sense, but only to power-on/off the HDMI TX, but only returns the HPD status to DRM without the RX SENSE state,
so it only saves power and doesn't change anything on DRM HPD detection.

So not sure if we should merge this as-is.

Neil

> 
> Signed-off-by: Yunlongli <liyunlonga@uniontech.com>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 2f2a09adb4bc..9ed4fa298d11 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -523,7 +523,7 @@ static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
>  	if (regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val))
>  		return false;
>  
> -	return val & IT66121_SYS_STATUS_HPDETECT;
> +	return ((val & IT66121_SYS_STATUS_HPDETECT) && (val & IT66121_SYS_STATUS_SENDECTECT));
>  }
>  
>  static int it66121_bridge_attach(struct drm_bridge *bridge,
> 

Re: [PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment

[李云龙]

[-- Attachment #1: Type: text/plain, Size: 3506 bytes --]

I tested it on Loongarch and MIPS, and the results were fine。
 
 
------------------ Original ------------------
From: &nbsp;"Neil&nbsp;Armstrong"<narmstrong@baylibre.com&gt;;
Date: &nbsp;Tue, Sep 21, 2021 08:28 AM
To: &nbsp;"Yunlongli"<liyunlonga@uniontech.com&gt;; "Phong LE"<ple@baylibre.com&gt;; "Andrzej Hajda"<a.hajda@samsung.com&gt;; "Robert Foss"<robert.foss@linaro.org&gt;; "David Airlie"<airlied@linux.ie&gt;; "Daniel Vetter"<daniel@ffwll.ch&gt;; 
Cc: &nbsp;"Laurent.pinchart"<Laurent.pinchart@ideasonboard.com&gt;; "Jonas Karlman"<jonas@kwiboo.se&gt;; "Jernej Skrabec"<jernej.skrabec@gmail.com&gt;; "dri-devel"<dri-devel@lists.freedesktop.org&gt;; "linux-kernel"<linux-kernel@vger.kernel.org&gt;; 
Subject: &nbsp;Re: [PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment

&nbsp;

Hi,

On 21/09/2021 09:54, Yunlongli wrote:
&gt; In the actual tests,&nbsp; the IT66121 chip sometimes misjudged whether
&gt; it had an external screen, so, reference the it66121_user_guid.pdf
&gt; about Audio/Video data is stable or not A typical initialization
&gt; of HDMI link should be based on interrupt signal and appropriate
&gt; register probing. Recommended flow is detailed in IT66121
&gt; Programming Guide. Simply put, the microcontroller should monitor
&gt; the HPD status first. Upon valid HPD event, move on to check
&gt; RxSENDetect register to see if the receiver chip is ready for
&gt; further handshaking. When RxSENDetect is asserted, start reading EDID
&gt; data through DDC channels and carry on the rest of the handshaking
&gt; subsequently.If the micro-controller makes no use of the interrupt
&gt; signal as well as the above-mentioned status&nbsp; registers, the link
&gt; establishment might fail. Please do follow the suggested
&gt; initialization flow recommended in IT66121 Programming Guide.
&gt; So, I add the IT66121_SYS_STATUS_SENDECTECT register status detection.

Ok, the RxSENDetect is the "rx-sense" detection bit as described in the same doc:

Receiver detection circuit reports the presence or absence of an active termination at the TMDS Clock Channel (RxSENDetect)

The usage of the rx-sense signal in hpd_detect() is not clear because this would break detection of "Fake" EDID dongles or idle monitors.

The dw-hdmi handles the rx-sense, but only to power-on/off the HDMI TX, but only returns the HPD status to DRM without the RX SENSE state,
so it only saves power and doesn't change anything on DRM HPD detection.

So not sure if we should merge this as-is.

Neil

&gt; 
&gt; Signed-off-by: Yunlongli <liyunlonga@uniontech.com&gt;
&gt; ---
&gt;&nbsp; drivers/gpu/drm/bridge/ite-it66121.c | 2 +-
&gt;&nbsp; 1 file changed, 1 insertion(+), 1 deletion(-)
&gt; 
&gt; diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
&gt; index 2f2a09adb4bc..9ed4fa298d11 100644
&gt; --- a/drivers/gpu/drm/bridge/ite-it66121.c
&gt; +++ b/drivers/gpu/drm/bridge/ite-it66121.c
&gt; @@ -523,7 +523,7 @@ static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
&gt;&nbsp; 	if (regmap_read(ctx-&gt;regmap, IT66121_SYS_STATUS_REG, &amp;val))
&gt;&nbsp; 		return false;
&gt;&nbsp; 
&gt; -	return val &amp; IT66121_SYS_STATUS_HPDETECT;
&gt; +	return ((val &amp; IT66121_SYS_STATUS_HPDETECT) &amp;&amp; (val &amp; IT66121_SYS_STATUS_SENDECTECT));
&gt;&nbsp; }
&gt;&nbsp; 
&gt;&nbsp; static int it66121_bridge_attach(struct drm_bridge *bridge,
&gt;

[-- Attachment #2: Type: text/html, Size: 4134 bytes --]

Re: [PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment.

[Robert Foss]

Hey Yunlongli,

Thanks for submitting this fix.

On Sat, 18 Sept 2021 at 05:51, Yunlongli <liyunlonga@uniontech.com> wrote:

The formatting of this commit message is a bit unusual, let's try to
change it to the normal formatting.

Remove the dot from the commit title:
"drm: bridge: it66121: Added it66121 chip external screen status
judgment." -> "drm: bridge: it66121: Added it66121 chip external
screen status judgment"


>
> fix: Add further confirm if external screens are involved.

The "fix:" tag is not needed. However if this commit fixes a bug
introduced in an earlier commit a machine readable tag like the the
one below could be added after the commit message.

Fixes: 988156dc2fc9 ("drm: bridge: add it66121 driver")

>
> log: In the actual tests,  the IT66121 chip sometimes misjudged whether
>      it had an external screen, so, reference the it66121_user_guid.pdf
>      about Audio/Video data is stable or not A typical initialization
>      of HDMI link should be based on interrupt signal and appropriate
>      register probing. Recommended flow is detailed in IT66121
>      Programming Guide. Simply put, the microcontroller should monitor
>      the HPD status first. Upon valid HPD event, move on to check
>      RxSENDetect register to see if the receiver chip is ready for
>      further handshaking. When RxSENDetect is asserted, start reading EDID
>      data through DDC channels and carry on the rest of the handshaking
>      subsequently.If the micro-controller makes no use of the interrupt
>      signal as well as the above-mentioned status  registers, the link
>      establishment might fail. Please do follow the suggested
>      initialization flow recommended in IT66121 Programming Guide.
>      So, I add the IT66121_SYS_STATUS_SENDECTECT register status detection.
>

The "log:" prefix is not needed, and neither is the indentation of the text.

Secondly maybe it would be nice to format the above chunk of text into
paragraphs just to make it easier to read.

> Signed-off-by: Yunlongli <liyunlonga@uniontech.com>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 2f2a09adb4bc..9ed4fa298d11 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -523,7 +523,7 @@ static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
>         if (regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val))
>                 return false;
>
> -       return val & IT66121_SYS_STATUS_HPDETECT;
> +       return ((val & IT66121_SYS_STATUS_HPDETECT) && (val & IT66121_SYS_STATUS_SENDECTECT));
>  }
>
>  static int it66121_bridge_attach(struct drm_bridge *bridge,
> --
> 2.20.1
>
>
>

With the above suggestions fixed, feel free to add my r-b and submit a
v2 of this patch.
Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment.

[Robert Foss]

Hey Yunlongli,

Thanks for submitting this fix.

On Sat, 18 Sept 2021 at 05:51, Yunlongli <liyunlonga@uniontech.com> wrote:

The formatting of this commit message is a bit unusual, let's try to
change it to the normal formatting.

Remove the dot from the commit title:
"drm: bridge: it66121: Added it66121 chip external screen status
judgment." -> "drm: bridge: it66121: Added it66121 chip external
screen status judgment"


>
> fix: Add further confirm if external screens are involved.

The "fix:" tag is not needed. However if this commit fixes a bug
introduced in an earlier commit a machine readable tag like the the
one below could be added after the commit message.

Fixes: 988156dc2fc9 ("drm: bridge: add it66121 driver")

>
> log: In the actual tests,  the IT66121 chip sometimes misjudged whether
>      it had an external screen, so, reference the it66121_user_guid.pdf
>      about Audio/Video data is stable or not A typical initialization
>      of HDMI link should be based on interrupt signal and appropriate
>      register probing. Recommended flow is detailed in IT66121
>      Programming Guide. Simply put, the microcontroller should monitor
>      the HPD status first. Upon valid HPD event, move on to check
>      RxSENDetect register to see if the receiver chip is ready for
>      further handshaking. When RxSENDetect is asserted, start reading EDID
>      data through DDC channels and carry on the rest of the handshaking
>      subsequently.If the micro-controller makes no use of the interrupt
>      signal as well as the above-mentioned status  registers, the link
>      establishment might fail. Please do follow the suggested
>      initialization flow recommended in IT66121 Programming Guide.
>      So, I add the IT66121_SYS_STATUS_SENDECTECT register status detection.
>

The "log:" prefix is not needed, and neither is the indentation of the text.

Secondly maybe it would be nice to format the above chunk of text into
paragraphs just to make it easier to read.

> Signed-off-by: Yunlongli <liyunlonga@uniontech.com>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 2f2a09adb4bc..9ed4fa298d11 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -523,7 +523,7 @@ static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
>         if (regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val))
>                 return false;
>
> -       return val & IT66121_SYS_STATUS_HPDETECT;
> +       return ((val & IT66121_SYS_STATUS_HPDETECT) && (val & IT66121_SYS_STATUS_SENDECTECT));
>  }
>
>  static int it66121_bridge_attach(struct drm_bridge *bridge,
> --
> 2.20.1
>
>
>

With the above suggestions fixed, feel free to add my r-b and submit a
v2 of this patch.
Reviewed-by: Robert Foss <robert.foss@linaro.org>

[PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment.

[Yunlongli]

fix: Add further confirm if external screens are involved.

log: In the actual tests,  the IT66121 chip sometimes misjudged whether
     it had an external screen, so, reference the it66121_user_guid.pdf
     about Audio/Video data is stable or not A typical initialization
     of HDMI link should be based on interrupt signal and appropriate
     register probing. Recommended flow is detailed in IT66121
     Programming Guide. Simply put, the microcontroller should monitor
     the HPD status first. Upon valid HPD event, move on to check
     RxSENDetect register to see if the receiver chip is ready for
     further handshaking. When RxSENDetect is asserted, start reading EDID
     data through DDC channels and carry on the rest of the handshaking
     subsequently.If the micro-controller makes no use of the interrupt
     signal as well as the above-mentioned status  registers, the link
     establishment might fail. Please do follow the suggested
     initialization flow recommended in IT66121 Programming Guide.
     So, I add the IT66121_SYS_STATUS_SENDECTECT register status detection.

Signed-off-by: Yunlongli <liyunlonga@uniontech.com>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 2f2a09adb4bc..9ed4fa298d11 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -523,7 +523,7 @@ static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
 	if (regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val))
 		return false;
 
-	return val & IT66121_SYS_STATUS_HPDETECT;
+	return ((val & IT66121_SYS_STATUS_HPDETECT) && (val & IT66121_SYS_STATUS_SENDECTECT));
 }
 
 static int it66121_bridge_attach(struct drm_bridge *bridge,
-- 
2.20.1

[PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment.

[Yunlongli]

fix: Add further confirm if external screens are involved.

log: In the actual tests,  the IT66121 chip sometimes misjudged whether
     it had an external screen, so, reference the it66121_user_guid.pdf
     about Audio/Video data is stable or not A typical initialization
     of HDMI link should be based on interrupt signal and appropriate
     register probing. Recommended flow is detailed in IT66121
     Programming Guide. Simply put, the microcontroller should monitor
     the HPD status first. Upon valid HPD event, move on to check
     RxSENDetect register to see if the receiver chip is ready for
     further handshaking. When RxSENDetect is asserted, start reading EDID
     data through DDC channels and carry on the rest of the handshaking
     subsequently.If the micro-controller makes no use of the interrupt
     signal as well as the above-mentioned status  registers, the link
     establishment might fail. Please do follow the suggested
     initialization flow recommended in IT66121 Programming Guide.
     So, I add the IT66121_SYS_STATUS_SENDECTECT register status detection.

Signed-off-by: Yunlongli <liyunlonga@uniontech.com>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 2f2a09adb4bc..9ed4fa298d11 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -523,7 +523,7 @@ static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
 	if (regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val))
 		return false;
 
-	return val & IT66121_SYS_STATUS_HPDETECT;
+	return ((val & IT66121_SYS_STATUS_HPDETECT) && (val & IT66121_SYS_STATUS_SENDECTECT));
 }
 
 static int it66121_bridge_attach(struct drm_bridge *bridge,
-- 
2.20.1