Re: [PATCH 06/17] drm/tegra: dc: Add missing call to drm_vblank_on()

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel