From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 22682C433E1 for ; Thu, 2 Jul 2020 11:29:04 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D8608208D5 for ; Thu, 2 Jul 2020 11:29:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="VqwTVT+a"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="islN+/5c" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D8608208D5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Fmv58FjAzbBcFgLmUxiKgmhh8T3Cz/Oxk7sGEKGYKLA=; b=VqwTVT+a7QVwvRcuS61lxMwDB kxLcMKfpuFinGbXFDi7A4r88zOWoVnmz+c9oySfBQIUyG4CkPbKN5+qiBtyWnW57xJO9USOpLKcMZ uPXgFF9bua7K2eUIAkgJ5AzYmuhvPFnDBWGtsOzRs7YBC7Guy0kAxhodvPJcc+da9sjZBmS1wFeOQ z+qfpNXNWkZ7aef3/orXOhq+gx5W07TEdrSNos6D6JUl9GHD5cQ4NVEdxgAypkxlN/LSlR5FiluEf 5Q+WQctPpJn5aASZ9Ggd2MVeueRV4/hRgRjmMKp8Y1CchAXycxl38dseu0A5mMKCZAKEcoPgB1t8v nmm8FMidQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jqxNR-0003gZ-Q9; Thu, 02 Jul 2020 11:27:37 +0000 Received: from perceval.ideasonboard.com ([2001:4b98:dc2:55:216:3eff:fef7:d647]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jqxNL-0003dX-Qv for linux-arm-kernel@lists.infradead.org; Thu, 02 Jul 2020 11:27:36 +0000 Received: from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id D83C0293; Thu, 2 Jul 2020 13:27:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1593689246; bh=g2lIxs47leX/mB93KVA/O9epkrEmj+JszqtCJsZa/So=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=islN+/5cMLvVnzCc4kC4QAjJbrsq/zoS48zAAlX0aQnBoJkLKnsj7mdxBSiXhQ8MS j9l89bUq1T8pucDGMXgX+ezRDxq254XTcVKDcLWtK6b5ZmxwOOofJBkEMw9TOZHQw0 uWs7BaPw6S9r6ZA6G+tem0Xl73vZGpA0X72tipwM= Date: Thu, 2 Jul 2020 14:27:22 +0300 From: Laurent Pinchart To: Daniel Vetter Subject: Re: [PATCH 1/8] drm/atomic-helper: reset vblank on crtc reset Message-ID: <20200702112722.GA18036@pendragon.ideasonboard.com> References: <20200612160056.2082681-1-daniel.vetter@ffwll.ch> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200612160056.2082681-1-daniel.vetter@ffwll.ch> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200702_072732_210355_E8999522 X-CRM114-Status: GOOD ( 41.09 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alexandre Belloni , Tetsuo Handa , Liviu Dudau , DRI Development , Thierry Reding , Daniel Vetter , Mihail Atanassov , Sam Ravnborg , Emil Velikov , linux-renesas-soc@vger.kernel.org, Jonathan Hunter , David Airlie , Ludovic Desroches , Tomi Valkeinen , "James \(Qian\) Wang" , Thierry Reding , syzbot+0871b14ca2e2fb64f6e3@syzkaller.appspotmail.com, Thomas Zimmermann , Intel Graphics Development , Maarten Lankhorst , Maxime Ripard , Sean Paul , Jyri Sarha , linux-tegra@vger.kernel.org, Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Boris Brezillon , Kieran Bingham , zhengbin , Rob Clark , Boris Brezillon , Daniel Vetter , Brian Masney , Brian Starkey Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Daniel, Thank you for the patch. On Fri, Jun 12, 2020 at 06:00:49PM +0200, Daniel Vetter wrote: > Only when vblanks are supported ofc. > > Some drivers do this already, but most unfortunately missed it. This > opens up bugs after driver load, before the crtc is enabled for the > first time. syzbot spotted this when loading vkms as a secondary > output. Given how many drivers are buggy it's best to solve this once > and for all in shared helper code. > > Aside from moving the few existing calls to drm_crtc_vblank_reset into > helpers (i915 doesn't use helpers, so keeps its own) I think the > regression risk is minimal: atomic helpers already rely on drivers > calling drm_crtc_vblank_on/off correctly in their hooks when they > support vblanks. And driver that's failing to handle vblanks after > this is missing those calls already, and vblanks could only work by > accident when enabling a CRTC for the first time right after boot. > > Big thanks to Tetsuo for helping track down what's going wrong here. > > There's only a few drivers which already had the necessary call and > needed some updating: > - komeda, atmel and tidss also needed to be changed to call > __drm_atomic_helper_crtc_reset() intead of open coding it > - tegra and msm even had it in the same place already, just code > motion, and malidp already uses __drm_atomic_helper_crtc_reset(). Should you mention rcar-du and omapdrm here ? > Only call left is in i915, which doesn't use drm_mode_config_reset, > but has its own fastboot infrastructure. So that's the only case where > we actually want this in the driver still. > > I've also reviewed all other drivers which set up vblank support with > drm_vblank_init. After the previous patch fixing mxsfb all atomic > drivers do call drm_crtc_vblank_on/off as they should, the remaining > drivers are either legacy kms or legacy dri1 drivers, so not affected > by this change to atomic helpers. > > v2: Use the drm_dev_has_vblank() helper. > > v3: Laurent pointed out that omap and rcar-du used drm_crtc_vblank_off > instead of drm_crtc_vblank_reset. Adjust them too. > > v4: Laurent noticed that rcar-du and omap open-code their crtc reset > and hence would actually be broken by this patch now. So fix them up > by reusing the helpers, which brings the drm_crtc_vblank_reset() back. > > Cc: Laurent Pinchart > Reviewed-by: Boris Brezillon > Acked-by: Liviu Dudau > Acked-by: Thierry Reding > Link: https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb > Reported-by: Tetsuo Handa > Reported-by: syzbot+0871b14ca2e2fb64f6e3@syzkaller.appspotmail.com > Cc: Tetsuo Handa > Cc: "James (Qian) Wang" > Cc: Liviu Dudau > Cc: Mihail Atanassov > Cc: Brian Starkey > Cc: Sam Ravnborg > Cc: Boris Brezillon > Cc: Nicolas Ferre > Cc: Alexandre Belloni > Cc: Ludovic Desroches > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: Jyri Sarha > Cc: Tomi Valkeinen > Cc: Rob Clark > Cc: Sean Paul > Cc: Brian Masney > Cc: Emil Velikov > Cc: zhengbin > Cc: Thomas Gleixner > Cc: linux-tegra@vger.kernel.org > Cc: Kieran Bingham > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-renesas-soc@vger.kernel.org > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++----- > drivers/gpu/drm/arm/malidp_drv.c | 1 - > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++----- > drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++ > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 2 -- > drivers/gpu/drm/omapdrm/omap_crtc.c | 8 +++++--- > drivers/gpu/drm/omapdrm/omap_drv.c | 4 ---- > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 6 +----- > drivers/gpu/drm/tegra/dc.c | 1 - > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +-- > drivers/gpu/drm/tidss/tidss_kms.c | 4 ---- > 11 files changed, 15 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c > index 56bd938961ee..f33418d6e1a0 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c > @@ -492,10 +492,8 @@ static void komeda_crtc_reset(struct drm_crtc *crtc) > crtc->state = NULL; > > state = kzalloc(sizeof(*state), GFP_KERNEL); > - if (state) { > - crtc->state = &state->base; > - crtc->state->crtc = crtc; > - } > + if (state) > + __drm_atomic_helper_crtc_reset(crtc, &state->base); > } > > static struct drm_crtc_state * > @@ -616,7 +614,6 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms, > return err; > > drm_crtc_helper_add(crtc, &komeda_crtc_helper_funcs); > - drm_crtc_vblank_reset(crtc); > > crtc->port = kcrtc->master->of_output_port; > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c > index 6feda7cb37a6..c9e1ee84b4e8 100644 > --- a/drivers/gpu/drm/arm/malidp_drv.c > +++ b/drivers/gpu/drm/arm/malidp_drv.c > @@ -861,7 +861,6 @@ static int malidp_bind(struct device *dev) > drm->irq_enabled = true; > > ret = drm_vblank_init(drm, drm->mode_config.num_crtc); > - drm_crtc_vblank_reset(&malidp->crtc); > if (ret < 0) { > DRM_ERROR("failed to initialise vblank\n"); > goto vblank_fail; > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 10985134ce0b..ce246b96330b 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -411,10 +411,8 @@ static void atmel_hlcdc_crtc_reset(struct drm_crtc *crtc) > } > > state = kzalloc(sizeof(*state), GFP_KERNEL); > - if (state) { > - crtc->state = &state->base; > - crtc->state->crtc = crtc; > - } > + if (state) > + __drm_atomic_helper_crtc_reset(crtc, &state->base); > } > > static struct drm_crtc_state * > @@ -528,7 +526,6 @@ int atmel_hlcdc_crtc_create(struct drm_device *dev) > } > > drm_crtc_helper_add(&crtc->base, &lcdc_crtc_helper_funcs); > - drm_crtc_vblank_reset(&crtc->base); > > drm_mode_crtc_set_gamma_size(&crtc->base, ATMEL_HLCDC_CLUT_SIZE); > drm_crtc_enable_color_mgmt(&crtc->base, 0, false, > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > index 8fce6a115dfe..9ad74045158e 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -32,6 +32,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -93,6 +94,9 @@ __drm_atomic_helper_crtc_reset(struct drm_crtc *crtc, > if (crtc_state) > __drm_atomic_helper_crtc_state_reset(crtc_state, crtc); > > + if (drm_dev_has_vblank(crtc->dev)) > + drm_crtc_vblank_reset(crtc); > + > crtc->state = crtc_state; > } > EXPORT_SYMBOL(__drm_atomic_helper_crtc_reset); > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > index e152016a6a7d..c39dad151bb6 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > @@ -1117,8 +1117,6 @@ static void mdp5_crtc_reset(struct drm_crtc *crtc) > mdp5_crtc_destroy_state(crtc, crtc->state); > > __drm_atomic_helper_crtc_reset(crtc, &mdp5_cstate->base); > - > - drm_crtc_vblank_reset(crtc); > } > > static const struct drm_crtc_funcs mdp5_crtc_funcs = { > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c > index fce7e944a280..6d40914675da 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -697,14 +697,16 @@ static int omap_crtc_atomic_get_property(struct drm_crtc *crtc, > > static void omap_crtc_reset(struct drm_crtc *crtc) > { > + struct omap_crtc_state *state; > + > if (crtc->state) > __drm_atomic_helper_crtc_destroy_state(crtc->state); > > kfree(crtc->state); > - crtc->state = kzalloc(sizeof(struct omap_crtc_state), GFP_KERNEL); > > - if (crtc->state) > - crtc->state->crtc = crtc; > + state = kzalloc(sizeof(*state), GFP_KERNEL); > + if (state) > + __drm_atomic_helper_crtc_reset(crtc, &state->base); > } > > static struct drm_crtc_state * > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c > index 242d28281784..4526967978b7 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -595,7 +595,6 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) > { > const struct soc_device_attribute *soc; > struct drm_device *ddev; > - unsigned int i; > int ret; > > DBG("%s", dev_name(dev)); > @@ -642,9 +641,6 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) > goto err_cleanup_modeset; > } > > - for (i = 0; i < priv->num_pipes; i++) > - drm_crtc_vblank_off(priv->pipes[i].crtc); > - > omap_fbdev_init(ddev); > > drm_kms_helper_poll_init(ddev); > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > index d73e88ddecd0..fe86a3e67757 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -975,8 +975,7 @@ static void rcar_du_crtc_reset(struct drm_crtc *crtc) > state->crc.source = VSP1_DU_CRC_NONE; > state->crc.index = 0; > > - crtc->state = &state->state; > - crtc->state->crtc = crtc; > + __drm_atomic_helper_crtc_reset(crtc, &state->state); > } > > static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc) > @@ -1271,9 +1270,6 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex, > > drm_crtc_helper_add(crtc, &crtc_helper_funcs); > > - /* Start with vertical blanking interrupt reporting disabled. */ > - drm_crtc_vblank_off(crtc); > - Could this cause an issue, as the interrupt handler can now be registered with the interrupt left enabled in the hardware after a reboot, while drm_crtc_vblank_off() would disable it ? It's something that should likely be handled elsewhere in the driver, with all interrupts disabled explicitly early in probe, and I don't think the driver handles enabled interrupts very well today, so it's not a blocker for me: Reviewed-by: Laurent Pinchart I would however appreciate your thoughts on this topic, to know if my understanding is correct (and if this issue could affect other drivers). > /* Register the interrupt handler. */ > if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CRTC_IRQ_CLOCK)) { > /* The IRQ's are associated with the CRTC (sw)index. */ > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > index 83f31c6e891c..9b308b572eac 100644 > --- a/drivers/gpu/drm/tegra/dc.c > +++ b/drivers/gpu/drm/tegra/dc.c > @@ -1168,7 +1168,6 @@ static void tegra_crtc_reset(struct drm_crtc *crtc) > tegra_crtc_atomic_destroy_state(crtc, crtc->state); > > __drm_atomic_helper_crtc_reset(crtc, &state->base); > - drm_crtc_vblank_reset(crtc); > } > > static struct drm_crtc_state * > diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c > index 89a226912de8..4d01c4af61cd 100644 > --- a/drivers/gpu/drm/tidss/tidss_crtc.c > +++ b/drivers/gpu/drm/tidss/tidss_crtc.c > @@ -352,8 +352,7 @@ static void tidss_crtc_reset(struct drm_crtc *crtc) > return; > } > > - crtc->state = &tcrtc->base; > - crtc->state->crtc = crtc; > + __drm_atomic_helper_crtc_reset(crtc, &tcrtc->base); > } > > static struct drm_crtc_state *tidss_crtc_duplicate_state(struct drm_crtc *crtc) > diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c > index 4b99e9fa84a5..e6ab59eed259 100644 > --- a/drivers/gpu/drm/tidss/tidss_kms.c > +++ b/drivers/gpu/drm/tidss/tidss_kms.c > @@ -278,10 +278,6 @@ int tidss_modeset_init(struct tidss_device *tidss) > if (ret) > return ret; > > - /* Start with vertical blanking interrupt reporting disabled. */ > - for (i = 0; i < tidss->num_crtcs; ++i) > - drm_crtc_vblank_reset(tidss->crtcs[i]); > - > drm_mode_config_reset(ddev); > > dev_dbg(tidss->dev, "%s done\n", __func__); -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel