Re: [PATCH 2/2] drm/i915: Free pending page flip events at .preclose()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 06, 2014 at 02:02:51PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If there are pending page flips when the fd gets closed those page
> flips may have events associated to them. When the page flip eventually
> completes it will queue the event to file_priv->event_list, but that
> may be too late and file_priv->event_list has already been cleaned up.
> Thus we leak a bit of kernel memory in the form of the event structure.
> 
> To avoid such problems clear out such pending events from
> intel_crtc->unpin_work at ->preclose(). Any event that already made it
> to file_priv->event_list will get cleaned up by the drm_release_events()
> a bit later.
> 
> We can ignore the file_priv->event_space accounting since file_priv is
> going away. This is already how drm core deals with pending vblank
> events, which are maintained by the drm core.
> 
> What saves us from a total disaster (ie. dereferencing and alrady
> freed file_priv) is the fact that the fb descruction triggers a modeset
> and there we wait for pending flips.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  3 +++
>  drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 2e7f03a..c965698 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1981,6 +1981,9 @@ void i915_driver_preclose(struct drm_device *dev, struct drm_file *file)
>  	i915_gem_context_close(dev, file);
>  	i915_gem_release(dev, file);
>  	mutex_unlock(&dev->struct_mutex);
> +
> +	if (drm_core_check_feature(dev, DRIVER_MODESET))
> +		intel_modeset_preclose(dev, file);
>  }
>  
>  void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 883af0b..4230e4a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13416,3 +13416,25 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>  		err_printf(m, "  VSYNC: %08x\n", error->transcoder[i].vsync);
>  	}
>  }
> +
> +void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
> +{
> +	struct intel_crtc *crtc;
> +
> +	for_each_intel_crtc(dev, crtc) {
> +		struct intel_unpin_work *work;
> +		unsigned long irqflags;
> +
> +		spin_lock_irqsave(&dev->event_lock, irqflags);
> +
> +		work = crtc->unpin_work;
> +
> +		if (work && work->event &&
> +		    work->event->base.file_priv == file) {
> +			kfree(work->event);
> +			work->event = NULL;
> +		}
> +
> +		spin_unlock_irqrestore(&dev->event_lock, irqflags);
> +	}

I wonder whether we shouldn't do this in the drm core, with a per-file
event list. Anyway, good for now together with the igt, we can pimp this
later.

Queued for -next, thanks for the patch.
-Daniel

> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 28d185d..8f04ba8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -888,6 +888,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_config *pipe_config);
>  int intel_format_to_fourcc(int format);
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
> +void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  
>  
>  /* intel_dp.c */
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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