On Wed, May 21, 2014 at 01:32:35PM +0200, Thierry Reding wrote: > On Wed, May 14, 2014 at 08:51:06PM +0200, Daniel Vetter wrote: > > From: Ville Syrj?l? <ville.syrjala at linux.intel.com> > > > > drm_vblank_off() will turn off vblank interrupts, but as long as the > > refcount is elevated drm_vblank_get() will not re-enable them. This > > is a problem is someone is holding a vblank reference while a modeset is > > happening, and the driver requires vblank interrupt to work during that > > time. > > > > Add drm_vblank_on() as a counterpart to drm_vblank_off() which will > > re-enabled vblank interrupts if the refcount is already elevated. This > > will allow drivers to choose the specific places in the modeset sequence > > at which vblank interrupts get disabled and enabled. > > > > Testcase: igt/kms_flip/*-vs-suspend > > Signed-off-by: Ville Syrj?l? <ville.syrjala at linux.intel.com> > > [danvet: Add Testcase tag for the igt I've written.] > > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch> > > --- > > drivers/gpu/drm/drm_irq.c | 72 > > ++++++++++++++++++++++++++---------- > > drivers/gpu/drm/i915/intel_display.c | 8 ++++ > > include/drm/drmP.h | 1 + > > 3 files changed, 62 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > index 13d671ed3421..dd786d84daab 100644 > > --- a/drivers/gpu/drm/drm_irq.c > > +++ b/drivers/gpu/drm/drm_irq.c > > @@ -840,6 +840,41 @@ static void drm_update_vblank_count(struct drm_device > > *dev, int crtc) > > } > > > > /** > > + * drm_vblank_enable - enable the vblank interrupt on a CRTC > > + * @dev: DRM device > > + * @crtc: CRTC in question > > + */ > > Perhaps the kernel-doc here should contain some of what's described in > the commit message? Also a "Return:" section would be useful here to > specify what's an error and what isn't. > > > +static int drm_vblank_enable(struct drm_device *dev, int crtc) > > On second thought, since this is a local function, my comments above > apply to drm_vblank_on() below rather than drm_vblank_enable().
Yeah, we don't pull this into the kerneldoc, only functions exported to drivers. Follow-up patch should (hopefully) sufficiently pimp the kerneldoc for drm_vblank_on. If not please complain. > > > +{ > > + int ret = 0; > > + > > + assert_spin_locked(&dev->vbl_lock); > > + > > + spin_lock(&dev->vblank_time_lock); > > + > > + if (!dev->vblank[crtc].enabled) { > > + /* Enable vblank irqs under vblank_time_lock protection. > > + * All vblank count & timestamp updates are held off > > + * until we are done reinitializing master counter and > > + * timestamps. Filtercode in drm_handle_vblank() will > > + * prevent double-accounting of same vblank interval. > > + */ > > Coding style: > > /* > * Enable... > */ > > ? Yeah, will polish. > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > Perhaps split off the i915 changes into a separate patch? Yeah, should have. But I've got fed up with the conflicts this topic branch caused so pulled it in. I'll apply your review feedback as follow-up patches. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch