| Message ID | 1491823014-1780-1-git-send-email-jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jyri, Thank you for the patch. On Monday 10 Apr 2017 14:16:54 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 do not like the above patch duplicating the already existing code > for CRTC disable. Another issue is making the check every time the > CRTC is turned on. > > 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); Doesn't all this belong to the CRTC's ->reset() operation, whose role is to create an initial software state that corresponds to the hardware state ? > + tilcdc_crtc_off(crtc, false); > + } > + pm_runtime_put_sync(dev->dev); > +} Is there a way to reset the LCDC completely ? Sure, stopping the CRTC is one step towards reconciling the hardware and software states, but I'm concerned that the driver may hardcode other assumptions regarding the hardware state that don't correspond to what the boot loader may have programmed. > 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);
On 04/11/17 10:01, Laurent Pinchart wrote: > Hi Jyri, > > Thank you for the patch. > > On Monday 10 Apr 2017 14:16:54 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 do not like the above patch duplicating the already existing code >> for CRTC disable. Another issue is making the check every time the >> CRTC is turned on. >> >> 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); > > Doesn't all this belong to the CRTC's ->reset() operation, whose role is to > create an initial software state that corresponds to the hardware state ? > Oh, it crossed my mind that there may be a proper call back for this purpose, but I forgot it while implementing this. I'll check if the reset callback suits the purpose. >> + tilcdc_crtc_off(crtc, false); >> + } >> + pm_runtime_put_sync(dev->dev); >> +} > > Is there a way to reset the LCDC completely ? Sure, stopping the CRTC is one > step towards reconciling the hardware and software states, but I'm concerned > that the driver may hardcode other assumptions regarding the hardware state > that don't correspond to what the boot loader may have programmed. > If there is, I have not figured that out yet. The main reset bit toggle does not reset do a full reset but, for instance leaves raster on, and probably pretty much all other registers intact too. The driver currently toggles the main reset in CRTC enable, before setting the mode, which in turn overwrites pretty much every LCDC register (there are not many). Unfortunately that does not appear to be enough if the raster was on before the CRTC enable. >> 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); >
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
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);
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 do not like the above patch duplicating the already existing code for CRTC disable. Another issue is making the check every time the CRTC is turned on. 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(+)