diff mbox series

drm/panel: nt35510: Make new commands optional

Message ID 20240906-fix-nt35510-v1-1-1971f3af7dda@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/panel: nt35510: Make new commands optional | expand

Commit Message

Linus Walleij Sept. 6, 2024, 9:54 p.m. UTC 
The commit introducing the Frida display started to write the
SETVCL, BT3CTR and SETVCMOFF registers unconditionally, and some
(not all!) Hydis display seem to be affected by ghosting after
the commit.

Make them all optional and only send these commands on the
Frida display for now.

Reported-by: Stefan Hansson <newbyte@postmarketos.org>
Fixes: 219a1f49094f ("drm/panel: nt35510: support FRIDA FRD400B25025-A-CTK")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Stefan: if this patch works and you have time you can try adding
the three flags one at a time to your device and we can see which
command is problematic.
---
 drivers/gpu/drm/panel/panel-novatek-nt35510.c | 45 +++++++++++++++++----------
 1 file changed, 29 insertions(+), 16 deletions(-)


---
base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
change-id: 20240906-fix-nt35510-a8ec6e47e036

Best regards,

Comments

Jessica Zhang Sept. 6, 2024, 10:12 p.m. UTC | #1 
On 9/6/2024 2:54 PM, Linus Walleij wrote:
> The commit introducing the Frida display started to write the
> SETVCL, BT3CTR and SETVCMOFF registers unconditionally, and some
> (not all!) Hydis display seem to be affected by ghosting after
> the commit.
> 
> Make them all optional and only send these commands on the
> Frida display for now.
> 
> Reported-by: Stefan Hansson <newbyte@postmarketos.org>
> Fixes: 219a1f49094f ("drm/panel: nt35510: support FRIDA FRD400B25025-A-CTK")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Acked-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
> Stefan: if this patch works and you have time you can try adding
> the three flags one at a time to your device and we can see which
> command is problematic.
> ---
>   drivers/gpu/drm/panel/panel-novatek-nt35510.c | 45 +++++++++++++++++----------
>   1 file changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> index d3bfdfc9cff6..e07409dcbd1d 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> @@ -38,6 +38,9 @@
>   
>   #define NT35510_CMD_CORRECT_GAMMA BIT(0)
>   #define NT35510_CMD_CONTROL_DISPLAY BIT(1)
> +#define NT35510_CMD_SETVCL BIT(2)
> +#define NT35510_CMD_BT3CTR BIT(3)
> +#define NT35510_CMD_SETVCMOFF BIT(4)
>   
>   #define MCS_CMD_MAUCCTR		0xF0 /* Manufacturer command enable */
>   #define MCS_CMD_READ_ID1	0xDA
> @@ -675,16 +678,23 @@ static int nt35510_setup_power(struct nt35510 *nt)
>   				nt->conf->bt2ctr);
>   	if (ret)
>   		return ret;
> -	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
> -				NT35510_P1_VCL_LEN,
> -				nt->conf->vcl);
> -	if (ret)
> -		return ret;
> -	ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
> -				NT35510_P1_BT3CTR_LEN,
> -				nt->conf->bt3ctr);
> -	if (ret)
> -		return ret;
> +
> +	if (nt->conf->cmds & NT35510_CMD_SETVCL) {
> +		ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
> +					NT35510_P1_VCL_LEN,
> +					nt->conf->vcl);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (nt->conf->cmds & NT35510_CMD_BT3CTR) {
> +		ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
> +					NT35510_P1_BT3CTR_LEN,
> +					nt->conf->bt3ctr);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVGH,
>   				NT35510_P1_VGH_LEN,
>   				nt->conf->vgh);
> @@ -721,11 +731,13 @@ static int nt35510_setup_power(struct nt35510 *nt)
>   	if (ret)
>   		return ret;
>   
> -	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
> -				NT35510_P1_VCMOFF_LEN,
> -				nt->conf->vcmoff);
> -	if (ret)
> -		return ret;
> +	if (nt->conf->cmds & NT35510_CMD_SETVCMOFF) {
> +		ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
> +					NT35510_P1_VCMOFF_LEN,
> +					nt->conf->vcmoff);
> +		if (ret)
> +			return ret;
> +	}
>   
>   	/* Typically 10 ms */
>   	usleep_range(10000, 20000);
> @@ -1319,7 +1331,8 @@ static const struct nt35510_config nt35510_frida_frd400b25025 = {
>   	},
>   	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
>   			MIPI_DSI_MODE_LPM,
> -	.cmds = NT35510_CMD_CONTROL_DISPLAY,
> +	.cmds = NT35510_CMD_CONTROL_DISPLAY | NT35510_CMD_SETVCL |
> +			NT35510_CMD_BT3CTR | NT35510_CMD_SETVCMOFF,
>   	/* 0x03: AVDD = 6.2V */
>   	.avdd = { 0x03, 0x03, 0x03 },
>   	/* 0x46: PCK = 2 x Hsync, BTP = 2.5 x VDDB */
> 
> ---
> base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
> change-id: 20240906-fix-nt35510-a8ec6e47e036
> 
> Best regards,
> -- 
> Linus Walleij <linus.walleij@linaro.org>
>
Stefan Hansson Sept. 8, 2024, 7:32 p.m. UTC | #2 
Hi Linus!

