mbox series

[v3,00/16] Implement H3/H5 HDMI driver

Message ID 20180301213442.16677-1-jernej.skrabec@siol.net
Headers show
Series Implement H3/H5 HDMI driver | expand

Message

Jernej Škrabec March 1, 2018, 9:34 p.m. UTC
This series implements H3/H5 HDMI driver. It was tested on OrangePi 2 (H3),
OrangePi Plus2e (H3) and OrangePi PC2 (H5) with many resolutions and it
works well.

Code is based on linux-next, next-20180228 tag.

Best regards,
Jernej

Changes in v3:
- Removed TCON patch to skip LVDS procesing
- Added TCON patch to release exclusive clock lock
- Minimal clock rate is now returned immediately in round_rate for NM PLLs

Changes in v2:
- Fixed build warning on arm64
- Fixed condition in determine_rate function in HDMI PHY clock driver
- Added defines for polarity settings in HDMI PHY
- Added patch to skip LVDS code path altogether if TCON doesn't support it.
- round_rate for NM PLLs now rounds to minimal rate if requested rate is
  lower.
- set_rate for NM PLLs doesn't fail if requested rate is lower than minimal
  (round_rate is called before which already guarantees that rate is not
   lower than minimal).

Jernej Skrabec (16):
  clk: sunxi-ng: Add check for minimal rate to NM PLLs
  clk: sunxi-ng: h3: h5: Add minimal rate for video PLL
  clk: sunxi-ng: h3: h5: Allow some clocks to set parent rate
  clk: sunxi-ng: h3: h5: export CLK_PLL_VIDEO
  dt-bindings: display: sun4i-drm: Add compatibles for H3 HDMI pipeline
  drm/sun4i: Release exclusive clock lock when disabling TCON
  drm/sun4i: Add support for H3 display engine
  drm/sun4i: Add support for H3 mixer 0
  drm/sun4i: Fix polarity configuration for DW HDMI PHY
  drm/sun4i: Add support for variants to DW HDMI PHY
  drm/sun4i: Move and expand DW HDMI PHY register macros
  drm/sun4i: Add support for H3 HDMI PHY variant
  drm/sun4i: Allow building on arm64
  ARM: dts: sunxi: h3/h5: Add HDMI pipeline
  ARM: dts: sun8i: h3: Enable HDMI output on H3 boards
  ARM64: dts: sun50i: h5: Enable HDMI output on H5 boards

 .../bindings/display/sunxi/sun4i-drm.txt           |   6 +
 arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts    |  25 ++
 arch/arm/boot/dts/sun8i-h3-beelink-x2.dts          |  25 ++
 arch/arm/boot/dts/sun8i-h3-libretech-all-h3-cc.dts |  25 ++
 arch/arm/boot/dts/sun8i-h3-nanopi-m1.dts           |  25 ++
 arch/arm/boot/dts/sun8i-h3-orangepi-2.dts          |  25 ++
 arch/arm/boot/dts/sun8i-h3-orangepi-lite.dts       |  25 ++
 arch/arm/boot/dts/sun8i-h3-orangepi-one.dts        |  24 ++
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts         |  25 ++
 arch/arm/boot/dts/sunxi-h3-h5.dtsi                 | 108 ++++++
 .../boot/dts/allwinner/sun50i-h5-orangepi-pc2.dts  |  25 ++
 .../dts/allwinner/sun50i-h5-orangepi-prime.dts     |  25 ++
 .../allwinner/sun50i-h5-orangepi-zero-plus2.dts    |  25 ++
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c                |  32 +-
 drivers/clk/sunxi-ng/ccu-sun8i-h3.h                |   4 +-
 drivers/clk/sunxi-ng/ccu_nm.c                      |   7 +
 drivers/clk/sunxi-ng/ccu_nm.h                      |  27 ++
 drivers/gpu/drm/sun4i/Kconfig                      |   2 +-
 drivers/gpu/drm/sun4i/Makefile                     |   1 +
 drivers/gpu/drm/sun4i/sun4i_drv.c                  |   1 +
 drivers/gpu/drm/sun4i/sun4i_tcon.c                 |   6 +-
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h              | 157 ++++++++-
 drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c             | 369 ++++++++++++++++++---
 drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c         | 132 ++++++++
 drivers/gpu/drm/sun4i/sun8i_mixer.c                |  12 +
 include/dt-bindings/clock/sun8i-h3-ccu.h           |   2 +
 26 files changed, 1070 insertions(+), 70 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c

Comments

Maxime Ripard March 2, 2018, 9:38 a.m. UTC | #1
On Thu, Mar 01, 2018 at 10:34:26PM +0100, Jernej Skrabec wrote:
> This series implements H3/H5 HDMI driver. It was tested on OrangePi 2 (H3),
> OrangePi Plus2e (H3) and OrangePi PC2 (H5) with many resolutions and it
> works well.
> 
> Code is based on linux-next, next-20180228 tag.

Applied everything, thanks!
Maxime
Joonas Kylmälä March 5, 2018, 3:27 p.m. UTC | #2
Jernej Skrabec:
> +&hdmi_out {
> +	hdmi_out_con: endpoint {
> +		remote-endpoint = <&hdmi_con_in>;
> +	};
> +};

This node is added to all the DTS files you enabled HDMI on. Is it
something that could be instead put to the DTSI file?

Joonas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jernej Škrabec March 5, 2018, 6:24 p.m. UTC | #3
Hi!

Dne ponedeljek, 05. marec 2018 ob 16:27:00 CET je Joonas Kylmälä napisal(a):
> Jernej Skrabec:
> > +&hdmi_out {
> > +	hdmi_out_con: endpoint {
> > +		remote-endpoint = <&hdmi_con_in>;
> > +	};
> > +};
> 
> This node is added to all the DTS files you enabled HDMI on. Is it
> something that could be instead put to the DTSI file?

I guess that would mean also including connector node (hdmi_con_in) in DTSI, 
since it is referenced inside. However, not all boards have HDMI connector, so 
I didn't include it in DTSI.

Best regards,
Jernej


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joonas Kylmälä March 5, 2018, 8:43 p.m. UTC | #4
Jernej Škrabec:
> I guess that would mean also including connector node (hdmi_con_in) in DTSI, 
> since it is referenced inside. However, not all boards have HDMI connector, so 
> I didn't include it in DTSI.

You're absolutely right on this. I wish there was someway to get rid of
this duplication but that's a whole different problem to discuss.

Great work by the way!

Joonas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ondřej Jirman March 8, 2018, 10:47 p.m. UTC | #5
Hi,

On Thu, Mar 01, 2018 at 10:34:32PM +0100, Jernej Skrabec wrote:
> Currently exclusive TCON clock lock is never released, which, for
> example, prevents changing resolution on HDMI.
> 
> In order to fix that, release clock when disabling TCON. TCON is always
> disabled first before new mode is set.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 1d714c06ec9d..7f6c4125c89f 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -102,10 +102,12 @@ static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
>  		return;
>  	}
>  
> -	if (enabled)
> +	if (enabled) {
>  		clk_prepare_enable(clk);
> -	else
> +	} else {
> +		clk_rate_exclusive_put(clk);
>  		clk_disable_unprepare(clk);
> +	}
>  }

At least in the linux-next/master I can't see clk_rate_exclusive_get call:

$ git grep 'exclusive' linux-next/master -- drivers/gpu/drm/sun4i

