| Message ID | 54362066.9000504@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)