On Fri, Feb 21, 2014 at 09:03:33PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Allow the driver to specify whether all new vblank requests after
> drm_vblank_off() should be rejected. And add a counterpart called
> drm_vblank_on() which will again allow vblank requests to come in.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Not really happy about this - drm_irq.c is already a giant mess, adding
more driver-specific hacks doesn't help. I think we need a few bits of
polish on top of your code:
- Add stern warnings to pre/post_modeset that they're inherently racy.
- Add calls to drm_vblank_off to every driver lacking them. Put it at the
beginning of their crtc disable functions expect when there's a call to
pre_modeset. Then it should be right after that.
- Sprinkle calls to drm_vblank_on over all drivers. Put them at the end of
their crtc enable functions except when there's a call to post_modeset.
Then put it right before that.
- Rip out the reject argument again and make it the default. If we have
drm_vblank_off everywhere then all old vblank waits should complete and
there's no userspace yet out there which races a modeset with vblank
ioctl calls. Then only issue would be userspace which does vblank waits
on disabled ioctls which a) is buggy b) we can easily fix with a driver
quirk flag if _really_ needed.
This way the drm_irq.c mess will at least converge a bit and so should
help generic display servers (like Wayland) a lot.
I can volunteer for this if you want to punt on it yourself.
-Daniel
> ---
> drivers/gpu/drm/armada/armada_crtc.c | 2 +-
> drivers/gpu/drm/drm_irq.c | 29 ++++++++++++++++++++++++++++-
> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 +-
> drivers/gpu/drm/gma500/gma_display.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 6 +++---
> drivers/gpu/drm/tegra/dc.c | 2 +-
> include/drm/drmP.h | 4 +++-
> 7 files changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> index d8e3982..74317b2 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -257,7 +257,7 @@ static void armada_drm_vblank_off(struct armada_crtc *dcrtc)
> * Tell the DRM core that vblank IRQs aren't going to happen for
> * a while. This cleans up any pending vblank events for us.
> */
> - drm_vblank_off(dev, dcrtc->num);
> + drm_vblank_off(dev, dcrtc->num, false);
>
> /* Handle any pending flip event. */
> spin_lock_irq(&dev->event_lock);
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 3211158..6e5d820 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -890,6 +890,12 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
> int ret = 0;
>
> spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +
> + if (dev->vblank[crtc].reject) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> /* Going from 0->1 means we have to enable interrupts again */
> if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
> spin_lock(&dev->vblank_time_lock);
> @@ -917,6 +923,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
> ret = -EINVAL;
> }
> }
> +
> + out:
> spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>
> return ret;
> @@ -947,8 +955,9 @@ EXPORT_SYMBOL(drm_vblank_put);
> * drm_vblank_off - disable vblank events on a CRTC
> * @dev: DRM device
> * @crtc: CRTC in question
> + * @reject: reject drm_vblank_get() until drm_vblank_on() has been called?
> */
> -void drm_vblank_off(struct drm_device *dev, int crtc)
> +void drm_vblank_off(struct drm_device *dev, int crtc, bool reject)
> {
> struct drm_pending_vblank_event *e, *t;
> struct timeval now;
> @@ -956,6 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
> unsigned int seq;
>
> spin_lock_irqsave(&dev->vbl_lock, irqflags);
> + dev->vblank[crtc].reject = reject;
> vblank_disable_and_save(dev, crtc);
> wake_up(&dev->vblank[crtc].queue);
>
> @@ -979,6 +989,22 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
> }
> EXPORT_SYMBOL(drm_vblank_off);
>
> +
> +/**
> + * drm_vblank_on - enable vblank events on a CRTC
> + * @dev: DRM device
> + * @crtc: CRTC in question
> + */
> +void drm_vblank_on(struct drm_device *dev, int crtc)
> +{
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(&dev->vbl_lock, irqflags);
> + dev->vblank[crtc].reject = false;
> + spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +}
> +EXPORT_SYMBOL(drm_vblank_on);
> +
> /**
> * drm_vblank_pre_modeset - account for vblanks across mode sets
> * @dev: DRM device
> @@ -1224,6 +1250,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ,
> (((drm_vblank_count(dev, crtc) -
> vblwait->request.sequence) <= (1 << 23)) ||
> + dev->vblank[crtc].reject ||
> !dev->irq_enabled));
>
> if (ret != -EINTR) {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index ebc0150..e2d6b9d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -68,7 +68,7 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
> /* wait for the completion of page flip. */
> wait_event(exynos_crtc->pending_flip_queue,
> atomic_read(&exynos_crtc->pending_flip) == 0);
> - drm_vblank_off(crtc->dev, exynos_crtc->pipe);
> + drm_vblank_off(crtc->dev, exynos_crtc->pipe, false);
> }
>
> exynos_drm_fn_encoder(crtc, &mode, exynos_drm_encoder_crtc_dpms);
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index 386de2c..ff18220 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -281,7 +281,7 @@ void gma_crtc_dpms(struct drm_crtc *crtc, int mode)
> REG_WRITE(VGACNTRL, VGA_DISP_DISABLE);
>
> /* Turn off vblank interrupts */
> - drm_vblank_off(dev, pipe);
> + drm_vblank_off(dev, pipe, false);
>
> /* Wait for vblank for the disable to take effect */
> gma_wait_for_vblank(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f19e6ea..bab0d08 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3643,7 +3643,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
> int plane = intel_crtc->plane;
>
> intel_crtc_wait_for_pending_flips(crtc);
> - drm_vblank_off(dev, pipe);
> + drm_vblank_off(dev, pipe, false);
>
> /* FBC must be disabled before disabling the plane on HSW. */
> if (dev_priv->fbc.plane == plane)
> @@ -3774,7 +3774,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> encoder->disable(encoder);
>
> intel_crtc_wait_for_pending_flips(crtc);
> - drm_vblank_off(dev, pipe);
> + drm_vblank_off(dev, pipe, false);
>
> if (dev_priv->fbc.plane == plane)
> intel_disable_fbc(dev);
> @@ -4239,7 +4239,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>
> /* Give the overlay scaler a chance to disable if it's on this pipe */
> intel_crtc_wait_for_pending_flips(crtc);
> - drm_vblank_off(dev, pipe);
> + drm_vblank_off(dev, pipe, false);
>
> if (dev_priv->fbc.plane == plane)
> intel_disable_fbc(dev);
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 9336006..480bfec 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -324,7 +324,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc)
> }
> }
>
> - drm_vblank_off(drm, dc->pipe);
> + drm_vblank_off(drm, dc->pipe, false);
> }
>
> static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index f974da9..ee40483 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1090,6 +1090,7 @@ struct drm_vblank_crtc {
> int crtc; /* crtc index */
> bool enabled; /* so we don't call enable more than
> once per disable */
> + bool reject; /* reject drm_vblank_get()? */
> };
>
> /**
> @@ -1400,7 +1401,8 @@ extern void drm_send_vblank_event(struct drm_device *dev, int crtc,
> extern bool drm_handle_vblank(struct drm_device *dev, int crtc);
> extern int drm_vblank_get(struct drm_device *dev, int crtc);
> extern void drm_vblank_put(struct drm_device *dev, int crtc);
> -extern void drm_vblank_off(struct drm_device *dev, int crtc);
> +extern void drm_vblank_off(struct drm_device *dev, int crtc, bool reject);
> +extern void drm_vblank_on(struct drm_device *dev, int crtc);
> extern void drm_vblank_cleanup(struct drm_device *dev);
> extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
> struct timeval *tvblank, unsigned flags);
> --
> 1.8.3.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