linux-next/master:sun4i_hdmi.h:    * On sun5i the threshold is exclusive, i.e. does not include,
linux-next/master:sun4i_tcon.c:           clk_rate_exclusive_put(clk);
linux-next/master:sun4i_tcon.c:   clk_set_rate_exclusive(tcon->dclk, mode->crtc_clock * 1000);
linux-next/master:sun4i_tcon.c:   clk_set_rate_exclusive(tcon->sclk1, mode->crtc_clock * 1000);

and the kernel complains too:

[  841.915161] ------------[ cut here ]------------
[  841.915187] WARNING: CPU: 0 PID: 273 at /workspace/megous.com/orangepi-pc/linux-4.16/drivers/clk/clk.c:608 clk_rate_exclusive_put+0x48/0x4c
[  841.915194] CPU: 0 PID: 273 Comm: Xorg Not tainted 4.16.0-rc4-00268-gbac2ecf73ed0 #13
[  841.915196] Hardware name: Allwinner sun8i Family
[  841.915217] [<c0228440>] (unwind_backtrace) from [<c0225b90>] (show_stack+0x10/0x14)
[  841.915228] [<c0225b90>] (show_stack) from [<c0b094bc>] (dump_stack+0x88/0x9c)
[  841.915237] [<c0b094bc>] (dump_stack) from [<c0240294>] (__warn+0xd4/0xf0)
[  841.915244] [<c0240294>] (__warn) from [<c0240380>] (warn_slowpath_null+0x40/0x48)
[  841.915250] [<c0240380>] (warn_slowpath_null) from [<c0619b6c>] (clk_rate_exclusive_put+0x48/0x4c)
[  841.915261] [<c0619b6c>] (clk_rate_exclusive_put) from [<c0694624>] (sun4i_tcon_set_status+0x178/0x2f0)
[  841.915269] [<c0694624>] (sun4i_tcon_set_status) from [<c06930c0>] (sun4i_crtc_atomic_disable+0x7c/0xe4)
[  841.915279] [<c06930c0>] (sun4i_crtc_atomic_disable) from [<c06637fc>] (drm_atomic_helper_commit_modeset_disables+0x390/0x46c)
[  841.915288] [<c06637fc>] (drm_atomic_helper_commit_modeset_disables) from [<c0664df4>] (drm_atomic_helper_commit_tail_rpm+0x18/0x64)
[  841.915295] [<c0664df4>] (drm_atomic_helper_commit_tail_rpm) from [<c0664da8>] (commit_tail+0x40/0x6c)
[  841.915302] [<c0664da8>] (commit_tail) from [<c06652b0>] (drm_atomic_helper_commit+0xbc/0x128)
[  841.915311] [<c06652b0>] (drm_atomic_helper_commit) from [<c0668d40>] (restore_fbdev_mode_atomic+0x100/0x1fc)
[  841.915319] [<c0668d40>] (restore_fbdev_mode_atomic) from [<c0668f1c>] (drm_fb_helper_dpms+0x50/0x130)
[  841.915328] [<c0668f1c>] (drm_fb_helper_dpms) from [<c0669e9c>] (drm_fb_helper_blank+0x54/0x90)
[  841.915337] [<c0669e9c>] (drm_fb_helper_blank) from [<c06088cc>] (fb_blank+0x54/0xa8)
[  841.915343] [<c06088cc>] (fb_blank) from [<c0608c80>] (do_fb_ioctl+0x360/0x62c)
[  841.915351] [<c0608c80>] (do_fb_ioctl) from [<c0362648>] (do_vfs_ioctl+0x9c/0x774)
[  841.915358] [<c0362648>] (do_vfs_ioctl) from [<c0362d54>] (SyS_ioctl+0x34/0x58)
[  841.915364] [<c0362d54>] (SyS_ioctl) from [<c0201120>] (ret_fast_syscall+0x0/0x4c)
[  841.915368] Exception stack(0xe5fc5fa8 to 0xe5fc5ff0)
[  841.915373] 5fa0:                   00674f10 00675460 0000000b 00004611 00000001 00000000
[  841.915379] 5fc0: 00674f10 00675460 00000001 00000036 00650474 00000000 006504a4 00675df8
[  841.915382] 5fe0: b61a502c be8acb2c b6192c38 b6b152ec
[  841.915386] ---[ end trace fa81b956197707f8 ]---

It looks like clk_rate_exclusive_put inside sun4i_tcon_channel_set_status is
called without first calling some function that would call
clk_set_rate_exclusive, leading to unbalanced get/put calls.

regards,
  o.

>  static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
> -- 
> 2.16.2
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jernej Škrabec March 8, 2018, 10:57 p.m. UTC | #6
Hi,

Dne četrtek, 08. marec 2018 ob 23:47:17 CET je Ondřej Jirman napisal(a):
> Hi,
> 
> On Thu, Mar 01, 2018 at 10:34:32PM +0100, Jernej Skrabec wrote:
> > Currently exclusive TCON clock lock is never released, which, for
> > example, prevents changing resolution on HDMI.
> > 
> > In order to fix that, release clock when disabling TCON. TCON is always
> > disabled first before new mode is set.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 1d714c06ec9d..7f6c4125c89f
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -102,10 +102,12 @@ static void sun4i_tcon_channel_set_status(struct
> > sun4i_tcon *tcon, int channel,> 
> >  		return;
> >  	
> >  	}
> > 
> > -	if (enabled)
> > +	if (enabled) {
> > 
> >  		clk_prepare_enable(clk);
> > 
> > -	else
> > +	} else {
> > +		clk_rate_exclusive_put(clk);
> > 
> >  		clk_disable_unprepare(clk);
> > 
> > +	}
> > 
> >  }
> 
> At least in the linux-next/master I can't see clk_rate_exclusive_get call:
> 

It is in drm-misc/drm-misc-fixes:
https://cgit.freedesktop.org/drm/drm-misc/commit/?h=drm-misc-fixes&id=79d103a565d16b1893d990b2ee5e0fe71767759f

My patch is also applied in same branch, so there should be no issues while 
merging all those branches.

linux-next doesn't have either patches currently: https://git.kernel.org/pub/
scm/linux/kernel/git/next/linux-next.git/log/drivers/gpu/drm/sun4i

This is issue only if you manually apply one patch without the other.

Best regards,
Jernej

