diff mbox series

drm/mcde: Fix stability issue

Message ID 20200718233323.3407670-1-linus.walleij@linaro.org (mailing list archive)
State Mainlined
Commit aa7bf898d4bf921f61fab078040e8baec3f28126
Headers show
Series drm/mcde: Fix stability issue | expand

Commit Message

Linus Walleij July 18, 2020, 11:33 p.m. UTC
Whenener a display update was sent, apart from updating
the memory base address we called mcde_display_send_one_frame()
which also sent a command to the display requesting the TE IRQ
and enabling the FIFO.

When continous updates are running this is wrong: we need
to only send this to start the flow to the display on
the very first update. This lead to the display pipeline
locking up and crashing.

Check if the flow is already running and in that case
do not call mcde_display_send_one_frame().

This fixes crashes on the Samsung GT-S7710 (Skomer).

Cc: Stephan Gerhold <stephan@gerhold.net>
Cc: stable@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/mcde/mcde_display.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Sasha Levin July 21, 2020, 2:20 p.m. UTC | #1
Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.7.9, v5.4.52, v4.19.133, v4.14.188, v4.9.230, v4.4.230.

v5.4.52: Failed to apply! Possible dependencies:
    768859c239922 ("drm/mcde: Provide vblank handling unconditionally")
    d920e8da3d837 ("drm/mcde: Fix frame sync setup for video mode panels")

v4.19.133: Failed to apply! Possible dependencies:
    01648890f336a ("staging: vboxvideo: Embed drm_device into driver structure")
    0424d7ba4574b ("staging: vboxvideo: Init fb_info.fix.smem once from fbdev_create")
    0fdda2ce74e5f ("staging: vboxvideo: Move pin / unpin of fb out of vbox_crtc_set_base_and_mode")
    131abc56e1bac ("drm/vboxvideo: Move the vboxvideo driver out of staging")
    3498ea8b7e3c8 ("staging: vboxvideo: Fold vbox_drm_resume() into vbox_pm_resume()")
    35f3288c453e2 ("staging: vboxvideo: Atomic phase 1: convert cursor to universal plane")
    438340aa20975 ("staging: vboxvideo: Atomic phase 3: Switch last bits over to atomic")
    4f2a8f5898ecd ("drm: Add ASPEED GFX driver")
    5cf5332d529bf ("staging: vboxvideo: Restore page-flip support")
    5fc537bfd0003 ("drm/mcde: Add new driver for ST-Ericsson MCDE")
    685bb884e0a42 ("staging: vboxvideo: Drop duplicate vbox_err.h file")
    79815ee23890c ("staging: vboxvideo: Move setup of modesetting from driver_load to mode_init")
    96bae04347b24 ("staging/vboxvideo: prepare for drmP.h removal from drm_modeset_helper.h")
    a1d2a6339961e ("drm/lima: driver for ARM Mali4xx GPUs")
    a5aca20574693 ("staging: vboxvideo: Fix modeset / page_flip error handling")
    acc962c514007 ("staging: vboxvideo: Change licence headers over to SPDX")
    cb5eaf187d1d9 ("staging: vboxvideo: Expose creation of universal primary plane")
    cc0ec5eb721f1 ("staging: vboxvideo: Atomic phase 1: Use drm_plane_helpers for primary plane")
    cd76c287a52fe ("staging: vboxvideo: Cleanup the comments")
    ce8ec32cbd420 ("staging: vboxvideo: Remove vboxfb_create_object() wrapper")
    cfc1fc63be447 ("staging: vboxvideo: Move bo_[un]resere calls into vbox_bo_[un]pin")
    d46709094deb6 ("staging: vboxvideo: Fold driver_load/unload into probe/remove functions")
    f4d6d90f83147 ("staging: vboxvideo: Add fl_flag argument to vbox_fb_pin() helper")

v4.14.188: Failed to apply! Possible dependencies:
    01648890f336a ("staging: vboxvideo: Embed drm_device into driver structure")
    0424d7ba4574b ("staging: vboxvideo: Init fb_info.fix.smem once from fbdev_create")
    0fdda2ce74e5f ("staging: vboxvideo: Move pin / unpin of fb out of vbox_crtc_set_base_and_mode")
    131abc56e1bac ("drm/vboxvideo: Move the vboxvideo driver out of staging")
    179c02fe90a41 ("drm/tve200: Add new driver for TVE200")
    1daddbc8dec56 ("staging: vboxvideo: Update driver to use drm_dev_register.")
    1ebafd1561a05 ("staging: vboxvideo: Fix IRQs no longer working")
    2408898e3b6c9 ("staging: vboxvideo: Add page-flip support")
    3498ea8b7e3c8 ("staging: vboxvideo: Fold vbox_drm_resume() into vbox_pm_resume()")
    35f3288c453e2 ("staging: vboxvideo: Atomic phase 1: convert cursor to universal plane")
    438340aa20975 ("staging: vboxvideo: Atomic phase 3: Switch last bits over to atomic")
    4f2a8f5898ecd ("drm: Add ASPEED GFX driver")
    5b8ea816e8e05 ("drm/tinydrm: add driver for ST7735R panels")
    5cf5332d529bf ("staging: vboxvideo: Restore page-flip support")
    5fc537bfd0003 ("drm/mcde: Add new driver for ST-Ericsson MCDE")
    65aac17423284 ("staging: vboxvideo: Change address of scanout buffer on page-flip")
    685bb884e0a42 ("staging: vboxvideo: Drop duplicate vbox_err.h file")
    6d544fd6f4e15 ("drm/doc: Put all driver docs into a separate chapter")
    79815ee23890c ("staging: vboxvideo: Move setup of modesetting from driver_load to mode_init")
    96bae04347b24 ("staging/vboxvideo: prepare for drmP.h removal from drm_modeset_helper.h")
    a1d2a6339961e ("drm/lima: driver for ARM Mali4xx GPUs")
    a5aca20574693 ("staging: vboxvideo: Fix modeset / page_flip error handling")
    acc962c514007 ("staging: vboxvideo: Change licence headers over to SPDX")
    ba67f54d911c3 ("staging: vboxvideo: Pass a new framebuffer to vbox_crtc_do_set_base")
    c575b7eeb89f9 ("drm/xen-front: Add support for Xen PV display frontend")
    cb5eaf187d1d9 ("staging: vboxvideo: Expose creation of universal primary plane")
    cc0ec5eb721f1 ("staging: vboxvideo: Atomic phase 1: Use drm_plane_helpers for primary plane")
    cd76c287a52fe ("staging: vboxvideo: Cleanup the comments")
    ce8ec32cbd420 ("staging: vboxvideo: Remove vboxfb_create_object() wrapper")
    cfc1fc63be447 ("staging: vboxvideo: Move bo_[un]resere calls into vbox_bo_[un]pin")
    d46709094deb6 ("staging: vboxvideo: Fold driver_load/unload into probe/remove functions")
    f4d6d90f83147 ("staging: vboxvideo: Add fl_flag argument to vbox_fb_pin() helper")

