From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [RFC 13/44] drm/i915: Added scheduler hook when closing DRM file handles Date: Wed, 23 Jul 2014 08:39:38 -0700 Message-ID: <20140723083938.25766a0a@jbarnes-desktop> References: <1403803475-16337-1-git-send-email-John.C.Harrison@Intel.com> <1403803475-16337-14-git-send-email-John.C.Harrison@Intel.com> <20140702112017.02b927ec@jbarnes-desktop> <53CFD068.5010707@Intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pa0-f47.google.com (mail-pa0-f47.google.com [209.85.220.47]) by gabe.freedesktop.org (Postfix) with ESMTP id 67BF76E65B for ; Wed, 23 Jul 2014 08:38:22 -0700 (PDT) Received: by mail-pa0-f47.google.com with SMTP id kx10so1948747pab.34 for ; Wed, 23 Jul 2014 08:38:22 -0700 (PDT) In-Reply-To: <53CFD068.5010707@Intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: John Harrison Cc: Intel-GFX@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, 23 Jul 2014 16:10:32 +0100 John Harrison wrote: > On 02/07/2014 19:20, Jesse Barnes wrote: > > On Thu, 26 Jun 2014 18:24:04 +0100 > > John.C.Harrison@Intel.com wrote: > > > >> From: John Harrison > >> > >> The scheduler decouples the submission of batch buffers to the driver with > >> submission of batch buffers to the hardware. Thus it is possible for an > >> application to submit work, then close the DRM handle and free up all the > >> resources that piece of work wishes to use before the work has even been > >> submitted to the hardware. To prevent this, the scheduler needs to be informed > >> of the DRM close event so that it can force through any outstanding work > >> attributed to that file handle. > >> --- > >> drivers/gpu/drm/i915/i915_dma.c | 3 +++ > >> drivers/gpu/drm/i915/i915_scheduler.c | 18 ++++++++++++++++++ > >> drivers/gpu/drm/i915/i915_scheduler.h | 2 ++ > >> 3 files changed, 23 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > >> index 494b156..6c9ce82 100644 > >> --- a/drivers/gpu/drm/i915/i915_dma.c > >> +++ b/drivers/gpu/drm/i915/i915_dma.c > >> @@ -42,6 +42,7 @@ > >> #include > >> #include > >> #include > >> +#include "i915_scheduler.h" > >> #include > >> #include > >> #include > >> @@ -1930,6 +1931,8 @@ void i915_driver_lastclose(struct drm_device * dev) > >> > >> void i915_driver_preclose(struct drm_device *dev, struct drm_file *file) > >> { > >> + i915_scheduler_closefile(dev, file); > >> + > >> mutex_lock(&dev->struct_mutex); > >> i915_gem_context_close(dev, file); > >> i915_gem_release(dev, file); > >> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c > >> index d9c1879..66a6568 100644 > >> --- a/drivers/gpu/drm/i915/i915_scheduler.c > >> +++ b/drivers/gpu/drm/i915/i915_scheduler.c > >> @@ -78,6 +78,19 @@ bool i915_scheduler_is_seqno_in_flight(struct intel_engine_cs *ring, > >> return found; > >> } > >> > >> +int i915_scheduler_closefile(struct drm_device *dev, struct drm_file *file) > >> +{ > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + struct i915_scheduler *scheduler = dev_priv->scheduler; > >> + > >> + if (!scheduler) > >> + return 0; > >> + > >> + /* Do stuff... */ > >> + > >> + return 0; > >> +} > >> + > >> #else /* CONFIG_DRM_I915_SCHEDULER */ > >> > >> int i915_scheduler_init(struct drm_device *dev) > >> @@ -85,4 +98,9 @@ int i915_scheduler_init(struct drm_device *dev) > >> return 0; > >> } > >> > >> +int i915_scheduler_closefile(struct drm_device *dev, struct drm_file *file) > >> +{ > >> + return 0; > >> +} > >> + > >> #endif /* CONFIG_DRM_I915_SCHEDULER */ > >> diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h > >> index 4044b6e..95641f6 100644 > >> --- a/drivers/gpu/drm/i915/i915_scheduler.h > >> +++ b/drivers/gpu/drm/i915/i915_scheduler.h > >> @@ -27,6 +27,8 @@ > >> > >> bool i915_scheduler_is_enabled(struct drm_device *dev); > >> int i915_scheduler_init(struct drm_device *dev); > >> +int i915_scheduler_closefile(struct drm_device *dev, > >> + struct drm_file *file); > >> > >> #ifdef CONFIG_DRM_I915_SCHEDULER > >> > > Yeah I guess the client could have passed a ref to some other process > > for tracking the outstanding work, so we need to complete it. > > > > But shouldn't that happen as part of the clearing of the outstanding > > requests in i915_gem_suspend() which is called from lastclose()? We do > > a gpu_idle() and retire_requests() in there already... > > > > Note that this is per file close not the global close. Individual DRM > file handles are closed whenever a user land app stops using DRM. When > that happens, the scheduler needs to clean up all references to that > handle. It is not just to ensure all work belonging to that handle has > completed but also to ensure the scheduler does not attempt to deference > dodgy file pointers later on. Ah yeah sorry, mixed it up with lastclose. Looks fine for per-client close. -- Jesse Barnes, Intel Open Source Technology Center