Re: [PATCH] drm/tilcdc: If CRTC is enabled at init phase, disable it

[Emiliano Ingrassia <ingrassia@epigenesys.com>]

Hi Jyri,

thank you for the response.

On Mon, Apr 10, 2017 at 02:16:54PM +0300, Jyri Sarha wrote:
> If the LCDC is already enabled (e.g. by the bootloader) at the
> initialization phase, disable it before returning from the probe.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
> ---
> This patch should fix the same issus as this patch does:
> https://lists.freedesktop.org/archives/dri-devel/2017-March/137091.html

I tried your patch but that didn't solve the problem.

Here is the backtrace:

[    0.793581] [drm] Initialized
[    0.796935] panel panel: found backlight
[    0.801628] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[    0.808277] [drm] No driver support for vblank timestamp query.
[    0.872845] tilcdc 4830e000.lcdc: tilcdc_crtc_load_palette: Palette loading timeout
[    0.928822] ------------[ cut here ]------------
[    0.928858] WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_atomic_helper.c:1140 drm_atomic_helper_wait_for_vblanks+0x177/0x17c
[    0.928861] [CRTC:24] vblank wait timed out
[    0.928868] Modules linked in:
[    0.928876] CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.21-ti-rt-r29 #1
[    0.928879] Hardware name: Generic AM33XX (Flattened Device Tree)
[    0.928918] [<c010a8d1>] (unwind_backtrace) from [<c0108acf>] (show_stack+0xb/0xc)
[    0.928933] [<c0108acf>] (show_stack) from [<c012424d>] (__warn+0xa9/0xbc)
[    0.928943] [<c012424d>] (__warn) from [<c0124281>] (warn_slowpath_fmt+0x21/0x2c)
[    0.928954] [<c0124281>] (warn_slowpath_fmt) from [<c0308a17>] (drm_atomic_helper_wait_for_vblanks+0x177/0x17c)
[    0.928974] [<c0308a17>] (drm_atomic_helper_wait_for_vblanks) from [<c0337e65>] (tilcdc_commit+0x39/0x4c)
[    0.928991] [<c0337e65>] (tilcdc_commit) from [<c030cd63>] (drm_fb_helper_restore_fbdev_mode_unlocked+0xff/0x1f4)
[    0.929000] [<c030cd63>] (drm_fb_helper_restore_fbdev_mode_unlocked) from [<c030cbb7>] (drm_fb_helper_set_par+0x1f/0x44)
[    0.929011] [<c030cbb7>] (drm_fb_helper_set_par) from [<c02be665>] (fbcon_init+0x409/0x444)
[    0.929026] [<c02be665>] (fbcon_init) from [<c02f0edf>] (visual_init+0x87/0xc8)
[    0.929037] [<c02f0edf>] (visual_init) from [<c02f24bd>] (do_bind_con_driver+0x10d/0x2a0)
[    0.929047] [<c02f24bd>] (do_bind_con_driver) from [<c02f28eb>] (do_take_over_console+0xeb/0x140)
[    0.929056] [<c02f28eb>] (do_take_over_console) from [<c02be6f9>] (do_fbcon_takeover+0x59/0xa0)
[    0.929066] [<c02be6f9>] (do_fbcon_takeover) from [<c0136b1d>] (notifier_call_chain+0x45/0x60)
[    0.929075] [<c0136b1d>] (notifier_call_chain) from [<c0136d0b>] (__blocking_notifier_call_chain+0x27/0x34)
[    0.929085] [<c0136d0b>] (__blocking_notifier_call_chain) from [<c0136d29>] (blocking_notifier_call_chain+0x11/0x14)
[    0.929098] [<c0136d29>] (blocking_notifier_call_chain) from [<c02c4ea5>] (register_framebuffer+0x195/0x228)
[    0.929108] [<c02c4ea5>] (register_framebuffer) from [<c030cfd5>] (drm_fb_helper_initial_config+0x17d/0x2bc)
[    0.929117] [<c030cfd5>] (drm_fb_helper_initial_config) from [<c030d607>] (drm_fbdev_cma_init_with_funcs+0x53/0xbc)
[    0.929125] [<c030d607>] (drm_fbdev_cma_init_with_funcs) from [<c030d683>] (drm_fbdev_cma_init+0x13/0x18)
[    0.929134] [<c030d683>] (drm_fbdev_cma_init) from [<c03381d9>] (tilcdc_init.constprop.2+0x339/0x4c4)
[    0.929143] [<c03381d9>] (tilcdc_init.constprop.2) from [<c0338397>] (tilcdc_pdev_probe+0x33/0x50)
[    0.929158] [<c0338397>] (tilcdc_pdev_probe) from [<c033dfa5>] (platform_drv_probe+0x31/0x74)
[    0.929168] [<c033dfa5>] (platform_drv_probe) from [<c033ce13>] (driver_probe_device+0xbf/0x308)
[    0.929176] [<c033ce13>] (driver_probe_device) from [<c033d0f7>] (__driver_attach+0x9b/0xb0)
[    0.929184] [<c033d0f7>] (__driver_attach) from [<c033b94b>] (bus_for_each_dev+0x3b/0x5c)
[    0.929192] [<c033b94b>] (bus_for_each_dev) from [<c033c6c1>] (bus_add_driver+0x155/0x1c4)
[    0.929200] [<c033c6c1>] (bus_add_driver) from [<c033d4d3>] (driver_register+0x33/0x84)
[    0.929209] [<c033d4d3>] (driver_register) from [<c01014f9>] (do_one_initcall+0x2d/0x118)
[    0.929225] [<c01014f9>] (do_one_initcall) from [<c0800b19>] (kernel_init_freeable+0x10d/0x184)
[    0.929239] [<c0800b19>] (kernel_init_freeable) from [<c0463f97>] (kernel_init+0x7/0xd8)
[    0.929250] [<c0463f97>] (kernel_init) from [<c0106705> (ret_from_fork+0x11/0x2c)
[    0.929254] ---[ end trace 0000000000000001 ]---
[    0.934018] Console: switching to colour frame buffer device 128x37
[    1.275779] tilcdc 4830e000.lcdc: fb0:  frame buffer device
[    1.792818] tilcdc 4830e000.lcdc: tilcdc_crtc_off: timeout waiting for framedone

This is the same problem that I was having without the patch I proposed:

https://lists.freedesktop.org/archives/dri-devel/2017-March/137091.html

which simply implements the complete software reset sequence for the LCD controller,
according to the AM335x SOC manual [SPRUH73O, sec. 13.4.6].

> 
> I do not like the above patch duplicating the already existing code
> for CRTC disable. Another issue is making the check every time the
> 
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 21 +++++++++++++++++++++
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  3 +++
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  1 +
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index afd2a7b..540378a 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -566,6 +566,26 @@ void tilcdc_crtc_shutdown(struct drm_crtc *crtc)
>  	tilcdc_crtc_off(crtc, true);
>  }
>  
> +void tilcdc_crtc_disable_init(struct drm_crtc *crtc)
> +{
> +	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> +	struct drm_device *dev = crtc->dev;
> +
> +	/*
> +	 * If the LCDC was already enabled (e.g. by the bootloader)
> +	 * disable the CRTC before finishing the probe.
> +	 */
> +	pm_runtime_get_sync(dev->dev);
> +	if (tilcdc_read(dev, LCDC_RASTER_CTRL_REG) & LCDC_RASTER_ENABLE) {
> +		tilcdc_crtc->enabled = true;
> +		tilcdc_crtc_enable_irqs(dev);
> +		/* To match pm_runtime_put_sync() in tilcdc_crtc_off() */
> +		pm_runtime_get_sync(dev->dev);
> +		tilcdc_crtc_off(crtc, false);
> +	}
> +	pm_runtime_put_sync(dev->dev);
> +}
> +
>  static bool tilcdc_crtc_is_on(struct drm_crtc *crtc)
>  {
>  	return crtc->state && crtc->state->enable && crtc->state->active;
> @@ -1054,6 +1074,7 @@ int tilcdc_crtc_create(struct drm_device *dev)
>  	}
>  
>  	priv->crtc = crtc;
> +
>  	return 0;
>  
>  fail:
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index d7ae5be..7dabe55 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -415,6 +415,9 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
>  		goto init_failed;
>  
>  	priv->is_registered = true;
> +
> +	tilcdc_crtc_disable_init(priv->crtc);
> +
>  	return 0;
>  
>  init_failed:
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index 8caa11b..adde1e4 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -179,6 +179,7 @@ void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
>  int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
>  		struct drm_framebuffer *fb,
>  		struct drm_pending_vblank_event *event);
> +void tilcdc_crtc_disable_init(struct drm_crtc *crtc);
>  
>  int tilcdc_plane_init(struct drm_device *dev, struct drm_plane *plane);
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Thank you,

Emiliano Ingrassia
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel