On Wed, May 14, 2014 at 08:51:06PM +0200, Daniel Vetter wrote:

From: Ville Syrjälä ville.syrjala@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@linux.intel.com [danvet: Add Testcase tag for the igt I've written.] Signed-off-by: Daniel Vetter daniel.vetter@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().

+{

• 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... */

?

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?

Thierry

Leftover from the old days of ums and should be used any longer. Since

commit 29935554b384b1b3a7377d6f0b03b21d18a61683 Author: Laurent Pinchart laurent.pinchart@ideasonboard.com Date: Wed May 30 00:58:09 2012 +0200

drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers

it is a complete no-Op for kms drivers.

v2: Fix up mangled sentence spotted by Michel.

Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- Documentation/DocBook/drm.tmpl | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 83dd0b043c28..96cee6438472 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -2861,12 +2861,12 @@ int num_ioctls;</synopsis> <term>DRM_IOCTL_MODESET_CTL</term> <listitem> <para> - This should be called by application level drivers before and - after mode setting, since on many devices the vertical blank - counter is reset at that time. Internally, the DRM snapshots - the last vblank count when the ioctl is called with the - _DRM_PRE_MODESET command, so that the counter won't go backwards - (which is dealt with when _DRM_POST_MODESET is used). + This was only used for user-mode-settind drivers around + modesetting changes to allow the kernel to update the vblank + interrupt after mode setting, since on many devices the vertical + blank counter is reset to 0 at some point during modeset. Modern + drivers should not call this any more since with kernel mode + setting it is a no-op. </para> </listitem> </varlistentry>

On Thu, May 15, 2014 at 01:44:04PM +0900, Michel Dänzer wrote:

On 15.05.2014 03:51, Daniel Vetter wrote:

...

@@ -964,8 +1005,13 @@ EXPORT_SYMBOL(drm_vblank_off);

/**

• drm_vblank_on - enable vblank events on a CRTC

• • @dev: DRM device

• • @dev: drm device
  • @crtc: CRTC in question
• • This functions restores the vblank interrupt state captured with
• • drm_vblank_off() again. Note that calls to drm_vblank_on() and
• • drm_vblank_off() can be unbalanced and so can also be unconditionaly called
• • in driver load code to reflect the current hardware state of the crtc.

Given this description, maybe something like drm_vblank_save/restore would describe better what these functions do than drm_vblank_off/on?

It also enables/disables the vblank machinery itself, which at least in i915 will be important shortly - we slowly switch over our code to be more interrupt driven, including the initial plane enabling/disabling in modeset changes.

...

@@ -981,11 +1027,26 @@ EXPORT_SYMBOL(drm_vblank_on);

/**

• drm_vblank_pre_modeset - account for vblanks across mode sets

• • @dev: DRM device

• • @dev: drm device
  • @crtc: CRTC in question
  • Account for vblank events across mode setting events, which will likely
  • reset the hardware frame counter.
• • This is done by grabbing a temporary vblank reference to ensure that the
• • vblank interrupt keeps running across the modeset sequence. With this the
• • software-side vblank frame counting will ensure that there are no jumps or
• • discontinuities.
• • Unfortunately this approach is racy and also doesn't work when the vblank
• • interrupt stops running, e.g. across system suspend resume. It is therefore
• • highly recommended that drivers use the newer drm_vblank_off() and
• • drm_vblank_on() instead. drm_vblank_pre_modeset() only works correctly when
• • using "cooked" software vblank frame counters and not relying on any hardware
• • counters.

That last statement is not true IME with radeon[0].

Basically, it sounds to me like drm_vblank_off/on do more or less what drm_vblank_pre/post_modeset are intended to do (e.g. the latter can also be nested arbitrarily). Still not really sure why we need two sets of these instead of fixing any problems in one set.

The problem is that the driver situation is a mess. So the right plan imo is: 1) Create new functions that work, beat on them. 2) Convert all drivers. 3) Rip out the old stuff.

I've tried to look into this a bit and decided that this isn't something I can do without the hardware at hand and a few solid tests. So this series is 1) done for i915, with an rfc for 2).

Also there's the issue of old ums using the same stuff and I think we shouldn't touch that can of worms at all.

[0] Though we certainly don't have as rigorous testing for that as you guys do in intel-gpu-tools. Any chance some of that could be moved to somewhere more generic?

igt is mostly ready - what we need is some rather thin abstraction for the kms tests to run also on other drivers, with dumb objects. Otherwise all the test framework can cope with random bits not being there and skipping those subtests, e.g. the CRC based stuff.

I don't want to extract the generic parts of the tests into a different repo (at least not if there's not _lots_ of people working on them) since the code sharing we currently do between tests is fairly massive.

But I'm willing to deal with the hassle of supporting other drivers for e.g. the kms tests. And I think it would make a nice gsoc project. But it's definitely not something I can throw intel resource (or too much of my own time) at ;-)

Having a shared set of tests which clearly spells out all the corner cases and tests for races would imo be awesome and greatly improve the overall health of the drm/kms drivers and wider ecosystem. -Daniel

- Integrate into the drm DocBook - Disable kerneldoc for functions not exported to drivers. - Properly document the new drm_vblank_on|off and add cautious comments explaining when drm_vblank_pre|post_modesets shouldn't be used. - General polish and OCD.

v2: Polish as suggested by Thierry.

Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- Documentation/DocBook/drm.tmpl | 5 +- drivers/gpu/drm/drm_irq.c | 165 +++++++++++++++++++++++++++++------------ 2 files changed, 121 insertions(+), 49 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 96cee6438472..9c47e3147ad1 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -2519,6 +2519,10 @@ void (*disable_vblank) (struct drm_device *dev, int crtc);</synopsis> with a call to <function>drm_vblank_cleanup</function> in the driver <methodname>unload</methodname> operation handler. </para> + <sect2> + <title>Vertical Blanking and Interrupt Handling Functions Reference</title> +!Edrivers/gpu/drm/drm_irq.c + </sect2> </sect1>

<!-- Internals: open/close, file operations and ioctls --> @@ -2871,7 +2875,6 @@ int num_ioctls;</synopsis> </listitem> </varlistentry> </variablelist> -<!--!Edrivers/char/drm/drm_irq.c--> </para> </sect1>

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index dd786d84daab..f3dafccca456 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1,6 +1,5 @@ -/** - * \file drm_irq.c - * IRQ support +/* + * drm_irq.c IRQ and vblank support * * \author Rickard E. (Rik) Faith faith@valinux.com * \author Gareth Hughes gareth@valinux.com @@ -156,6 +155,12 @@ static void vblank_disable_fn(unsigned long arg) spin_unlock_irqrestore(&dev->vbl_lock, irqflags); }

+/** + * drm_vblank_cleanup - cleanup vblank support + * @dev: DRM device + * + * This function cleans up any resources allocated in drm_vblank_init. + */ void drm_vblank_cleanup(struct drm_device *dev) { int crtc; @@ -175,6 +180,16 @@ void drm_vblank_cleanup(struct drm_device *dev) } EXPORT_SYMBOL(drm_vblank_cleanup);

+/** + * drm_vblank_init - initialize vblank support + * @dev: drm_device + * @num_crtcs: number of crtcs supported by @dev + * + * This function initializes vblank support for @num_crtcs display pipelines. + * + * Returns: + * Zero on success or a negative error code on failure. + */ int drm_vblank_init(struct drm_device *dev, int num_crtcs) { int i, ret = -ENOMEM; @@ -238,13 +253,21 @@ static void drm_irq_vgaarb_nokms(void *cookie, bool state) }

/** - * Install IRQ handler. - * - * \param dev DRM device. + * drm_irq_install - install IRQ handler + * @dev: DRM device + * @irq: IRQ number to install the handler for * * Initializes the IRQ related data. Installs the handler, calling the driver - * \c irq_preinstall() and \c irq_postinstall() functions - * before and after the installation. + * irq_preinstall() and irq_postinstall() functions before and after the + * installation. + * + * This is the simplified helper interface provided for drivers with no special + * needs. Drivers which need to install interrupt handlers for multiple + * interrupts must instead set drm_device->irq_enabled to signal the DRM core + * that vblank interrupts are available. + * + * Returns: + * Zero on success or a negative error code on failure. */ int drm_irq_install(struct drm_device *dev, int irq) { @@ -304,11 +327,20 @@ int drm_irq_install(struct drm_device *dev, int irq) EXPORT_SYMBOL(drm_irq_install);

/** - * Uninstall the IRQ handler. + * drm_irq_uninstall - uninstall the IRQ handler + * @dev: DRM device + * + * Calls the driver's irq_uninstall() function and unregisters the IRQ handler. + * This should only be called by drivers which used drm_irq_install() to set up + * their interrupt handler. Other drivers must only reset + * drm_device->irq_enabled to false. * - * \param dev DRM device. + * Note that for kernel modesetting drivers it is a bug if this function fails. + * The sanity checks are only to catch buggy user modesetting drivers which call + * the same function through an ioctl. * - * Calls the driver's \c irq_uninstall() function, and stops the irq. + * Returns: + * Zero on success or a negative error code on failure. */ int drm_irq_uninstall(struct drm_device *dev) { @@ -353,7 +385,7 @@ int drm_irq_uninstall(struct drm_device *dev) } EXPORT_SYMBOL(drm_irq_uninstall);

-/** +/* * IRQ control ioctl. * * \param inode device inode. @@ -406,15 +438,14 @@ int drm_control(struct drm_device *dev, void *data, }

/** - * drm_calc_timestamping_constants - Calculate vblank timestamp constants - * - * @crtc drm_crtc whose timestamp constants should be updated. - * @mode display mode containing the scanout timings + * drm_calc_timestamping_constants - calculate vblank timestamp constants + * @crtc: drm_crtc whose timestamp constants should be updated. + * @mode: display mode containing the scanout timings * * Calculate and store various constants which are later * needed by vblank and swap-completion timestamping, e.g, * by drm_calc_vbltimestamp_from_scanoutpos(). They are - * derived from crtc's true scanout timing, so they take + * derived from CRTC's true scanout timing, so they take * things like panel scaling or other adjustments into account. */ void drm_calc_timestamping_constants(struct drm_crtc *crtc, @@ -459,11 +490,22 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, EXPORT_SYMBOL(drm_calc_timestamping_constants);

/** - * drm_calc_vbltimestamp_from_scanoutpos - helper routine for kms - * drivers. Implements calculation of exact vblank timestamps from - * given drm_display_mode timings and current video scanout position - * of a crtc. This can be called from within get_vblank_timestamp() - * implementation of a kms driver to implement the actual timestamping. + * drm_calc_vbltimestamp_from_scanoutpos - precise vblank timestamp helper + * @dev: DRM device + * @crtc: Which CRTC's vblank timestamp to retrieve + * @max_error: Desired maximum allowable error in timestamps (nanosecs) + * On return contains true maximum error of timestamp + * @vblank_time: Pointer to struct timeval which should receive the timestamp + * @flags: Flags to pass to driver: + * 0 = Default, + * DRM_CALLED_FROM_VBLIRQ = If function is called from vbl IRQ handler + * @refcrtc: CRTC which defines scanout timing + * @mode: mode which defines the scanout timings + * + * Implements calculation of exact vblank timestamps from given drm_display_mode + * timings and current video scanout position of a CRTC. This can be called from + * within get_vblank_timestamp() implementation of a kms driver to implement the + * actual timestamping. * * Should return timestamps conforming to the OML_sync_control OpenML * extension specification. The timestamp corresponds to the end of @@ -478,21 +520,11 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants); * returns as no operation if a doublescan or interlaced video mode is * active. Higher level code is expected to handle this. * - * @dev: DRM device. - * @crtc: Which crtc's vblank timestamp to retrieve. - * @max_error: Desired maximum allowable error in timestamps (nanosecs). - * On return contains true maximum error of timestamp. - * @vblank_time: Pointer to struct timeval which should receive the timestamp. - * @flags: Flags to pass to driver: - * 0 = Default. - * DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler. - * @refcrtc: drm_crtc* of crtc which defines scanout timing. - * @mode: mode which defines the scanout timings - * - * Returns negative value on error, failure or if not supported in current + * Returns: + * Negative value on error, failure or if not supported in current * video mode: * - * -EINVAL - Invalid crtc. + * -EINVAL - Invalid CRTC. * -EAGAIN - Temporary unavailable, e.g., called before initial modeset. * -ENOTSUPP - Function not supported in current display mode. * -EIO - Failed, e.g., due to failed scanout position query. @@ -641,23 +673,23 @@ static struct timeval get_drm_timestamp(void)

/** * drm_get_last_vbltimestamp - retrieve raw timestamp for the most recent - * vblank interval. - * + * vblank interval * @dev: DRM device - * @crtc: which crtc's vblank timestamp to retrieve + * @crtc: which CRTC's vblank timestamp to retrieve * @tvblank: Pointer to target struct timeval which should receive the timestamp * @flags: Flags to pass to driver: - * 0 = Default. - * DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler. + * 0 = Default, + * DRM_CALLED_FROM_VBLIRQ = If function is called from vbl IRQ handler * * Fetches the system timestamp corresponding to the time of the most recent - * vblank interval on specified crtc. May call into kms-driver to + * vblank interval on specified CRTC. May call into kms-driver to * compute the timestamp with a high-precision GPU specific method. * * Returns zero if timestamp originates from uncorrected do_gettimeofday() * call, i.e., it isn't very precisely locked to the true vblank. * - * Returns non-zero if timestamp is considered to be very precise. + * Returns: + * Non-zero if timestamp is considered to be very precise, zero otherwise. */ u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, struct timeval *tvblank, unsigned flags) @@ -692,6 +724,9 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp); * Fetches the "cooked" vblank count value that represents the number of * vblank events since the system was booted, including lost events due to * modesetting activity. + * + * Returns: + * The software vblank counter. */ u32 drm_vblank_count(struct drm_device *dev, int crtc) { @@ -710,8 +745,7 @@ EXPORT_SYMBOL(drm_vblank_count); * Fetches the "cooked" vblank count value that represents the number of * vblank events since the system was booted, including lost events due to * modesetting activity. Returns corresponding system timestamp of the time - * of the vblank interval that corresponds to the current value vblank counter - * value. + * of the vblank interval that corresponds to the current vblank counter value. */ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, struct timeval *vblanktime) @@ -882,7 +916,7 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc) * Acquire a reference count on vblank events to avoid having them disabled * while in use. * - * RETURNS + * Returns: * Zero on success, nonzero on failure. */ int drm_vblank_get(struct drm_device *dev, int crtc) @@ -930,6 +964,13 @@ EXPORT_SYMBOL(drm_vblank_put); * drm_vblank_off - disable vblank events on a CRTC * @dev: DRM device * @crtc: CRTC in question + * + * Drivers can use this function to shut down the vblank interrupt handling when + * disabling a crtc. This function ensures that the latest vblank frame count is + * stored so that drm_vblank_on() can restore it again. + * + * Drivers must use this function when the hardware vblank counter can get + * reset, e.g. when suspending. */ void drm_vblank_off(struct drm_device *dev, int crtc) { @@ -966,6 +1007,11 @@ EXPORT_SYMBOL(drm_vblank_off); * drm_vblank_on - enable vblank events on a CRTC * @dev: DRM device * @crtc: CRTC in question + * + * This functions restores the vblank interrupt state captured with + * drm_vblank_off() again. Note that calls to drm_vblank_on() and + * drm_vblank_off() can be unbalanced and so can also be unconditionaly called + * in driver load code to reflect the current hardware state of the crtc. */ void drm_vblank_on(struct drm_device *dev, int crtc) { @@ -986,6 +1032,21 @@ EXPORT_SYMBOL(drm_vblank_on); * * Account for vblank events across mode setting events, which will likely * reset the hardware frame counter. + * + * This is done by grabbing a temporary vblank reference to ensure that the + * vblank interrupt keeps running across the modeset sequence. With this the + * software-side vblank frame counting will ensure that there are no jumps or + * discontinuities. + * + * Unfortunately this approach is racy and also doesn't work when the vblank + * interrupt stops running, e.g. across system suspend resume. It is therefore + * highly recommended that drivers use the newer drm_vblank_off() and + * drm_vblank_on() instead. drm_vblank_pre_modeset() only works correctly when + * using "cooked" software vblank frame counters and not relying on any hardware + * counters. + * + * Drivers must call drm_vblank_post_modeset() when re-enabling the same crtc + * again. */ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc) { @@ -1007,6 +1068,14 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_pre_modeset);

+/** + * drm_vblank_post_modeset - undo drm_vblank_pre_modeset changes + * @dev: DRM device + * @crtc: CRTC in question + * + * This function again drops the temporary vblank reference acquired in + * drm_vblank_pre_modeset. + */ void drm_vblank_post_modeset(struct drm_device *dev, int crtc) { unsigned long irqflags; @@ -1028,7 +1097,7 @@ void drm_vblank_post_modeset(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_post_modeset);

-/** +/* * drm_modeset_ctl - handle vblank event counter changes across mode switch * @DRM_IOCTL_ARGS: standard ioctl arguments * @@ -1141,7 +1210,7 @@ err_put: return ret; }

-/** +/* * Wait for VBLANK. * * \param inode device inode. @@ -1152,7 +1221,7 @@ err_put: * * This function enables the vblank interrupt on the pipe requested, then * sleeps waiting for the requested sequence number to occur, and drops - * the vblank interrupt refcount afterwards. (vblank irq disable follows that + * the vblank interrupt refcount afterwards. (vblank IRQ disable follows that * after a timeout with no further vblank waits scheduled). */ int drm_wait_vblank(struct drm_device *dev, void *data,

We need to start somewhere ... With this the only places left in i915 where we use pipe integers is in the interrupt handling code. And there it actually makes some amount of sense.

v2: - Polish kerneldoc a bit (Thierry). - Drop "dev" parameter since it's unecessary. - Split out i915 changes (Thierry).

Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_irq.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 5 +++ 2 files changed, 82 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index f3dafccca456..5ba9a614e7ad 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -916,6 +916,8 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc) * Acquire a reference count on vblank events to avoid having them disabled * while in use. * + * This is the legacy version of drm_crtc_vblank_get(). + * * Returns: * Zero on success, nonzero on failure. */ @@ -941,12 +943,32 @@ int drm_vblank_get(struct drm_device *dev, int crtc) EXPORT_SYMBOL(drm_vblank_get);

/** + * drm_crtc_vblank_get - get a reference count on vblank events + * @crtc: which CRTC to own + * + * Acquire a reference count on vblank events to avoid having them disabled + * while in use. + * + * This is the native kms version of drm_vblank_off(). + * + * Returns: + * Zero on success, nonzero on failure. + */ +int drm_crtc_vblank_get(struct drm_crtc *crtc) +{ + return drm_vblank_get(crtc->dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_vblank_get); + +/** * drm_vblank_put - give up ownership of vblank events * @dev: DRM device * @crtc: which counter to give up * * Release ownership of a given vblank counter, turning off interrupts * if possible. Disable interrupts after drm_vblank_offdelay milliseconds. + * + * This is the legacy version of drm_crtc_vblank_put(). */ void drm_vblank_put(struct drm_device *dev, int crtc) { @@ -961,6 +983,21 @@ void drm_vblank_put(struct drm_device *dev, int crtc) EXPORT_SYMBOL(drm_vblank_put);

/** + * drm_crtc_vblank_put - give up ownership of vblank events + * @crtc: which counter to give up + * + * Release ownership of a given vblank counter, turning off interrupts + * if possible. Disable interrupts after drm_vblank_offdelay milliseconds. + * + * This is the native kms version of drm_vblank_put(). + */ +void drm_crtc_vblank_put(struct drm_crtc *crtc) +{ + drm_vblank_put(crtc->dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_vblank_put); + +/** * drm_vblank_off - disable vblank events on a CRTC * @dev: DRM device * @crtc: CRTC in question @@ -971,6 +1008,8 @@ EXPORT_SYMBOL(drm_vblank_put); * * Drivers must use this function when the hardware vblank counter can get * reset, e.g. when suspending. + * + * This is the legacy version of drm_crtc_vblank_off(). */ void drm_vblank_off(struct drm_device *dev, int crtc) { @@ -1004,6 +1043,25 @@ void drm_vblank_off(struct drm_device *dev, int crtc) EXPORT_SYMBOL(drm_vblank_off);

/** + * drm_crtc_vblank_off - disable vblank events on a CRTC + * @crtc: CRTC in question + * + * Drivers can use this function to shut down the vblank interrupt handling when + * disabling a crtc. This function ensures that the latest vblank frame count is + * stored so that drm_vblank_on can restore it again. + * + * Drivers must use this function when the hardware vblank counter can get + * reset, e.g. when suspending. + * + * This is the native kms version of drm_vblank_off(). + */ +void drm_crtc_vblank_off(struct drm_crtc *crtc) +{ + drm_vblank_off(crtc->dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_vblank_off); + +/** * drm_vblank_on - enable vblank events on a CRTC * @dev: DRM device * @crtc: CRTC in question @@ -1012,6 +1070,8 @@ EXPORT_SYMBOL(drm_vblank_off); * drm_vblank_off() again. Note that calls to drm_vblank_on() and * drm_vblank_off() can be unbalanced and so can also be unconditionaly called * in driver load code to reflect the current hardware state of the crtc. + * + * This is the legacy version of drm_crtc_vblank_on(). */ void drm_vblank_on(struct drm_device *dev, int crtc) { @@ -1026,6 +1086,23 @@ void drm_vblank_on(struct drm_device *dev, int crtc) EXPORT_SYMBOL(drm_vblank_on);

/** + * drm_crtc_vblank_on - enable vblank events on a CRTC + * @crtc: CRTC in question + * + * This functions restores the vblank interrupt state captured with + * drm_vblank_off() again. Note that calls to drm_vblank_on() and + * drm_vblank_off() can be unbalanced and so can also be unconditionaly called + * in driver load code to reflect the current hardware state of the crtc. + * + * This is the native kms version of drm_vblank_on(). + */ +void drm_crtc_vblank_on(struct drm_crtc *crtc) +{ + drm_vblank_on(crtc->dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_vblank_on); + +/** * drm_vblank_pre_modeset - account for vblanks across mode sets * @dev: DRM device * @crtc: CRTC in question diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 7339b2b00724..76ccaabd0418 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1359,9 +1359,14 @@ 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 int drm_crtc_vblank_get(struct drm_crtc *crtc); +extern void drm_crtc_vblank_put(struct drm_crtc *crtc); extern void drm_vblank_off(struct drm_device *dev, int crtc); extern void drm_vblank_on(struct drm_device *dev, int crtc); +extern void drm_crtc_vblank_off(struct drm_crtc *crtc); +extern void drm_crtc_vblank_on(struct drm_crtc *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); extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,

On Tue, May 20, 2014 at 03:03:41PM +0300, Ville Syrjälä wrote:

On Wed, May 14, 2014 at 08:51:11PM +0200, Daniel Vetter wrote:

...

Now that we unconditionally dtrt when disabling/enabling crtcs we don't need any hacks any longer to keep the vblank logic sane when all the registers go poof. So let's rip it all out.

Hmm. drm_update_vblank_count() will now see some kind of diff between the last and current value when the registers got cloberred. So the vblank counter reported to userspace will jump. But I guess that's fine as long as userspace realizes that the counter is not at all reliable across modesets.

I've added checks for this (the rpm varianst) and for the similiar suspend/resume issues (the suspend variants) to kms_flip. It seems to work and we don't actually jump to far. But maybe the tests are horribly broken.

Can you please take a closer look? I've thought that the entire point of this series (well, one of them) was to finally fix this gag and avoid handing totally bogus frame counter values to userspace. Especially for system suspend/resume where userspace might get susprised ... -Daniel

...

This essentially undoes

commit 9dbd8febb4dbc9199fcf340b882eb930e36b65b6 Author: Paulo Zanoni paulo.r.zanoni@intel.com Date: Tue Jul 23 10:48:11 2013 -0300

drm/i915: update last_vblank when disabling the power well

Apparently igt/kms_flip is already powerful enough to exercise this properly, yay! See the reference regression report for details.

References: https://bugs.freedesktop.org/show_bug.cgi?id=66808 Testcase: igt/kms_flip/*-vs-rpm Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/i915/intel_pm.c | 34 ---------------------------------- 1 file changed, 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 75c1c766b507..45fa43f16bb3 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5423,33 +5423,6 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv) } }

-static void reset_vblank_counter(struct drm_device *dev, enum pipe pipe) -{

• assert_spin_locked(&dev->vbl_lock);
• dev->vblank[pipe].last = 0;

-}

-static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv) -{

• struct drm_device *dev = dev_priv->dev;
• enum pipe pipe;
• unsigned long irqflags;
• /*

• * After this, the registers on the pipes that are part of the power
  

• * well will become zero, so we have to adjust our counters according to
  

• * that.
  

• *
  

• * FIXME: Should we do this in general in drm_vblank_post_modeset?
  

• */
  

• spin_lock_irqsave(&dev->vbl_lock, irqflags);
• for_each_pipe(pipe)

• if (pipe != PIPE_A)
  

• 	reset_vblank_counter(dev, pipe);
  

• spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

-}

static void hsw_set_power_well(struct drm_i915_private *dev_priv, struct i915_power_well *power_well, bool enable) { @@ -5478,8 +5451,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv, I915_WRITE(HSW_PWR_WELL_DRIVER, 0); POSTING_READ(HSW_PWR_WELL_DRIVER); DRM_DEBUG_KMS("Requesting to disable the power well\n");

• 	hsw_power_well_post_disable(dev_priv);
  

  } }

} @@ -5646,11 +5617,6 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv, valleyview_disable_display_irqs(dev_priv); spin_unlock_irq(&dev_priv->irq_lock);

• spin_lock_irq(&dev->vbl_lock);
• for_each_pipe(pipe)

• reset_vblank_counter(dev, pipe);
  

• spin_unlock_irq(&dev->vbl_lock);
• vlv_set_power_well(dev_priv, power_well, false);

}

-- 1.8.3.1

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- Ville Syrjälä Intel OTC

On Tue, May 20, 2014 at 05:03:33PM +0300, Ville Syrjälä wrote:

On Tue, May 20, 2014 at 03:38:14PM +0200, Daniel Vetter wrote:

...

On Tue, May 20, 2014 at 03:03:41PM +0300, Ville Syrjälä wrote:

...

On Wed, May 14, 2014 at 08:51:11PM +0200, Daniel Vetter wrote:

...

Now that we unconditionally dtrt when disabling/enabling crtcs we don't need any hacks any longer to keep the vblank logic sane when all the registers go poof. So let's rip it all out.

Hmm. drm_update_vblank_count() will now see some kind of diff between the last and current value when the registers got cloberred. So the vblank counter reported to userspace will jump. But I guess that's fine as long as userspace realizes that the counter is not at all reliable across modesets.

I've added checks for this (the rpm varianst) and for the similiar suspend/resume issues (the suspend variants) to kms_flip. It seems to work and we don't actually jump to far. But maybe the tests are horribly broken.

Hmm. If we can force the power well off at the start of the test and then set a mode, I'd expect the vblank counter to jump by almost max_vblank_count since the hw counter would appear to wrap.

Oh, I think the tests are busted ... Need to look at this again. -Daniel

...

Can you please take a closer look? I've thought that the entire point of this series (well, one of them) was to finally fix this gag and avoid handing totally bogus frame counter values to userspace. Especially for system suspend/resume where userspace might get susprised ...

I was mostly interested in making vblank interrupts work during plane enable/disable. Anything else is a bonus.

...

-Daniel

...

...

This essentially undoes

commit 9dbd8febb4dbc9199fcf340b882eb930e36b65b6 Author: Paulo Zanoni paulo.r.zanoni@intel.com Date: Tue Jul 23 10:48:11 2013 -0300

drm/i915: update last_vblank when disabling the power well

Apparently igt/kms_flip is already powerful enough to exercise this properly, yay! See the reference regression report for details.

References: https://bugs.freedesktop.org/show_bug.cgi?id=66808 Testcase: igt/kms_flip/*-vs-rpm Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/i915/intel_pm.c | 34 ---------------------------------- 1 file changed, 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 75c1c766b507..45fa43f16bb3 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5423,33 +5423,6 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv) } }

-static void reset_vblank_counter(struct drm_device *dev, enum pipe pipe) -{

• assert_spin_locked(&dev->vbl_lock);
• dev->vblank[pipe].last = 0;

-}

-static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv) -{

• struct drm_device *dev = dev_priv->dev;
• enum pipe pipe;
• unsigned long irqflags;
• /*

• * After this, the registers on the pipes that are part of the power
  

• * well will become zero, so we have to adjust our counters according to
  

• * that.
  

• *
  

• * FIXME: Should we do this in general in drm_vblank_post_modeset?
  

• */
  

• spin_lock_irqsave(&dev->vbl_lock, irqflags);
• for_each_pipe(pipe)

• if (pipe != PIPE_A)
  

• 	reset_vblank_counter(dev, pipe);
  

• spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

-}

static void hsw_set_power_well(struct drm_i915_private *dev_priv, struct i915_power_well *power_well, bool enable) { @@ -5478,8 +5451,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv, I915_WRITE(HSW_PWR_WELL_DRIVER, 0); POSTING_READ(HSW_PWR_WELL_DRIVER); DRM_DEBUG_KMS("Requesting to disable the power well\n");

• 	hsw_power_well_post_disable(dev_priv);
  

  } }

} @@ -5646,11 +5617,6 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv, valleyview_disable_display_irqs(dev_priv); spin_unlock_irq(&dev_priv->irq_lock);

• spin_lock_irq(&dev->vbl_lock);
• for_each_pipe(pipe)

• reset_vblank_counter(dev, pipe);
  

• spin_unlock_irq(&dev->vbl_lock);
• vlv_set_power_well(dev_priv, power_well, false);

}

-- 1.8.3.1

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- Ville Syrjälä Intel OTC

-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- Ville Syrjälä Intel OTC

On Wed, 2014-05-14 at 20:51 +0200, Daniel Vetter wrote:

We don't have hardware based disable bits on gmch platforms, so need to block spurious underrun reports in software. Which means that we _must_ start out with fifo underrun reporting disabled everywhere.

This is in big contrast to ilk/hsw/cpt where there's only _one_ disable bit for all platforms and hence we must allow underrun reporting on disabled pipes. Otherwise nothing really works, especially the CRC support since that's key'ed off the same irq disable bit.

This allows us to ditch the fifo underrun reporting hack from the vlv runtime pm code and unexport the internal function from i915_irq.c again. Yay!

v2: Keep the display irq disabling, spotted by Imre.

Cc: Imre Deak imre.deak@intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/i915/i915_irq.c | 4 ++-- drivers/gpu/drm/i915/intel_display.c | 9 ++++++++- drivers/gpu/drm/i915/intel_drv.h | 2 -- drivers/gpu/drm/i915/intel_pm.c | 6 ------ 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index afa55199b829..a502faae0d0b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -415,8 +415,8 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,

• Returns the previous state of underrun reporting.

*/ -bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,

• 			     enum pipe pipe, bool enable)
  

+static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,

• 				    enum pipe pipe, bool enable)
  

{ struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d4abaa4bf2f4..e78003ac71a0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11510,11 +11510,18 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) encoder->base.crtc = NULL; } }

