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