[PATCH 1/8] drm/atomic-helper: reset vblank on crtc reset

[PATCH 1/8] drm/atomic-helper: reset vblank on crtc reset

[Daniel Vetter]

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=0ba17d70d062b2595e1f061231474800f076c7cb
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__);
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] drm/amdgpu: Use __drm_atomic_helper_crtc_reset

[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.

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 *
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] drm/amdgpu: Use __drm_atomic_helper_crtc_reset

[Harry Wentland]

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 *
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] drm/amdgpu: Use __drm_atomic_helper_crtc_reset

[Alex Deucher]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] drm/amdgpu: Use __drm_atomic_helper_crtc_reset

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/8] drm/imx: Use __drm_atomic_helper_crtc_reset

[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.

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)
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm/imx: Use __drm_atomic_helper_crtc_reset

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm/imx: Use __drm_atomic_helper_crtc_reset

[Daniel Vetter]

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm/imx: Use __drm_atomic_helper_crtc_reset

[Philipp Zabel]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/8] drm/mtk: Use __drm_atomic_helper_crtc_reset

[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.

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/8] drm/mtk: Use __drm_atomic_helper_crtc_reset

[Chun-Kuang Hu]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/8] drm/vc4: Use __drm_atomic_helper_crtc_reset

[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.

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 = {
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] drm/vc4: Use __drm_atomic_helper_crtc_reset

[Maxime Ripard]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/8] drm/vmwgfx: Use __drm_atomic_helper_crtc_reset

[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.

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);
 }
 
 
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/vmwgfx: Use __drm_atomic_helper_crtc_reset

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2633 bytes --]

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-reset-vblank-on-crtc-reset/20200613-000414
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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 73519 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/vmwgfx: Use __drm_atomic_helper_crtc_reset

[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);
 }
 
 
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vmwgfx: Use __drm_atomic_helper_crtc_reset

[Roland Scheidegger]

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);
>  }
>  
>  
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/vmwgfx: Use __drm_atomic_helper_crtc_reset

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3453 bytes --]

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-reset-vblank-on-crtc-reset/20200613-000414
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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48661 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/8] drm/mipi-dbi: Remove ->enabled

[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>
---
 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)
 	 */
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/mipi-dbi: Remove ->enabled

[Noralf Trønnes]

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>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/mipi-dbi: Remove ->enabled

[David Lechner]

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>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled

[Emil Velikov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled

[Daniel Vetter]

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled

[Emil Velikov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/8] drm/tiny/repaper: Drop edp->enabled

[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>
---
 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);
 }
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/tiny/repaper: Drop edp->enabled

[Noralf Trønnes]

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>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/tiny/repaper: Drop edp->enabled

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/atomic-helper: reset vblank on crtc reset

[Maxime Ripard]

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=0ba17d70d062b2595e1f061231474800f076c7cb
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/atomic-helper: reset vblank on crtc reset

[Laurent Pinchart]

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=0ba17d70d062b2595e1f061231474800f076c7cb
> 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__);

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/atomic-helper: reset vblank on crtc reset

[Daniel Vetter]

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=0ba17d70d062b2595e1f061231474800f076c7cb
> > 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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/atomic-helper: reset vblank on crtc reset

[Tetsuo Handa]

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..


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/atomic-helper: reset vblank on crtc reset

[daniel]

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..
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel