Re: [Intel-gfx] [PATCH 2/3] drm/i915: Use RPM as the barrier for controlling user mmap access

[Daniel Vetter]

On Thu, Oct 13, 2016 at 04:15:23PM +0100, Chris Wilson wrote:
> On Thu, Oct 13, 2016 at 04:44:23PM +0200, Daniel Vetter wrote:
> > On Tue, Oct 11, 2016 at 03:37:58PM +0100, Chris Wilson wrote:
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > > index 91910ffe0964..587a91af5a3f 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1469,7 +1469,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void 
> > > *data,
> > >*/
> > >   if (!i915_gem_object_has_struct_page(obj) ||
> > >   cpu_write_needs_clflush(obj)) {
> > > + intel_runtime_pm_get(dev_priv);
> > >   ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file);
> > > + intel_runtime_pm_put(dev_priv);
> > 
> > I'd move the rpm_get/put into gtt_pwrite_fast - there's only one caller,
> > and it's in the spirit of this patch of moving the rpm get/put closer to
> > where we really need it.
> 
> What I've also done is move rpm_get/rpm_put into i915_gem_release_mmap()
> and split out the RPM-suspend only i915_gem_release_all_mmaps() (if I
> can think of a good name to better distinguish the two I'll do that as
> well). The benefit being is that instead of simply asserting in one that
> we hold the rpm-wakeref we take it.
> 
> i915_gem_runtime_suspend__mmaps() ?

Or maybe throw a ggtt in there for giggles, but yeah.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Use RPM as the barrier for controlling user mmap access

[Chris Wilson]

On Thu, Oct 13, 2016 at 04:44:23PM +0200, Daniel Vetter wrote:
> On Tue, Oct 11, 2016 at 03:37:58PM +0100, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 91910ffe0964..587a91af5a3f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1469,7 +1469,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void 
> > *data,
> >  */
> > if (!i915_gem_object_has_struct_page(obj) ||
> > cpu_write_needs_clflush(obj)) {
> > +   intel_runtime_pm_get(dev_priv);
> > ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file);
> > +   intel_runtime_pm_put(dev_priv);
> 
> I'd move the rpm_get/put into gtt_pwrite_fast - there's only one caller,
> and it's in the spirit of this patch of moving the rpm get/put closer to
> where we really need it.

What I've also done is move rpm_get/rpm_put into i915_gem_release_mmap()
and split out the RPM-suspend only i915_gem_release_all_mmaps() (if I
can think of a good name to better distinguish the two I'll do that as
well). The benefit being is that instead of simply asserting in one that
we hold the rpm-wakeref we take it.

i915_gem_runtime_suspend__mmaps() ?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Use RPM as the barrier for controlling user mmap access

[Daniel Vetter]

On Tue, Oct 11, 2016 at 03:37:58PM +0100, Chris Wilson wrote:
> We can remove the false coupling between RPM and struct mutex by the
> observation that we can use the RPM wakeref as the barrier around user
> mmap access. That is as we tear down the user's PTE atomically from
> within rpm suspend and then to fault in new PTE requires the rpm
> wakeref, means that no user access is possible through those PTE without
> RPM being awake. Having made that observation, we can then remove the
> presumption of having to take rpm outside of struct_mutex and so allow
> fine grained acquisition of a wakeref around hw access rather than
> having to remember to acquire the wakeref early on.
> 
> Signed-off-by: Chris Wilson 
> Cc: Imre Deak 
> Cc: Daniel Vetter 
> Cc: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c| 22 --
>  drivers/gpu/drm/i915/i915_drv.c| 19 ---
>  drivers/gpu/drm/i915/i915_gem.c| 14 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c| 17 +
>  drivers/gpu/drm/i915/i915_gem_tiling.c |  4 
>  drivers/gpu/drm/i915/intel_uncore.c|  6 +++---
>  6 files changed, 21 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index d4ecc5283c2a..d4779abd89e3 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1400,14 +1400,9 @@ static int i915_hangcheck_info(struct seq_file *m, 
> void *unused)
>  static int ironlake_drpc_info(struct seq_file *m)
>  {
>   struct drm_i915_private *dev_priv = node_to_i915(m->private);
> - struct drm_device *dev = _priv->drm;
>   u32 rgvmodectl, rstdbyctl;
>   u16 crstandvid;
> - int ret;
>  
> - ret = mutex_lock_interruptible(>struct_mutex);
> - if (ret)
> - return ret;
>   intel_runtime_pm_get(dev_priv);
>  
>   rgvmodectl = I915_READ(MEMMODECTL);
> @@ -1415,7 +1410,6 @@ static int ironlake_drpc_info(struct seq_file *m)
>   crstandvid = I915_READ16(CRSTANDVID);
>  
>   intel_runtime_pm_put(dev_priv);
> - mutex_unlock(>struct_mutex);
>  
>   seq_printf(m, "HD boost: %s\n", yesno(rgvmodectl & MEMMODE_BOOST_EN));
>   seq_printf(m, "Boost freq: %d\n",
> @@ -2093,12 +2087,7 @@ static const char *swizzle_string(unsigned swizzle)
>  static int i915_swizzle_info(struct seq_file *m, void *data)
>  {
>   struct drm_i915_private *dev_priv = node_to_i915(m->private);
> - struct drm_device *dev = _priv->drm;
> - int ret;
>  
> - ret = mutex_lock_interruptible(>struct_mutex);
> - if (ret)
> - return ret;
>   intel_runtime_pm_get(dev_priv);
>  
>   seq_printf(m, "bit6 swizzle for X-tiling = %s\n",
> @@ -2138,7 +2127,6 @@ static int i915_swizzle_info(struct seq_file *m, void 
> *data)
>   seq_puts(m, "L-shaped memory detected\n");
>  
>   intel_runtime_pm_put(dev_priv);
> - mutex_unlock(>struct_mutex);
>  
>   return 0;
>  }
> @@ -4797,13 +4785,9 @@ i915_wedged_set(void *data, u64 val)
>   if (i915_reset_in_progress(_priv->gpu_error))
>   return -EAGAIN;
>  
> - intel_runtime_pm_get(dev_priv);
> -
>   i915_handle_error(dev_priv, val,
> "Manually setting wedged to %llu", val);
>  
> - intel_runtime_pm_put(dev_priv);
> -
>   return 0;
>  }
>  
> @@ -5038,22 +5022,16 @@ static int
>  i915_cache_sharing_get(void *data, u64 *val)
>  {
>   struct drm_i915_private *dev_priv = data;
> - struct drm_device *dev = _priv->drm;
>   u32 snpcr;
> - int ret;
>  
>   if (!(IS_GEN6(dev_priv) || IS_GEN7(dev_priv)))
>   return -ENODEV;
>  
> - ret = mutex_lock_interruptible(>struct_mutex);
> - if (ret)
> - return ret;
>   intel_runtime_pm_get(dev_priv);
>  
>   snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
>  
>   intel_runtime_pm_put(dev_priv);
> - mutex_unlock(>struct_mutex);
>  
>   *val = (snpcr & GEN6_MBC_SNPCR_MASK) >> GEN6_MBC_SNPCR_SHIFT;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 89d322215c84..31eee32fcf6d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2313,24 +2313,6 @@ static int intel_runtime_suspend(struct device *kdev)
>  
>   DRM_DEBUG_KMS("Suspending device\n");
>  
> - /*
> -  * We could deadlock here in case another thread holding struct_mutex
> -  * calls RPM suspend concurrently, since the RPM suspend will wait
> -  * first for this RPM suspend to finish. In this case the concurrent
> -  * RPM resume will be followed by its RPM suspend counterpart. Still
> -  * for consistency return -EAGAIN, which will reschedule this suspend.
> -  */
> - if (!mutex_trylock(>struct_mutex)) {
> - 

[Intel-gfx] [PATCH 2/3] drm/i915: Use RPM as the barrier for controlling user mmap access

[Chris Wilson]

We can remove the false coupling between RPM and struct mutex by the
observation that we can use the RPM wakeref as the barrier around user
mmap access. That is as we tear down the user's PTE atomically from
within rpm suspend and then to fault in new PTE requires the rpm
wakeref, means that no user access is possible through those PTE without
RPM being awake. Having made that observation, we can then remove the
presumption of having to take rpm outside of struct_mutex and so allow
fine grained acquisition of a wakeref around hw access rather than
having to remember to acquire the wakeref early on.

Signed-off-by: Chris Wilson 
Cc: Imre Deak 
Cc: Daniel Vetter 
Cc: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_debugfs.c| 22 --
 drivers/gpu/drm/i915/i915_drv.c| 19 ---
 drivers/gpu/drm/i915/i915_gem.c| 14 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c| 17 +
 drivers/gpu/drm/i915/i915_gem_tiling.c |  4 
 drivers/gpu/drm/i915/intel_uncore.c|  6 +++---
 6 files changed, 21 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index d4ecc5283c2a..d4779abd89e3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1400,14 +1400,9 @@ static int i915_hangcheck_info(struct seq_file *m, void 
*unused)
 static int ironlake_drpc_info(struct seq_file *m)
 {
struct drm_i915_private *dev_priv = node_to_i915(m->private);
-   struct drm_device *dev = _priv->drm;
u32 rgvmodectl, rstdbyctl;
u16 crstandvid;
-   int ret;
 
-   ret = mutex_lock_interruptible(>struct_mutex);
-   if (ret)
-   return ret;
intel_runtime_pm_get(dev_priv);
 
rgvmodectl = I915_READ(MEMMODECTL);
@@ -1415,7 +1410,6 @@ static int ironlake_drpc_info(struct seq_file *m)
crstandvid = I915_READ16(CRSTANDVID);
 
intel_runtime_pm_put(dev_priv);
-   mutex_unlock(>struct_mutex);
 
seq_printf(m, "HD boost: %s\n", yesno(rgvmodectl & MEMMODE_BOOST_EN));
seq_printf(m, "Boost freq: %d\n",
@@ -2093,12 +2087,7 @@ static const char *swizzle_string(unsigned swizzle)
 static int i915_swizzle_info(struct seq_file *m, void *data)
 {
struct drm_i915_private *dev_priv = node_to_i915(m->private);
-   struct drm_device *dev = _priv->drm;
-   int ret;
 
-   ret = mutex_lock_interruptible(>struct_mutex);
-   if (ret)
-   return ret;
intel_runtime_pm_get(dev_priv);
 
seq_printf(m, "bit6 swizzle for X-tiling = %s\n",
@@ -2138,7 +2127,6 @@ static int i915_swizzle_info(struct seq_file *m, void 
*data)
seq_puts(m, "L-shaped memory detected\n");
 
intel_runtime_pm_put(dev_priv);
-   mutex_unlock(>struct_mutex);
 
return 0;
 }
@@ -4797,13 +4785,9 @@ i915_wedged_set(void *data, u64 val)
if (i915_reset_in_progress(_priv->gpu_error))
return -EAGAIN;
 
-   intel_runtime_pm_get(dev_priv);
-
i915_handle_error(dev_priv, val,
  "Manually setting wedged to %llu", val);
 
-   intel_runtime_pm_put(dev_priv);
-
return 0;
 }
 
@@ -5038,22 +5022,16 @@ static int
 i915_cache_sharing_get(void *data, u64 *val)
 {
struct drm_i915_private *dev_priv = data;
-   struct drm_device *dev = _priv->drm;
u32 snpcr;
-   int ret;
 
if (!(IS_GEN6(dev_priv) || IS_GEN7(dev_priv)))
return -ENODEV;
 
-   ret = mutex_lock_interruptible(>struct_mutex);
-   if (ret)
-   return ret;
intel_runtime_pm_get(dev_priv);
 
snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
 
intel_runtime_pm_put(dev_priv);
-   mutex_unlock(>struct_mutex);
 
*val = (snpcr & GEN6_MBC_SNPCR_MASK) >> GEN6_MBC_SNPCR_SHIFT;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 89d322215c84..31eee32fcf6d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2313,24 +2313,6 @@ static int intel_runtime_suspend(struct device *kdev)
 
DRM_DEBUG_KMS("Suspending device\n");
 
-   /*
-* We could deadlock here in case another thread holding struct_mutex
-* calls RPM suspend concurrently, since the RPM suspend will wait
-* first for this RPM suspend to finish. In this case the concurrent
-* RPM resume will be followed by its RPM suspend counterpart. Still
-* for consistency return -EAGAIN, which will reschedule this suspend.
-*/
-   if (!mutex_trylock(>struct_mutex)) {
-   DRM_DEBUG_KMS("device lock contention, deffering suspend\n");
-   /*
-* Bump the expiration timestamp, otherwise the suspend won't
-* be rescheduled.
-*/
-