> $ git grep 'exclusive' linux-next/master -- drivers/gpu/drm/sun4i
> 
> linux-next/master:sun4i_hdmi.h:    * On sun5i the threshold is exclusive,
> i.e. does not include, linux-next/master:sun4i_tcon.c:          
> clk_rate_exclusive_put(clk); linux-next/master:sun4i_tcon.c:  
> clk_set_rate_exclusive(tcon->dclk, mode->crtc_clock * 1000);
> linux-next/master:sun4i_tcon.c:   clk_set_rate_exclusive(tcon->sclk1,
> mode->crtc_clock * 1000);
> 
> and the kernel complains too:
> 
> [  841.915161] ------------[ cut here ]------------
> [  841.915187] WARNING: CPU: 0 PID: 273 at
> /workspace/megous.com/orangepi-pc/linux-4.16/drivers/clk/clk.c:608
> clk_rate_exclusive_put+0x48/0x4c [  841.915194] CPU: 0 PID: 273 Comm: Xorg
> Not tainted 4.16.0-rc4-00268-gbac2ecf73ed0 #13 [  841.915196] Hardware
> name: Allwinner sun8i Family
> [  841.915217] [<c0228440>] (unwind_backtrace) from [<c0225b90>]
> (show_stack+0x10/0x14) [  841.915228] [<c0225b90>] (show_stack) from
> [<c0b094bc>] (dump_stack+0x88/0x9c) [  841.915237] [<c0b094bc>]
> (dump_stack) from [<c0240294>] (__warn+0xd4/0xf0) [  841.915244]
> [<c0240294>] (__warn) from [<c0240380>] (warn_slowpath_null+0x40/0x48) [ 
> 841.915250] [<c0240380>] (warn_slowpath_null) from [<c0619b6c>]
> (clk_rate_exclusive_put+0x48/0x4c) [  841.915261] [<c0619b6c>]
> (clk_rate_exclusive_put) from [<c0694624>]
> (sun4i_tcon_set_status+0x178/0x2f0) [  841.915269] [<c0694624>]
> (sun4i_tcon_set_status) from [<c06930c0>]
> (sun4i_crtc_atomic_disable+0x7c/0xe4) [  841.915279] [<c06930c0>]
> (sun4i_crtc_atomic_disable) from [<c06637fc>]
> (drm_atomic_helper_commit_modeset_disables+0x390/0x46c) [  841.915288]
> [<c06637fc>] (drm_atomic_helper_commit_modeset_disables) from [<c0664df4>]
> (drm_atomic_helper_commit_tail_rpm+0x18/0x64) [  841.915295] [<c0664df4>]
> (drm_atomic_helper_commit_tail_rpm) from [<c0664da8>]
> (commit_tail+0x40/0x6c) [  841.915302] [<c0664da8>] (commit_tail) from
> [<c06652b0>] (drm_atomic_helper_commit+0xbc/0x128) [  841.915311]
> [<c06652b0>] (drm_atomic_helper_commit) from [<c0668d40>]
> (restore_fbdev_mode_atomic+0x100/0x1fc) [  841.915319] [<c0668d40>]
> (restore_fbdev_mode_atomic) from [<c0668f1c>]
> (drm_fb_helper_dpms+0x50/0x130) [  841.915328] [<c0668f1c>]
> (drm_fb_helper_dpms) from [<c0669e9c>] (drm_fb_helper_blank+0x54/0x90) [ 
> 841.915337] [<c0669e9c>] (drm_fb_helper_blank) from [<c06088cc>]
> (fb_blank+0x54/0xa8) [  841.915343] [<c06088cc>] (fb_blank) from
> [<c0608c80>] (do_fb_ioctl+0x360/0x62c) [  841.915351] [<c0608c80>]
> (do_fb_ioctl) from [<c0362648>] (do_vfs_ioctl+0x9c/0x774) [  841.915358]
> [<c0362648>] (do_vfs_ioctl) from [<c0362d54>] (SyS_ioctl+0x34/0x58) [ 
> 841.915364] [<c0362d54>] (SyS_ioctl) from [<c0201120>]
> (ret_fast_syscall+0x0/0x4c) [  841.915368] Exception stack(0xe5fc5fa8 to
> 0xe5fc5ff0)
> [  841.915373] 5fa0:                   00674f10 00675460 0000000b 00004611
> 00000001 00000000 [  841.915379] 5fc0: 00674f10 00675460 00000001 00000036
> 00650474 00000000 006504a4 00675df8 [  841.915382] 5fe0: b61a502c be8acb2c
> b6192c38 b6b152ec
> [  841.915386] ---[ end trace fa81b956197707f8 ]---
> 
> It looks like clk_rate_exclusive_put inside sun4i_tcon_channel_set_status is
> called without first calling some function that would call
> clk_set_rate_exclusive, leading to unbalanced get/put calls.
> 
> regards,
>   o.
> 
> >  static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
> > 
> > --
> > 2.16.2
> > 
> > --
> > You received this message because you are subscribed to the Google Groups
> > "linux-sunxi" group. To unsubscribe from this group and stop receiving
> > emails from it, send an email to
> > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit
> > https://groups.google.com/d/optout.




--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ondřej Jirman March 9, 2018, 12:13 a.m. UTC | #7
Hi Jernej,

On Thu, Mar 08, 2018 at 11:57:40PM +0100, Jernej Škrabec wrote:
> Hi,
> 
> Dne četrtek, 08. marec 2018 ob 23:47:17 CET je Ondřej Jirman napisal(a):
> > Hi,
> > 
> > On Thu, Mar 01, 2018 at 10:34:32PM +0100, Jernej Skrabec wrote:
> > > Currently exclusive TCON clock lock is never released, which, for
> > > example, prevents changing resolution on HDMI.
> > > 
> > > In order to fix that, release clock when disabling TCON. TCON is always
> > > disabled first before new mode is set.
> > > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > > 
> > >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 1d714c06ec9d..7f6c4125c89f
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > @@ -102,10 +102,12 @@ static void sun4i_tcon_channel_set_status(struct
> > > sun4i_tcon *tcon, int channel,> 
> > >  		return;
> > >  	
> > >  	}
> > > 
> > > -	if (enabled)
> > > +	if (enabled) {
> > > 
> > >  		clk_prepare_enable(clk);
> > > 
> > > -	else
> > > +	} else {
> > > +		clk_rate_exclusive_put(clk);
> > > 
> > >  		clk_disable_unprepare(clk);
> > > 
> > > +	}
> > > 
> > >  }
> > 
> > At least in the linux-next/master I can't see clk_rate_exclusive_get call:
> > 
> 
> It is in drm-misc/drm-misc-fixes:
> https://cgit.freedesktop.org/drm/drm-misc/commit/?h=drm-misc-fixes&id=79d103a565d16b1893d990b2ee5e0fe71767759f
> 
> My patch is also applied in same branch, so there should be no issues while 
> merging all those branches.
> 
> linux-next doesn't have either patches currently: https://git.kernel.org/pub/
> scm/linux/kernel/git/next/linux-next.git/log/drivers/gpu/drm/sun4i
> 
> This is issue only if you manually apply one patch without the other.

I have (and had) both patches applied. There's perhaps some code path,
where sun4i_tcon_channel_set_status(..., false) is called in unbalanced
way with regards to clk_set_rate_exclusive().

The issue occurs when starting Xorg. But Xorg works fine.

regards,
  o.