On 2024-09-06 23:54, Linus Walleij wrote:
> The commit introducing the Frida display started to write the
> SETVCL, BT3CTR and SETVCMOFF registers unconditionally, and some
> (not all!) Hydis display seem to be affected by ghosting after
> the commit.
> 
> Make them all optional and only send these commands on the
> Frida display for now.
> 
> Reported-by: Stefan Hansson <newbyte@postmarketos.org>
> Fixes: 219a1f49094f ("drm/panel: nt35510: support FRIDA FRD400B25025-A-CTK")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Stefan: if this patch works and you have time you can try adding
> the three flags one at a time to your device and we can see which
> command is problematic.

I did just that.

I tried different combinations of the .cmds property of 
nt35510_hydis_hva40wv1 and came to these conclusions:

Good:

     .cmds = NT35510_CMD_CORRECT_GAMMA
     .cmds = NT35510_CMD_CORRECT_GAMMA | NT35510_CMD_SETVCL
     .cmds = NT35510_CMD_CORRECT_GAMMA | NT35510_CMD_SETVCL | 
NT35510_CMD_BT3CTR

Bad:

     .cmds = NT35510_CMD_CORRECT_GAMMA | NT35510_CMD_SETVCL | 
NT35510_CMD_BT3CTR | NT35510_CMD_SETVCMOFF
     .cmds = NT35510_CMD_CORRECT_GAMMA | NT35510_CMD_SETVCMOFF

By good, I mean "no visible ghosting" and by bad I mean "visible ghosting".

Based on this, it seems to me that NT35510_CMD_SETVCMOFF is the culprit. 
However, this patch is also good as-is.

> ---
>   drivers/gpu/drm/panel/panel-novatek-nt35510.c | 45 +++++++++++++++++----------
>   1 file changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> index d3bfdfc9cff6..e07409dcbd1d 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> @@ -38,6 +38,9 @@
>   
>   #define NT35510_CMD_CORRECT_GAMMA BIT(0)
>   #define NT35510_CMD_CONTROL_DISPLAY BIT(1)
> +#define NT35510_CMD_SETVCL BIT(2)
> +#define NT35510_CMD_BT3CTR BIT(3)
> +#define NT35510_CMD_SETVCMOFF BIT(4)
>   
>   #define MCS_CMD_MAUCCTR		0xF0 /* Manufacturer command enable */
>   #define MCS_CMD_READ_ID1	0xDA
> @@ -675,16 +678,23 @@ static int nt35510_setup_power(struct nt35510 *nt)
>   				nt->conf->bt2ctr);
>   	if (ret)
>   		return ret;
> -	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
> -				NT35510_P1_VCL_LEN,
> -				nt->conf->vcl);
> -	if (ret)
> -		return ret;
> -	ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
> -				NT35510_P1_BT3CTR_LEN,
> -				nt->conf->bt3ctr);
> -	if (ret)
> -		return ret;
> +
> +	if (nt->conf->cmds & NT35510_CMD_SETVCL) {
> +		ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
> +					NT35510_P1_VCL_LEN,
> +					nt->conf->vcl);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (nt->conf->cmds & NT35510_CMD_BT3CTR) {
> +		ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
> +					NT35510_P1_BT3CTR_LEN,
> +					nt->conf->bt3ctr);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVGH,
>   				NT35510_P1_VGH_LEN,
>   				nt->conf->vgh);
> @@ -721,11 +731,13 @@ static int nt35510_setup_power(struct nt35510 *nt)
>   	if (ret)
>   		return ret;
>   
> -	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
> -				NT35510_P1_VCMOFF_LEN,
> -				nt->conf->vcmoff);
> -	if (ret)
> -		return ret;
> +	if (nt->conf->cmds & NT35510_CMD_SETVCMOFF) {
> +		ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
> +					NT35510_P1_VCMOFF_LEN,
> +					nt->conf->vcmoff);
> +		if (ret)
> +			return ret;
> +	}
>   
>   	/* Typically 10 ms */
>   	usleep_range(10000, 20000);
> @@ -1319,7 +1331,8 @@ static const struct nt35510_config nt35510_frida_frd400b25025 = {
>   	},
>   	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
>   			MIPI_DSI_MODE_LPM,
> -	.cmds = NT35510_CMD_CONTROL_DISPLAY,
> +	.cmds = NT35510_CMD_CONTROL_DISPLAY | NT35510_CMD_SETVCL |
> +			NT35510_CMD_BT3CTR | NT35510_CMD_SETVCMOFF,
>   	/* 0x03: AVDD = 6.2V */
>   	.avdd = { 0x03, 0x03, 0x03 },
>   	/* 0x46: PCK = 2 x Hsync, BTP = 2.5 x VDDB */
> 
> ---
> base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
> change-id: 20240906-fix-nt35510-a8ec6e47e036
> 
> Best regards,

Stefan Hansson
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
index d3bfdfc9cff6..e07409dcbd1d 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
@@ -38,6 +38,9 @@ 
 
 #define NT35510_CMD_CORRECT_GAMMA BIT(0)
 #define NT35510_CMD_CONTROL_DISPLAY BIT(1)
+#define NT35510_CMD_SETVCL BIT(2)
+#define NT35510_CMD_BT3CTR BIT(3)
+#define NT35510_CMD_SETVCMOFF BIT(4)
 
 #define MCS_CMD_MAUCCTR		0xF0 /* Manufacturer command enable */
 #define MCS_CMD_READ_ID1	0xDA
@@ -675,16 +678,23 @@  static int nt35510_setup_power(struct nt35510 *nt)
 				nt->conf->bt2ctr);
 	if (ret)
 		return ret;
-	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
-				NT35510_P1_VCL_LEN,
-				nt->conf->vcl);
-	if (ret)
-		return ret;
-	ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
-				NT35510_P1_BT3CTR_LEN,
-				nt->conf->bt3ctr);
-	if (ret)
-		return ret;
+
+	if (nt->conf->cmds & NT35510_CMD_SETVCL) {
+		ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
+					NT35510_P1_VCL_LEN,
+					nt->conf->vcl);
+		if (ret)
+			return ret;
+	}
+
+	if (nt->conf->cmds & NT35510_CMD_BT3CTR) {
+		ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
+					NT35510_P1_BT3CTR_LEN,
+					nt->conf->bt3ctr);
+		if (ret)
+			return ret;
+	}
+
 	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVGH,
 				NT35510_P1_VGH_LEN,
 				nt->conf->vgh);
@@ -721,11 +731,13 @@  static int nt35510_setup_power(struct nt35510 *nt)
 	if (ret)
 		return ret;
 
-	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
-				NT35510_P1_VCMOFF_LEN,
-				nt->conf->vcmoff);
-	if (ret)
-		return ret;
+	if (nt->conf->cmds & NT35510_CMD_SETVCMOFF) {
+		ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
+					NT35510_P1_VCMOFF_LEN,
+					nt->conf->vcmoff);
+		if (ret)
+			return ret;
+	}
 
 	/* Typically 10 ms */
 	usleep_range(10000, 20000);
@@ -1319,7 +1331,8 @@  static const struct nt35510_config nt35510_frida_frd400b25025 = {
 	},
 	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
 			MIPI_DSI_MODE_LPM,
-	.cmds = NT35510_CMD_CONTROL_DISPLAY,
+	.cmds = NT35510_CMD_CONTROL_DISPLAY | NT35510_CMD_SETVCL |
+			NT35510_CMD_BT3CTR | NT35510_CMD_SETVCMOFF,
 	/* 0x03: AVDD = 6.2V */
 	.avdd = { 0x03, 0x03, 0x03 },
 	/* 0x46: PCK = 2 x Hsync, BTP = 2.5 x VDDB */