| Message ID | 1415006868-318-6-git-send-email-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 3, 2014 at 4:27 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > From: Thierry Reding <treding@nvidia.com> > > When the CRTC is enabled, make sure the VBLANK machinery is enabled. > Failure to do so will cause drm_vblank_get() to not enable the VBLANK on > the CRTC and VBLANK-synchronized page-flips won't work. > > While at it, get rid of the legacy drm_vblank_pre_modeset() and > drm_vblank_post_modeset() calls that are replaced by drm_vblank_on() > and drm_vblank_off(). > > Reported-by: Alexandre Courbot <acourbot@nvidia.com> > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/tegra/dc.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > index 4a015232e2e8..4da366a4d78a 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,8 +843,6 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, > u32 value; > int err; > > - drm_vblank_pre_modeset(crtc->dev, dc->pipe); > - Should you replace this with a call to drm_crtc_vblank_off in prepare()? AFAICT, crtc_funcs->disable() isn't guaranteed to be called before modeset, and it's unclear to me whether the vblank counter will be reset in prepare. Sean > err = tegra_crtc_setup_clk(crtc, mode); > if (err) { > dev_err(dc->dev, "failed to setup clock for CRTC: %d\n", err); > @@ -946,7 +943,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) > -- > 2.1.2 >
On Mon, Nov 03, 2014 at 01:45:56PM -0500, Sean Paul wrote: > On Mon, Nov 3, 2014 at 4:27 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > When the CRTC is enabled, make sure the VBLANK machinery is enabled. > > Failure to do so will cause drm_vblank_get() to not enable the VBLANK on > > the CRTC and VBLANK-synchronized page-flips won't work. > > > > While at it, get rid of the legacy drm_vblank_pre_modeset() and > > drm_vblank_post_modeset() calls that are replaced by drm_vblank_on() > > and drm_vblank_off(). > > > > Reported-by: Alexandre Courbot <acourbot@nvidia.com> > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/gpu/drm/tegra/dc.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > > index 4a015232e2e8..4da366a4d78a 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,8 +843,6 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, > > u32 value; > > int err; > > > > - drm_vblank_pre_modeset(crtc->dev, dc->pipe); > > - > > Should you replace this with a call to drm_crtc_vblank_off in > prepare()? AFAICT, crtc_funcs->disable() isn't guaranteed to be called > before modeset, and it's unclear to me whether the vblank counter will > be reset in prepare. ->disable is only called when fully disabling the crtc and should be implemented in terms of ->prepare and additional release any crtc related resources acquire. Examples include unpinning the framebuffer or releasing shared resources like dplls. Shutting down the vblank logic should definitely happen in the prepare logic, which should be shared with the dpms off logic. /me has looked a few too many times too closely at the crtc helpers ;-) Aside if you'll read the atomic helpers that should be a lot clearer - the corresponding code even explains what exact callback is called under which conditions. And it's all in one place thanks to the less insane calling sequence compared to crtc helpers. Hint, hint, ... Daniel
On 11/03/2014 06:27 PM, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > When the CRTC is enabled, make sure the VBLANK machinery is enabled. > Failure to do so will cause drm_vblank_get() to not enable the VBLANK on > the CRTC and VBLANK-synchronized page-flips won't work. > > While at it, get rid of the legacy drm_vblank_pre_modeset() and > drm_vblank_post_modeset() calls that are replaced by drm_vblank_on() > and drm_vblank_off(). > > Reported-by: Alexandre Courbot <acourbot@nvidia.com> > Signed-off-by: Thierry Reding <treding@nvidia.com> Tested-by: Alexandre Courbot <acourbot@nvidia.com> I also think this patch should go for 3.18 since even kmscube won't work without it.
On Mon, Nov 03, 2014 at 01:45:56PM -0500, Sean Paul wrote: > On Mon, Nov 3, 2014 at 4:27 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > When the CRTC is enabled, make sure the VBLANK machinery is enabled. > > Failure to do so will cause drm_vblank_get() to not enable the VBLANK on > > the CRTC and VBLANK-synchronized page-flips won't work. > > > > While at it, get rid of the legacy drm_vblank_pre_modeset() and > > drm_vblank_post_modeset() calls that are replaced by drm_vblank_on() > > and drm_vblank_off(). > > > > Reported-by: Alexandre Courbot <acourbot@nvidia.com> > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/gpu/drm/tegra/dc.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > > index 4a015232e2e8..4da366a4d78a 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,8 +843,6 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, > > u32 value; > > int err; > > > > - drm_vblank_pre_modeset(crtc->dev, dc->pipe); > > - > > Should you replace this with a call to drm_crtc_vblank_off in > prepare()? AFAICT, crtc_funcs->disable() isn't guaranteed to be called > before modeset, and it's unclear to me whether the vblank counter will > be reset in prepare. Done. Thanks, Thierry
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 4a015232e2e8..4da366a4d78a 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,8 +843,6 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, u32 value; int err; - drm_vblank_pre_modeset(crtc->dev, dc->pipe); - err = tegra_crtc_setup_clk(crtc, mode); if (err) { dev_err(dc->dev, "failed to setup clock for CRTC: %d\n", err); @@ -946,7 +943,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)