• if (crtc->active) {

• if (crtc->active || IS_VALLEYVIEW(dev) || INTEL_INFO(dev)->gen < 5) { /*
  • We start out with underrun reporting disabled to avoid races.
  • For correct bookkeeping mark this on active crtcs.

•  * Also on gmch platforms we dont have any hardware bits to
  

•  * disable the underrun reporting. Which means we need to start
  

•  * out with underrun reporting disabled also on inactive pipes,
  

•  * since otherwise we'll complain about the garbage we read when
  

•  * e.g. coming up after runtime pm.
  

•  *
  

  • No protection against concurrent access is required - at
  • worst a fifo underrun happens which also sets this to false.
  */

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index b885df150910..d3fa5e0a13bd 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -642,8 +642,6 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi) /* i915_irq.c */ bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, enum pipe pipe, bool enable); -bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,

• 			     enum pipe pipe, bool enable);
  

bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev, enum transcoder pch_transcoder, bool enable); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 45fa43f16bb3..08d5d4c16fdf 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5605,15 +5605,9 @@ static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv, static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv, struct i915_power_well *power_well) {

• struct drm_device *dev = dev_priv->dev;

• enum pipe pipe;

• WARN_ON_ONCE(power_well->data != PUNIT_POWER_WELL_DISP2D);

  spin_lock_irq(&dev_priv->irq_lock);

• for_each_pipe(pipe)

• __intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
  

As we discussed on IRC, something like the following could be added to the commit log:

Originally the purpose of disabling the reporting on vlv was to prevent spurious underflow reports when the power well is turned off and the pipestat register containing the underflow flag will read 0xffffffff. After the change in intel_sanitize_crtc() to disable the reporting for all pipes initially, it's guaranteed that reporting is disabled for all pipes by the time we call vlv_display_power_well_disable(), so there is no need to disable it there explicitly.

Reviewed-by: Imre Deak imre.deak@intel.com

valleyview_disable_display_irqs(dev_priv); spin_unlock_irq(&dev_priv->irq_lock);