> Best regards,
> Jernej
> 
> > $ git grep 'exclusive' linux-next/master -- drivers/gpu/drm/sun4i
> > 
> > linux-next/master:sun4i_hdmi.h:    * On sun5i the threshold is exclusive,
> > i.e. does not include, linux-next/master:sun4i_tcon.c:          
> > clk_rate_exclusive_put(clk); linux-next/master:sun4i_tcon.c:  
> > clk_set_rate_exclusive(tcon->dclk, mode->crtc_clock * 1000);
> > linux-next/master:sun4i_tcon.c:   clk_set_rate_exclusive(tcon->sclk1,
> > mode->crtc_clock * 1000);
> > 
> > and the kernel complains too:
> > 
> > [  841.915161] ------------[ cut here ]------------
> > [  841.915187] WARNING: CPU: 0 PID: 273 at
> > /workspace/megous.com/orangepi-pc/linux-4.16/drivers/clk/clk.c:608
> > clk_rate_exclusive_put+0x48/0x4c [  841.915194] CPU: 0 PID: 273 Comm: Xorg
> > Not tainted 4.16.0-rc4-00268-gbac2ecf73ed0 #13 [  841.915196] Hardware
> > name: Allwinner sun8i Family
> > [  841.915217] [<c0228440>] (unwind_backtrace) from [<c0225b90>]
> > (show_stack+0x10/0x14) [  841.915228] [<c0225b90>] (show_stack) from
> > [<c0b094bc>] (dump_stack+0x88/0x9c) [  841.915237] [<c0b094bc>]
> > (dump_stack) from [<c0240294>] (__warn+0xd4/0xf0) [  841.915244]
> > [<c0240294>] (__warn) from [<c0240380>] (warn_slowpath_null+0x40/0x48) [ 
> > 841.915250] [<c0240380>] (warn_slowpath_null) from [<c0619b6c>]
> > (clk_rate_exclusive_put+0x48/0x4c) [  841.915261] [<c0619b6c>]
> > (clk_rate_exclusive_put) from [<c0694624>]
> > (sun4i_tcon_set_status+0x178/0x2f0) [  841.915269] [<c0694624>]
> > (sun4i_tcon_set_status) from [<c06930c0>]
> > (sun4i_crtc_atomic_disable+0x7c/0xe4) [  841.915279] [<c06930c0>]
> > (sun4i_crtc_atomic_disable) from [<c06637fc>]
> > (drm_atomic_helper_commit_modeset_disables+0x390/0x46c) [  841.915288]
> > [<c06637fc>] (drm_atomic_helper_commit_modeset_disables) from [<c0664df4>]
> > (drm_atomic_helper_commit_tail_rpm+0x18/0x64) [  841.915295] [<c0664df4>]
> > (drm_atomic_helper_commit_tail_rpm) from [<c0664da8>]
> > (commit_tail+0x40/0x6c) [  841.915302] [<c0664da8>] (commit_tail) from
> > [<c06652b0>] (drm_atomic_helper_commit+0xbc/0x128) [  841.915311]
> > [<c06652b0>] (drm_atomic_helper_commit) from [<c0668d40>]
> > (restore_fbdev_mode_atomic+0x100/0x1fc) [  841.915319] [<c0668d40>]
> > (restore_fbdev_mode_atomic) from [<c0668f1c>]
> > (drm_fb_helper_dpms+0x50/0x130) [  841.915328] [<c0668f1c>]
> > (drm_fb_helper_dpms) from [<c0669e9c>] (drm_fb_helper_blank+0x54/0x90) [ 
> > 841.915337] [<c0669e9c>] (drm_fb_helper_blank) from [<c06088cc>]
> > (fb_blank+0x54/0xa8) [  841.915343] [<c06088cc>] (fb_blank) from
> > [<c0608c80>] (do_fb_ioctl+0x360/0x62c) [  841.915351] [<c0608c80>]
> > (do_fb_ioctl) from [<c0362648>] (do_vfs_ioctl+0x9c/0x774) [  841.915358]
> > [<c0362648>] (do_vfs_ioctl) from [<c0362d54>] (SyS_ioctl+0x34/0x58) [ 
> > 841.915364] [<c0362d54>] (SyS_ioctl) from [<c0201120>]
> > (ret_fast_syscall+0x0/0x4c) [  841.915368] Exception stack(0xe5fc5fa8 to
> > 0xe5fc5ff0)
> > [  841.915373] 5fa0:                   00674f10 00675460 0000000b 00004611
> > 00000001 00000000 [  841.915379] 5fc0: 00674f10 00675460 00000001 00000036
> > 00650474 00000000 006504a4 00675df8 [  841.915382] 5fe0: b61a502c be8acb2c
> > b6192c38 b6b152ec
> > [  841.915386] ---[ end trace fa81b956197707f8 ]---
> > 
> > It looks like clk_rate_exclusive_put inside sun4i_tcon_channel_set_status is
> > called without first calling some function that would call
> > clk_set_rate_exclusive, leading to unbalanced get/put calls.
> > 
> > regards,
> >   o.
> > 
> > >  static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
> > > 
> > > --
> > > 2.16.2
> > > 
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "linux-sunxi" group. To unsubscribe from this group and stop receiving
> > > emails from it, send an email to
> > > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit
> > > https://groups.google.com/d/optout.
> 
> 
> 
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ondřej Jirman March 9, 2018, 12:44 a.m. UTC | #8
Hi,

I've debugged this further and it seems that the code has incorrect assumptions.
See docs for mode_set_nofb.

https://elixir.bootlin.com/linux/v4.16-rc4/source/include/drm/drm_modeset_helper_vtables.h#L209

I've added traces to a few functions that call clk_.*exclusive.*() functions,
and I see this after a few restarts of Xorg server:

[    0.783842] *** sun4i_tcon1_mode_set()
[    0.783886] *** sun4i_tcon_channel_set_status(..., 1, true)
[    6.881690] *** sun4i_tcon_channel_set_status(..., 1, false)
[    6.881758] *** sun4i_tcon_channel_set_status(..., 1, true)
[   52.335085] *** sun4i_tcon_channel_set_status(..., 1, false)
[   52.335343] *** sun4i_tcon_channel_set_status(..., 1, true)
[   68.921079] *** sun4i_tcon_channel_set_status(..., 1, false)
[   68.921337] *** sun4i_tcon_channel_set_status(..., 1, true)

mode_set_nofb is called just once, yet sun4i_tcon_channel_set_status(..., 1, false)
is called multiple times, which leads to unbalanced calls to clk_set_rate_exclusive
and clk_rate_exclusive_put.

I don't know how to fix this.

regards,
  o.

