There's no reason for drivers to call this, and all the ones I've removed looked very fishy: - Proper quiescenting of the vblank machinery should be done by calling drm_crtc_vblank_off(), which is best done by shutting down the entire display engine with drm_atomic_helper_shutdown.

- Releasing of allocated memory is done by the core already, it calls drm_vblank_cleanup as a fallback.

- drm_vblank_cleanup also has checks for drivers which forget to clean up vblank interrupts.

This essentially reverts

commit e77cef9c2d87db835ad9d70cde4a9b00b0ca2262 Author: Jerome Glisse jglisse@redhat.com Date: Thu Jan 7 15:39:13 2010 +0100

drm: Avoid calling vblank function is vblank wasn't initialized

which was done to fix a bug in radeon code with msi interrupts:

commit 003e69f9862bcda89a75c27750efdbc17ac02945 Author: Jerome Glisse jglisse@redhat.com Date: Thu Jan 7 15:39:14 2010 +0100

drm/radeon/kms: Don't try to enable IRQ if we have no handler installed

Afaict from digging around in old code, this was needed to avoid blowing up in the ums fallback, and has stopped serving it's purpose long ago - if irq init fails, the driver fails to load, and there's really no way to blow up anymore.

Long story short, this was most likely a small ums compat/fallback hack that became a thing of it's own and got cargo-cult duplicated all over the drm codebase for essentially no gain at all.

v2: Mention that for drivers with a ->release callback cleanup is handled by drm_dev_fini() (Thierry).

Cc: Thierry Reding treding@nvidia.com Acked-by: Thierry Reding treding@nvidia.com Cc: Jerome Glisse jglisse@redhat.com Reviewed-by: Sean Paul seanpaul@chromium.org Acked-by: Alex Deucher alexander.deucher@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_internal.h | 1 + drivers/gpu/drm/drm_vblank.c | 19 ++----------------- include/drm/drm_vblank.h | 1 - 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index f89371e920e6..068b685608cf 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -57,6 +57,7 @@ int drm_gem_name_info(struct seq_file *m, void *data); /* drm_vblank.c */ extern unsigned int drm_timestamp_monotonic; void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe); +void drm_vblank_cleanup(struct drm_device *dev);

/* IOCTLS */ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 7e3f59182571..05d043e9219f 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -394,19 +394,6 @@ 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(). It is - * called by the DRM core when @dev is finalized. - * - * Drivers can call drm_vblank_cleanup() if they need to quiescent the vblank - * interrupt in their unload code. But in general this should be handled by - * disabling all active &drm_crtc through e.g. drm_atomic_helper_shutdown, which - * should end up calling drm_crtc_vblank_off(). - * - */ void drm_vblank_cleanup(struct drm_device *dev) { unsigned int pipe; @@ -428,7 +415,6 @@ void drm_vblank_cleanup(struct drm_device *dev)

dev->num_crtcs = 0; } -EXPORT_SYMBOL(drm_vblank_cleanup);

/** * drm_vblank_init - initialize vblank support @@ -436,9 +422,8 @@ EXPORT_SYMBOL(drm_vblank_cleanup); * @num_crtcs: number of CRTCs supported by @dev * * This function initializes vblank support for @num_crtcs display pipelines. - * Drivers do not need to call drm_vblank_cleanup(), cleanup is already handled - * by the DRM core, or through calling drm_dev_fini() for drivers with a - * &drm_driver.release callback. + * Cleanup is handled by the DRM core, or through calling drm_dev_fini() for + * drivers with a &drm_driver.release callback. * * Returns: * Zero on success or a negative error code on failure. diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index 4ceef128582f..7fba9efe4951 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -168,7 +168,6 @@ void drm_crtc_wait_one_vblank(struct drm_crtc *crtc); void drm_crtc_vblank_off(struct drm_crtc *crtc); void drm_crtc_vblank_reset(struct drm_crtc *crtc); void drm_crtc_vblank_on(struct drm_crtc *crtc); -void drm_vblank_cleanup(struct drm_device *dev); u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);

bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,

Thanks!

Minor typo "dsiplay", otherwise: Reviewed-by: Sinclair Yeh syeh@vmware.com

On Mon, Jun 26, 2017 at 06:19:48PM +0200, Daniel Vetter wrote:

Again stopping the vblank before uninstalling the irq handler is kinda the wrong way round, but the fb_off stuff should take care of disabling the dsiplay at least in most cases. So drop the drm_vblank_cleanup code since it's not really doing anything, it looks all cargo-culted.

v2: Appease gcc better.

v3: Simplify code (Sean Paul)

Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: Sean Paul seanpaul@chromium.org Reviewed-by: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++------ drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 2 -- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 4 ---- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 9 --------- drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 27 +-------------------------- 5 files changed, 3 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 3d94ea67a825..1cd67b10a0d9 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -1658,7 +1658,7 @@ int vmw_kms_init(struct vmw_private *dev_priv)

int vmw_kms_close(struct vmw_private *dev_priv) {

• int ret;

• int ret = 0;

  /*

  • Docs says we should take the lock before calling this function

@@ -1666,11 +1666,7 @@ int vmw_kms_close(struct vmw_private *dev_priv) * drm_encoder_cleanup which takes the lock we deadlock. */ drm_mode_config_cleanup(dev_priv->dev);

