| Message ID | 542D13CC.5000304@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2014? 10? 02? 17:58, Joonyoung Shim wrote: > Hi Andrzej, > > On 10/01/2014 05:14 PM, Andrzej Hajda wrote: >> The patch disables vblanks during dpms off only if pagefilp has >> not been finished. It also replaces drm_vblank_off with drm_crtc_vblank_put. >> It fixes issue with page_flip ioctl not being able to acquire vblank counter. > > This problem isn't related with pageflip, it just causes from > 7ffd7a68511c710b84db3548a1997fd2625f580a commit (drm: Always reject > drm_vblank_get() after drm_vblank_off()). > > We need to use drm_vblank_on() as a counterpart to drm_vblank_off() > after the commit . > > How about below patch? Thanks you Joonyoung and Andrzej, drm_vblank_on/off() are legacy api so it would be better to use drm_vblank_crtc_on/off functions instead. And drm_vblank_crtc_off() makes sure that the latest vblank frame count is stored and restored by drm_vblank_crtc_on() again. So my opinion is, static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode) { [snip] if (mode > DRM_MODE_DPMS_ON) { /* wait for the completion of page flip. */ if (!wait_event_timeout(exynos_crtc->pending_flip_queue, !atomic_read(&exynos_crtc->pending_flip), HZ/20)) atomic_set(&exynos_crtc->pending_flip, 0); drm_crtc_vblank_off(crtc); //<- store the latest vblank frame count. } else { drm_crtc_vblank_on(crtc); //<- restore the vblank frame count. } [snip] } Tested and worked well with above patch. How about it? Thanks, Inki Dae > >>From 6de01473746af225c688ee430123001d57d9af2a Mon Sep 17 00:00:00 2001 > From: Joonyoung Shim <jy0922.shim@samsung.com> > Date: Thu, 2 Oct 2014 17:48:27 +0900 > Subject: [PATCH] drm/exynos: use drm_vblank_on() > > We need to use drm_vblank_on() as a counterpart to drm_vblank_off() > after the commit 7ffd7a68511c ("drm: Always reject drm_vblank_get() > after drm_vblank_off()"). If not, drm_vblank_get() will be failed > always after drm_vblank_off(). > > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > index 8e38e9f..dfa209a 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > @@ -71,7 +71,6 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode) > !atomic_read(&exynos_crtc->pending_flip), > HZ/20)) > atomic_set(&exynos_crtc->pending_flip, 0); > - drm_vblank_off(crtc->dev, exynos_crtc->pipe); > } > > if (manager->ops->dpms) > @@ -90,6 +89,7 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc) > struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); > struct exynos_drm_manager *manager = exynos_crtc->manager; > > + drm_vblank_on(crtc->dev, exynos_crtc->pipe); > exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON); > > exynos_plane_commit(crtc->primary); > @@ -177,10 +177,12 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, > > static void exynos_drm_crtc_disable(struct drm_crtc *crtc) > { > + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); > struct drm_plane *plane; > int ret; > > exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF); > + drm_vblank_off(crtc->dev, exynos_crtc->pipe); > > drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) { > if (plane->crtc != crtc) >
Hi, +CC possible victims On 10/02/2014 12:52 PM, Inki Dae wrote: > On 2014? 10? 02? 17:58, Joonyoung Shim wrote: >> Hi Andrzej, >> >> On 10/01/2014 05:14 PM, Andrzej Hajda wrote: >>> The patch disables vblanks during dpms off only if pagefilp has >>> not been finished. It also replaces drm_vblank_off with drm_crtc_vblank_put. >>> It fixes issue with page_flip ioctl not being able to acquire vblank counter. >> This problem isn't related with pageflip, it just causes from >> 7ffd7a68511c710b84db3548a1997fd2625f580a commit (drm: Always reject >> drm_vblank_get() after drm_vblank_off()). >> >> We need to use drm_vblank_on() as a counterpart to drm_vblank_off() >> after the commit . This patch should break also other drivers, it seems at least following drms could be affected: armada, sti, tegra. I guess after disabling and re-enabling crtc vblanks should stop working, unless there are other bugs. Regards Andrzej
On Thu, Oct 09, 2014 at 02:43:02PM +0900, Alexandre Courbot wrote: > But there might be another issue, which is that calls to > drm_vblank_get() will return -EINVAL if invoked between drm_blank_off > and drm_blank_on. Is this really the desired behavior? Can it at least > happen? If so, how are drivers supposed to react to this situation? I've not yet seen the commit which causes this problem, but I hope that drm_wait_vblank() isn't affected by this. In current mainline, drm_vblank_get() is used inside drm_wait_vblank(), which is called as a result of userspace calling DRM_IOCTL_WAIT_VBLANK. So, what is the effect of this change on user applications making use of the vblank wait ioctl - and is that change intended?
On Thu, Oct 09, 2014 at 09:52:58AM +0100, Russell King - ARM Linux wrote: > On Thu, Oct 09, 2014 at 02:43:02PM +0900, Alexandre Courbot wrote: > > But there might be another issue, which is that calls to > > drm_vblank_get() will return -EINVAL if invoked between drm_blank_off > > and drm_blank_on. Is this really the desired behavior? Can it at least > > happen? If so, how are drivers supposed to react to this situation? > > I've not yet seen the commit which causes this problem, but I hope > that drm_wait_vblank() isn't affected by this. In current mainline, > drm_vblank_get() is used inside drm_wait_vblank(), which is called as > a result of userspace calling DRM_IOCTL_WAIT_VBLANK. > > So, what is the effect of this change on user applications making use > of the vblank wait ioctl - and is that change intended? There's no effect on user applications if the driver behaves properly. As far as I can tell, every driver that calls drm_vblank_off() but not drm_vblank_on() will break. You can easily test this by running libdrm modetest -s ... -v, which instead of toggling between the test pattern and an all-gray framebuffer will switch to the gray one once and then hang. I guess that was probably not intended, but according to the new rules all these drivers have now become buggy. So before merging this patch I think we need to fix existing drivers to avoid regressions. Thierry
On Thu, Oct 09, 2014 at 02:43:02PM +0900, Alexandre Courbot wrote: > On 10/02/2014 08:52 PM, Andrzej Hajda wrote: > >Hi, > > > >+CC possible victims > > > >On 10/02/2014 12:52 PM, Inki Dae wrote: > >>On 2014? 10? 02? 17:58, Joonyoung Shim wrote: > >>>Hi Andrzej, > >>> > >>>On 10/01/2014 05:14 PM, Andrzej Hajda wrote: > >>>>The patch disables vblanks during dpms off only if pagefilp has > >>>>not been finished. It also replaces drm_vblank_off with drm_crtc_vblank_put. > >>>>It fixes issue with page_flip ioctl not being able to acquire vblank counter. > >>>This problem isn't related with pageflip, it just causes from > >>>7ffd7a68511c710b84db3548a1997fd2625f580a commit (drm: Always reject > >>>drm_vblank_get() after drm_vblank_off()). > >>> > >>>We need to use drm_vblank_on() as a counterpart to drm_vblank_off() > >>>after the commit . > > > >This patch should break also other drivers, it seems at least following > >drms could be affected: > >armada, sti, tegra. > > Indeed we (tegra) have just been hit by this. The problem seems to come from > the fact that we have been using drm_vblank_pre_modeset, > drm_vblank_post_modeset and drm_vblank_off conjointly. All these functions > depend on the value of vblank->inmodeset, and 7ffd7a68511 increases the > vblank reference counter only in drm_vblank_off, which can result in the > acquired reference never being released. > > The following seems to fix this for Tegra, by stopping using > drm_vblank_pre/post_modeset and relying on drm_vblank_off/on instead: > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > index b08df07cad47..3955d81236d0 100644 > --- a/drivers/gpu/drm/tegra/dc.c > +++ b/drivers/gpu/drm/tegra/dc.c > @@ -739,7 +739,6 @@ static const struct drm_crtc_funcs tegra_crtc_funcs = { > > static void tegra_crtc_disable(struct drm_crtc *crtc) > { > - struct tegra_dc *dc = to_tegra_dc(crtc); > struct drm_device *drm = crtc->dev; > struct drm_plane *plane; > > @@ -755,7 +754,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc) > } > } > > - drm_vblank_off(drm, dc->pipe); > + drm_crtc_vblank_off(crtc); > } > > static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc, > @@ -844,7 +843,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, > u32 value; > int err; > > - drm_vblank_pre_modeset(crtc->dev, dc->pipe); > + drm_crtc_vblank_off(crtc); > > err = tegra_crtc_setup_clk(crtc, mode); > if (err) { > @@ -946,7 +945,7 @@ static void tegra_crtc_commit(struct drm_crtc *crtc) > value = GENERAL_ACT_REQ | WIN_A_ACT_REQ; > tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL); > > - drm_vblank_post_modeset(crtc->dev, dc->pipe); > + drm_crtc_vblank_on(crtc); > } > > static void tegra_crtc_load_lut(struct drm_crtc *crtc) > > Thierry, does this look ok to you? Yes, that looks like almost the same patch that I sent out yesterday. The difference is that I didn't replace the drm_vblank_pre_modeset() call with drm_vblank_off() like you did, but rather just dropped the former. I /think/ your version is more correct in that regard. Thierry > But there might be another issue, which is that calls to drm_vblank_get() > will return -EINVAL if invoked between drm_blank_off and drm_blank_on. Is > this really the desired behavior? Can it at least happen? If so, how are > drivers supposed to react to this situation? It shouldn't happen. If drm_vblank_off() and drm_vblank_on() are called around a modeset they should never conflict with drm_vblank_get(), at least on Tegra, because the modeset and page-flip IOCTLs will be serialized. Thierry
On Thu, Oct 9, 2014 at 7:08 PM, Thierry Reding <treding@nvidia.com> wrote: > On Thu, Oct 09, 2014 at 02:43:02PM +0900, Alexandre Courbot wrote: >> On 10/02/2014 08:52 PM, Andrzej Hajda wrote: >> >Hi, >> > >> >+CC possible victims >> > >> >On 10/02/2014 12:52 PM, Inki Dae wrote: >> >>On 2014? 10? 02? 17:58, Joonyoung Shim wrote: >> >>>Hi Andrzej, >> >>> >> >>>On 10/01/2014 05:14 PM, Andrzej Hajda wrote: >> >>>>The patch disables vblanks during dpms off only if pagefilp has >> >>>>not been finished. It also replaces drm_vblank_off with drm_crtc_vblank_put. >> >>>>It fixes issue with page_flip ioctl not being able to acquire vblank counter. >> >>>This problem isn't related with pageflip, it just causes from >> >>>7ffd7a68511c710b84db3548a1997fd2625f580a commit (drm: Always reject >> >>>drm_vblank_get() after drm_vblank_off()). >> >>> >> >>>We need to use drm_vblank_on() as a counterpart to drm_vblank_off() >> >>>after the commit . >> > >> >This patch should break also other drivers, it seems at least following >> >drms could be affected: >> >armada, sti, tegra. >> >> Indeed we (tegra) have just been hit by this. The problem seems to come from >> the fact that we have been using drm_vblank_pre_modeset, >> drm_vblank_post_modeset and drm_vblank_off conjointly. All these functions >> depend on the value of vblank->inmodeset, and 7ffd7a68511 increases the >> vblank reference counter only in drm_vblank_off, which can result in the >> acquired reference never being released. >> >> The following seems to fix this for Tegra, by stopping using >> drm_vblank_pre/post_modeset and relying on drm_vblank_off/on instead: >> >> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c >> index b08df07cad47..3955d81236d0 100644 >> --- a/drivers/gpu/drm/tegra/dc.c >> +++ b/drivers/gpu/drm/tegra/dc.c >> @@ -739,7 +739,6 @@ static const struct drm_crtc_funcs tegra_crtc_funcs = { >> >> static void tegra_crtc_disable(struct drm_crtc *crtc) >> { >> - struct tegra_dc *dc = to_tegra_dc(crtc); >> struct drm_device *drm = crtc->dev; >> struct drm_plane *plane; >> >> @@ -755,7 +754,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc) >> } >> } >> >> - drm_vblank_off(drm, dc->pipe); >> + drm_crtc_vblank_off(crtc); >> } >> >> static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc, >> @@ -844,7 +843,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, >> u32 value; >> int err; >> >> - drm_vblank_pre_modeset(crtc->dev, dc->pipe); >> + drm_crtc_vblank_off(crtc); >> >> err = tegra_crtc_setup_clk(crtc, mode); >> if (err) { >> @@ -946,7 +945,7 @@ static void tegra_crtc_commit(struct drm_crtc *crtc) >> value = GENERAL_ACT_REQ | WIN_A_ACT_REQ; >> tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL); >> >> - drm_vblank_post_modeset(crtc->dev, dc->pipe); >> + drm_crtc_vblank_on(crtc); >> } >> >> static void tegra_crtc_load_lut(struct drm_crtc *crtc) >> >> Thierry, does this look ok to you? > > Yes, that looks like almost the same patch that I sent out yesterday. > The difference is that I didn't replace the drm_vblank_pre_modeset() > call with drm_vblank_off() like you did, but rather just dropped the > former. > > I /think/ your version is more correct in that regard. Feel free to take that extra line in your patch then. ;) > > Thierry > >> But there might be another issue, which is that calls to drm_vblank_get() >> will return -EINVAL if invoked between drm_blank_off and drm_blank_on. Is >> this really the desired behavior? Can it at least happen? If so, how are >> drivers supposed to react to this situation? > > It shouldn't happen. If drm_vblank_off() and drm_vblank_on() are called > around a modeset they should never conflict with drm_vblank_get(), at > least on Tegra, because the modeset and page-flip IOCTLs will be > serialized. Ok, that's good. I was just wondering whether this case has been thought of. Actually, and seeing how drm_vblank_pre/post_modeset() have become useless (if not harmful), maybe it would be a good idea to come with a fixup patch that gets rid of them altogether and make their callers invoke drm_vblank_off/on() instead?
On 10/02/2014 12:52 PM, Inki Dae wrote: > On 2014? 10? 02? 17:58, Joonyoung Shim wrote: >> Hi Andrzej, >> >> On 10/01/2014 05:14 PM, Andrzej Hajda wrote: >>> The patch disables vblanks during dpms off only if pagefilp has >>> not been finished. It also replaces drm_vblank_off with drm_crtc_vblank_put. >>> It fixes issue with page_flip ioctl not being able to acquire vblank counter. >> This problem isn't related with pageflip, it just causes from >> 7ffd7a68511c710b84db3548a1997fd2625f580a commit (drm: Always reject >> drm_vblank_get() after drm_vblank_off()). >> >> We need to use drm_vblank_on() as a counterpart to drm_vblank_off() >> after the commit . >> >> How about below patch? > Thanks you Joonyoung and Andrzej, > > drm_vblank_on/off() are legacy api so it would be better to use > drm_vblank_crtc_on/off functions instead. > > And drm_vblank_crtc_off() makes sure that the latest vblank frame count > is stored and restored by drm_vblank_crtc_on() again. So my opinion is, > > static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode) > { > [snip] > > if (mode > DRM_MODE_DPMS_ON) { > /* wait for the completion of page flip. */ > if (!wait_event_timeout(exynos_crtc->pending_flip_queue, > !atomic_read(&exynos_crtc->pending_flip), > HZ/20)) > atomic_set(&exynos_crtc->pending_flip, 0); > drm_crtc_vblank_off(crtc); //<- store the latest vblank frame count. > } else { > drm_crtc_vblank_on(crtc); //<- restore the vblank frame count. > } > > [snip] > } > > > Tested and worked well with above patch. How about it? > > drm_crtc_vblank_on should be called after dpms on, otherwise it can fail enabling vblank. I have provided full explanation in my other email [1]. You can modify your patch or just use the one provided in [1]. [1]: http://permalink.gmane.org/gmane.comp.video.dri.devel/116152 Regards Andrzej
On 2014? 10? 10? 21:39, Andrzej Hajda wrote: > On 10/02/2014 12:52 PM, Inki Dae wrote: >> On 2014? 10? 02? 17:58, Joonyoung Shim wrote: >>> Hi Andrzej, >>> >>> On 10/01/2014 05:14 PM, Andrzej Hajda wrote: >>>> The patch disables vblanks during dpms off only if pagefilp has >>>> not been finished. It also replaces drm_vblank_off with drm_crtc_vblank_put. >>>> It fixes issue with page_flip ioctl not being able to acquire vblank counter. >>> This problem isn't related with pageflip, it just causes from >>> 7ffd7a68511c710b84db3548a1997fd2625f580a commit (drm: Always reject >>> drm_vblank_get() after drm_vblank_off()). >>> >>> We need to use drm_vblank_on() as a counterpart to drm_vblank_off() >>> after the commit . >>> >>> How about below patch? >> Thanks you Joonyoung and Andrzej, >> >> drm_vblank_on/off() are legacy api so it would be better to use >> drm_vblank_crtc_on/off functions instead. >> >> And drm_vblank_crtc_off() makes sure that the latest vblank frame count >> is stored and restored by drm_vblank_crtc_on() again. So my opinion is, >> >> static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode) >> { >> [snip] >> >> if (mode > DRM_MODE_DPMS_ON) { >> /* wait for the completion of page flip. */ >> if (!wait_event_timeout(exynos_crtc->pending_flip_queue, >> !atomic_read(&exynos_crtc->pending_flip), >> HZ/20)) >> atomic_set(&exynos_crtc->pending_flip, 0); >> drm_crtc_vblank_off(crtc); //<- store the latest vblank frame count. >> } else { >> drm_crtc_vblank_on(crtc); //<- restore the vblank frame count. >> } >> >> [snip] >> } >> >> >> Tested and worked well with above patch. How about it? >> >> > > drm_crtc_vblank_on should be called after dpms on, otherwise it can fail enabling vblank. I have provided > full explanation in my other email [1]. > You can modify your patch or just use the one provided in [1]. I will just merge your patch set after review and test. :) Thanks, Inki Dae > > [1]: http://permalink.gmane.org/gmane.comp.video.dri.devel/116152 > > Regards > Andrzej > >
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 8e38e9f..dfa209a 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -71,7 +71,6 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode) !atomic_read(&exynos_crtc->pending_flip), HZ/20)) atomic_set(&exynos_crtc->pending_flip, 0); - drm_vblank_off(crtc->dev, exynos_crtc->pipe); } if (manager->ops->dpms) @@ -90,6 +89,7 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc) struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); struct exynos_drm_manager *manager = exynos_crtc->manager; + drm_vblank_on(crtc->dev, exynos_crtc->pipe); exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON); exynos_plane_commit(crtc->primary); @@ -177,10 +177,12 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, static void exynos_drm_crtc_disable(struct drm_crtc *crtc) { + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); struct drm_plane *plane; int ret; exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF); + drm_vblank_off(crtc->dev, exynos_crtc->pipe); drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) { if (plane->crtc != crtc)