On Fri, Mar 09, 2018 at 01:13:14AM +0100, 'Ondřej Jirman' via linux-sunxi wrote:
> Hi Jernej,
> 
> On Thu, Mar 08, 2018 at 11:57:40PM +0100, Jernej Škrabec wrote:
> > Hi,
> > 
> > Dne četrtek, 08. marec 2018 ob 23:47:17 CET je Ondřej Jirman napisal(a):
> > > Hi,
> > > 
> > > On Thu, Mar 01, 2018 at 10:34:32PM +0100, Jernej Skrabec wrote:
> > > > Currently exclusive TCON clock lock is never released, which, for
> > > > example, prevents changing resolution on HDMI.
> > > > 
> > > > In order to fix that, release clock when disabling TCON. TCON is always
> > > > disabled first before new mode is set.
> > > > 
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > ---
> > > > 
> > > >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 1d714c06ec9d..7f6c4125c89f
> > > > 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > @@ -102,10 +102,12 @@ static void sun4i_tcon_channel_set_status(struct
> > > > sun4i_tcon *tcon, int channel,> 
> > > >  		return;
> > > >  	
> > > >  	}
> > > > 
> > > > -	if (enabled)
> > > > +	if (enabled) {
> > > > 
> > > >  		clk_prepare_enable(clk);
> > > > 
> > > > -	else
> > > > +	} else {
> > > > +		clk_rate_exclusive_put(clk);
> > > > 
> > > >  		clk_disable_unprepare(clk);
> > > > 
> > > > +	}
> > > > 
> > > >  }
> > > 
> > > At least in the linux-next/master I can't see clk_rate_exclusive_get call:
> > > 
> > 
> > It is in drm-misc/drm-misc-fixes:
> > https://cgit.freedesktop.org/drm/drm-misc/commit/?h=drm-misc-fixes&id=79d103a565d16b1893d990b2ee5e0fe71767759f
> > 
> > My patch is also applied in same branch, so there should be no issues while 
> > merging all those branches.
> > 
> > linux-next doesn't have either patches currently: https://git.kernel.org/pub/
> > scm/linux/kernel/git/next/linux-next.git/log/drivers/gpu/drm/sun4i
> > 
> > This is issue only if you manually apply one patch without the other.
> 
> I have (and had) both patches applied. There's perhaps some code path,
> where sun4i_tcon_channel_set_status(..., false) is called in unbalanced
> way with regards to clk_set_rate_exclusive().
> 
> The issue occurs when starting Xorg. But Xorg works fine.
> 
> regards,
>   o.
> 
> > Best regards,
> > Jernej
> > 
> > > $ git grep 'exclusive' linux-next/master -- drivers/gpu/drm/sun4i
> > > 
> > > linux-next/master:sun4i_hdmi.h:    * On sun5i the threshold is exclusive,
> > > i.e. does not include, linux-next/master:sun4i_tcon.c:          
> > > clk_rate_exclusive_put(clk); linux-next/master:sun4i_tcon.c:  
> > > clk_set_rate_exclusive(tcon->dclk, mode->crtc_clock * 1000);
> > > linux-next/master:sun4i_tcon.c:   clk_set_rate_exclusive(tcon->sclk1,
> > > mode->crtc_clock * 1000);
> > > 
> > > and the kernel complains too:
> > > 
> > > [  841.915161] ------------[ cut here ]------------
> > > [  841.915187] WARNING: CPU: 0 PID: 273 at
> > > /workspace/megous.com/orangepi-pc/linux-4.16/drivers/clk/clk.c:608
> > > clk_rate_exclusive_put+0x48/0x4c [  841.915194] CPU: 0 PID: 273 Comm: Xorg
> > > Not tainted 4.16.0-rc4-00268-gbac2ecf73ed0 #13 [  841.915196] Hardware
> > > name: Allwinner sun8i Family
> > > [  841.915217] [<c0228440>] (unwind_backtrace) from [<c0225b90>]
> > > (show_stack+0x10/0x14) [  841.915228] [<c0225b90>] (show_stack) from
> > > [<c0b094bc>] (dump_stack+0x88/0x9c) [  841.915237] [<c0b094bc>]
> > > (dump_stack) from [<c0240294>] (__warn+0xd4/0xf0) [  841.915244]
> > > [<c0240294>] (__warn) from [<c0240380>] (warn_slowpath_null+0x40/0x48) [ 
> > > 841.915250] [<c0240380>] (warn_slowpath_null) from [<c0619b6c>]
> > > (clk_rate_exclusive_put+0x48/0x4c) [  841.915261] [<c0619b6c>]
> > > (clk_rate_exclusive_put) from [<c0694624>]
> > > (sun4i_tcon_set_status+0x178/0x2f0) [  841.915269] [<c0694624>]
> > > (sun4i_tcon_set_status) from [<c06930c0>]
> > > (sun4i_crtc_atomic_disable+0x7c/0xe4) [  841.915279] [<c06930c0>]
> > > (sun4i_crtc_atomic_disable) from [<c06637fc>]
> > > (drm_atomic_helper_commit_modeset_disables+0x390/0x46c) [  841.915288]
> > > [<c06637fc>] (drm_atomic_helper_commit_modeset_disables) from [<c0664df4>]
> > > (drm_atomic_helper_commit_tail_rpm+0x18/0x64) [  841.915295] [<c0664df4>]
> > > (drm_atomic_helper_commit_tail_rpm) from [<c0664da8>]
> > > (commit_tail+0x40/0x6c) [  841.915302] [<c0664da8>] (commit_tail) from
> > > [<c06652b0>] (drm_atomic_helper_commit+0xbc/0x128) [  841.915311]
> > > [<c06652b0>] (drm_atomic_helper_commit) from [<c0668d40>]
> > > (restore_fbdev_mode_atomic+0x100/0x1fc) [  841.915319] [<c0668d40>]
> > > (restore_fbdev_mode_atomic) from [<c0668f1c>]
> > > (drm_fb_helper_dpms+0x50/0x130) [  841.915328] [<c0668f1c>]
> > > (drm_fb_helper_dpms) from [<c0669e9c>] (drm_fb_helper_blank+0x54/0x90) [ 
> > > 841.915337] [<c0669e9c>] (drm_fb_helper_blank) from [<c06088cc>]
> > > (fb_blank+0x54/0xa8) [  841.915343] [<c06088cc>] (fb_blank) from
> > > [<c0608c80>] (do_fb_ioctl+0x360/0x62c) [  841.915351] [<c0608c80>]
> > > (do_fb_ioctl) from [<c0362648>] (do_vfs_ioctl+0x9c/0x774) [  841.915358]
> > > [<c0362648>] (do_vfs_ioctl) from [<c0362d54>] (SyS_ioctl+0x34/0x58) [ 
> > > 841.915364] [<c0362d54>] (SyS_ioctl) from [<c0201120>]
> > > (ret_fast_syscall+0x0/0x4c) [  841.915368] Exception stack(0xe5fc5fa8 to
> > > 0xe5fc5ff0)
> > > [  841.915373] 5fa0:                   00674f10 00675460 0000000b 00004611
> > > 00000001 00000000 [  841.915379] 5fc0: 00674f10 00675460 00000001 00000036
> > > 00650474 00000000 006504a4 00675df8 [  841.915382] 5fe0: b61a502c be8acb2c
> > > b6192c38 b6b152ec
> > > [  841.915386] ---[ end trace fa81b956197707f8 ]---
> > > 
> > > It looks like clk_rate_exclusive_put inside sun4i_tcon_channel_set_status is
> > > called without first calling some function that would call
> > > clk_set_rate_exclusive, leading to unbalanced get/put calls.
> > > 
> > > regards,
> > >   o.
> > > 
> > > >  static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
> > > > 
> > > > --
> > > > 2.16.2
> > > > 
> > > > --
> > > > You received this message because you are subscribed to the Google Groups
> > > > "linux-sunxi" group. To unsubscribe from this group and stop receiving
> > > > emails from it, send an email to
> > > > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit
> > > > https://groups.google.com/d/optout.
> > 
> > 
> > 
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jernej Škrabec March 9, 2018, 6:19 a.m. UTC | #9
Hi,

Dne petek, 09. marec 2018 ob 01:44:55 CET je Ondřej Jirman napisal(a):
> Hi,
> 
> I've debugged this further and it seems that the code has incorrect
> assumptions. See docs for mode_set_nofb.
> 
> https://elixir.bootlin.com/linux/v4.16-rc4/source/include/drm/drm_modeset_he
> lper_vtables.h#L209
>

Does this happen with https://github.com/jernejsk/linux-1/tree/h3_hdmi_v3 ?

I also tested running X, but I don't remember having the issue.
 