v4.9.230: Failed to apply! Possible dependencies:
    0a886f59528aa ("drm: zte: add initial vou drm driver")
    179c02fe90a41 ("drm/tve200: Add new driver for TVE200")
    3650c25ad0a7b ("drm/meson: Add RST to bring together kerneldoc")
    384fe7a4d732c ("drivers: net: xgene-v2: Add DMA descriptor")
    3b3f9a75d9318 ("drivers: net: xgene-v2: Add base driver")
    413dfbbfca549 ("MAINTAINERS: add entry for Aspeed I2C driver")
    45d59d704080c ("drm: Add new driver for MXSFB controller")
    51c5d8447bd71 ("MMC: meson: initial support for GX platforms")
    5fc537bfd0003 ("drm/mcde: Add new driver for ST-Ericsson MCDE")
    60c5d3b72989e ("drm/vc4: Add RST to bring together vc4 kerneldoc.")
    6d544fd6f4e15 ("drm/doc: Put all driver docs into a separate chapter")
    70dbd9b258d5a ("MAINTAINERS: Add entry for APM X-Gene SoC Ethernet (v2) driver")
    7683e9e529258 ("Properly alphabetize MAINTAINERS file")
    81ccd0cab29b6 ("drivers: net: xgene-v2: Add mac configuration")
    b105bcdaaa0ef ("drivers: net: xgene-v2: Add transmit and receive")
    bbbe775ec5b5d ("drm: Add support for Amlogic Meson Graphic Controller")
    bed41005e6174 ("drm/pl111: Initial drm/kms driver for pl111")
    d52ea7e3d4938 ("MAINTAINERS: Add drm-misc")
    fa201ac2c61f5 ("drm: Add DRM support for tiny LCD displays")
    fa6d095eb23a8 ("drm/tegra: Add driver documentation")
    fd33f3eca6bfb ("MAINTAINERS: Add maintainers for the meson clock driver")

v4.4.230: Failed to apply! Possible dependencies:
    0a886f59528aa ("drm: zte: add initial vou drm driver")
    119f5173628aa ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
    22cba31bae9dc ("Documentation/sphinx: add basic working Sphinx configuration and build")
    23e7b2ab9a8ff ("drm/hisilicon: Add hisilicon kirin drm master driver")
    3650c25ad0a7b ("drm/meson: Add RST to bring together kerneldoc")
    45d59d704080c ("drm: Add new driver for MXSFB controller")
    51dacf208988e ("drm: Add support of ARC PGU display controller")
    5fc537bfd0003 ("drm/mcde: Add new driver for ST-Ericsson MCDE")
    6d544fd6f4e15 ("drm/doc: Put all driver docs into a separate chapter")
    8e22d79240d95 ("drm: Add support for ARM's HDLCD controller.")
    a8c21a5451d83 ("drm/etnaviv: add initial etnaviv DRM driver")
    bbbe775ec5b5d ("drm: Add support for Amlogic Meson Graphic Controller")
    bed41005e6174 ("drm/pl111: Initial drm/kms driver for pl111")
    ca00c2b986eaf ("Documentation/gpu: split up the gpu documentation")
    cb597fcea5c28 ("Documentation/gpu: add new gpu.rst converted from DocBook gpu.tmpl")
    d92d9c3a14488 ("drm: hide legacy drivers with CONFIG_DRM_LEGACY")
    fa201ac2c61f5 ("drm: Add DRM support for tiny LCD displays")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?
Sam Ravnborg July 26, 2020, 5:05 p.m. UTC | #2
Hi Linus.

On Sun, Jul 19, 2020 at 01:33:22AM +0200, Linus Walleij wrote:
> Whenener a display update was sent, apart from updating
s/Whenener/Whenever
> the memory base address we called mcde_display_send_one_frame()
                         ^ insert comma?

