Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset
On Sat, May 30, 2020 at 06:22:58AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Wed, May 27, 2020 at 11:53:32AM +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().
>
> Have you intentionally not touched drivers that use
> drm_crtc_vblank_off() at init time instead of drm_crtc_vblank_reset() ?
> I'm thinking about omapdrm and rcar-du that both call neither
> drm_crtc_vblank_reset() nor __drm_atomic_helper_crtc_reset() in their
> CRTC reset handler, and call drm_crtc_vblank_off() at init time. Should
> these (and likely other) drivers be updated ?
Good catch, I think we should remove those too with this patch.
I also used that opportunity to review all callers fo drm_crtc_vblank_off,
and I found two bogus callers in malidp and hdlcd. But only in the unload
code, so doesn't matter much.
I'll resend a new version.
-Daniel
> Other than that the patch looks good to me,
>
> Reviewed-by: Laurent Pinchart
>
> > 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.
> >
> > Link:
> > https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> > Reported-by: Tetsuo Handa
> > Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
> > Cc: Tetsuo Handa
> > Cc: "James (Qian) Wang"
> > Cc: Liviu Dudau
> > Cc: Mihail Atanassov
> > Cc: Brian Starkey
> > Cc: Sam Ravnborg
> > Cc: Boris Brezillon
> > Cc: Nicolas Ferre
> > Cc: Alexandre Belloni
> > Cc: Ludovic Desroches
> > Cc: Maarten Lankhorst
> > Cc: Maxime Ripard
> > Cc: Thomas Zimmermann
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: Thierry Reding
> > Cc: Jonathan Hunter
> > Cc: Jyri Sarha
> > Cc: Tomi Valkeinen
> > Cc: Rob Clark
> > Cc: Sean Paul
> > Cc: Brian Masney
> > Cc: Emil Velikov
> > Cc: zhengbin
> > Cc: Thomas Gleixner
> > Cc: linux-te...@vger.kernel.org
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
> > drivers/gpu/drm/arm/malidp_drv.c | 1 -
> > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-
> > drivers/gpu/drm/drm_atomic_state_helper.c| 4
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 --
> > drivers/gpu/drm/tegra/dc.c | 1 -
> > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> > drivers/gpu/drm/tidss/tidss_kms.c| 4
> > 8 files changed, 9 insertions(+), 20 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 = >base;
> > - crtc->state->crtc = crtc;
> > - }
> > + if (state)
> > + __drm_atomic_helper_crtc_reset(crtc, >base);
> > }
> >
> > static struct drm_crtc_state *
> > @@ -616,7 +614,6 @@ static int
Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset
On Wed, May 27, 2020 at 11:53:32AM +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.
>
> Link:
> https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> Reported-by: Tetsuo Handa
> Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
> Cc: Tetsuo Handa
> Cc: "James (Qian) Wang"
> Cc: Liviu Dudau
> Cc: Mihail Atanassov
> Cc: Brian Starkey
> Cc: Sam Ravnborg
> Cc: Boris Brezillon
> Cc: Nicolas Ferre
> Cc: Alexandre Belloni
> Cc: Ludovic Desroches
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Thierry Reding
> Cc: Jonathan Hunter
> Cc: Jyri Sarha
> Cc: Tomi Valkeinen
> Cc: Rob Clark
> Cc: Sean Paul
> Cc: Brian Masney
> Cc: Emil Velikov
> Cc: zhengbin
> Cc: Thomas Gleixner
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
> drivers/gpu/drm/arm/malidp_drv.c | 1 -
For the komeda and malidp drivers:
Acked-by: Liviu Dudau
Best regards,
Liviu
> 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/tegra/dc.c | 1 -
> drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> drivers/gpu/drm/tidss/tidss_kms.c| 4
> 8 files changed, 9 insertions(+), 20 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 = >base;
> - crtc->state->crtc = crtc;
> - }
> + if (state)
> + __drm_atomic_helper_crtc_reset(crtc, >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, _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 c2507b7d8512..02904392e370 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -870,7 +870,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(>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
> +++
Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset
Hi Daniel,
Thank you for the patch.
On Wed, May 27, 2020 at 11:53:32AM +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().
Have you intentionally not touched drivers that use
drm_crtc_vblank_off() at init time instead of drm_crtc_vblank_reset() ?
I'm thinking about omapdrm and rcar-du that both call neither
drm_crtc_vblank_reset() nor __drm_atomic_helper_crtc_reset() in their
CRTC reset handler, and call drm_crtc_vblank_off() at init time. Should
these (and likely other) drivers be updated ?
Other than that the patch looks good to me,
Reviewed-by: Laurent Pinchart
> 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.
>
> Link:
> https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> Reported-by: Tetsuo Handa
> Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
> Cc: Tetsuo Handa
> Cc: "James (Qian) Wang"
> Cc: Liviu Dudau
> Cc: Mihail Atanassov
> Cc: Brian Starkey
> Cc: Sam Ravnborg
> Cc: Boris Brezillon
> Cc: Nicolas Ferre
> Cc: Alexandre Belloni
> Cc: Ludovic Desroches
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Thierry Reding
> Cc: Jonathan Hunter
> Cc: Jyri Sarha
> Cc: Tomi Valkeinen
> Cc: Rob Clark
> Cc: Sean Paul
> Cc: Brian Masney
> Cc: Emil Velikov
> Cc: zhengbin
> Cc: Thomas Gleixner
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
> drivers/gpu/drm/arm/malidp_drv.c | 1 -
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-
> drivers/gpu/drm/drm_atomic_state_helper.c| 4
> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 --
> drivers/gpu/drm/tegra/dc.c | 1 -
> drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> drivers/gpu/drm/tidss/tidss_kms.c| 4
> 8 files changed, 9 insertions(+), 20 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 = >base;
> - crtc->state->crtc = crtc;
> - }
> + if (state)
> + __drm_atomic_helper_crtc_reset(crtc, >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, _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 c2507b7d8512..02904392e370 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -870,7 +870,6 @@ static int malidp_bind(struct device *dev)
> drm->irq_enabled = true;
>
> ret =
Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset
On Wed, May 27, 2020 at 2:08 PM Liviu Dudau wrote:
>
> On Wed, May 27, 2020 at 01:07:05PM +0200, Daniel Vetter wrote:
> > On Wed, May 27, 2020 at 12:57 PM Liviu Dudau wrote:
> > >
> > > Hi Daniel,
> > >
> > > On Wed, May 27, 2020 at 11:53:32AM +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.
> > > >
> > > > Link:
> > > > https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> > > > Reported-by: Tetsuo Handa
> > > > Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
> > > > Cc: Tetsuo Handa
> > > > Cc: "James (Qian) Wang"
> > > > Cc: Liviu Dudau
> > > > Cc: Mihail Atanassov
> > > > Cc: Brian Starkey
> > > > Cc: Sam Ravnborg
> > > > Cc: Boris Brezillon
> > > > Cc: Nicolas Ferre
> > > > Cc: Alexandre Belloni
> > > > Cc: Ludovic Desroches
> > > > Cc: Maarten Lankhorst
> > > > Cc: Maxime Ripard
> > > > Cc: Thomas Zimmermann
> > > > Cc: David Airlie
> > > > Cc: Daniel Vetter
> > > > Cc: Thierry Reding
> > > > Cc: Jonathan Hunter
> > > > Cc: Jyri Sarha
> > > > Cc: Tomi Valkeinen
> > > > Cc: Rob Clark
> > > > Cc: Sean Paul
> > > > Cc: Brian Masney
> > > > Cc: Emil Velikov
> > > > Cc: zhengbin
> > > > Cc: Thomas Gleixner
> > > > Cc: linux-te...@vger.kernel.org
> > > > Signed-off-by: Daniel Vetter
> > > > ---
> > > > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
> > > > drivers/gpu/drm/arm/malidp_drv.c | 1 -
> > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-
> > > > drivers/gpu/drm/drm_atomic_state_helper.c| 4
> > > > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 --
> > > > drivers/gpu/drm/tegra/dc.c | 1 -
> > > > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> > > > drivers/gpu/drm/tidss/tidss_kms.c| 4
> > > > 8 files changed, 9 insertions(+), 20 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 = >base;
> > > > - crtc->state->crtc = crtc;
> > > > - }
> > > > + if (state)
> > > > + __drm_atomic_helper_crtc_reset(crtc, >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, _crtc_helper_funcs);
> > > > - drm_crtc_vblank_reset(crtc);
> > > >
> > > > crtc->port =
Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset
On Wed, May 27, 2020 at 01:07:05PM +0200, Daniel Vetter wrote:
> On Wed, May 27, 2020 at 12:57 PM Liviu Dudau wrote:
> >
> > Hi Daniel,
> >
> > On Wed, May 27, 2020 at 11:53:32AM +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.
> > >
> > > Link:
> > > https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> > > Reported-by: Tetsuo Handa
> > > Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
> > > Cc: Tetsuo Handa
> > > Cc: "James (Qian) Wang"
> > > Cc: Liviu Dudau
> > > Cc: Mihail Atanassov
> > > Cc: Brian Starkey
> > > Cc: Sam Ravnborg
> > > Cc: Boris Brezillon
> > > Cc: Nicolas Ferre
> > > Cc: Alexandre Belloni
> > > Cc: Ludovic Desroches
> > > Cc: Maarten Lankhorst
> > > Cc: Maxime Ripard
> > > Cc: Thomas Zimmermann
> > > Cc: David Airlie
> > > Cc: Daniel Vetter
> > > Cc: Thierry Reding
> > > Cc: Jonathan Hunter
> > > Cc: Jyri Sarha
> > > Cc: Tomi Valkeinen
> > > Cc: Rob Clark
> > > Cc: Sean Paul
> > > Cc: Brian Masney
> > > Cc: Emil Velikov
> > > Cc: zhengbin
> > > Cc: Thomas Gleixner
> > > Cc: linux-te...@vger.kernel.org
> > > Signed-off-by: Daniel Vetter
> > > ---
> > > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
> > > drivers/gpu/drm/arm/malidp_drv.c | 1 -
> > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-
> > > drivers/gpu/drm/drm_atomic_state_helper.c| 4
> > > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 --
> > > drivers/gpu/drm/tegra/dc.c | 1 -
> > > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> > > drivers/gpu/drm/tidss/tidss_kms.c| 4
> > > 8 files changed, 9 insertions(+), 20 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 = >base;
> > > - crtc->state->crtc = crtc;
> > > - }
> > > + if (state)
> > > + __drm_atomic_helper_crtc_reset(crtc, >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, _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 c2507b7d8512..02904392e370 100644
> > > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > > @@ -870,7 +870,6 @@ static int
Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset
On Wed, May 27, 2020 at 12:57 PM Liviu Dudau wrote:
>
> Hi Daniel,
>
> On Wed, May 27, 2020 at 11:53:32AM +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.
> >
> > Link:
> > https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> > Reported-by: Tetsuo Handa
> > Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
> > Cc: Tetsuo Handa
> > Cc: "James (Qian) Wang"
> > Cc: Liviu Dudau
> > Cc: Mihail Atanassov
> > Cc: Brian Starkey
> > Cc: Sam Ravnborg
> > Cc: Boris Brezillon
> > Cc: Nicolas Ferre
> > Cc: Alexandre Belloni
> > Cc: Ludovic Desroches
> > Cc: Maarten Lankhorst
> > Cc: Maxime Ripard
> > Cc: Thomas Zimmermann
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: Thierry Reding
> > Cc: Jonathan Hunter
> > Cc: Jyri Sarha
> > Cc: Tomi Valkeinen
> > Cc: Rob Clark
> > Cc: Sean Paul
> > Cc: Brian Masney
> > Cc: Emil Velikov
> > Cc: zhengbin
> > Cc: Thomas Gleixner
> > Cc: linux-te...@vger.kernel.org
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
> > drivers/gpu/drm/arm/malidp_drv.c | 1 -
> > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-
> > drivers/gpu/drm/drm_atomic_state_helper.c| 4
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 --
> > drivers/gpu/drm/tegra/dc.c | 1 -
> > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> > drivers/gpu/drm/tidss/tidss_kms.c| 4
> > 8 files changed, 9 insertions(+), 20 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 = >base;
> > - crtc->state->crtc = crtc;
> > - }
> > + if (state)
> > + __drm_atomic_helper_crtc_reset(crtc, >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, _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 c2507b7d8512..02904392e370 100644
> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > @@ -870,7 +870,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(>crtc);
>
> It was a couple of years ago but I remember Alexandru-Cosmin tracking an
> issue around
> this. The reason for reseting the vblank
Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset
Hi Daniel,
On Wed, May 27, 2020 at 11:53:32AM +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.
>
> Link:
> https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> Reported-by: Tetsuo Handa
> Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com
> Cc: Tetsuo Handa
> Cc: "James (Qian) Wang"
> Cc: Liviu Dudau
> Cc: Mihail Atanassov
> Cc: Brian Starkey
> Cc: Sam Ravnborg
> Cc: Boris Brezillon
> Cc: Nicolas Ferre
> Cc: Alexandre Belloni
> Cc: Ludovic Desroches
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Thierry Reding
> Cc: Jonathan Hunter
> Cc: Jyri Sarha
> Cc: Tomi Valkeinen
> Cc: Rob Clark
> Cc: Sean Paul
> Cc: Brian Masney
> Cc: Emil Velikov
> Cc: zhengbin
> Cc: Thomas Gleixner
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-
> drivers/gpu/drm/arm/malidp_drv.c | 1 -
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-
> drivers/gpu/drm/drm_atomic_state_helper.c| 4
> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 --
> drivers/gpu/drm/tegra/dc.c | 1 -
> drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> drivers/gpu/drm/tidss/tidss_kms.c| 4
> 8 files changed, 9 insertions(+), 20 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 = >base;
> - crtc->state->crtc = crtc;
> - }
> + if (state)
> + __drm_atomic_helper_crtc_reset(crtc, >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, _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 c2507b7d8512..02904392e370 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -870,7 +870,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(>crtc);
It was a couple of years ago but I remember Alexandru-Cosmin tracking an issue
around
this. The reason for reseting the vblank so early after calling drm_vblank_init
was that
you could have userspace calling drm_wait_vblank() before crtc activation and
in our
case it will trigger some warnings in the driver (details are fuzzy right now).
The
relevant commit was cabce6343fd ("drm: mali-dp: Call
Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset
On Wed, May 27, 2020 at 11:53:32AM +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. > > Link: > https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb > Reported-by: Tetsuo Handa > Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com > Cc: Tetsuo Handa > Cc: "James (Qian) Wang" > Cc: Liviu Dudau > Cc: Mihail Atanassov > Cc: Brian Starkey > Cc: Sam Ravnborg > Cc: Boris Brezillon > Cc: Nicolas Ferre > Cc: Alexandre Belloni > Cc: Ludovic Desroches > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: Jyri Sarha > Cc: Tomi Valkeinen > Cc: Rob Clark > Cc: Sean Paul > Cc: Brian Masney > Cc: Emil Velikov > Cc: zhengbin > Cc: Thomas Gleixner > Cc: linux-te...@vger.kernel.org > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++- > drivers/gpu/drm/arm/malidp_drv.c | 1 - > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++- > drivers/gpu/drm/drm_atomic_state_helper.c| 4 > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 -- > drivers/gpu/drm/tegra/dc.c | 1 - > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +-- > drivers/gpu/drm/tidss/tidss_kms.c| 4 > 8 files changed, 9 insertions(+), 20 deletions(-) Looks good, and nice cleanup: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