> I've added traces to a few functions that call clk_.*exclusive.*()
> functions, and I see this after a few restarts of Xorg server:
> 
> [    0.783842] *** sun4i_tcon1_mode_set()
> [    0.783886] *** sun4i_tcon_channel_set_status(..., 1, true)
> [    6.881690] *** sun4i_tcon_channel_set_status(..., 1, false)
> [    6.881758] *** sun4i_tcon_channel_set_status(..., 1, true)
> [   52.335085] *** sun4i_tcon_channel_set_status(..., 1, false)
> [   52.335343] *** sun4i_tcon_channel_set_status(..., 1, true)
> [   68.921079] *** sun4i_tcon_channel_set_status(..., 1, false)
> [   68.921337] *** sun4i_tcon_channel_set_status(..., 1, true)
> 
> mode_set_nofb is called just once, yet sun4i_tcon_channel_set_status(..., 1,
> false) is called multiple times, which leads to unbalanced calls to
> clk_set_rate_exclusive and clk_rate_exclusive_put.
> 
> I don't know how to fix this.

Since there is no function to check if clock is protected, flag can be 
introduced within TCON which is set when clock rate is protected and unset 
when it is unprotected. That way we could track if clk_rate_exclusive_put() 
needs to be called or not.

Best regards,
Jernej

> 
> regards,
>   o.
> 
> On Fri, Mar 09, 2018 at 01:13:14AM +0100, 'Ondřej Jirman' via linux-sunxi 
wrote:
> > Hi Jernej,
> > 
> > On Thu, Mar 08, 2018 at 11:57:40PM +0100, Jernej Škrabec wrote:
> > > Hi,
> > > 
> > > Dne četrtek, 08. marec 2018 ob 23:47:17 CET je Ondřej Jirman napisal(a):
> > > > Hi,
> > > > 
> > > > On Thu, Mar 01, 2018 at 10:34:32PM +0100, Jernej Skrabec wrote:
> > > > > Currently exclusive TCON clock lock is never released, which, for
> > > > > example, prevents changing resolution on HDMI.
> > > > > 
> > > > > In order to fix that, release clock when disabling TCON. TCON is
> > > > > always
> > > > > disabled first before new mode is set.
> > > > > 
> > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > > ---
> > > > > 
> > > > >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c index
> > > > > 1d714c06ec9d..7f6c4125c89f
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > @@ -102,10 +102,12 @@ static void
> > > > > sun4i_tcon_channel_set_status(struct
> > > > > sun4i_tcon *tcon, int channel,>
> > > > > 
> > > > >  		return;
> > > > >  	
> > > > >  	}
> > > > > 
> > > > > -	if (enabled)
> > > > > +	if (enabled) {
> > > > > 
> > > > >  		clk_prepare_enable(clk);
> > > > > 
> > > > > -	else
> > > > > +	} else {
> > > > > +		clk_rate_exclusive_put(clk);
> > > > > 
> > > > >  		clk_disable_unprepare(clk);
> > > > > 
> > > > > +	}
> > > > > 
> > > > >  }
> > > > 
> > > > At least in the linux-next/master I can't see clk_rate_exclusive_get 
call:
> > > It is in drm-misc/drm-misc-fixes:
> > > https://cgit.freedesktop.org/drm/drm-misc/commit/?h=drm-misc-fixes&id=79
> > > d103a565d16b1893d990b2ee5e0fe71767759f
> > > 
> > > My patch is also applied in same branch, so there should be no issues
> > > while
> > > merging all those branches.
> > > 
> > > linux-next doesn't have either patches currently:
> > > https://git.kernel.org/pub/
> > > scm/linux/kernel/git/next/linux-next.git/log/drivers/gpu/drm/sun4i
> > > 
> > > This is issue only if you manually apply one patch without the other.
> > 
> > I have (and had) both patches applied. There's perhaps some code path,
> > where sun4i_tcon_channel_set_status(..., false) is called in unbalanced
> > way with regards to clk_set_rate_exclusive().
> > 
> > The issue occurs when starting Xorg. But Xorg works fine.
> > 
> > regards,
> > 
> >   o.
> >   
> > > Best regards,
> > > Jernej
> > > 
> > > > $ git grep 'exclusive' linux-next/master -- drivers/gpu/drm/sun4i
> > > > 
> > > > linux-next/master:sun4i_hdmi.h:    * On sun5i the threshold is
> > > > exclusive,
> > > > i.e. does not include, linux-next/master:sun4i_tcon.c:
> > > > clk_rate_exclusive_put(clk); linux-next/master:sun4i_tcon.c:
> > > > clk_set_rate_exclusive(tcon->dclk, mode->crtc_clock * 1000);
> > > > linux-next/master:sun4i_tcon.c:   clk_set_rate_exclusive(tcon->sclk1,
> > > > mode->crtc_clock * 1000);
> > > > 
> > > > and the kernel complains too:
> > > > 
> > > > [  841.915161] ------------[ cut here ]------------
> > > > [  841.915187] WARNING: CPU: 0 PID: 273 at
> > > > /workspace/megous.com/orangepi-pc/linux-4.16/drivers/clk/clk.c:608
> > > > clk_rate_exclusive_put+0x48/0x4c [  841.915194] CPU: 0 PID: 273 Comm:
> > > > Xorg
> > > > Not tainted 4.16.0-rc4-00268-gbac2ecf73ed0 #13 [  841.915196] Hardware
> > > > name: Allwinner sun8i Family
> > > > [  841.915217] [<c0228440>] (unwind_backtrace) from [<c0225b90>]
> > > > (show_stack+0x10/0x14) [  841.915228] [<c0225b90>] (show_stack) from
> > > > [<c0b094bc>] (dump_stack+0x88/0x9c) [  841.915237] [<c0b094bc>]
> > > > (dump_stack) from [<c0240294>] (__warn+0xd4/0xf0) [  841.915244]
> > > > [<c0240294>] (__warn) from [<c0240380>] (warn_slowpath_null+0x40/0x48)
> > > > [
> > > > 841.915250] [<c0240380>] (warn_slowpath_null) from [<c0619b6c>]
> > > > (clk_rate_exclusive_put+0x48/0x4c) [  841.915261] [<c0619b6c>]
> > > > (clk_rate_exclusive_put) from [<c0694624>]
> > > > (sun4i_tcon_set_status+0x178/0x2f0) [  841.915269] [<c0694624>]
> > > > (sun4i_tcon_set_status) from [<c06930c0>]
> > > > (sun4i_crtc_atomic_disable+0x7c/0xe4) [  841.915279] [<c06930c0>]
> > > > (sun4i_crtc_atomic_disable) from [<c06637fc>]
> > > > (drm_atomic_helper_commit_modeset_disables+0x390/0x46c) [  841.915288]
> > > > [<c06637fc>] (drm_atomic_helper_commit_modeset_disables) from
> > > > [<c0664df4>]
> > > > (drm_atomic_helper_commit_tail_rpm+0x18/0x64) [  841.915295]
> > > > [<c0664df4>]
> > > > (drm_atomic_helper_commit_tail_rpm) from [<c0664da8>]
> > > > (commit_tail+0x40/0x6c) [  841.915302] [<c0664da8>] (commit_tail) from
> > > > [<c06652b0>] (drm_atomic_helper_commit+0xbc/0x128) [  841.915311]
> > > > [<c06652b0>] (drm_atomic_helper_commit) from [<c0668d40>]
> > > > (restore_fbdev_mode_atomic+0x100/0x1fc) [  841.915319] [<c0668d40>]
> > > > (restore_fbdev_mode_atomic) from [<c0668f1c>]
> > > > (drm_fb_helper_dpms+0x50/0x130) [  841.915328] [<c0668f1c>]
> > > > (drm_fb_helper_dpms) from [<c0669e9c>] (drm_fb_helper_blank+0x54/0x90)
> > > > [
> > > > 841.915337] [<c0669e9c>] (drm_fb_helper_blank) from [<c06088cc>]
> > > > (fb_blank+0x54/0xa8) [  841.915343] [<c06088cc>] (fb_blank) from
> > > > [<c0608c80>] (do_fb_ioctl+0x360/0x62c) [  841.915351] [<c0608c80>]
> > > > (do_fb_ioctl) from [<c0362648>] (do_vfs_ioctl+0x9c/0x774) [ 
> > > > 841.915358]
> > > > [<c0362648>] (do_vfs_ioctl) from [<c0362d54>] (SyS_ioctl+0x34/0x58) [
> > > > 841.915364] [<c0362d54>] (SyS_ioctl) from [<c0201120>]
> > > > (ret_fast_syscall+0x0/0x4c) [  841.915368] Exception stack(0xe5fc5fa8
> > > > to
> > > > 0xe5fc5ff0)
> > > > [  841.915373] 5fa0:                   00674f10 00675460 0000000b
> > > > 00004611
> > > > 00000001 00000000 [  841.915379] 5fc0: 00674f10 00675460 00000001
> > > > 00000036
> > > > 00650474 00000000 006504a4 00675df8 [  841.915382] 5fe0: b61a502c
> > > > be8acb2c
> > > > b6192c38 b6b152ec
> > > > [  841.915386] ---[ end trace fa81b956197707f8 ]---
> > > > 
> > > > It looks like clk_rate_exclusive_put inside
> > > > sun4i_tcon_channel_set_status is called without first calling some
> > > > function that would call
> > > > clk_set_rate_exclusive, leading to unbalanced get/put calls.
> > > > 
> > > > regards,
> > > > 
> > > >   o.
> > > >   
> > > > >  static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
> > > > > 
> > > > > --
> > > > > 2.16.2
> > > > > 
> > > > > --
> > > > > You received this message because you are subscribed to the Google
> > > > > Groups
> > > > > "linux-sunxi" group. To unsubscribe from this group and stop
> > > > > receiving
> > > > > emails from it, send an email to
> > > > > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit
> > > > > https://groups.google.com/d/optout.
> > > 
> > > --
> > > You received this message because you are subscribed to the Google
> > > Groups "linux-sunxi" group. To unsubscribe from this group and stop
> > > receiving emails from it, send an email to
> > > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit
> > > https://groups.google.com/d/optout.
> > 
> > --
> > You received this message because you are subscribed to the Google Groups
> > "linux-sunxi" group. To unsubscribe from this group and stop receiving
> > emails from it, send an email to
> > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit
> > https://groups.google.com/d/optout.