> which also sent a command to the display requesting the TE IRQ
> and enabling the FIFO.
> 
> When continous updates are running this is wrong: we need
> to only send this to start the flow to the display on
> the very first update. This lead to the display pipeline
> locking up and crashing.
> 
> Check if the flow is already running and in that case
> do not call mcde_display_send_one_frame().
> 
> This fixes crashes on the Samsung GT-S7710 (Skomer).
> 
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Patch looks fine.
Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/mcde/mcde_display.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
> index 212aee60cf61..1d8ea8830a17 100644
> --- a/drivers/gpu/drm/mcde/mcde_display.c
> +++ b/drivers/gpu/drm/mcde/mcde_display.c
> @@ -1086,9 +1086,14 @@ static void mcde_display_update(struct drm_simple_display_pipe *pipe,
>  	 */
>  	if (fb) {
>  		mcde_set_extsrc(mcde, drm_fb_cma_get_gem_addr(fb, pstate, 0));
> -		if (!mcde->video_mode)
> -			/* Send a single frame using software sync */
> -			mcde_display_send_one_frame(mcde);
> +		if (!mcde->video_mode) {
> +			/*
> +			 * Send a single frame using software sync if the flow
> +			 * is not active yet.
> +			 */
> +			if (mcde->flow_active == 0)
> +				mcde_display_send_one_frame(mcde);
> +		}
>  		dev_info_once(mcde->dev, "sent first display update\n");
>  	} else {
>  		/*
> -- 
> 2.26.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Stephan Gerhold July 26, 2020, 5:51 p.m. UTC | #3
On Sun, Jul 19, 2020 at 01:33:22AM +0200, Linus Walleij wrote:
> Whenener a display update was sent, apart from updating
> the memory base address we called mcde_display_send_one_frame()
> which also sent a command to the display requesting the TE IRQ
> and enabling the FIFO.
> 
> When continous updates are running this is wrong: we need
> to only send this to start the flow to the display on
> the very first update. This lead to the display pipeline
> locking up and crashing.
> 
> Check if the flow is already running and in that case
> do not call mcde_display_send_one_frame().
> 
> This fixes crashes on the Samsung GT-S7710 (Skomer).
> 
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/mcde/mcde_display.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
> index 212aee60cf61..1d8ea8830a17 100644
> --- a/drivers/gpu/drm/mcde/mcde_display.c
> +++ b/drivers/gpu/drm/mcde/mcde_display.c
> @@ -1086,9 +1086,14 @@ static void mcde_display_update(struct drm_simple_display_pipe *pipe,
>  	 */
>  	if (fb) {
>  		mcde_set_extsrc(mcde, drm_fb_cma_get_gem_addr(fb, pstate, 0));
> -		if (!mcde->video_mode)
> -			/* Send a single frame using software sync */
> -			mcde_display_send_one_frame(mcde);
> +		if (!mcde->video_mode) {
> +			/*
> +			 * Send a single frame using software sync if the flow
> +			 * is not active yet.
> +			 */
> +			if (mcde->flow_active == 0)
> +				mcde_display_send_one_frame(mcde);
> +		}

I think this makes sense as a fix for the issue you described, so FWIW:
Acked-by: Stephan Gerhold <stephan@gerhold.net>

While looking at this I had a few thoughts for potential future patches:

 - Clearly mcde_display_send_one_frame() does not only send a single
   frame only in some cases (when te_sync = true), so maybe it should
   be named differently?

 - I was a bit confused because with this change we also call
   mcde_dsi_te_request() only once. Looking at the vendor driver the
   nova_dsilink_te_request() function that is very similar is only
   called within mcde_add_bta_te_oneshot_listener(), which is only
   called for MCDE_SYNCSRC_BTA.

   However, the rest of the MCDE code looks more similar to
   MCDE_SYNCSRC_TE0, which does not call that function in the vendor
   driver. I wonder if mcde_dsi_te_request() is needed at all?

Thanks,
Stephan
diff mbox series

Patch

diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
index 212aee60cf61..1d8ea8830a17 100644
--- a/drivers/gpu/drm/mcde/mcde_display.c
+++ b/drivers/gpu/drm/mcde/mcde_display.c
@@ -1086,9 +1086,14 @@  static void mcde_display_update(struct drm_simple_display_pipe *pipe,
 	 */
 	if (fb) {
 		mcde_set_extsrc(mcde, drm_fb_cma_get_gem_addr(fb, pstate, 0));
-		if (!mcde->video_mode)
-			/* Send a single frame using software sync */
-			mcde_display_send_one_frame(mcde);
+		if (!mcde->video_mode) {
+			/*
+			 * Send a single frame using software sync if the flow
+			 * is not active yet.
+			 */
+			if (mcde->flow_active == 0)
+				mcde_display_send_one_frame(mcde);
+		}
 		dev_info_once(mcde->dev, "sent first display update\n");
 	} else {
 		/*