[PATCH] Revert "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE"

[PATCH] Revert "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE"

[Paul Kocialkowski]

This reverts commit 2c17a4368aad2b88b68e4390c819e226cf320f70.

The offending commit triggers a run-time fault when accessing the panel
element of the sun4i_tcon structure when no such panel is attached.

It was apparently assumed in said commit that a panel is always used with
the TCON. Although it is often the case, this is not always true.
For instance a bridge might be used instead of a panel.

This issue was discovered using an A13-OLinuXino, that uses the TCON
in RGB mode for a simple DAC-based VGA bridge.

Cc: stable@vger.kernel.org
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c3d92d537240..8045871335b5 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -17,7 +17,6 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_modes.h>
 #include <drm/drm_of.h>
-#include <drm/drm_panel.h>
 
 #include <uapi/drm/drm_mode.h>
 
@@ -350,9 +349,6 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
 static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 				     const struct drm_display_mode *mode)
 {
-	struct drm_panel *panel = tcon->panel;
-	struct drm_connector *connector = panel->connector;
-	struct drm_display_info display_info = connector->display_info;
 	unsigned int bp, hsync, vsync;
 	u8 clk_delay;
 	u32 val = 0;
@@ -410,27 +406,6 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 		val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
 
-	/*
-	 * On A20 and similar SoCs, the only way to achieve Positive Edge
-	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
-	 * By default TCON works in Negative Edge(Falling Edge),
-	 * this is why phase is set to 0 in that case.
-	 * Unfortunately there's no way to logically invert dclk through
-	 * IO_POL register.
-	 * The only acceptable way to work, triple checked with scope,
-	 * is using clock phase set to 0° for Negative Edge and set to 240°
-	 * for Positive Edge.
-	 * On A33 and similar SoCs there would be a 90° phase option,
-	 * but it divides also dclk by 2.
-	 * Following code is a way to avoid quirks all around TCON
-	 * and DOTCLOCK drivers.
-	 */
-	if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)
-		clk_set_phase(tcon->dclk, 240);
-
-	if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
-		clk_set_phase(tcon->dclk, 0);
-
 	regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
 			   SUN4I_TCON0_IO_POL_HSYNC_POSITIVE | SUN4I_TCON0_IO_POL_VSYNC_POSITIVE,
 			   val);
-- 
2.17.0

Re: [PATCH] Revert "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE"

[Giulio Benetti]

Hello,

sorry for my ignorance.
I don't know the right patch workflow in the case of "revert commit".
When I fix this bug, should I have to re-submit the previous patch 
entire plus bug-fix?
Or do I have to submit patch with bug-fix only?

Thanks in advance to everybody

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

Il 13/06/2018 10:16, Paul Kocialkowski ha scritto:
> This reverts commit 2c17a4368aad2b88b68e4390c819e226cf320f70.
> 
> The offending commit triggers a run-time fault when accessing the panel
> element of the sun4i_tcon structure when no such panel is attached.
> 
> It was apparently assumed in said commit that a panel is always used with
> the TCON. Although it is often the case, this is not always true.
> For instance a bridge might be used instead of a panel.
> 
> This issue was discovered using an A13-OLinuXino, that uses the TCON
> in RGB mode for a simple DAC-based VGA bridge.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>   drivers/gpu/drm/sun4i/sun4i_tcon.c | 25 -------------------------
>   1 file changed, 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index c3d92d537240..8045871335b5 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -17,7 +17,6 @@
>   #include <drm/drm_encoder.h>
>   #include <drm/drm_modes.h>
>   #include <drm/drm_of.h>
> -#include <drm/drm_panel.h>
>   
>   #include <uapi/drm/drm_mode.h>
>   
> @@ -350,9 +349,6 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
>   static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>   				     const struct drm_display_mode *mode)
>   {
> -	struct drm_panel *panel = tcon->panel;
> -	struct drm_connector *connector = panel->connector;
> -	struct drm_display_info display_info = connector->display_info;
>   	unsigned int bp, hsync, vsync;
>   	u8 clk_delay;
>   	u32 val = 0;
> @@ -410,27 +406,6 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>   	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>   		val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>   
> -	/*
> -	 * On A20 and similar SoCs, the only way to achieve Positive Edge
> -	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
> -	 * By default TCON works in Negative Edge(Falling Edge),
> -	 * this is why phase is set to 0 in that case.
> -	 * Unfortunately there's no way to logically invert dclk through
> -	 * IO_POL register.
> -	 * The only acceptable way to work, triple checked with scope,
> -	 * is using clock phase set to 0° for Negative Edge and set to 240°
> -	 * for Positive Edge.
> -	 * On A33 and similar SoCs there would be a 90° phase option,
> -	 * but it divides also dclk by 2.
> -	 * Following code is a way to avoid quirks all around TCON
> -	 * and DOTCLOCK drivers.
> -	 */
> -	if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)
> -		clk_set_phase(tcon->dclk, 240);
> -
> -	if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
> -		clk_set_phase(tcon->dclk, 0);
> -
>   	regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
>   			   SUN4I_TCON0_IO_POL_HSYNC_POSITIVE | SUN4I_TCON0_IO_POL_VSYNC_POSITIVE,
>   			   val);
> 