• if (dev_priv->active_display_unit == vmw_du_screen_object)

• ret = vmw_kms_sou_close_display(dev_priv);
  

• else if (dev_priv->active_display_unit == vmw_du_screen_target)

• ret = vmw_kms_stdu_close_display(dev_priv);
  

• else

• if (dev_priv->active_display_unit == vmw_du_legacy) ret = vmw_kms_ldu_close_display(dev_priv);

  return ret;

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h index 5f8d678ae675..ff9c8389ff21 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h @@ -390,7 +390,6 @@ int vmw_kms_update_proxy(struct vmw_resource *res,

• Screen Objects display functions - vmwgfx_scrn.c

*/ int vmw_kms_sou_init_display(struct vmw_private *dev_priv); -int vmw_kms_sou_close_display(struct vmw_private *dev_priv); int vmw_kms_sou_do_surface_dirty(struct vmw_private *dev_priv, struct vmw_framebuffer *framebuffer, struct drm_clip_rect *clips, @@ -418,7 +417,6 @@ int vmw_kms_sou_readback(struct vmw_private *dev_priv,

• Screen Target Display Unit functions - vmwgfx_stdu.c

*/ int vmw_kms_stdu_init_display(struct vmw_private *dev_priv); -int vmw_kms_stdu_close_display(struct vmw_private *dev_priv); int vmw_kms_stdu_surface_dirty(struct vmw_private *dev_priv, struct vmw_framebuffer *framebuffer, struct drm_clip_rect *clips, diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c index d3987bcf53f8..449ed4fba0f2 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c @@ -582,13 +582,9 @@ int vmw_kms_ldu_init_display(struct vmw_private *dev_priv)

int vmw_kms_ldu_close_display(struct vmw_private *dev_priv) {

• struct drm_device *dev = dev_priv->dev;

• if (!dev_priv->ldu_priv) return -ENOSYS;

• drm_vblank_cleanup(dev);

• BUG_ON(!list_empty(&dev_priv->ldu_priv->active));

  kfree(dev_priv->ldu_priv);

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c index 8d7dc9def7c2..3b917c9b0c21 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c @@ -746,15 +746,6 @@ int vmw_kms_sou_init_display(struct vmw_private *dev_priv) return 0; }

-int vmw_kms_sou_close_display(struct vmw_private *dev_priv) -{

• struct drm_device *dev = dev_priv->dev;
• drm_vblank_cleanup(dev);
• return 0;

-}

static int do_dmabuf_define_gmrfb(struct vmw_private *dev_priv, struct vmw_framebuffer *framebuffer) { diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c index 50be1f034f9e..6aecba6cd5e2 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c @@ -1651,36 +1651,11 @@ int vmw_kms_stdu_init_display(struct vmw_private *dev_priv)

if (unlikely(ret != 0)) {
	DRM_ERROR("Failed to initialize STDU %d", i);

• 	goto err_vblank_cleanup;
  

• 	return ret;
  

  } }

  DRM_INFO("Screen Target Display device initialized\n");

  return 0;

-err_vblank_cleanup:

• drm_vblank_cleanup(dev);
• return ret;

-}

-/**

• • vmw_kms_stdu_close_display - Cleans up after vmw_kms_stdu_init_display
• • @dev_priv: VMW DRM device
• • Frees up any resources allocated by vmw_kms_stdu_init_display
• • RETURNS:
• • 0 on success
• */

-int vmw_kms_stdu_close_display(struct vmw_private *dev_priv) -{

• struct drm_device *dev = dev_priv->dev;
• drm_vblank_cleanup(dev);
• return 0;

}

2.11.0