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().
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 laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@collabora.com Acked-by: Liviu Dudau liviu.dudau@arm.com Acked-by: Thierry Reding treding@nvidia.com Link: https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7c... Reported-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Reported-by: syzbot+0871b14ca2e2fb64f6e3@syzkaller.appspotmail.com Cc: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Cc: "James (Qian) Wang" james.qian.wang@arm.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Mihail Atanassov mihail.atanassov@arm.com Cc: Brian Starkey brian.starkey@arm.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Boris Brezillon bbrezillon@kernel.org Cc: Nicolas Ferre nicolas.ferre@microchip.com Cc: Alexandre Belloni alexandre.belloni@bootlin.com Cc: Ludovic Desroches ludovic.desroches@microchip.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Thierry Reding thierry.reding@gmail.com Cc: Jonathan Hunter jonathanh@nvidia.com Cc: Jyri Sarha jsarha@ti.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul seanpaul@chromium.org Cc: Brian Masney masneyb@onstation.org Cc: Emil Velikov emil.velikov@collabora.com Cc: zhengbin zhengbin13@huawei.com Cc: Thomas Gleixner tglx@linutronix.de Cc: linux-tegra@vger.kernel.org Cc: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-renesas-soc@vger.kernel.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- 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 <drm/drm_device.h> #include <drm/drm_plane.h> #include <drm/drm_print.h> +#include <drm/drm_vblank.h> #include <drm/drm_writeback.h>
#include <linux/slab.h> @@ -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); - /* 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__);
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Cc: Roman Li roman.li@amd.com Cc: Mikita Lipski mikita.lipski@amd.com Cc: Stylon Wang stylon.wang@amd.com --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 68a73065b516..36d605a6eb16 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4594,9 +4594,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc) if (WARN_ON(!state)) return;
- crtc->state = &state->base; - crtc->state->crtc = crtc; - + __drm_atomic_helper_crtc_reset(crtc, &state->base); }
static struct drm_crtc_state *
On 2020-06-12 12:00 p.m., Daniel Vetter wrote:
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Cc: Roman Li roman.li@amd.com Cc: Mikita Lipski mikita.lipski@amd.com Cc: Stylon Wang stylon.wang@amd.com
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 68a73065b516..36d605a6eb16 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4594,9 +4594,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc) if (WARN_ON(!state)) return;
- crtc->state = &state->base;
- crtc->state->crtc = crtc;
- __drm_atomic_helper_crtc_reset(crtc, &state->base);
}
static struct drm_crtc_state *
On Fri, Jun 12, 2020 at 1:24 PM Harry Wentland hwentlan@amd.com wrote:
On 2020-06-12 12:00 p.m., Daniel Vetter wrote:
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Cc: Roman Li roman.li@amd.com Cc: Mikita Lipski mikita.lipski@amd.com Cc: Stylon Wang stylon.wang@amd.com
Reviewed-by: Harry Wentland harry.wentland@amd.com
Daniel, do you want to take the whole series, or should I pull this in through my tree? Either way works for me. Thanks for the patch!
Alex
Harry
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 68a73065b516..36d605a6eb16 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4594,9 +4594,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc) if (WARN_ON(!state)) return;
crtc->state = &state->base;crtc->state->crtc = crtc;
__drm_atomic_helper_crtc_reset(crtc, &state->base);}
static struct drm_crtc_state *
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Jun 12, 2020 at 01:41:17PM -0400, Alex Deucher wrote:
On Fri, Jun 12, 2020 at 1:24 PM Harry Wentland hwentlan@amd.com wrote:
On 2020-06-12 12:00 p.m., Daniel Vetter wrote:
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Cc: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Cc: Roman Li roman.li@amd.com Cc: Mikita Lipski mikita.lipski@amd.com Cc: Stylon Wang stylon.wang@amd.com
Reviewed-by: Harry Wentland harry.wentland@amd.com
Daniel, do you want to take the whole series, or should I pull this in through my tree? Either way works for me. Thanks for the patch!
Merged the entire series through drm-misc-next now. -Daniel
Alex
Harry
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 68a73065b516..36d605a6eb16 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4594,9 +4594,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc) if (WARN_ON(!state)) return;
crtc->state = &state->base;crtc->state->crtc = crtc;
__drm_atomic_helper_crtc_reset(crtc, &state->base);}
static struct drm_crtc_state *
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Shawn Guo shawnguo@kernel.org Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Fabio Estevam festevam@gmail.com Cc: NXP Linux Team linux-imx@nxp.com Cc: linux-arm-kernel@lists.infradead.org --- drivers/gpu/drm/imx/ipuv3-crtc.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 63c0284f8b3c..02c2f848f2d1 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -109,20 +109,15 @@ static void imx_drm_crtc_reset(struct drm_crtc *crtc) { struct imx_crtc_state *state;
- if (crtc->state) { - if (crtc->state->mode_blob) - drm_property_blob_put(crtc->state->mode_blob); - - state = to_imx_crtc_state(crtc->state); - memset(state, 0, sizeof(*state)); - } else { - state = kzalloc(sizeof(*state), GFP_KERNEL); - if (!state) - return; - crtc->state = &state->base; - } + if (crtc->state) + __drm_atomic_helper_crtc_destroy_state(crtc->state);
- state->base.crtc = crtc; + kfree(to_imx_crtc_state(crtc->state)); + crtc->state = NULL; + + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (state) + __drm_atomic_helper_crtc_reset(crtc, &state->base); }
static struct drm_crtc_state *imx_drm_crtc_duplicate_state(struct drm_crtc *crtc)
On Fri, Jun 12, 2020 at 06:00:51PM +0200, Daniel Vetter wrote:
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Shawn Guo shawnguo@kernel.org Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Fabio Estevam festevam@gmail.com Cc: NXP Linux Team linux-imx@nxp.com Cc: linux-arm-kernel@lists.infradead.org
Ping for some ack/review on this pls.
Thanks, Daniel
drivers/gpu/drm/imx/ipuv3-crtc.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 63c0284f8b3c..02c2f848f2d1 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -109,20 +109,15 @@ static void imx_drm_crtc_reset(struct drm_crtc *crtc) { struct imx_crtc_state *state;
- if (crtc->state) {
if (crtc->state->mode_blob)drm_property_blob_put(crtc->state->mode_blob);state = to_imx_crtc_state(crtc->state);memset(state, 0, sizeof(*state));- } else {
state = kzalloc(sizeof(*state), GFP_KERNEL);if (!state)return;crtc->state = &state->base;- }
- if (crtc->state)
__drm_atomic_helper_crtc_destroy_state(crtc->state);
- state->base.crtc = crtc;
- kfree(to_imx_crtc_state(crtc->state));
- crtc->state = NULL;
- state = kzalloc(sizeof(*state), GFP_KERNEL);
- if (state)
__drm_atomic_helper_crtc_reset(crtc, &state->base);}
static struct drm_crtc_state *imx_drm_crtc_duplicate_state(struct drm_crtc *crtc)
2.26.2
On Wed, Jun 24, 2020 at 9:25 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 12, 2020 at 06:00:51PM +0200, Daniel Vetter wrote:
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Shawn Guo shawnguo@kernel.org Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Fabio Estevam festevam@gmail.com Cc: NXP Linux Team linux-imx@nxp.com Cc: linux-arm-kernel@lists.infradead.org
Ping for some ack/review on this pls.
Still looking for an ack here so I can land this entire pile. -Daniel
Thanks, Daniel
drivers/gpu/drm/imx/ipuv3-crtc.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 63c0284f8b3c..02c2f848f2d1 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -109,20 +109,15 @@ static void imx_drm_crtc_reset(struct drm_crtc *crtc) { struct imx_crtc_state *state;
if (crtc->state) {if (crtc->state->mode_blob)drm_property_blob_put(crtc->state->mode_blob);state = to_imx_crtc_state(crtc->state);memset(state, 0, sizeof(*state));} else {state = kzalloc(sizeof(*state), GFP_KERNEL);if (!state)return;crtc->state = &state->base;}
if (crtc->state)__drm_atomic_helper_crtc_destroy_state(crtc->state);
state->base.crtc = crtc;
kfree(to_imx_crtc_state(crtc->state));crtc->state = NULL;state = kzalloc(sizeof(*state), GFP_KERNEL);if (state)__drm_atomic_helper_crtc_reset(crtc, &state->base);}
static struct drm_crtc_state *imx_drm_crtc_duplicate_state(struct drm_crtc *crtc)
2.26.2
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Thu, 2020-07-02 at 11:41 +0200, Daniel Vetter wrote:
On Wed, Jun 24, 2020 at 9:25 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 12, 2020 at 06:00:51PM +0200, Daniel Vetter wrote:
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Shawn Guo shawnguo@kernel.org Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Fabio Estevam festevam@gmail.com Cc: NXP Linux Team linux-imx@nxp.com Cc: linux-arm-kernel@lists.infradead.org
Ping for some ack/review on this pls.
Still looking for an ack here so I can land this entire pile. -Daniel
Acked-by: Philipp Zabel p.zabel@pengutronix.de
regards Philipp
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Chun-Kuang Hu chunkuang.hu@kernel.org Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Matthias Brugger matthias.bgg@gmail.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org --- drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index a7dba4ced902..d654c7d514bd 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -112,19 +112,15 @@ static void mtk_drm_crtc_reset(struct drm_crtc *crtc) { struct mtk_crtc_state *state;
- if (crtc->state) { + if (crtc->state) __drm_atomic_helper_crtc_destroy_state(crtc->state);
- state = to_mtk_crtc_state(crtc->state); - memset(state, 0, sizeof(*state)); - } else { - state = kzalloc(sizeof(*state), GFP_KERNEL); - if (!state) - return; - crtc->state = &state->base; - } + kfree(to_mtk_crtc_state(crtc->state)); + crtc->state = NULL;
- state->base.crtc = crtc; + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (state) + __drm_atomic_helper_crtc_reset(crtc, &state->base); }
static struct drm_crtc_state *mtk_drm_crtc_duplicate_state(struct drm_crtc *crtc)
Hi, Daniel:
Daniel Vetter daniel.vetter@ffwll.ch 於 2020年6月13日 週六 上午12:01寫道：
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Acked-by: Chun-Kuang Hu chunkuang.hu@kernel.org
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Chun-Kuang Hu chunkuang.hu@kernel.org Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Matthias Brugger matthias.bgg@gmail.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index a7dba4ced902..d654c7d514bd 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -112,19 +112,15 @@ static void mtk_drm_crtc_reset(struct drm_crtc *crtc) { struct mtk_crtc_state *state;
if (crtc->state) {
if (crtc->state) __drm_atomic_helper_crtc_destroy_state(crtc->state);
state = to_mtk_crtc_state(crtc->state);memset(state, 0, sizeof(*state));} else {state = kzalloc(sizeof(*state), GFP_KERNEL);if (!state)return;crtc->state = &state->base;}
kfree(to_mtk_crtc_state(crtc->state));crtc->state = NULL;
state->base.crtc = crtc;
state = kzalloc(sizeof(*state), GFP_KERNEL);if (state)__drm_atomic_helper_crtc_reset(crtc, &state->base);}
static struct drm_crtc_state *mtk_drm_crtc_duplicate_state(struct drm_crtc *crtc)
2.26.2
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Eric Anholt eric@anholt.net --- drivers/gpu/drm/vc4/vc4_crtc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 29131409a4de..5371e63cf6e2 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -993,10 +993,9 @@ vc4_crtc_reset(struct drm_crtc *crtc) { if (crtc->state) vc4_crtc_destroy_state(crtc, crtc->state); - crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL); if (crtc->state) - crtc->state->crtc = crtc; + __drm_atomic_helper_crtc_reset(crtc, crtc->state); }
static const struct drm_crtc_funcs vc4_crtc_funcs = {
On Fri, Jun 12, 2020 at 06:00:53PM +0200, Daniel Vetter wrote:
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Eric Anholt eric@anholt.net
Reviewed-by: Maxime Ripard mripard@kernel.org
Maxime
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Roland Scheidegger sroland@vmware.com --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 3c97654b5a43..e91dfc65a93f 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -629,8 +629,7 @@ void vmw_du_crtc_reset(struct drm_crtc *crtc) return; }
- crtc->state = &vcs->base; - crtc->state->crtc = crtc; + __drm_atomic_helper_crtc_reset(crtc, &state->base); }
Hi Daniel,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master v5.7 next-20200612] [cannot apply to drm/drm-next] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-atomic-helper-res... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-allyesconfig (attached as .config) compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3b43f006294971b8049d4807110032169780e5b8) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>, old ones prefixed by <<):
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:632:40: error: use of undeclared identifier 'state'
__drm_atomic_helper_crtc_reset(crtc, &state->base); ^ 1 error generated.
vim +/state +632 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
604 605 606 /** 607 * vmw_du_crtc_reset - creates a blank vmw crtc state 608 * @crtc: DRM crtc 609 * 610 * Resets the atomic state for @crtc by freeing the state pointer (which 611 * might be NULL, e.g. at driver load time) and allocating a new empty state 612 * object. 613 */ 614 void vmw_du_crtc_reset(struct drm_crtc *crtc) 615 { 616 struct vmw_crtc_state *vcs; 617 618 619 if (crtc->state) { 620 __drm_atomic_helper_crtc_destroy_state(crtc->state); 621 622 kfree(vmw_crtc_state_to_vcs(crtc->state)); 623 } 624 625 vcs = kzalloc(sizeof(*vcs), GFP_KERNEL); 626 627 if (!vcs) { 628 DRM_ERROR("Cannot allocate vmw_crtc_state\n"); 629 return; 630 } 631
632 __drm_atomic_helper_crtc_reset(crtc, &state->base);
633 } 634
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
v2: Compile fix. Oops.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Roland Scheidegger sroland@vmware.com --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 3c97654b5a43..bbce45d142aa 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -629,8 +629,7 @@ void vmw_du_crtc_reset(struct drm_crtc *crtc) return; }
- crtc->state = &vcs->base; - crtc->state->crtc = crtc; + __drm_atomic_helper_crtc_reset(crtc, &vcs->base); }
Looks great to me. Reviewed-by: Roland Scheidegger sroland@vmware.com
Am 12.06.20 um 22:49 schrieb Daniel Vetter:
Now also comes with the added benefit of doing a drm_crtc_vblank_off(), which means vblank state isn't ill-defined and fail-y at driver load before the first modeset on each crtc.
v2: Compile fix. Oops.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Roland Scheidegger sroland@vmware.com
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 3c97654b5a43..bbce45d142aa 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -629,8 +629,7 @@ void vmw_du_crtc_reset(struct drm_crtc *crtc) return; }
- crtc->state = &vcs->base;
- crtc->state->crtc = crtc;
- __drm_atomic_helper_crtc_reset(crtc, &vcs->base);
}
Hi Daniel,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master v5.7 next-20200612] [cannot apply to drm/drm-next] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-atomic-helper-res... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-rhel-7.6 (attached as .config) compiler: gcc-9 (Debian 9.3.0-13) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>, old ones prefixed by <<):
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c: In function 'vmw_du_primary_plane_atomic_check': drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:460:31: warning: variable 'vcs' set but not used [-Wunused-but-set-variable] 460 | struct vmw_connector_state *vcs; | ^~~ drivers/gpu/drm/vmwgfx/vmwgfx_kms.c: In function 'vmw_du_crtc_reset': << In file included from drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:37:
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:632:40: error: 'state' undeclared (first use in this function); did you mean 'statx'?
632 | __drm_atomic_helper_crtc_reset(crtc, &state->base); | ^~~~~ | statx drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:632:40: note: each undeclared identifier is reported only once for each function it appears in In file included from drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:37: At top level: drivers/gpu/drm/vmwgfx/vmwgfx_kms.h:256:23: warning: 'vmw_cursor_plane_formats' defined but not used [-Wunused-const-variable=] 256 | static const uint32_t vmw_cursor_plane_formats[] = { | ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/vmwgfx/vmwgfx_kms.h:248:23: warning: 'vmw_primary_plane_formats' defined but not used [-Wunused-const-variable=] 248 | static const uint32_t vmw_primary_plane_formats[] = { | ^~~~~~~~~~~~~~~~~~~~~~~~~
vim +632 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
604 605 606 /** 607 * vmw_du_crtc_reset - creates a blank vmw crtc state 608 * @crtc: DRM crtc 609 * 610 * Resets the atomic state for @crtc by freeing the state pointer (which 611 * might be NULL, e.g. at driver load time) and allocating a new empty state 612 * object. 613 */ 614 void vmw_du_crtc_reset(struct drm_crtc *crtc) 615 { 616 struct vmw_crtc_state *vcs; 617 618 619 if (crtc->state) { 620 __drm_atomic_helper_crtc_destroy_state(crtc->state); 621 622 kfree(vmw_crtc_state_to_vcs(crtc->state)); 623 } 624 625 vcs = kzalloc(sizeof(*vcs), GFP_KERNEL); 626 627 if (!vcs) { 628 DRM_ERROR("Cannot allocate vmw_crtc_state\n"); 629 return; 630 } 631
632 __drm_atomic_helper_crtc_reset(crtc, &state->base);
633 } 634
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
The atomic helpers try really hard to not lose track of things, duplicating enabled tracking in the driver is at best confusing. Double-enabling or disabling is a bug in atomic helpers.
In the fb_dirty function we can just assume that the fb always exists, simple display pipe helpers guarantee that the crtc is only enabled together with the output, so we always have a primary plane around.
Now in the update function we need to be a notch more careful, since that can also get called when the crtc is off. And we don't want to upload frames when that's the case, so filter that out too.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: David Lechner david@lechnology.com --- drivers/gpu/drm/drm_mipi_dbi.c | 16 ++++++---------- drivers/gpu/drm/tiny/ili9225.c | 12 +++--------- drivers/gpu/drm/tiny/st7586.c | 11 +++-------- include/drm/drm_mipi_dbi.h | 5 ----- 4 files changed, 12 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index fd8d672972a9..79532b9a324a 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -268,7 +268,7 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) bool full; void *tr;
- if (!dbidev->enabled) + if (WARN_ON(!fb)) return;
if (!drm_dev_enter(fb->dev, &idx)) @@ -314,6 +314,9 @@ void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *state = pipe->plane.state; struct drm_rect rect;
+ if (!pipe->crtc.state->active) + return; + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) mipi_dbi_fb_dirty(state->fb, &rect); } @@ -325,9 +328,8 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update); * @crtc_state: CRTC state * @plane_state: Plane state * - * This function sets &mipi_dbi->enabled, flushes the whole framebuffer and - * enables the backlight. Drivers can use this in their - * &drm_simple_display_pipe_funcs->enable callback. + * Flushes the whole framebuffer and enables the backlight. Drivers can use this + * in their &drm_simple_display_pipe_funcs->enable callback. * * Note: Drivers which don't use mipi_dbi_pipe_update() because they have custom * framebuffer flushing, can't use this function since they both use the same @@ -349,7 +351,6 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev, if (!drm_dev_enter(&dbidev->drm, &idx)) return;
- dbidev->enabled = true; mipi_dbi_fb_dirty(fb, &rect); backlight_enable(dbidev->backlight);
@@ -390,13 +391,8 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) { struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
- if (!dbidev->enabled) - return; - DRM_DEBUG_KMS("\n");
- dbidev->enabled = false; - if (dbidev->backlight) backlight_disable(dbidev->backlight); else diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c index 16400064320f..97a77262d791 100644 --- a/drivers/gpu/drm/tiny/ili9225.c +++ b/drivers/gpu/drm/tiny/ili9225.c @@ -89,9 +89,6 @@ static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) bool full; void *tr;
- if (!dbidev->enabled) - return; - if (!drm_dev_enter(fb->dev, &idx)) return;
@@ -167,6 +164,9 @@ static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *state = pipe->plane.state; struct drm_rect rect;
+ if (!pipe->crtc.state->active) + return; + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) ili9225_fb_dirty(state->fb, &rect); } @@ -275,7 +275,6 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
ili9225_command(dbi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
- dbidev->enabled = true; ili9225_fb_dirty(fb, &rect); out_exit: drm_dev_exit(idx); @@ -295,16 +294,11 @@ static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe) * unplug. */
- if (!dbidev->enabled) - return; - ili9225_command(dbi, ILI9225_DISPLAY_CONTROL_1, 0x0000); msleep(50); ili9225_command(dbi, ILI9225_POWER_CONTROL_2, 0x0007); msleep(50); ili9225_command(dbi, ILI9225_POWER_CONTROL_1, 0x0a02); - - dbidev->enabled = false; }
static int ili9225_dbi_command(struct mipi_dbi *dbi, u8 *cmd, u8 *par, diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c index 1311e5df8721..d05de03891f8 100644 --- a/drivers/gpu/drm/tiny/st7586.c +++ b/drivers/gpu/drm/tiny/st7586.c @@ -118,9 +118,6 @@ static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) struct mipi_dbi *dbi = &dbidev->dbi; int start, end, idx, ret = 0;
- if (!dbidev->enabled) - return; - if (!drm_dev_enter(fb->dev, &idx)) return;
@@ -161,6 +158,9 @@ static void st7586_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *state = pipe->plane.state; struct drm_rect rect;
+ if (!pipe->crtc.state->active) + return; + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) st7586_fb_dirty(state->fb, &rect); } @@ -237,7 +237,6 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
msleep(100);
- dbidev->enabled = true; st7586_fb_dirty(fb, &rect);
mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON); @@ -258,11 +257,7 @@ static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
DRM_DEBUG_KMS("\n");
- if (!dbidev->enabled) - return; - mipi_dbi_command(&dbidev->dbi, MIPI_DCS_SET_DISPLAY_OFF); - dbidev->enabled = false; }
static const u32 st7586_formats[] = { diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h index 4d0e49c0ed2c..c2827ceaba0d 100644 --- a/include/drm/drm_mipi_dbi.h +++ b/include/drm/drm_mipi_dbi.h @@ -94,11 +94,6 @@ struct mipi_dbi_dev { */ struct drm_display_mode mode;
- /** - * @enabled: Pipeline is enabled - */ - bool enabled; - /** * @tx_buf: Buffer used for transfer (copy clip rect area) */
Den 12.06.2020 18.00, skrev Daniel Vetter:
The atomic helpers try really hard to not lose track of things, duplicating enabled tracking in the driver is at best confusing. Double-enabling or disabling is a bug in atomic helpers.
In the fb_dirty function we can just assume that the fb always exists, simple display pipe helpers guarantee that the crtc is only enabled together with the output, so we always have a primary plane around.
Now in the update function we need to be a notch more careful, since that can also get called when the crtc is off. And we don't want to upload frames when that's the case, so filter that out too.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: David Lechner david@lechnology.com
Thanks for fixing this.
Reviewed-by: Noralf Trønnes noralf@tronnes.org
On 6/12/20 11:00 AM, Daniel Vetter wrote:
The atomic helpers try really hard to not lose track of things, duplicating enabled tracking in the driver is at best confusing. Double-enabling or disabling is a bug in atomic helpers.
In the fb_dirty function we can just assume that the fb always exists, simple display pipe helpers guarantee that the crtc is only enabled together with the output, so we always have a primary plane around.
Now in the update function we need to be a notch more careful, since that can also get called when the crtc is off. And we don't want to upload frames when that's the case, so filter that out too.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: David Lechner david@lechnology.com
Acked-by: David Lechner david@lechnology.com
Hi Daniel,
On Fri, 12 Jun 2020 at 17:01, Daniel Vetter daniel.vetter@ffwll.ch wrote:
The atomic helpers try really hard to not lose track of things, duplicating enabled tracking in the driver is at best confusing. Double-enabling or disabling is a bug in atomic helpers.
In the fb_dirty function we can just assume that the fb always exists, simple display pipe helpers guarantee that the crtc is only enabled together with the output, so we always have a primary plane around.
Now in the update function we need to be a notch more careful, since that can also get called when the crtc is off. And we don't want to upload frames when that's the case, so filter that out too.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: David Lechner david@lechnology.com
drivers/gpu/drm/drm_mipi_dbi.c | 16 ++++++---------- drivers/gpu/drm/tiny/ili9225.c | 12 +++--------- drivers/gpu/drm/tiny/st7586.c | 11 +++-------- include/drm/drm_mipi_dbi.h | 5 ----- 4 files changed, 12 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index fd8d672972a9..79532b9a324a 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -268,7 +268,7 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) bool full; void *tr;
if (!dbidev->enabled)
if (WARN_ON(!fb)) return;
AFAICT no other driver has such WARN_ON. Let's drop that - it is pretty confusing and misleading as-is.
With that, patches 7/8 and 8/8 are: Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
-Emil
On Mon, Jun 15, 2020 at 11:35 PM Emil Velikov emil.l.velikov@gmail.com wrote:
Hi Daniel,
On Fri, 12 Jun 2020 at 17:01, Daniel Vetter daniel.vetter@ffwll.ch wrote:
The atomic helpers try really hard to not lose track of things, duplicating enabled tracking in the driver is at best confusing. Double-enabling or disabling is a bug in atomic helpers.
In the fb_dirty function we can just assume that the fb always exists, simple display pipe helpers guarantee that the crtc is only enabled together with the output, so we always have a primary plane around.
Now in the update function we need to be a notch more careful, since that can also get called when the crtc is off. And we don't want to upload frames when that's the case, so filter that out too.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: David Lechner david@lechnology.com
drivers/gpu/drm/drm_mipi_dbi.c | 16 ++++++---------- drivers/gpu/drm/tiny/ili9225.c | 12 +++--------- drivers/gpu/drm/tiny/st7586.c | 11 +++-------- include/drm/drm_mipi_dbi.h | 5 ----- 4 files changed, 12 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index fd8d672972a9..79532b9a324a 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -268,7 +268,7 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) bool full; void *tr;
if (!dbidev->enabled)
if (WARN_ON(!fb)) return;AFAICT no other driver has such WARN_ON. Let's drop that - it is pretty confusing and misleading as-is.
Yeah, this is a helper library which might be used wrongly by drivers. That's why I put it in - if you don't put all the various calls together correctly, this should at least catch one case. So really would like to keep this, can I convince you? -Daniel
With that, patches 7/8 and 8/8 are: Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
-Emil
On Tue, 16 Jun 2020 at 07:50, Daniel Vetter daniel.vetter@ffwll.ch wrote:
On Mon, Jun 15, 2020 at 11:35 PM Emil Velikov emil.l.velikov@gmail.com wrote:
Hi Daniel,
On Fri, 12 Jun 2020 at 17:01, Daniel Vetter daniel.vetter@ffwll.ch wrote:
The atomic helpers try really hard to not lose track of things, duplicating enabled tracking in the driver is at best confusing. Double-enabling or disabling is a bug in atomic helpers.
In the fb_dirty function we can just assume that the fb always exists, simple display pipe helpers guarantee that the crtc is only enabled together with the output, so we always have a primary plane around.
Now in the update function we need to be a notch more careful, since that can also get called when the crtc is off. And we don't want to upload frames when that's the case, so filter that out too.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: David Lechner david@lechnology.com
drivers/gpu/drm/drm_mipi_dbi.c | 16 ++++++---------- drivers/gpu/drm/tiny/ili9225.c | 12 +++--------- drivers/gpu/drm/tiny/st7586.c | 11 +++-------- include/drm/drm_mipi_dbi.h | 5 ----- 4 files changed, 12 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index fd8d672972a9..79532b9a324a 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -268,7 +268,7 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) bool full; void *tr;
if (!dbidev->enabled)
if (WARN_ON(!fb)) return;AFAICT no other driver has such WARN_ON. Let's drop that - it is pretty confusing and misleading as-is.
Yeah, this is a helper library which might be used wrongly by drivers. That's why I put it in - if you don't put all the various calls together correctly, this should at least catch one case. So really would like to keep this, can I convince you?
There are plenty of similar places where a drm library/helper can be misused, lacking a WARN. Nevertheless - sure feel free to keep it.
-Emil
On Tue, Jun 16, 2020 at 3:57 PM Emil Velikov emil.l.velikov@gmail.com wrote:
On Tue, 16 Jun 2020 at 07:50, Daniel Vetter daniel.vetter@ffwll.ch wrote:
On Mon, Jun 15, 2020 at 11:35 PM Emil Velikov emil.l.velikov@gmail.com wrote:
Hi Daniel,
On Fri, 12 Jun 2020 at 17:01, Daniel Vetter daniel.vetter@ffwll.ch wrote:
The atomic helpers try really hard to not lose track of things, duplicating enabled tracking in the driver is at best confusing. Double-enabling or disabling is a bug in atomic helpers.
In the fb_dirty function we can just assume that the fb always exists, simple display pipe helpers guarantee that the crtc is only enabled together with the output, so we always have a primary plane around.
Now in the update function we need to be a notch more careful, since that can also get called when the crtc is off. And we don't want to upload frames when that's the case, so filter that out too.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: David Lechner david@lechnology.com
drivers/gpu/drm/drm_mipi_dbi.c | 16 ++++++---------- drivers/gpu/drm/tiny/ili9225.c | 12 +++--------- drivers/gpu/drm/tiny/st7586.c | 11 +++-------- include/drm/drm_mipi_dbi.h | 5 ----- 4 files changed, 12 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index fd8d672972a9..79532b9a324a 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -268,7 +268,7 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) bool full; void *tr;
if (!dbidev->enabled)
if (WARN_ON(!fb)) return;AFAICT no other driver has such WARN_ON. Let's drop that - it is pretty confusing and misleading as-is.
Yeah, this is a helper library which might be used wrongly by drivers. That's why I put it in - if you don't put all the various calls together correctly, this should at least catch one case. So really would like to keep this, can I convince you?
There are plenty of similar places where a drm library/helper can be misused, lacking a WARN. Nevertheless - sure feel free to keep it.
Yeah I agree, we can't check for everything. Personally I think a check is warranted in two conditions: - drivers got it wrong, and the WARNING helps catch driver-bugs we've seen in the wild. Not really the case here - drivers do check something as defensive programming, but it's an invariant enforced by higher levels or helpers. Those I like to convert to WARNING so that other driver authors learn that this should never happen. This is such a case imo, I removed a bunch of fb checks from drivers here.
But yeah I think we should only add WARNING checks if this is actually something people have gotten wrong, otherwise there's just too many of them, distracting from the code. -Daniel
On Tue, Jun 16, 2020 at 07:16:45PM +0200, Daniel Vetter wrote:
On Tue, Jun 16, 2020 at 3:57 PM Emil Velikov emil.l.velikov@gmail.com wrote:
On Tue, 16 Jun 2020 at 07:50, Daniel Vetter daniel.vetter@ffwll.ch wrote:
On Mon, Jun 15, 2020 at 11:35 PM Emil Velikov emil.l.velikov@gmail.com wrote:
Hi Daniel,
On Fri, 12 Jun 2020 at 17:01, Daniel Vetter daniel.vetter@ffwll.ch wrote:
The atomic helpers try really hard to not lose track of things, duplicating enabled tracking in the driver is at best confusing. Double-enabling or disabling is a bug in atomic helpers.
In the fb_dirty function we can just assume that the fb always exists, simple display pipe helpers guarantee that the crtc is only enabled together with the output, so we always have a primary plane around.
Now in the update function we need to be a notch more careful, since that can also get called when the crtc is off. And we don't want to upload frames when that's the case, so filter that out too.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: David Lechner david@lechnology.com
drivers/gpu/drm/drm_mipi_dbi.c | 16 ++++++---------- drivers/gpu/drm/tiny/ili9225.c | 12 +++--------- drivers/gpu/drm/tiny/st7586.c | 11 +++-------- include/drm/drm_mipi_dbi.h | 5 ----- 4 files changed, 12 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index fd8d672972a9..79532b9a324a 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -268,7 +268,7 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect) bool full; void *tr;
if (!dbidev->enabled)
if (WARN_ON(!fb)) return;AFAICT no other driver has such WARN_ON. Let's drop that - it is pretty confusing and misleading as-is.
Yeah, this is a helper library which might be used wrongly by drivers. That's why I put it in - if you don't put all the various calls together correctly, this should at least catch one case. So really would like to keep this, can I convince you?
There are plenty of similar places where a drm library/helper can be misused, lacking a WARN. Nevertheless - sure feel free to keep it.
Yeah I agree, we can't check for everything. Personally I think a check is warranted in two conditions:
- drivers got it wrong, and the WARNING helps catch driver-bugs we've
seen in the wild. Not really the case here
- drivers do check something as defensive programming, but it's an
invariant enforced by higher levels or helpers. Those I like to convert to WARNING so that other driver authors learn that this should never happen. This is such a case imo, I removed a bunch of fb checks from drivers here.
But yeah I think we should only add WARNING checks if this is actually something people have gotten wrong, otherwise there's just too many of them, distracting from the code.
Merged this patch here too, thanks everyone for reviewing. -Daniel
Same patch as the mipi-dbi one, atomic tracks this for us already, we just have to check the right thing.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Noralf Trønnes" noralf@tronnes.org --- drivers/gpu/drm/tiny/repaper.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c index 08164e2a2d13..2e01cf0a9876 100644 --- a/drivers/gpu/drm/tiny/repaper.c +++ b/drivers/gpu/drm/tiny/repaper.c @@ -88,7 +88,6 @@ struct repaper_epd { u8 *line_buffer; void *current_frame;
- bool enabled; bool cleared; bool partial; }; @@ -538,9 +537,6 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb) int idx, ret = 0; u8 *buf = NULL;
- if (!epd->enabled) - return 0; - if (!drm_dev_enter(fb->dev, &idx)) return -ENODEV;
@@ -786,7 +782,6 @@ static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe, */ repaper_write_val(spi, 0x02, 0x04);
- epd->enabled = true; epd->partial = false; out_exit: drm_dev_exit(idx); @@ -805,13 +800,8 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe) * unplug. */
- if (!epd->enabled) - return; - DRM_DEBUG_DRIVER("\n");
- epd->enabled = false; - /* Nothing frame */ for (line = 0; line < epd->height; line++) repaper_one_line(epd, 0x7fffu, NULL, 0x00, NULL, @@ -859,6 +849,9 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *state = pipe->plane.state; struct drm_rect rect;
+ if (!pipe->crtc.state->active) + return; + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) repaper_fb_dirty(state->fb); }
Den 12.06.2020 18.00, skrev Daniel Vetter:
Same patch as the mipi-dbi one, atomic tracks this for us already, we just have to check the right thing.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Noralf Trønnes" noralf@tronnes.org
Reviewed-by: Noralf Trønnes noralf@tronnes.org
On Sat, Jun 13, 2020 at 03:43:23PM +0200, Noralf Trønnes wrote:
Den 12.06.2020 18.00, skrev Daniel Vetter:
Same patch as the mipi-dbi one, atomic tracks this for us already, we just have to check the right thing.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Noralf Trønnes" noralf@tronnes.org
Reviewed-by: Noralf Trønnes noralf@tronnes.org
Thanks for your review, patch applied. -Daniel
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().
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 laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@collabora.com Acked-by: Liviu Dudau liviu.dudau@arm.com Acked-by: Thierry Reding treding@nvidia.com Link: https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7c... Reported-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Reported-by: syzbot+0871b14ca2e2fb64f6e3@syzkaller.appspotmail.com Cc: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Cc: "James (Qian) Wang" james.qian.wang@arm.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Mihail Atanassov mihail.atanassov@arm.com Cc: Brian Starkey brian.starkey@arm.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Boris Brezillon bbrezillon@kernel.org Cc: Nicolas Ferre nicolas.ferre@microchip.com Cc: Alexandre Belloni alexandre.belloni@bootlin.com Cc: Ludovic Desroches ludovic.desroches@microchip.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Thierry Reding thierry.reding@gmail.com Cc: Jonathan Hunter jonathanh@nvidia.com Cc: Jyri Sarha jsarha@ti.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul seanpaul@chromium.org Cc: Brian Masney masneyb@onstation.org Cc: Emil Velikov emil.velikov@collabora.com Cc: zhengbin zhengbin13@huawei.com Cc: Thomas Gleixner tglx@linutronix.de Cc: linux-tegra@vger.kernel.org Cc: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-renesas-soc@vger.kernel.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Acked-by: Maxime Ripard mripard@kernel.org
Maxime
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 laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@collabora.com Acked-by: Liviu Dudau liviu.dudau@arm.com Acked-by: Thierry Reding treding@nvidia.com Link: https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7c... Reported-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Reported-by: syzbot+0871b14ca2e2fb64f6e3@syzkaller.appspotmail.com Cc: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Cc: "James (Qian) Wang" james.qian.wang@arm.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Mihail Atanassov mihail.atanassov@arm.com Cc: Brian Starkey brian.starkey@arm.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Boris Brezillon bbrezillon@kernel.org Cc: Nicolas Ferre nicolas.ferre@microchip.com Cc: Alexandre Belloni alexandre.belloni@bootlin.com Cc: Ludovic Desroches ludovic.desroches@microchip.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Thierry Reding thierry.reding@gmail.com Cc: Jonathan Hunter jonathanh@nvidia.com Cc: Jyri Sarha jsarha@ti.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul seanpaul@chromium.org Cc: Brian Masney masneyb@onstation.org Cc: Emil Velikov emil.velikov@collabora.com Cc: zhengbin zhengbin13@huawei.com Cc: Thomas Gleixner tglx@linutronix.de Cc: linux-tegra@vger.kernel.org Cc: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-renesas-soc@vger.kernel.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com
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 <drm/drm_device.h> #include <drm/drm_plane.h> #include <drm/drm_print.h> +#include <drm/drm_vblank.h> #include <drm/drm_writeback.h>
#include <linux/slab.h> @@ -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 laurent.pinchart@ideasonboard.com
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__);
On Thu, Jul 2, 2020 at 1:27 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
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 ?
Uh yes need to mention them too here, and how exactly they're a bit different. Will shuffle that from the v4: block below when applying.
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 laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@collabora.com Acked-by: Liviu Dudau liviu.dudau@arm.com Acked-by: Thierry Reding treding@nvidia.com Link: https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7c... Reported-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Reported-by: syzbot+0871b14ca2e2fb64f6e3@syzkaller.appspotmail.com Cc: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Cc: "James (Qian) Wang" james.qian.wang@arm.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Mihail Atanassov mihail.atanassov@arm.com Cc: Brian Starkey brian.starkey@arm.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Boris Brezillon bbrezillon@kernel.org Cc: Nicolas Ferre nicolas.ferre@microchip.com Cc: Alexandre Belloni alexandre.belloni@bootlin.com Cc: Ludovic Desroches ludovic.desroches@microchip.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Thierry Reding thierry.reding@gmail.com Cc: Jonathan Hunter jonathanh@nvidia.com Cc: Jyri Sarha jsarha@ti.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul seanpaul@chromium.org Cc: Brian Masney masneyb@onstation.org Cc: Emil Velikov emil.velikov@collabora.com Cc: zhengbin zhengbin13@huawei.com Cc: Thomas Gleixner tglx@linutronix.de Cc: linux-tegra@vger.kernel.org Cc: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-renesas-soc@vger.kernel.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com
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 <drm/drm_device.h> #include <drm/drm_plane.h> #include <drm/drm_print.h> +#include <drm/drm_vblank.h> #include <drm/drm_writeback.h>
#include <linux/slab.h> @@ -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:
Atomic helpers, specifically the reset helpers I'm adjusting here assume that at driver load time everything is completely off. They _only_ reset the sw state.
If you want to have more smooth takeover (flicker-free boot eventually even), or have some hw that's not getting reset as part of power-up or driver load, then that would be for the driver to handle. I think we recently had a discussion about what would need to be added to make atomic helpers support take-over of actual hw state at driver load. And yes if that's the case, then you'd need a different flow here to make sure vblank state is matching crtc state (e.g. i915 does that).
Wrt this case here specifically drm_handle_vblank needs to handle races anyway, so it's robust against being called when the vblanks are disabled in software. So I don't think you'll have any serious problem here.
Actual hw irq enable/disable is only done around drm_vblank_get/put (well with some delay), so I don't think that would have changed anything for you wrt actually getting an interrupt or not.
So tldr; I think just drm_vblank_reset here fits the best with overall helpers we have.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
I would however appreciate your thoughts on this topic, to know if my understanding is correct (and if this issue could affect other drivers).
Thanks a lot for your review, I'll apply the entire bunch later today. -Daniel
/* 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
As of commit 47ec5303d73ea344 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next") on linux.git , my VMware environment cannot boot. Do I need to bisect?
[ 9.314496][ T1] vga16fb: mapped to 0x0000000071050562 [ 9.467770][ T1] Console: switching to colour frame buffer device 80x30 [ 9.632092][ T1] fb0: VGA16 VGA frame buffer device [ 9.651768][ T1] ACPI: AC Adapter [ACAD] (on-line) [ 9.672544][ T1] input: Power Button as /devices/LNXSYSTM:00/LNXPWRBN:00/input/input0 [ 9.722373][ T1] ACPI: Power Button [PWRF] [ 9.744231][ T1] ioatdma: Intel(R) QuickData Technology Driver 5.00 [ 9.820147][ T1] N_HDLC line discipline registered with maxframe=4096 [ 9.835649][ T1] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled [ 9.852567][ T1] 00:05: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A [ 10.033372][ T1] Cyclades driver 2.6 [ 10.049928][ T1] Initializing Nozomi driver 2.1d [ 10.065493][ T1] RocketPort device driver module, version 2.09, 12-June-2003 [ 10.095368][ T1] No rocketport ports found; unloading driver [ 10.112430][ T1] Non-volatile memory driver v1.3 [ 10.127090][ T1] Linux agpgart interface v0.103 [ 10.144037][ T1] agpgart-intel 0000:00:00.0: Intel 440BX Chipset [ 10.162275][ T1] agpgart-intel 0000:00:00.0: AGP aperture is 256M @ 0x0 [ 10.181130][ T1] [drm] DMA map mode: Caching DMA mappings. [ 10.195150][ T1] [drm] Capabilities: [ 10.208728][ T1] [drm] Rect copy. [ 10.222772][ T1] [drm] Cursor. [ 10.235364][ T1] [drm] Cursor bypass. [ 10.249121][ T1] [drm] Cursor bypass 2. [ 10.260590][ T1] [drm] 8bit emulation. [ 10.272220][ T1] [drm] Alpha cursor. [ 10.284670][ T1] [drm] 3D. [ 10.295051][ T1] [drm] Extended Fifo. [ 10.305180][ T1] [drm] Multimon. [ 10.315506][ T1] [drm] Pitchlock. [ 10.325167][ T1] [drm] Irq mask. [ 10.334262][ T1] [drm] Display Topology. [ 10.343519][ T1] [drm] GMR. [ 10.352775][ T1] [drm] Traces. [ 10.362166][ T1] [drm] GMR2. [ 10.370716][ T1] [drm] Screen Object 2. [ 10.379220][ T1] [drm] Command Buffers. [ 10.388489][ T1] [drm] Command Buffers 2. [ 10.396055][ T1] [drm] Guest Backed Resources. [ 10.403290][ T1] [drm] DX Features. [ 10.409911][ T1] [drm] HP Command Queue. [ 10.417820][ T1] [drm] Capabilities2: [ 10.424216][ T1] [drm] Grow oTable. [ 10.430423][ T1] [drm] IntraSurface copy. [ 10.436371][ T1] [drm] Max GMR ids is 64 [ 10.442651][ T1] [drm] Max number of GMR pages is 65536 [ 10.450317][ T1] [drm] Max dedicated hypervisor surface memory is 0 kiB [ 10.458809][ T1] [drm] Maximum display memory size is 262144 kiB [ 10.466330][ T1] [drm] VRAM at 0xe8000000 size is 4096 kiB [ 10.474704][ T1] [drm] MMIO at 0xfe000000 size is 256 kiB [ 10.484625][ T1] [TTM] Zone kernel: Available graphics memory: 4030538 KiB [ 10.500730][ T1] [TTM] Zone dma32: Available graphics memory: 2097152 KiB [ 10.516851][ T1] [TTM] Initializing pool allocator [ 10.527542][ T1] [TTM] Initializing DMA pool allocator [ 10.540197][ T1] BUG: kernel NULL pointer dereference, address: 0000000000000438 [ 10.550087][ T1] #PF: supervisor read access in kernel mode [ 10.550087][ T1] #PF: error_code(0x0000) - not-present page [ 10.550087][ T1] PGD 0 P4D 0 [ 10.550087][ T1] Oops: 0000 [#1] PREEMPT SMP [ 10.550087][ T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0+ #271 [ 10.550087][ T1] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020 [ 10.550087][ T1] RIP: 0010:drm_dev_has_vblank+0x9/0x20 [ 10.550087][ T1] Code: 5d 41 5e 41 5f e9 e7 fa 01 ff e8 e2 fa 01 ff 45 31 e4 41 8b 5f 48 eb a7 cc cc cc cc cc cc cc cc cc 53 48 89 fb e8 c7 fa 01 ff <8b> 83 38 04 00 00 5b 85 c0 0f 95 c0 c3 66 2e 0f 1f 84 00 00 00 00 [ 10.550087][ T1] RSP: 0000:ffffc90000027b80 EFLAGS: 00010293 [ 10.550087][ T1] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 10.550087][ T1] RDX: ffff88823544c040 RSI: ffffffff823265b9 RDI: 0000000000000000 [ 10.550087][ T1] RBP: ffff888227238800 R08: 0000000000000001 R09: 0000000000000000 [ 10.550087][ T1] R10: ffff888227238800 R11: 0000000000000001 R12: 0000000000000000 [ 10.550087][ T1] R13: ffff888235103000 R14: 0000000000000000 R15: ffff888226cc6af0 [ 10.850690][ T1] FS: 0000000000000000(0000) GS:ffff888236e00000(0000) knlGS:0000000000000000 [ 10.850690][ T1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 10.850690][ T1] CR2: 0000000000000438 CR3: 0000000004a89001 CR4: 00000000003706f0 [ 10.850690][ T1] Call Trace: [ 10.850690][ T1] __drm_atomic_helper_crtc_reset+0x28/0x50 [ 10.850690][ T1] vmw_du_crtc_reset+0x62/0x80 [ 10.850690][ T1] vmw_kms_stdu_init_display+0x302/0x3f0 [ 10.850690][ T1] vmw_kms_init+0x6f/0xe0 [ 10.850690][ T1] vmw_probe+0xd52/0x1730 [ 10.850690][ T1] local_pci_probe+0x3a/0x90 [ 10.850690][ T1] pci_device_probe+0x163/0x230 [ 10.850690][ T1] ? pci_device_remove+0x100/0x100 [ 10.850690][ T1] really_probe+0x228/0x480 [ 10.850690][ T1] ? rdinit_setup+0x3b/0x3b [ 10.850690][ T1] driver_probe_device+0x6c/0xe0 [ 10.850690][ T1] device_driver_attach+0x5a/0x60 [ 10.850690][ T1] __driver_attach+0xbd/0x100 [ 10.850690][ T1] ? device_driver_attach+0x60/0x60 [ 10.850690][ T1] bus_for_each_dev+0x9e/0x110 [ 10.850690][ T1] bus_add_driver+0x1c8/0x260 [ 10.850690][ T1] driver_register+0x96/0x160 [ 10.850690][ T1] ? i915_global_vma_init+0x51/0x51 [ 11.202435][ T1] vmwgfx_init+0x2e/0x4e [ 11.202435][ T1] do_one_initcall+0x84/0x4a0 [ 11.202435][ T1] ? rdinit_setup+0x3b/0x3b [ 11.202435][ T1] ? rcu_read_lock_sched_held+0x4d/0x80 [ 11.202435][ T1] ? cpumask_test_cpu.constprop.19+0x12/0x30 [ 11.202435][ T1] ? rdinit_setup+0x3b/0x3b [ 11.202435][ T1] kernel_init_freeable+0x298/0x30c [ 11.202435][ T1] ? rest_init+0x2c0/0x2c0 [ 11.202435][ T1] kernel_init+0xf/0x170 [ 11.282173][ T1] ? _raw_spin_unlock_irq+0x3a/0x50 [ 11.282173][ T1] ? rest_init+0x2c0/0x2c0 [ 11.282173][ T1] ret_from_fork+0x1f/0x30 [ 11.282173][ T1] Modules linked in: [ 11.282173][ T1] CR2: 0000000000000438 [ 11.282173][ T1] ---[ end trace fb560758d9d704d3 ]--- [ 11.282173][ T1] RIP: 0010:drm_dev_has_vblank+0x9/0x20 [ 11.282173][ T1] Code: 5d 41 5e 41 5f e9 e7 fa 01 ff e8 e2 fa 01 ff 45 31 e4 41 8b 5f 48 eb a7 cc cc cc cc cc cc cc cc cc 53 48 89 fb e8 c7 fa 01 ff <8b> 83 38 04 00 00 5b 85 c0 0f 95 c0 c3 66 2e 0f 1f 84 00 00 00 00 [ 11.282173][ T1] RSP: 0000:ffffc90000027b80 EFLAGS: 00010293 [ 11.282173][ T1] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 11.282173][ T1] RDX: ffff88823544c040 RSI: ffffffff823265b9 RDI: 0000000000000000 [ 11.282173][ T1] RBP: ffff888227238800 R08: 0000000000000001 R09: 0000000000000000 [ 11.282173][ T1] R10: ffff888227238800 R11: 0000000000000001 R12: 0000000000000000 [ 11.282173][ T1] R13: ffff888235103000 R14: 0000000000000000 R15: ffff888226cc6af0 [ 11.282173][ T1] FS: 0000000000000000(0000) GS:ffff888236e00000(0000) knlGS:0000000000000000 [ 11.282173][ T1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 11.282173][ T1] CR2: 0000000000000438 CR3: 0000000004a89001 CR4: 00000000003706f0 [ 11.282173][ T1] Kernel panic - not syncing: Fatal exception [ 11.282173][ T1] Kernel Offset: disabled [ 11.282173][ T1] Rebooting in 86400 seconds..
On Thu, Aug 06, 2020 at 03:43:02PM +0900, Tetsuo Handa wrote:
As of commit 47ec5303d73ea344 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next") on linux.git , my VMware environment cannot boot. Do I need to bisect?
That sounds like a good idea, but please start a new thread (not reply to some random existing ones), with maintainers for drivers/gpu/drm/vmwgfx only. Not a massive list of random folks who have no idea what's going on here. From get_maintainers.pl
$ scripts/get_maintainer.pl -f drivers/gpu/drm/vmwgfx/ VMware Graphics linux-graphics-maintainer@vmware.com (supporter:DRM DRIVER FOR VMWARE VIRTUAL GPU) Roland Scheidegger sroland@vmware.com (supporter:DRM DRIVER FOR VMWARE VIRTUAL GPU) David Airlie airlied@linux.ie (maintainer:DRM DRIVERS) Daniel Vetter daniel@ffwll.ch (maintainer:DRM DRIVERS) dri-devel@lists.freedesktop.org (open list:DRM DRIVER FOR VMWARE VIRTUAL GPU) linux-kernel@vger.kernel.org (open list)
Cheers, Daniel
[ 9.314496][ T1] vga16fb: mapped to 0x0000000071050562 [ 9.467770][ T1] Console: switching to colour frame buffer device 80x30 [ 9.632092][ T1] fb0: VGA16 VGA frame buffer device [ 9.651768][ T1] ACPI: AC Adapter [ACAD] (on-line) [ 9.672544][ T1] input: Power Button as /devices/LNXSYSTM:00/LNXPWRBN:00/input/input0 [ 9.722373][ T1] ACPI: Power Button [PWRF] [ 9.744231][ T1] ioatdma: Intel(R) QuickData Technology Driver 5.00 [ 9.820147][ T1] N_HDLC line discipline registered with maxframe=4096 [ 9.835649][ T1] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled [ 9.852567][ T1] 00:05: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A [ 10.033372][ T1] Cyclades driver 2.6 [ 10.049928][ T1] Initializing Nozomi driver 2.1d [ 10.065493][ T1] RocketPort device driver module, version 2.09, 12-June-2003 [ 10.095368][ T1] No rocketport ports found; unloading driver [ 10.112430][ T1] Non-volatile memory driver v1.3 [ 10.127090][ T1] Linux agpgart interface v0.103 [ 10.144037][ T1] agpgart-intel 0000:00:00.0: Intel 440BX Chipset [ 10.162275][ T1] agpgart-intel 0000:00:00.0: AGP aperture is 256M @ 0x0 [ 10.181130][ T1] [drm] DMA map mode: Caching DMA mappings. [ 10.195150][ T1] [drm] Capabilities: [ 10.208728][ T1] [drm] Rect copy. [ 10.222772][ T1] [drm] Cursor. [ 10.235364][ T1] [drm] Cursor bypass. [ 10.249121][ T1] [drm] Cursor bypass 2. [ 10.260590][ T1] [drm] 8bit emulation. [ 10.272220][ T1] [drm] Alpha cursor. [ 10.284670][ T1] [drm] 3D. [ 10.295051][ T1] [drm] Extended Fifo. [ 10.305180][ T1] [drm] Multimon. [ 10.315506][ T1] [drm] Pitchlock. [ 10.325167][ T1] [drm] Irq mask. [ 10.334262][ T1] [drm] Display Topology. [ 10.343519][ T1] [drm] GMR. [ 10.352775][ T1] [drm] Traces. [ 10.362166][ T1] [drm] GMR2. [ 10.370716][ T1] [drm] Screen Object 2. [ 10.379220][ T1] [drm] Command Buffers. [ 10.388489][ T1] [drm] Command Buffers 2. [ 10.396055][ T1] [drm] Guest Backed Resources. [ 10.403290][ T1] [drm] DX Features. [ 10.409911][ T1] [drm] HP Command Queue. [ 10.417820][ T1] [drm] Capabilities2: [ 10.424216][ T1] [drm] Grow oTable. [ 10.430423][ T1] [drm] IntraSurface copy. [ 10.436371][ T1] [drm] Max GMR ids is 64 [ 10.442651][ T1] [drm] Max number of GMR pages is 65536 [ 10.450317][ T1] [drm] Max dedicated hypervisor surface memory is 0 kiB [ 10.458809][ T1] [drm] Maximum display memory size is 262144 kiB [ 10.466330][ T1] [drm] VRAM at 0xe8000000 size is 4096 kiB [ 10.474704][ T1] [drm] MMIO at 0xfe000000 size is 256 kiB [ 10.484625][ T1] [TTM] Zone kernel: Available graphics memory: 4030538 KiB [ 10.500730][ T1] [TTM] Zone dma32: Available graphics memory: 2097152 KiB [ 10.516851][ T1] [TTM] Initializing pool allocator [ 10.527542][ T1] [TTM] Initializing DMA pool allocator [ 10.540197][ T1] BUG: kernel NULL pointer dereference, address: 0000000000000438 [ 10.550087][ T1] #PF: supervisor read access in kernel mode [ 10.550087][ T1] #PF: error_code(0x0000) - not-present page [ 10.550087][ T1] PGD 0 P4D 0 [ 10.550087][ T1] Oops: 0000 [#1] PREEMPT SMP [ 10.550087][ T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0+ #271 [ 10.550087][ T1] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020 [ 10.550087][ T1] RIP: 0010:drm_dev_has_vblank+0x9/0x20 [ 10.550087][ T1] Code: 5d 41 5e 41 5f e9 e7 fa 01 ff e8 e2 fa 01 ff 45 31 e4 41 8b 5f 48 eb a7 cc cc cc cc cc cc cc cc cc 53 48 89 fb e8 c7 fa 01 ff <8b> 83 38 04 00 00 5b 85 c0 0f 95 c0 c3 66 2e 0f 1f 84 00 00 00 00 [ 10.550087][ T1] RSP: 0000:ffffc90000027b80 EFLAGS: 00010293 [ 10.550087][ T1] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 10.550087][ T1] RDX: ffff88823544c040 RSI: ffffffff823265b9 RDI: 0000000000000000 [ 10.550087][ T1] RBP: ffff888227238800 R08: 0000000000000001 R09: 0000000000000000 [ 10.550087][ T1] R10: ffff888227238800 R11: 0000000000000001 R12: 0000000000000000 [ 10.550087][ T1] R13: ffff888235103000 R14: 0000000000000000 R15: ffff888226cc6af0 [ 10.850690][ T1] FS: 0000000000000000(0000) GS:ffff888236e00000(0000) knlGS:0000000000000000 [ 10.850690][ T1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 10.850690][ T1] CR2: 0000000000000438 CR3: 0000000004a89001 CR4: 00000000003706f0 [ 10.850690][ T1] Call Trace: [ 10.850690][ T1] __drm_atomic_helper_crtc_reset+0x28/0x50 [ 10.850690][ T1] vmw_du_crtc_reset+0x62/0x80 [ 10.850690][ T1] vmw_kms_stdu_init_display+0x302/0x3f0 [ 10.850690][ T1] vmw_kms_init+0x6f/0xe0 [ 10.850690][ T1] vmw_probe+0xd52/0x1730 [ 10.850690][ T1] local_pci_probe+0x3a/0x90 [ 10.850690][ T1] pci_device_probe+0x163/0x230 [ 10.850690][ T1] ? pci_device_remove+0x100/0x100 [ 10.850690][ T1] really_probe+0x228/0x480 [ 10.850690][ T1] ? rdinit_setup+0x3b/0x3b [ 10.850690][ T1] driver_probe_device+0x6c/0xe0 [ 10.850690][ T1] device_driver_attach+0x5a/0x60 [ 10.850690][ T1] __driver_attach+0xbd/0x100 [ 10.850690][ T1] ? device_driver_attach+0x60/0x60 [ 10.850690][ T1] bus_for_each_dev+0x9e/0x110 [ 10.850690][ T1] bus_add_driver+0x1c8/0x260 [ 10.850690][ T1] driver_register+0x96/0x160 [ 10.850690][ T1] ? i915_global_vma_init+0x51/0x51 [ 11.202435][ T1] vmwgfx_init+0x2e/0x4e [ 11.202435][ T1] do_one_initcall+0x84/0x4a0 [ 11.202435][ T1] ? rdinit_setup+0x3b/0x3b [ 11.202435][ T1] ? rcu_read_lock_sched_held+0x4d/0x80 [ 11.202435][ T1] ? cpumask_test_cpu.constprop.19+0x12/0x30 [ 11.202435][ T1] ? rdinit_setup+0x3b/0x3b [ 11.202435][ T1] kernel_init_freeable+0x298/0x30c [ 11.202435][ T1] ? rest_init+0x2c0/0x2c0 [ 11.202435][ T1] kernel_init+0xf/0x170 [ 11.282173][ T1] ? _raw_spin_unlock_irq+0x3a/0x50 [ 11.282173][ T1] ? rest_init+0x2c0/0x2c0 [ 11.282173][ T1] ret_from_fork+0x1f/0x30 [ 11.282173][ T1] Modules linked in: [ 11.282173][ T1] CR2: 0000000000000438 [ 11.282173][ T1] ---[ end trace fb560758d9d704d3 ]--- [ 11.282173][ T1] RIP: 0010:drm_dev_has_vblank+0x9/0x20 [ 11.282173][ T1] Code: 5d 41 5e 41 5f e9 e7 fa 01 ff e8 e2 fa 01 ff 45 31 e4 41 8b 5f 48 eb a7 cc cc cc cc cc cc cc cc cc 53 48 89 fb e8 c7 fa 01 ff <8b> 83 38 04 00 00 5b 85 c0 0f 95 c0 c3 66 2e 0f 1f 84 00 00 00 00 [ 11.282173][ T1] RSP: 0000:ffffc90000027b80 EFLAGS: 00010293 [ 11.282173][ T1] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 11.282173][ T1] RDX: ffff88823544c040 RSI: ffffffff823265b9 RDI: 0000000000000000 [ 11.282173][ T1] RBP: ffff888227238800 R08: 0000000000000001 R09: 0000000000000000 [ 11.282173][ T1] R10: ffff888227238800 R11: 0000000000000001 R12: 0000000000000000 [ 11.282173][ T1] R13: ffff888235103000 R14: 0000000000000000 R15: ffff888226cc6af0 [ 11.282173][ T1] FS: 0000000000000000(0000) GS:ffff888236e00000(0000) knlGS:0000000000000000 [ 11.282173][ T1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 11.282173][ T1] CR2: 0000000000000438 CR3: 0000000004a89001 CR4: 00000000003706f0 [ 11.282173][ T1] Kernel panic - not syncing: Fatal exception [ 11.282173][ T1] Kernel Offset: disabled [ 11.282173][ T1] Rebooting in 86400 seconds..
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Chun-Kuang Hu -
Daniel Vetter -
Daniel Vetter -
daniel＠ffwll.ch
-
David Lechner -
Emil Velikov -
Harry Wentland -
kernel test robot -
Laurent Pinchart -
Maxime Ripard -
Noralf Trønnes -
Philipp Zabel -
Roland Scheidegger -
Tetsuo Handa