Re: [PATCH] Revert "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE"

[Paul Kocialkowski]

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

Hi,

On Wed, 2018-06-13 at 23:52 +0200, Giulio Benetti wrote:
> Hello,
> 
> sorry for my ignorance.
> I don't know the right patch workflow in the case of "revert commit".
> When I fix this bug, should I have to re-submit the previous patch 
> entire plus bug-fix?
>
> Or do I have to submit patch with bug-fix only?

Yes, that is usually how it works! The revert patch will be picked up by
the maintainer (Maxime), integrated in his tree and eventually merged
into Linus' tree (along with stable trees).

Fixup patches for this will need to take into account the revert patch,
so it becomes equivalent to submitting the same patch with that issue
resolved.

> Thanks in advance to everybody

Cheers !

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Revert "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE"

[Giulio Benetti]

Hi Paul,

Il 14/06/2018 09:26, Paul Kocialkowski ha scritto:
> Hi,
> 
> On Wed, 2018-06-13 at 23:52 +0200, Giulio Benetti wrote:
>> Hello,
>>
>> sorry for my ignorance.
>> I don't know the right patch workflow in the case of "revert commit".
>> When I fix this bug, should I have to re-submit the previous patch
>> entire plus bug-fix?
>>
>> Or do I have to submit patch with bug-fix only?
> 
> Yes, that is usually how it works! The revert patch will be picked up by
> the maintainer (Maxime), integrated in his tree and eventually merged
> into Linus' tree (along with stable trees).
> 
> Fixup patches for this will need to take into account the revert patch,
> so it becomes equivalent to submitting the same patch with that issue
> resolved.

Thanks for explaining me.
I'm going to submit new patch asap.

Giulio

Re: [PATCH] Revert "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE"

[Maxime Ripard]

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

On Wed, Jun 13, 2018 at 10:16:47AM +0200, Paul Kocialkowski wrote:
> This reverts commit 2c17a4368aad2b88b68e4390c819e226cf320f70.
> 
> The offending commit triggers a run-time fault when accessing the panel
> element of the sun4i_tcon structure when no such panel is attached.
> 
> It was apparently assumed in said commit that a panel is always used with
> the TCON. Although it is often the case, this is not always true.
> For instance a bridge might be used instead of a panel.
> 
> This issue was discovered using an A13-OLinuXino, that uses the TCON
> in RGB mode for a simple DAC-based VGA bridge.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Applied, thanks

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[Giulio Benetti]

Handle both positive and negative dclk polarity,
according to bus_flags, taking care of this:

On A20 and similar SoCs, the only way to achieve Positive Edge
(Rising Edge), is setting dclk clock phase to 2/3(240°).
By default TCON works in Negative Edge(Falling Edge), this is why phase
is set to 0 in that case.
Unfortunately there's no way to logically invert dclk through IO_POL
register.
The only acceptable way to work, triple checked with scope,
is using clock phase set to 0° for Negative Edge and set to 240° for
Positive Edge.
On A33 and similar SoCs there would be a 90° phase option, but it divides
also dclk by 2.
This patch is a way to avoid quirks all around TCON and DOTCLOCK drivers
for using A33 90° phase divided by 2 and consequently increase code
complexity.

Check if panel is used. TCON can also handle VGA DAC, then panel could
be empty.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 8232b39e16ca..5c7d6ae53111 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -17,6 +17,7 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_modes.h>
 #include <drm/drm_of.h>
+#include <drm/drm_panel.h>
 
 #include <uapi/drm/drm_mode.h>
 
@@ -474,6 +475,33 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 		val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
 
+	/*
+	 * On A20 and similar SoCs, the only way to achieve Positive Edge
+	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
+	 * By default TCON works in Negative Edge(Falling Edge),
+	 * this is why phase is set to 0 in that case.
+	 * Unfortunately there's no way to logically invert dclk through
+	 * IO_POL register.
+	 * The only acceptable way to work, triple checked with scope,
+	 * is using clock phase set to 0° for Negative Edge and set to 240°
+	 * for Positive Edge.
+	 * On A33 and similar SoCs there would be a 90° phase option,
+	 * but it divides also dclk by 2.
+	 * Following code is a way to avoid quirks all around TCON
+	 * and DOTCLOCK drivers.
+	 */
+	if (!IS_ERR(tcon->panel)) {
+		struct drm_panel *panel = tcon->panel;
+		struct drm_connector *connector = panel->connector;
+		struct drm_display_info display_info = connector->display_info;
+
+		if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)
+			clk_set_phase(tcon->dclk, 240);
+
+		if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
+			clk_set_phase(tcon->dclk, 0);
+	}
+
 	regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
 			   SUN4I_TCON0_IO_POL_HSYNC_POSITIVE | SUN4I_TCON0_IO_POL_VSYNC_POSITIVE,
 			   val);
-- 
2.17.1

Re: [PATCH] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[Giulio Benetti]

Hi Paul,

can you give a try to this patch on A13 with VGA DAC?
Unfortunately I don't have an A13 board to test it.

Thanks in advance.

Giulio

Il 18/07/2018 16:23, Giulio Benetti ha scritto:
> Handle both positive and negative dclk polarity,
> according to bus_flags, taking care of this:
> 
> On A20 and similar SoCs, the only way to achieve Positive Edge
> (Rising Edge), is setting dclk clock phase to 2/3(240°).
> By default TCON works in Negative Edge(Falling Edge), this is why phase
> is set to 0 in that case.
> Unfortunately there's no way to logically invert dclk through IO_POL
> register.
> The only acceptable way to work, triple checked with scope,
> is using clock phase set to 0° for Negative Edge and set to 240° for
> Positive Edge.
> On A33 and similar SoCs there would be a 90° phase option, but it divides
> also dclk by 2.
> This patch is a way to avoid quirks all around TCON and DOTCLOCK drivers
> for using A33 90° phase divided by 2 and consequently increase code
> complexity.
> 
> Check if panel is used. TCON can also handle VGA DAC, then panel could
> be empty.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>   drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 8232b39e16ca..5c7d6ae53111 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -17,6 +17,7 @@
>   #include <drm/drm_encoder.h>
>   #include <drm/drm_modes.h>
>   #include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
>   
>   #include <uapi/drm/drm_mode.h>
>   
> @@ -474,6 +475,33 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>   	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>   		val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>   
> +	/*
> +	 * On A20 and similar SoCs, the only way to achieve Positive Edge
> +	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
> +	 * By default TCON works in Negative Edge(Falling Edge),
> +	 * this is why phase is set to 0 in that case.
> +	 * Unfortunately there's no way to logically invert dclk through
> +	 * IO_POL register.
> +	 * The only acceptable way to work, triple checked with scope,
> +	 * is using clock phase set to 0° for Negative Edge and set to 240°
> +	 * for Positive Edge.
> +	 * On A33 and similar SoCs there would be a 90° phase option,
> +	 * but it divides also dclk by 2.
> +	 * Following code is a way to avoid quirks all around TCON
> +	 * and DOTCLOCK drivers.
> +	 */
> +	if (!IS_ERR(tcon->panel)) {
> +		struct drm_panel *panel = tcon->panel;
> +		struct drm_connector *connector = panel->connector;
> +		struct drm_display_info display_info = connector->display_info;
> +
> +		if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)
> +			clk_set_phase(tcon->dclk, 240);
> +
> +		if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
> +			clk_set_phase(tcon->dclk, 0);
> +	}
> +
>   	regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
>   			   SUN4I_TCON0_IO_POL_HSYNC_POSITIVE | SUN4I_TCON0_IO_POL_VSYNC_POSITIVE,
>   			   val);
> 

Re: [PATCH] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[Maxime Ripard]

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

On Wed, Jul 18, 2018 at 04:23:57PM +0200, Giulio Benetti wrote:
> Handle both positive and negative dclk polarity,
> according to bus_flags, taking care of this:
> 
> On A20 and similar SoCs, the only way to achieve Positive Edge
> (Rising Edge), is setting dclk clock phase to 2/3(240°).
> By default TCON works in Negative Edge(Falling Edge), this is why phase
> is set to 0 in that case.
> Unfortunately there's no way to logically invert dclk through IO_POL
> register.
> The only acceptable way to work, triple checked with scope,
> is using clock phase set to 0° for Negative Edge and set to 240° for
> Positive Edge.
> On A33 and similar SoCs there would be a 90° phase option, but it divides
> also dclk by 2.
> This patch is a way to avoid quirks all around TCON and DOTCLOCK drivers
> for using A33 90° phase divided by 2 and consequently increase code
> complexity.
> 
> Check if panel is used. TCON can also handle VGA DAC, then panel could
> be empty.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

Applied, thanks

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]