| Message ID | 20170524145212.27837-17-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On Wed, May 24, 2017 at 04:51:51PM +0200, Daniel Vetter wrote: > IRQs are properly shut down, so it almost works as race-free shutdown. > Except the irq is stopped after the vblank stuff, so boom anyway. > Proper way would be to call drm_atomic_helper_shutdown before any of > the kms things gets stopped. So no harm in removing the > drm_vblank_cleanup here really. Slightly confused on the implied message in the commit text: is "Proper way would be to call drm_atomic_helper_shutdown" a hint? A promise of a future patch? A message to the future us on how to fix things if they blow up? If calling drm_atomic_helper_shutdown() is the proper thing to do, why does the patch (and the series) avoids doing that? Lack of understanding of the driver's internal workings? Then I want to help, if I can understand the new direction. Best regards, Liviu > > Same story for both hdlcd and mali. > > v2: Move misplaced malidp hunk to this patch (Liviu). > > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Brian Starkey <brian.starkey@arm.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/arm/hdlcd_drv.c | 2 -- > drivers/gpu/drm/arm/malidp_drv.c | 2 -- > 2 files changed, 4 deletions(-) > > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c > index 0f49c4b12772..345c8357b273 100644 > --- a/drivers/gpu/drm/arm/hdlcd_drv.c > +++ b/drivers/gpu/drm/arm/hdlcd_drv.c > @@ -340,7 +340,6 @@ static int hdlcd_drm_bind(struct device *dev) > } > err_fbdev: > drm_kms_helper_poll_fini(drm); > - drm_vblank_cleanup(drm); > err_vblank: > pm_runtime_disable(drm->dev); > err_pm_active: > @@ -368,7 +367,6 @@ static void hdlcd_drm_unbind(struct device *dev) > } > drm_kms_helper_poll_fini(drm); > component_unbind_all(dev, drm); > - drm_vblank_cleanup(drm); > pm_runtime_get_sync(drm->dev); > drm_irq_uninstall(drm); > pm_runtime_put_sync(drm->dev); > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c > index 0d3eb537d08b..01b13d219917 100644 > --- a/drivers/gpu/drm/arm/malidp_drv.c > +++ b/drivers/gpu/drm/arm/malidp_drv.c > @@ -652,7 +652,6 @@ static int malidp_bind(struct device *dev) > drm_kms_helper_poll_fini(drm); > fbdev_fail: > pm_runtime_get_sync(dev); > - drm_vblank_cleanup(drm); > vblank_fail: > malidp_se_irq_fini(drm); > malidp_de_irq_fini(drm); > @@ -692,7 +691,6 @@ static void malidp_unbind(struct device *dev) > } > drm_kms_helper_poll_fini(drm); > pm_runtime_get_sync(dev); > - drm_vblank_cleanup(drm); > malidp_se_irq_fini(drm); > malidp_de_irq_fini(drm); > component_unbind_all(dev, drm); > -- > 2.11.0 >
On Wed, May 31, 2017 at 12:57 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > On Wed, May 24, 2017 at 04:51:51PM +0200, Daniel Vetter wrote: >> IRQs are properly shut down, so it almost works as race-free shutdown. >> Except the irq is stopped after the vblank stuff, so boom anyway. >> Proper way would be to call drm_atomic_helper_shutdown before any of >> the kms things gets stopped. So no harm in removing the >> drm_vblank_cleanup here really. > > Slightly confused on the implied message in the commit text: is "Proper way > would be to call drm_atomic_helper_shutdown" a hint? A promise of a future > patch? A message to the future us on how to fix things if they blow up? > > If calling drm_atomic_helper_shutdown() is the proper thing to do, why does the > patch (and the series) avoids doing that? Lack of understanding of the driver's > internal workings? Then I want to help, if I can understand the new direction. Yes, I wanted to not make things worse. If you look at the overall result (especially last patch) I'm also trying to better document stuff in the vblank area, but summarized, if you want to make sure that vblank processing has stopped, you need to call drm_vblank_off on each active crtc. The simplest way to get that is by using drm_atomic_helper_shutdown(). Calling drm_vblank_cleanup doesn't really do anything useful (see the last patch for the only valid usecase there ever was). -Daniel
On Wed, May 31, 2017 at 01:03:34PM +0200, Daniel Vetter wrote: > On Wed, May 31, 2017 at 12:57 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > > On Wed, May 24, 2017 at 04:51:51PM +0200, Daniel Vetter wrote: > >> IRQs are properly shut down, so it almost works as race-free shutdown. > >> Except the irq is stopped after the vblank stuff, so boom anyway. > >> Proper way would be to call drm_atomic_helper_shutdown before any of > >> the kms things gets stopped. So no harm in removing the > >> drm_vblank_cleanup here really. > > > > Slightly confused on the implied message in the commit text: is "Proper way > > would be to call drm_atomic_helper_shutdown" a hint? A promise of a future > > patch? A message to the future us on how to fix things if they blow up? > > > > If calling drm_atomic_helper_shutdown() is the proper thing to do, why does the > > patch (and the series) avoids doing that? Lack of understanding of the driver's > > internal workings? Then I want to help, if I can understand the new direction. > > Yes, I wanted to not make things worse. If you look at the overall > result (especially last patch) I'm also trying to better document > stuff in the vblank area, but summarized, if you want to make sure > that vblank processing has stopped, you need to call drm_vblank_off on > each active crtc. The simplest way to get that is by using > drm_atomic_helper_shutdown(). Calling drm_vblank_cleanup doesn't > really do anything useful (see the last patch for the only valid > usecase there ever was). OK, so there should be a follow up patch adding drm_atomic_helper_shutdown(). I guess I need to call dibs on it? :) Best regards, Liviu > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, May 24, 2017 at 04:51:51PM +0200, Daniel Vetter wrote: > IRQs are properly shut down, so it almost works as race-free shutdown. > Except the irq is stopped after the vblank stuff, so boom anyway. > Proper way would be to call drm_atomic_helper_shutdown before any of > the kms things gets stopped. So no harm in removing the > drm_vblank_cleanup here really. > > Same story for both hdlcd and mali. > > v2: Move misplaced malidp hunk to this patch (Liviu). > > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Brian Starkey <brian.starkey@arm.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> On the assumption that a subsequent patch will fix the issue highlighted in the commit message (doesn't have to be part of this series): Acked-by: Liviu Dudau <liviu.dudau@arm.com> I'm assuming that you are going to carry this patch through one of your trees, so I will not pull it into mali-dp tree. Thanks, Liviu > --- > drivers/gpu/drm/arm/hdlcd_drv.c | 2 -- > drivers/gpu/drm/arm/malidp_drv.c | 2 -- > 2 files changed, 4 deletions(-) > > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c > index 0f49c4b12772..345c8357b273 100644 > --- a/drivers/gpu/drm/arm/hdlcd_drv.c > +++ b/drivers/gpu/drm/arm/hdlcd_drv.c > @@ -340,7 +340,6 @@ static int hdlcd_drm_bind(struct device *dev) > } > err_fbdev: > drm_kms_helper_poll_fini(drm); > - drm_vblank_cleanup(drm); > err_vblank: > pm_runtime_disable(drm->dev); > err_pm_active: > @@ -368,7 +367,6 @@ static void hdlcd_drm_unbind(struct device *dev) > } > drm_kms_helper_poll_fini(drm); > component_unbind_all(dev, drm); > - drm_vblank_cleanup(drm); > pm_runtime_get_sync(drm->dev); > drm_irq_uninstall(drm); > pm_runtime_put_sync(drm->dev); > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c > index 0d3eb537d08b..01b13d219917 100644 > --- a/drivers/gpu/drm/arm/malidp_drv.c > +++ b/drivers/gpu/drm/arm/malidp_drv.c > @@ -652,7 +652,6 @@ static int malidp_bind(struct device *dev) > drm_kms_helper_poll_fini(drm); > fbdev_fail: > pm_runtime_get_sync(dev); > - drm_vblank_cleanup(drm); > vblank_fail: > malidp_se_irq_fini(drm); > malidp_de_irq_fini(drm); > @@ -692,7 +691,6 @@ static void malidp_unbind(struct device *dev) > } > drm_kms_helper_poll_fini(drm); > pm_runtime_get_sync(dev); > - drm_vblank_cleanup(drm); > malidp_se_irq_fini(drm); > malidp_de_irq_fini(drm); > component_unbind_all(dev, drm); > -- > 2.11.0 >
On Wed, May 31, 2017 at 1:22 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > On Wed, May 31, 2017 at 01:03:34PM +0200, Daniel Vetter wrote: >> On Wed, May 31, 2017 at 12:57 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: >> > On Wed, May 24, 2017 at 04:51:51PM +0200, Daniel Vetter wrote: >> >> IRQs are properly shut down, so it almost works as race-free shutdown. >> >> Except the irq is stopped after the vblank stuff, so boom anyway. >> >> Proper way would be to call drm_atomic_helper_shutdown before any of >> >> the kms things gets stopped. So no harm in removing the >> >> drm_vblank_cleanup here really. >> > >> > Slightly confused on the implied message in the commit text: is "Proper way >> > would be to call drm_atomic_helper_shutdown" a hint? A promise of a future >> > patch? A message to the future us on how to fix things if they blow up? >> > >> > If calling drm_atomic_helper_shutdown() is the proper thing to do, why does the >> > patch (and the series) avoids doing that? Lack of understanding of the driver's >> > internal workings? Then I want to help, if I can understand the new direction. >> >> Yes, I wanted to not make things worse. If you look at the overall >> result (especially last patch) I'm also trying to better document >> stuff in the vblank area, but summarized, if you want to make sure >> that vblank processing has stopped, you need to call drm_vblank_off on >> each active crtc. The simplest way to get that is by using >> drm_atomic_helper_shutdown(). Calling drm_vblank_cleanup doesn't >> really do anything useful (see the last patch for the only valid >> usecase there ever was). > > OK, so there should be a follow up patch adding drm_atomic_helper_shutdown(). I guess > I need to call dibs on it? :) I googled what "to call dibs on smth" means and still can't figure out what you mean here. Can you pls explain? Thanks, Daniel
On Wed, May 31, 2017 at 06:41:05PM +0200, Daniel Vetter wrote: > On Wed, May 31, 2017 at 1:22 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > > On Wed, May 31, 2017 at 01:03:34PM +0200, Daniel Vetter wrote: > >> On Wed, May 31, 2017 at 12:57 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > >> > On Wed, May 24, 2017 at 04:51:51PM +0200, Daniel Vetter wrote: > >> >> IRQs are properly shut down, so it almost works as race-free shutdown. > >> >> Except the irq is stopped after the vblank stuff, so boom anyway. > >> >> Proper way would be to call drm_atomic_helper_shutdown before any of > >> >> the kms things gets stopped. So no harm in removing the > >> >> drm_vblank_cleanup here really. > >> > > >> > Slightly confused on the implied message in the commit text: is "Proper way > >> > would be to call drm_atomic_helper_shutdown" a hint? A promise of a future > >> > patch? A message to the future us on how to fix things if they blow up? > >> > > >> > If calling drm_atomic_helper_shutdown() is the proper thing to do, why does the > >> > patch (and the series) avoids doing that? Lack of understanding of the driver's > >> > internal workings? Then I want to help, if I can understand the new direction. > >> > >> Yes, I wanted to not make things worse. If you look at the overall > >> result (especially last patch) I'm also trying to better document > >> stuff in the vblank area, but summarized, if you want to make sure > >> that vblank processing has stopped, you need to call drm_vblank_off on > >> each active crtc. The simplest way to get that is by using > >> drm_atomic_helper_shutdown(). Calling drm_vblank_cleanup doesn't > >> really do anything useful (see the last patch for the only valid > >> usecase there ever was). > > > > OK, so there should be a follow up patch adding drm_atomic_helper_shutdown(). I guess > > I need to call dibs on it? :) > > I googled what "to call dibs on smth" means and still can't figure out > what you mean here. Can you pls explain? Means I put my name on it. Urban dictionary on "call dibs" gives it a very gender specific usage, my intent was to convey the message that I will take it on my TODO list. Best regards, Liviu > > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, May 31, 2017 at 05:57:07PM +0100, Liviu Dudau wrote: > On Wed, May 31, 2017 at 06:41:05PM +0200, Daniel Vetter wrote: > > On Wed, May 31, 2017 at 1:22 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > > > On Wed, May 31, 2017 at 01:03:34PM +0200, Daniel Vetter wrote: > > >> On Wed, May 31, 2017 at 12:57 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > > >> > On Wed, May 24, 2017 at 04:51:51PM +0200, Daniel Vetter wrote: > > >> >> IRQs are properly shut down, so it almost works as race-free shutdown. > > >> >> Except the irq is stopped after the vblank stuff, so boom anyway. > > >> >> Proper way would be to call drm_atomic_helper_shutdown before any of > > >> >> the kms things gets stopped. So no harm in removing the > > >> >> drm_vblank_cleanup here really. > > >> > > > >> > Slightly confused on the implied message in the commit text: is "Proper way > > >> > would be to call drm_atomic_helper_shutdown" a hint? A promise of a future > > >> > patch? A message to the future us on how to fix things if they blow up? > > >> > > > >> > If calling drm_atomic_helper_shutdown() is the proper thing to do, why does the > > >> > patch (and the series) avoids doing that? Lack of understanding of the driver's > > >> > internal workings? Then I want to help, if I can understand the new direction. > > >> > > >> Yes, I wanted to not make things worse. If you look at the overall > > >> result (especially last patch) I'm also trying to better document > > >> stuff in the vblank area, but summarized, if you want to make sure > > >> that vblank processing has stopped, you need to call drm_vblank_off on > > >> each active crtc. The simplest way to get that is by using > > >> drm_atomic_helper_shutdown(). Calling drm_vblank_cleanup doesn't > > >> really do anything useful (see the last patch for the only valid > > >> usecase there ever was). > > > > > > OK, so there should be a follow up patch adding drm_atomic_helper_shutdown(). I guess > > > I need to call dibs on it? :) > > > > I googled what "to call dibs on smth" means and still can't figure out > > what you mean here. Can you pls explain? > > Means I put my name on it. Urban dictionary on "call dibs" gives it a very gender > specific usage, my intent was to convey the message that I will take it on my TODO list. Ah ok, thanks for explaining. I was indeed thrown off be the ub entry ... -Daniel
On Wed, May 31, 2017 at 05:37:34PM +0100, Liviu Dudau wrote: > On Wed, May 24, 2017 at 04:51:51PM +0200, Daniel Vetter wrote: > > IRQs are properly shut down, so it almost works as race-free shutdown. > > Except the irq is stopped after the vblank stuff, so boom anyway. > > Proper way would be to call drm_atomic_helper_shutdown before any of > > the kms things gets stopped. So no harm in removing the > > drm_vblank_cleanup here really. > > > > Same story for both hdlcd and mali. > > > > v2: Move misplaced malidp hunk to this patch (Liviu). > > > > Cc: Liviu Dudau <liviu.dudau@arm.com> > > Cc: Brian Starkey <brian.starkey@arm.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > On the assumption that a subsequent patch will fix the issue highlighted in > the commit message (doesn't have to be part of this series): > > Acked-by: Liviu Dudau <liviu.dudau@arm.com> > > I'm assuming that you are going to carry this patch through one of your trees, > so I will not pull it into mali-dp tree. Ok, applied to drm-misc-next, thanks for checking things. -Daniel
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c index 0f49c4b12772..345c8357b273 100644 --- a/drivers/gpu/drm/arm/hdlcd_drv.c +++ b/drivers/gpu/drm/arm/hdlcd_drv.c @@ -340,7 +340,6 @@ static int hdlcd_drm_bind(struct device *dev) } err_fbdev: drm_kms_helper_poll_fini(drm); - drm_vblank_cleanup(drm); err_vblank: pm_runtime_disable(drm->dev); err_pm_active: @@ -368,7 +367,6 @@ static void hdlcd_drm_unbind(struct device *dev) } drm_kms_helper_poll_fini(drm); component_unbind_all(dev, drm); - drm_vblank_cleanup(drm); pm_runtime_get_sync(drm->dev); drm_irq_uninstall(drm); pm_runtime_put_sync(drm->dev); diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 0d3eb537d08b..01b13d219917 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -652,7 +652,6 @@ static int malidp_bind(struct device *dev) drm_kms_helper_poll_fini(drm); fbdev_fail: pm_runtime_get_sync(dev); - drm_vblank_cleanup(drm); vblank_fail: malidp_se_irq_fini(drm); malidp_de_irq_fini(drm); @@ -692,7 +691,6 @@ static void malidp_unbind(struct device *dev) } drm_kms_helper_poll_fini(drm); pm_runtime_get_sync(dev); - drm_vblank_cleanup(drm); malidp_se_irq_fini(drm); malidp_de_irq_fini(drm); component_unbind_all(dev, drm);
IRQs are properly shut down, so it almost works as race-free shutdown. Except the irq is stopped after the vblank stuff, so boom anyway. Proper way would be to call drm_atomic_helper_shutdown before any of the kms things gets stopped. So no harm in removing the drm_vblank_cleanup here really. Same story for both hdlcd and mali. v2: Move misplaced malidp hunk to this patch (Liviu). Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Brian Starkey <brian.starkey@arm.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/arm/hdlcd_drv.c | 2 -- drivers/gpu/drm/arm/malidp_drv.c | 2 -- 2 files changed, 4 deletions(-)