--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ondřej Jirman March 9, 2018, 7:55 a.m. UTC | #10
Hi,

On Fri, Mar 09, 2018 at 07:19:33AM +0100, Jernej Škrabec wrote:
> Hi,
> 
> Dne petek, 09. marec 2018 ob 01:44:55 CET je Ondřej Jirman napisal(a):
> > Hi,
> > 
> > I've debugged this further and it seems that the code has incorrect
> > assumptions. See docs for mode_set_nofb.
> > 
> > https://elixir.bootlin.com/linux/v4.16-rc4/source/include/drm/drm_modeset_he
> > lper_vtables.h#L209
> >
> 
> Does this happen with https://github.com/jernejsk/linux-1/tree/h3_hdmi_v3 ?

Haven't tested, but it probably does, looking at the code. Try
restarting the X server a bunch of times.

> I also tested running X, but I don't remember having the issue.
>  
> > I've added traces to a few functions that call clk_.*exclusive.*()
> > functions, and I see this after a few restarts of Xorg server:
> > 
> > [    0.783842] *** sun4i_tcon1_mode_set()
> > [    0.783886] *** sun4i_tcon_channel_set_status(..., 1, true)
> > [    6.881690] *** sun4i_tcon_channel_set_status(..., 1, false)
> > [    6.881758] *** sun4i_tcon_channel_set_status(..., 1, true)
> > [   52.335085] *** sun4i_tcon_channel_set_status(..., 1, false)
> > [   52.335343] *** sun4i_tcon_channel_set_status(..., 1, true)
> > [   68.921079] *** sun4i_tcon_channel_set_status(..., 1, false)
> > [   68.921337] *** sun4i_tcon_channel_set_status(..., 1, true)
> > 
> > mode_set_nofb is called just once, yet sun4i_tcon_channel_set_status(..., 1,
> > false) is called multiple times, which leads to unbalanced calls to
> > clk_set_rate_exclusive and clk_rate_exclusive_put.
> > 
> > I don't know how to fix this.
> 
> Since there is no function to check if clock is protected, flag can be 
> introduced within TCON which is set when clock rate is protected and unset 
> when it is unprotected. That way we could track if clk_rate_exclusive_put() 
> needs to be called or not.

I think that will not help. Protection is set in sun4i_tcon1_mode_set()
and that's called only once, but sun4i_tcon_channel_set_status(..., 1, false)
can be called multiple times, so the first call would disable protection
and the clock would be unprotected from then on, even though the display
would be active.

Perhaps the protection needs to be enabled in sun4i_tcon_channel_set_status(...,
true).

regards,
  o.

> Best regards,
> Jernej
> 
> > 
> > regards,
> >   o.
> > 
> > On Fri, Mar 09, 2018 at 01:13:14AM +0100, 'Ondřej Jirman' via linux-sunxi 
> wrote:
> > > Hi Jernej,
> > > 
> > > On Thu, Mar 08, 2018 at 11:57:40PM +0100, Jernej Škrabec wrote:
> > > > Hi,
> > > > 
> > > > Dne četrtek, 08. marec 2018 ob 23:47:17 CET je Ondřej Jirman napisal(a):
> > > > > Hi,
> > > > > 
> > > > > On Thu, Mar 01, 2018 at 10:34:32PM +0100, Jernej Skrabec wrote:
> > > > > > Currently exclusive TCON clock lock is never released, which, for
> > > > > > example, prevents changing resolution on HDMI.
> > > > > > 
> > > > > > In order to fix that, release clock when disabling TCON. TCON is
> > > > > > always
> > > > > > disabled first before new mode is set.
> > > > > > 
> > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 6 ++++--
> > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c index
> > > > > > 1d714c06ec9d..7f6c4125c89f
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > > @@ -102,10 +102,12 @@ static void
> > > > > > sun4i_tcon_channel_set_status(struct
> > > > > > sun4i_tcon *tcon, int channel,>
> > > > > > 
> > > > > >  		return;
> > > > > >  	
> > > > > >  	}
> > > > > > 
> > > > > > -	if (enabled)
> > > > > > +	if (enabled) {
> > > > > > 
> > > > > >  		clk_prepare_enable(clk);
> > > > > > 
> > > > > > -	else
> > > > > > +	} else {
> > > > > > +		clk_rate_exclusive_put(clk);
> > > > > > 
> > > > > >  		clk_disable_unprepare(clk);
> > > > > > 
> > > > > > +	}
> > > > > > 
> > > > > >  }
> > > > > 
> > > > > At least in the linux-next/master I can't see clk_rate_exclusive_get 
> call:
> > > > It is in drm-misc/drm-misc-fixes:
> > > > https://cgit.freedesktop.org/drm/drm-misc/commit/?h=drm-misc-fixes&id=79
> > > > d103a565d16b1893d990b2ee5e0fe71767759f
> > > > 
> > > > My patch is also applied in same branch, so there should be no issues
> > > > while
> > > > merging all those branches.
> > > > 
> > > > linux-next doesn't have either patches currently:
> > > > https://git.kernel.org/pub/
> > > > scm/linux/kernel/git/next/linux-next.git/log/drivers/gpu/drm/sun4i
> > > > 
> > > > This is issue only if you manually apply one patch without the other.
> > > 
> > > I have (and had) both patches applied. There's perhaps some code path,
> > > where sun4i_tcon_channel_set_status(..., false) is called in unbalanced
> > > way with regards to clk_set_rate_exclusive().
> > > 
> > > The issue occurs when starting Xorg. But Xorg works fine.
> > > 
> > > regards,
> > > 
> > >   o.
> > >   
> > > > Best regards,
> > > > Jernej
> > > > 
> > > > > $ git grep 'exclusive' linux-next/master -- drivers/gpu/drm/sun4i
> > > > > 
> > > > > linux-next/master:sun4i_hdmi.h:    * On sun5i the threshold is
> > > > > exclusive,
> > > > > i.e. does not include, linux-next/master:sun4i_tcon.c:
> > > > > clk_rate_exclusive_put(clk); linux-next/master:sun4i_tcon.c:
> > > > > clk_set_rate_exclusive(tcon->dclk, mode->crtc_clock * 1000);
> > > > > linux-next/master:sun4i_tcon.c:   clk_set_rate_exclusive(tcon->sclk1,
> > > > > mode->crtc_clock * 1000);
> > > > > 
> > > > > and the kernel complains too:
> > > > > 
> > > > > [  841.915161] ------------[ cut here ]------------
> > > > > [  841.915187] WARNING: CPU: 0 PID: 273 at
> > > > > /workspace/megous.com/orangepi-pc/linux-4.16/drivers/clk/clk.c:608
> > > > > clk_rate_exclusive_put+0x48/0x4c [  841.915194] CPU: 0 PID: 273 Comm:
> > > > > Xorg
> > > > > Not tainted 4.16.0-rc4-00268-gbac2ecf73ed0 #13 [  841.915196] Hardware
> > > > > name: Allwinner sun8i Family
> > > > > [  841.915217] [<c0228440>] (unwind_backtrace) from [<c0225b90>]
> > > > > (show_stack+0x10/0x14) [  841.915228] [<c0225b90>] (show_stack) from
> > > > > [<c0b094bc>] (dump_stack+0x88/0x9c) [  841.915237] [<c0b094bc>]
> > > > > (dump_stack) from [<c0240294>] (__warn+0xd4/0xf0) [  841.915244]
> > > > > [<c0240294>] (__warn) from [<c0240380>] (warn_slowpath_null+0x40/0x48)
> > > > > [
> > > > > 841.915250] [<c0240380>] (warn_slowpath_null) from [<c0619b6c>]
> > > > > (clk_rate_exclusive_put+0x48/0x4c) [  841.915261] [<c0619b6c>]
> > > > > (clk_rate_exclusive_put) from [<c0694624>]
> > > > > (sun4i_tcon_set_status+0x178/0x2f0) [  841.915269] [<c0694624>]
> > > > > (sun4i_tcon_set_status) from [<c06930c0>]
> > > > > (sun4i_crtc_atomic_disable+0x7c/0xe4) [  841.915279] [<c06930c0>]
> > > > > (sun4i_crtc_atomic_disable) from [<c06637fc>]
> > > > > (drm_atomic_helper_commit_modeset_disables+0x390/0x46c) [  841.915288]
> > > > > [<c06637fc>] (drm_atomic_helper_commit_modeset_disables) from
> > > > > [<c0664df4>]
> > > > > (drm_atomic_helper_commit_tail_rpm+0x18/0x64) [  841.915295]
> > > > > [<c0664df4>]
> > > > > (drm_atomic_helper_commit_tail_rpm) from [<c0664da8>]
> > > > > (commit_tail+0x40/0x6c) [  841.915302] [<c0664da8>] (commit_tail) from
> > > > > [<c06652b0>] (drm_atomic_helper_commit+0xbc/0x128) [  841.915311]
> > > > > [<c06652b0>] (drm_atomic_helper_commit) from [<c0668d40>]
> > > > > (restore_fbdev_mode_atomic+0x100/0x1fc) [  841.915319] [<c0668d40>]
> > > > > (restore_fbdev_mode_atomic) from [<c0668f1c>]
> > > > > (drm_fb_helper_dpms+0x50/0x130) [  841.915328] [<c0668f1c>]
> > > > > (drm_fb_helper_dpms) from [<c0669e9c>] (drm_fb_helper_blank+0x54/0x90)
> > > > > [
> > > > > 841.915337] [<c0669e9c>] (drm_fb_helper_blank) from [<c06088cc>]
> > > > > (fb_blank+0x54/0xa8) [  841.915343] [<c06088cc>] (fb_blank) from
> > > > > [<c0608c80>] (do_fb_ioctl+0x360/0x62c) [  841.915351] [<c0608c80>]
> > > > > (do_fb_ioctl) from [<c0362648>] (do_vfs_ioctl+0x9c/0x774) [ 
> > > > > 841.915358]
> > > > > [<c0362648>] (do_vfs_ioctl) from [<c0362d54>] (SyS_ioctl+0x34/0x58) [
> > > > > 841.915364] [<c0362d54>] (SyS_ioctl) from [<c0201120>]
> > > > > (ret_fast_syscall+0x0/0x4c) [  841.915368] Exception stack(0xe5fc5fa8
> > > > > to
> > > > > 0xe5fc5ff0)
> > > > > [  841.915373] 5fa0:                   00674f10 00675460 0000000b
> > > > > 00004611
> > > > > 00000001 00000000 [  841.915379] 5fc0: 00674f10 00675460 00000001
> > > > > 00000036
> > > > > 00650474 00000000 006504a4 00675df8 [  841.915382] 5fe0: b61a502c
> > > > > be8acb2c
> > > > > b6192c38 b6b152ec
> > > > > [  841.915386] ---[ end trace fa81b956197707f8 ]---
> > > > > 
> > > > > It looks like clk_rate_exclusive_put inside
> > > > > sun4i_tcon_channel_set_status is called without first calling some
> > > > > function that would call
> > > > > clk_set_rate_exclusive, leading to unbalanced get/put calls.
> > > > > 
> > > > > regards,
> > > > > 
> > > > >   o.
> > > > >   
> > > > > >  static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
> > > > > > 
> > > > > > --
> > > > > > 2.16.2
> > > > > > 
> > > > > > --
> > > > > > You received this message because you are subscribed to the Google
> > > > > > Groups
> > > > > > "linux-sunxi" group. To unsubscribe from this group and stop
> > > > > > receiving
> > > > > > emails from it, send an email to
> > > > > > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit
> > > > > > https://groups.google.com/d/optout.
> > > > 
> > > > --
> > > > You received this message because you are subscribed to the Google
> > > > Groups "linux-sunxi" group. To unsubscribe from this group and stop
> > > > receiving emails from it, send an email to
> > > > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit
> > > > https://groups.google.com/d/optout.
> > > 
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "linux-sunxi" group. To unsubscribe from this group and stop receiving
> > > emails from it, send an email to
> > > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit
> > > https://groups.google.com/d/optout.
> 
> 
> 
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html