FBC locking v3
Subsystems (2)
cover FBC locking v3
From: Paulo Zanoni <[email protected]>
Hi
So this is a new take on the FBC locking improvements. This time I continue with the "introduce fbc.lock first, then reduce struct_mutex later" strategy in order to minimize the risks and facilitate review.
I also fixed a "lack of struct_mutex lock" regression I introduced when I first moved FBC to the frontbuffer tracking infrastructure.
Right now the only places where we should be holding the struct_mutex lock for FBC is on the 2 intel_fbc_update() calls: one at intel_post_plane_update() and another at intel_unpin_work_fn(). Notice that we'll probably remove the intel_fbc_update() call that's inside intel_unpin_work_fn() later, so we'll only required struct_mutex locking once. We can't get rid of the single struct_mutex lock requirement since we need to deal with the CFB allocation. Of course we can always rework that code a little bit more, but let's do it on later patches.
Thanks,
Paulo
Paulo Zanoni (8):
drm/i915: don't increment the FBC threshold at fbc_enable drm/i915: add the FBC mutex
drm/i915: remove unneded locks on debugs FBC functions drm/i915: remove struct_mutex lock from the FBC work function drm/i915: simplify FBC start/stop at invalidate/flush drm/i915: add struct_mutex WARNs to i915_gem_stolen.c drm/i915: reduce struct_mutex coverage at intel_crtc_page_flip() drm/i915: remove struct_mutex lock from intel_modeset_cleanup()
drivers/gpu/drm/i915/i915_debugfs.c | 8 +-- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem_stolen.c | 20 ++++++ drivers/gpu/drm/i915/intel_display.c | 16 +---- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_fbc.c | 110 +++++++++++++++++++++++++++------ 6 files changed, 120 insertions(+), 36 deletions(-)
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/8 drm/i915: don't increment the FBC threshold at fbc_enable
From: Paulo Zanoni <[email protected]>
We first set the threshold value when we're allocating the CFB, and then later at {ilk,gen7}_fbc_enable() we increment it in case we're using 16bpp. While that is correct, it is dangerous: if we rework the code a little bit in a way that allows us to call intel_fbc_enable() without necessarily calling i915_gem_stolen_setup_compression() first, we might end up incrementing threshold more than once. To prevent that, increment a temporary variable instead.
v2: Rebase.
--- drivers/gpu/drm/i915/intel_fbc.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/intel_fbc.c +6 -4
@@ -188,14 +188,15 @@ static void ilk_fbc_enable(struct drm_crtc *crtc)struct drm_i915_gem_object *obj =intel_fb_obj (fb);struct intel_crtc *intel_crtc =to_intel_crtc (crtc);u32 dpfc_ctl; +int threshold = dev_priv->fbc.threshold; dev_priv->fbc.enabled = true; dpfc_ctl =DPFC_CTL_PLANE (intel_crtc->plane);if (drm_format_plane_cpp (fb->pixel_format,0 ) ==2 ) - dev_priv->fbc.threshold++; + threshold++; -switch (dev_priv->fbc.threshold) { +switch (threshold) {case 4 :case 3 : dpfc_ctl |=DPFC_CTL_LIMIT_4X ;
@@ -259,6 +260,7 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)struct drm_i915_gem_object *obj =intel_fb_obj (fb);struct intel_crtc *intel_crtc =to_intel_crtc (crtc);u32 dpfc_ctl; +int threshold = dev_priv->fbc.threshold; dev_priv->fbc.enabled = true;
@@ -267,9 +269,9 @@ static void gen7_fbc_enable(struct drm_crtc *crtc) dpfc_ctl |=IVB_DPFC_CTL_PLANE (intel_crtc->plane);if (drm_format_plane_cpp (fb->pixel_format,0 ) ==2 ) - dev_priv->fbc.threshold++; + threshold++; -switch (dev_priv->fbc.threshold) { +switch (threshold) {case 4 :case 3 : dpfc_ctl |=DPFC_CTL_LIMIT_4X ;
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 1/8] drm/i915: don't increment the FBC threshold at fbc_enable
Quoted 10 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
We first set the threshold value when we're allocating the CFB, and then later at {ilk,gen7}_fbc_enable() we increment it in case we're using 16bpp. While that is correct, it is dangerous: if we rework the code a little bit in a way that allows us to call
intel_fbc_enable()without necessarily callingi915_gem_stolen_setup_compression()first, we might end up incrementing threshold more than once. To prevent that, increment a temporary variable instead.
Quoted 3 lines
v2: Rebase.
Signed-off-by Paulo Zanoni <[email protected]>
You can move all the fbc out of i915_gem_stolen.c now that we have intel_fbc.c. I don't even think we need to export a function to provide drm_mm_insert_node(&dev_priv->mm.stolen), but we probably should for ease of future maintenance.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter
AppliedRe: [PATCH 1/8] drm/i915: don't increment the FBC threshold at fbc_enable
Quoted 11 lines · Chris Wilson
Quoted 10 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
We first set the threshold value when we're allocating the CFB, and then later at {ilk,gen7}_fbc_enable() we increment it in case we're using 16bpp. While that is correct, it is dangerous: if we rework the code a little bit in a way that allows us to call
intel_fbc_enable()without necessarily callingi915_gem_stolen_setup_compression()first, we might end up incrementing threshold more than once. To prevent that, increment a temporary variable instead.
Quoted 5 lines
Quoted 3 lines
v2: Rebase.
Signed-off-by Paulo Zanoni <[email protected]>Reviewed-by Chris Wilson <[email protected]>
Queued for -next, thanks for the patch.
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 2/8 drm/i915: add the FBC mutex
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Make sure we're not going to have weird races in really weird cases where a lot of different CRTCs are doing rendering and modesets at the same time.
With this change, we can start removing the struct_mutex locking we have around FBC in the next patches. The only struct_mutex requirement will be around intel_fbc_update(), since it's the place that does the CFB stolen memory allocations.
v2:
- Rebase (6 months later)
- Also lock debugfs and stolen.
v3:
- Don't lock a single value read (Chris).
- Replace lockdep assertions with WARNs (Daniel).
- Improve commit message.
- Don't forget intel_pre_plane_update() locking.
--- drivers/gpu/drm/i915/i915_debugfs.c | 4 ++ drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem_stolen.c | 7 +++ drivers/gpu/drm/i915/intel_display.c | 10 +--- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_fbc.c | 86 +++++++++++++++++++++++++++++----- 6 files changed, 89 insertions(+), 20 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +4 -0
@@ -1619,6 +1619,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused) }intel_runtime_pm_get (dev_priv); +mutex_lock (&dev_priv->fbc.lock);if (intel_fbc_enabled (dev))seq_puts (m,"FBC enabled\n" );
@@ -1631,6 +1632,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)yesno (I915_READ (FBC_STATUS2 ) & FBC_COMPRESSION_MASK)); +mutex_unlock (&dev_priv->fbc.lock);intel_runtime_pm_put (dev_priv);return 0 ;
@@ -1661,6 +1663,7 @@ static int i915_fbc_fc_set(void *data, u64 val)return -ENODEV ;drm_modeset_lock_all (dev); +mutex_lock (&dev_priv->fbc.lock); reg =I915_READ (ILK_DPFC_CONTROL ); dev_priv->fbc.false_color = val;
@@ -1669,6 +1672,7 @@ static int i915_fbc_fc_set(void *data, u64 val) (reg | FBC_CTL_FALSE_COLOR) : (reg & ~FBC_CTL_FALSE_COLOR)); +mutex_unlock (&dev_priv->fbc.lock);drm_modeset_unlock_all (dev);return 0 ; }
drivers/gpu/drm/i915/i915_drv.h +1 -0
@@ -892,6 +892,7 @@ enum fb_op_origin { };struct i915_fbc { +struct mutex lock;unsigned long uncompressed_size;unsigned threshold;unsigned int fb_id;
drivers/gpu/drm/i915/i915_gem_stolen.c +7 -0
@@ -250,6 +250,8 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c {struct drm_i915_private *dev_priv = dev->dev_private; +WARN_ON (!mutex_is_locked (&dev_priv->fbc.lock)); +if (!drm_mm_initialized (&dev_priv->mm.stolen))return -ENODEV ;
@@ -266,6 +268,8 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; +WARN_ON (!mutex_is_locked (&dev_priv->fbc.lock)); +if (dev_priv->fbc.uncompressed_size ==0 )return ;
@@ -286,7 +290,10 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)if (!drm_mm_initialized (&dev_priv->mm.stolen))return ; +mutex_lock (&dev_priv->fbc.lock);i915_gem_stolen_cleanup_compression (dev); +mutex_unlock (&dev_priv->fbc.lock); +drm_mm_takedown (&dev_priv->mm.stolen); }
drivers/gpu/drm/i915/intel_display.c +2 -8
@@ -4741,7 +4741,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)static void intel_pre_plane_update (struct intel_crtc *crtc) {struct drm_device *dev = crtc->base.dev; -struct drm_i915_private *dev_priv = dev->dev_private;struct intel_crtc_atomic_commit *atomic = &crtc->atomic;struct drm_plane *p;
@@ -4758,13 +4757,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)if (atomic->wait_for_flips)intel_crtc_wait_for_pending_flips (&crtc->base); -if (atomic->disable_fbc && - dev_priv->fbc.crtc == crtc) { -mutex_lock (&dev->struct_mutex); -if (dev_priv->fbc.crtc == crtc) -intel_fbc_disable (dev); -mutex_unlock (&dev->struct_mutex); - } +if (atomic->disable_fbc) +intel_fbc_disable_crtc (crtc);if (crtc->atomic.disable_ips)hsw_disable_ips (crtc);
drivers/gpu/drm/i915/intel_drv.h +1 -0
@@ -1236,6 +1236,7 @@ bool intel_fbc_enabled(struct drm_device *dev);void intel_fbc_update (struct drm_device *dev);void intel_fbc_init (struct drm_i915_private *dev_priv);void intel_fbc_disable (struct drm_device *dev); +void intel_fbc_disable_crtc (struct intel_crtc *crtc);void intel_fbc_invalidate (struct drm_i915_private *dev_priv,unsigned int frontbuffer_bits,enum fb_op_origin origin);
drivers/gpu/drm/i915/intel_fbc.c +74 -12
187 lines not on this page · raw message
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 2/8] drm/i915: add the FBC mutex
Quoted 15 lines · Paulo Zanoni
drivers/gpu/drm/i915/intel_display.c +0
-1@@ -4741,7 +4741,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)static void intel_pre_plane_update (struct intel_crtc *crtc) {struct drm_device *dev = crtc->base.dev; -struct drm_i915_private *dev_priv = dev->dev_private;struct intel_crtc_atomic_commit *atomic = &crtc->atomic;struct drm_plane *p;@@ -4758,13 +4757,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)if (atomic->wait_for_flips)intel_crtc_wait_for_pending_flips (&crtc->base);
drivers/gpu/drm/i915/intel_display.c static void intel_pre_plane_update(struct intel_crtc *crtc) · line 4761
Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +2
-7-if (atomic->disable_fbc && - dev_priv->fbc.crtc == crtc) { -mutex_lock (&dev->struct_mutex); -if (dev_priv->fbc.crtc == crtc) -intel_fbc_disable (dev); -mutex_unlock (&dev->struct_mutex); - } +if (atomic->disable_fbc) +intel_fbc_disable_crtc (crtc);
Wrong patch? (This patch is called add fbc_mutex without touching struct_mutex)
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 2/8] drm/i915: add the FBC mutex
In reply to Paulo ZanoniQuoted 1 line · Paulo Zanoni
struct
i915_fbc{
/* This is always the inner lock when overlapping with struct_mutex */
drivers/gpu/drm/i915/i915_drv.h enum fb_op_origin { · line 898
Quoted 4 lines
drivers/gpu/drm/i915/i915_drv.h +1
-0+struct mutex lock;unsigned long uncompressed_size;unsigned threshold;unsigned int fb_id;
Just a comment to ease usage and review for ABBA.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 2/8] drm/i915: add the FBC mutex
In reply to Paulo ZanoniQuoted 20 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Make sure we're not going to have weird races in really weird cases where a lot of different CRTCs are doing rendering and modesets at the same time.
With this change, we can start removing the
struct_mutexlocking we have around FBC in the next patches. The onlystruct_mutexrequirement will be aroundintel_fbc_update(), since it's the place that does the CFB stolen memory allocations.v2:
- Rebase (6 months later)
- Also lock debugfs and stolen.
v3:
- Don't lock a single value read (Chris).
- Replace lockdep assertions with WARNs (Daniel).
- Improve commit message.
- Don't forgetintel_pre_plane_update()locking.
Quoted 1 line
Signed-off-by Paulo Zanoni <[email protected]>
I haven't spotted anything scary, but I only did a cursory scan of the patch for errors, and not down a delve into each function looking for trouble.
Can I pretty please ask that you split fbc out of i915_gem_stolen.c before this patch (rather than teach stolen more about fbc)? -Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 2/8] drm/i915: add the FBC mutex
2015-06-30 11:25 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 21 lines · Paulo Zanoni
Quoted 20 lines
From: Paulo Zanoni <[email protected]>
Make sure we're not going to have weird races in really weird cases where a lot of different CRTCs are doing rendering and modesets at the same time.
With this change, we can start removing the
struct_mutexlocking we have around FBC in the next patches. The onlystruct_mutexrequirement will be aroundintel_fbc_update(), since it's the place that does the CFB stolen memory allocations.v2:
- Rebase (6 months later)
- Also lock debugfs and stolen.
v3:
- Don't lock a single value read (Chris).
- Replace lockdep assertions with WARNs (Daniel).
- Improve commit message.
- Don't forgetintel_pre_plane_update()locking.
Quoted 5 lines
Quoted 1 line
Signed-off-by Paulo Zanoni <[email protected]>I haven't spotted anything scary, but I only did a cursory scan of the patch for errors, and not down a delve into each function looking for trouble.
Thanks for the reviews! I'll add the ABBA comment and introduce a new patch for the struct_mutex removal you spotted.
Quoted 3 lines
Can I pretty please ask that you split fbc out of
i915_gem_stolen.cbefore this patch (rather than teach stolen more about fbc)?
Ok, I'll do it.
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 3/8 drm/i915: remove unneded locks on debugs FBC functions
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
The first one is helpless against a single variable read. The second one is not needed anymore now that we have fbc.lock.
--- drivers/gpu/drm/i915/i915_debugfs.c | 4 ---- 1 file changed, 4 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +0 -4
@@ -1646,9 +1646,7 @@ static int i915_fbc_fc_get(void *data, u64 *val)if (INTEL_INFO (dev)->gen <7 || !HAS_FBC (dev))return -ENODEV ; -drm_modeset_lock_all (dev); *val = dev_priv->fbc.false_color; -drm_modeset_unlock_all (dev);return 0 ; }
@@ -1662,7 +1660,6 @@ static int i915_fbc_fc_set(void *data, u64 val)if (INTEL_INFO (dev)->gen <7 || !HAS_FBC (dev))return -ENODEV ; -drm_modeset_lock_all (dev);mutex_lock (&dev_priv->fbc.lock); reg =I915_READ (ILK_DPFC_CONTROL );
@@ -1673,7 +1670,6 @@ static int i915_fbc_fc_set(void *data, u64 val) (reg & ~FBC_CTL_FALSE_COLOR));mutex_unlock (&dev_priv->fbc.lock); -drm_modeset_unlock_all (dev);return 0 ; }
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 4/8 drm/i915: remove struct_mutex lock from the FBC work function
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Now that we have the FBC mutex to protect the actual FBC data, we can remove the struct_mutex from here since we're not dealing with stolen memory at this point.
--- drivers/gpu/drm/i915/intel_fbc.c | 2 -- 1 file changed, 2 deletions(-)
drivers/gpu/drm/i915/intel_fbc.c +0 -2
@@ -335,7 +335,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)struct drm_device *dev = work->crtc->dev;struct drm_i915_private *dev_priv = dev->dev_private; -mutex_lock (&dev->struct_mutex);mutex_lock (&dev_priv->fbc.lock);if (work == dev_priv->fbc.fbc_work) {/* Double check that we haven't switched fb without cancelling
@@ -352,7 +351,6 @@ static void intel_fbc_work_fn(struct work_struct *__work) dev_priv->fbc.fbc_work =NULL ; }mutex_unlock (&dev_priv->fbc.lock); -mutex_unlock (&dev->struct_mutex);kfree (work); }
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 5/8 drm/i915: simplify FBC start/stop at invalidate/flush
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
The problem with calling intel_fbc_update() at flush is that it fully rechecks and recomputes the FBC state, and that includes reallocating the CFB, which requires a struct_mutex lock that we don't always have.
The lack of struct_mutex lock can be considered a regression from:
commit dbef0f15b5c83231dacb214dbf9a6dba063ca21c
Author: Paulo Zanoni <[email protected]> Date: Fri Feb 13 17:23:46 2015 -0200
drm/i915: add frontbuffer tracking to FBC
So introduce intel_fbc_stop() that doesn't unset fbc.crtc, then call stop/enable at invalidate/flush.
Notice that invalidate/flush is only called by the frontbuffer tracking infrastrucutre, so it's safe to not do a full intel_fbc_update() here.
--- drivers/gpu/drm/i915/intel_fbc.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/intel_fbc.c +14 -4
@@ -422,7 +422,7 @@ static void intel_fbc_enable(struct drm_crtc *crtc)schedule_delayed_work (&work->work,msecs_to_jiffies (50 )); } -static void __intel_fbc_disable (struct drm_device *dev) +static void intel_fbc_stop (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;
@@ -434,6 +434,13 @@ static void __intel_fbc_disable(struct drm_device *dev)return ; dev_priv->display.disable_fbc (dev); +} + +static void __intel_fbc_disable (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +intel_fbc_stop (dev); dev_priv->fbc.crtc =NULL ; }
@@ -753,7 +760,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);if (dev_priv->fbc.busy_bits) -__intel_fbc_disable (dev); +intel_fbc_stop (dev);mutex_unlock (&dev_priv->fbc.lock); }
@@ -770,8 +777,11 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, dev_priv->fbc.busy_bits &= ~frontbuffer_bits; -if (!dev_priv->fbc.busy_bits) -__intel_fbc_update (dev); +if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) { +if (dev_priv->fbc.enabled) +intel_fbc_stop (dev); +intel_fbc_enable (&dev_priv->fbc.crtc->base); + } out:mutex_unlock (&dev_priv->fbc.lock);
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 5/8] drm/i915: simplify FBC start/stop at invalidate/flush
Quoted 5 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
The problem with calling
intel_fbc_update()at flush is that it fully rechecks and recomputes the FBC state, and that includes reallocating the CFB, which requires astruct_mutexlock that we don't always have.
Actually, that is something we should fix with a stolen_mutex (will be needed elsewhere).
Quoted 5 lines
The lack of
struct_mutexlock can be considered a regression from:commit
dbef0f15b5c83231dacb214dbf9a6dba063ca21cAuthor: Paulo Zanoni <[email protected]> Date: Fri Feb 13 17:23:46 2015 -0200
Quoted 8 lines
drm/i915: add frontbuffer tracking to FBC
So introduce
intel_fbc_stop()that doesn't unset fbc.crtc, then call stop/enable at invalidate/flush.Notice that invalidate/flush is only called by the frontbuffer tracking infrastrucutre, so it's safe to not do a full
intel_fbc_update()here.
I think intel_fbc_update() is confusing. I can understand start/stop, but update to me is more akin to a flush. If it was called flush, then I would not expect it to enable or disable fbc or do any reallocations at all.
static void intel_fbc_flush(struct drm_device *dev)
{
if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) {
if (dev_priv->fbc.enabled)
intel_fbc_stop(dev);
intel_fbc_enable(&dev_priv->fbc.crtc->base);
}
}And now I look at enable/disable, start/stop and am confused as to what all the stages actually are.
I presume that start/stop are the highest, and control the sw state. And that enable/disable are just hw interaction. And who sets fbc.enabled? start()? enable()? disable()? stop()?
In confusion,
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 5/8] drm/i915: simplify FBC start/stop at invalidate/flush
2015-06-30 11:34 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 38 lines · Paulo Zanoni
Quoted 5 lines
From: Paulo Zanoni <[email protected]>
The problem with calling
intel_fbc_update()at flush is that it fully rechecks and recomputes the FBC state, and that includes reallocating the CFB, which requires astruct_mutexlock that we don't always have.Actually, that is something we should fix with a
stolen_mutex(will be needed elsewhere).Quoted 5 lines
The lack of
struct_mutexlock can be considered a regression from:commit
dbef0f15b5c83231dacb214dbf9a6dba063ca21cAuthor: Paulo Zanoni <[email protected]> Date: Fri Feb 13 17:23:46 2015 -0200Quoted 8 lines
drm/i915: add frontbuffer tracking to FBC
So introduce
intel_fbc_stop()that doesn't unset fbc.crtc, then call stop/enable at invalidate/flush.Notice that invalidate/flush is only called by the frontbuffer tracking infrastrucutre, so it's safe to not do a full
intel_fbc_update()here.I think
intel_fbc_update()is confusing. I can understand start/stop, but update to me is more akin to a flush. If it was called flush, then I would not expect it to enable or disable fbc or do any reallocations at all.static void
intel_fbc_flush(struct drm_device *dev){ if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) { if (dev_priv->fbc.enabled) intel_fbc_stop(dev); intel_fbc_enable(&dev_priv->fbc.crtc->base); } }
Quoted 8 lines
And now I look at enable/disable, start/stop and am confused as to what all the stages actually are.
I presume that start/stop are the highest, and control the sw state. And that enable/disable are just hw interaction. And who sets fbc.enabled?
start()?enable()?disable()?stop()?In confusion,
I understand your concerns and I agree with you that this is confusing. I also agree that the addition of stop() makes things even worse. One of the problems is that intel_fbc_update() does "everything": it picks the CRTC, it can enable FBC, it can disable FBC, it can change the CRTC, etc. So we have: update(), enable(), disable(), flush() and invalidate(), and the patch added stop().
I had some patches that would move us to enable/disable (high level) activate/deactivate (low level), flush/invalidate (wrappers for activate/deactivate) and update (highest level). This would make the naming scheme similar to PSR. I wanted to merge the locking fixes first, but I can put everything on the same series if you want. Or leave this patch out of the "locking" series and add it to the next series...
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 5/8] drm/i915: simplify FBC start/stop at invalidate/flush
Quoted 14 lines · Paulo Zanoni
2015-06-30 11:34 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 5 lines
I presume that start/stop are the highest, and control the sw state. And that enable/disable are just hw interaction. And who sets fbc.enabled?
start()?enable()?disable()?stop()?In confusion,
I understand your concerns and I agree with you that this is confusing. I also agree that the addition of
stop()makes things even worse. One of the problems is thatintel_fbc_update()does "everything": it picks the CRTC, it can enable FBC, it can disable FBC, it can change the CRTC, etc. So we have:update(),enable(),disable(),flush()andinvalidate(), and the patch addedstop().
Quoted 7 lines
I had some patches that would move us to enable/disable (high level) activate/deactivate (low level), flush/invalidate (wrappers for activate/deactivate) and update (highest level). This would make the naming scheme similar to PSR. I wanted to merge the locking fixes first, but I can put everything on the same series if you want. Or leave this patch out of the "locking" series and add it to the next series...
To keep it minimal, a quick outline comment telling me the layers and ordering would be of use right now to review the patches, and longer term to review the code.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 6/8 drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need struct_mutex when touching dev_priv->mm.stolen.
--- drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
drivers/gpu/drm/i915/i915_gem_stolen.c +13 -0
@@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,int compression_threshold =1 ;int ret; +WARN_ON (!mutex_is_locked (&dev->struct_mutex)); +/* HACK: This code depends on what we will do in *_enable_fbc. If that * code changes, this code needs to change as well. *
@@ -198,6 +200,8 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)struct drm_mm_node *uninitialized_var (compressed_llb );int ret; +WARN_ON (!mutex_is_locked (&dev->struct_mutex)); + ret =find_compression_threshold (dev, &dev_priv->fbc.compressed_fb, size, fb_cpp);if (!ret)
@@ -250,6 +254,7 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c {struct drm_i915_private *dev_priv = dev->dev_private; +WARN_ON (!mutex_is_locked (&dev->struct_mutex));WARN_ON (!mutex_is_locked (&dev_priv->fbc.lock));if (!drm_mm_initialized (&dev_priv->mm.stolen))
@@ -287,6 +292,8 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; +WARN_ON (!mutex_is_locked (&dev->struct_mutex)); +if (!drm_mm_initialized (&dev_priv->mm.stolen))return ;
@@ -349,6 +356,8 @@ i915_pages_create_for_stolen(struct drm_device *dev,struct sg_table *st;struct scatterlist *sg; +WARN_ON (!mutex_is_locked (&dev->struct_mutex)); +DRM_DEBUG_DRIVER ("offset=0x%x, size=%d\n" , offset, size);BUG_ON (offset > dev_priv->gtt.stolen_size - size);
@@ -445,6 +454,8 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)struct drm_mm_node *stolen;int ret; +WARN_ON (!mutex_is_locked (&dev->struct_mutex)); +if (!drm_mm_initialized (&dev_priv->mm.stolen))return NULL ;
@@ -485,6 +496,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,struct i915_vma *vma;int ret; +WARN_ON (!mutex_is_locked (&dev->struct_mutex)); +if (!drm_mm_initialized (&dev_priv->mm.stolen))return NULL ;
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
Quoted 15 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/gpu/drm/i915/i915_gem_stolen.c +0
-0
drivers/gpu/drm/i915/i915_gem_stolen.c static int find_compression_threshold(struct drm_device *dev, · line 163
Quoted 4 lines
drivers/gpu/drm/i915/i915_gem_stolen.c +1
-0@@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,int compression_threshold =1 ;int ret; +WARN_ON (!mutex_is_locked (&dev->struct_mutex));
I'm not a huge fan of vague mutex warnings that don't even check the owner. I'm espcially not a fan of adding a WARN and not handling the error. -Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
2015-06-30 11:15 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 19 lines · Paulo Zanoni
Quoted 18 lines
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/gpu/drm/i915/i915_gem_stolen.c +0
-0@@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,int compression_threshold =1 ;int ret;
Quoted 4 lines
drivers/gpu/drm/i915/i915_gem_stolen.c static int find_compression_threshold(struct drm_device *dev, · line 163
Quoted 1 line
drivers/gpu/drm/i915/i915_gem_stolen.c +1
-0+WARN_ON (!mutex_is_locked (&dev->struct_mutex));I'm not a huge fan of vague mutex warnings that don't even check the owner. I'm espcially not a fan of adding a WARN and not handling the error.
But then, what exactly is your proposal? What would you like to see here?
We can discard this patch if you want. But I hope you're not advocating for lockdep_assert_held(), because if I switch to lockdep, then Daniel is going to deny it again. Also, this type of WARN_ON is a common pattern on our codebase...
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
Quoted 25 lines · Paulo Zanoni
2015-06-30 11:15 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 19 lines · Paulo Zanoni
Quoted 18 lines
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/gpu/drm/i915/i915_gem_stolen.c +0
-0@@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,int compression_threshold =1 ;int ret;Quoted 4 lines
drivers/gpu/drm/i915/i915_gem_stolen.c static int find_compression_threshold(struct drm_device *dev, · line 163
Quoted 1 line
drivers/gpu/drm/i915/i915_gem_stolen.c +1
-0+WARN_ON (!mutex_is_locked (&dev->struct_mutex));I'm not a huge fan of vague mutex warnings that don't even check the owner. I'm espcially not a fan of adding a WARN and not handling the error.
Quoted 6 lines
But then, what exactly is your proposal? What would you like to see here?
We can discard this patch if you want. But I hope you're not advocating for
lockdep_assert_held(), because if I switch to lockdep, then Daniel is going to deny it again. Also, this type ofWARN_ONis a common pattern on our codebase...
I'm just trying to convince Daniel that blindly using this pattern is the wrong approach and encouraging a proliferation of unhandled WARN_ON doesn't improve driver robustness.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
Quoted 28 lines · Chris Wilson
Quoted 27 lines · Paulo Zanoni
2015-06-30 11:15 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 19 lines · Paulo Zanoni
Quoted 18 lines
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/gpu/drm/i915/i915_gem_stolen.c +0
-0@@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,int compression_threshold =1 ;int ret;Quoted 4 lines
drivers/gpu/drm/i915/i915_gem_stolen.c static int find_compression_threshold(struct drm_device *dev, · line 163
Quoted 1 line
drivers/gpu/drm/i915/i915_gem_stolen.c +1
-0+WARN_ON (!mutex_is_locked (&dev->struct_mutex));I'm not a huge fan of vague mutex warnings that don't even check the owner. I'm espcially not a fan of adding a WARN and not handling the error.
But then, what exactly is your proposal? What would you like to see here?
Quoted 8 lines
Quoted 4 lines
We can discard this patch if you want. But I hope you're not advocating for
lockdep_assert_held(), because if I switch to lockdep, then Daniel is going to deny it again. Also, this type ofWARN_ONis a common pattern on our codebase...I'm just trying to convince Daniel that blindly using this pattern is the wrong approach and encouraging a proliferation of unhandled
WARN_ONdoesn't improve driver robustness.
I think they serve as useful documentation at the very least, whether in lockdep form, WARN form, or BUG form. It's not really something we can recover from either (maybe returning early before touching data?), so...
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
Quoted 3 lines · Jesse Barnes
I think they serve as useful documentation at the very least, whether in lockdep form, WARN form, or BUG form. It's not really something we can recover from either (maybe returning early before touching data?), so...
As a debug aide they are less useful than lockdep and no more useful than lockdep for documentation purposes.
But it feels like more and more of the WARN_ONs are instead of error handling. See the olr fiasco.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
In reply to Jesse BarnesQuoted 34 lines · Jesse Barnes
Quoted 33 lines · Chris Wilson
Quoted 31 lines · Paulo Zanoni
2015-06-30 11:15 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 19 lines · Paulo Zanoni
Quoted 18 lines
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/gpu/drm/i915/i915_gem_stolen.c +0
-0@@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,int compression_threshold =1 ;int ret;Quoted 4 lines
drivers/gpu/drm/i915/i915_gem_stolen.c static int find_compression_threshold(struct drm_device *dev, · line 163
Quoted 1 line
drivers/gpu/drm/i915/i915_gem_stolen.c +1
-0+WARN_ON (!mutex_is_locked (&dev->struct_mutex));I'm not a huge fan of vague mutex warnings that don't even check the owner. I'm espcially not a fan of adding a WARN and not handling the error.
But then, what exactly is your proposal? What would you like to see here?
We can discard this patch if you want. But I hope you're not advocating for
lockdep_assert_held(), because if I switch to lockdep, then Daniel is going to deny it again. Also, this type ofWARN_ONis a common pattern on our codebase...
Quoted 7 lines
Quoted 3 lines
I'm just trying to convince Daniel that blindly using this pattern is the wrong approach and encouraging a proliferation of unhandled
WARN_ONdoesn't improve driver robustness.I think they serve as useful documentation at the very least, whether in lockdep form, WARN form, or BUG form. It's not really something we can recover from either (maybe returning early before touching data?), so...
Not grabbing a lock is generally a harmless error since real races out there are rare with X being single-threaded and all that. Especially in stuff called from modeset code. Hence I think just WARN_ON plus continuing on with blissful ignorance is the best approach.
I don't the lockdep versions personally since they don't work when lockdep is disabled, which is pretty much always the case. Might be useful to do an assert_mutex_held which always does the most paranoid check (i.e. WARN_ON without lockdep, lockdep_assert_held with lockdep). -Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
Quoted 48 lines · Daniel Vetter
Quoted 34 lines · Jesse Barnes
Quoted 33 lines · Chris Wilson
Quoted 31 lines · Paulo Zanoni
2015-06-30 11:15 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 19 lines · Paulo Zanoni
Quoted 18 lines
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/gpu/drm/i915/i915_gem_stolen.c +0
-0@@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,int compression_threshold =1 ;int ret;Quoted 4 lines
drivers/gpu/drm/i915/i915_gem_stolen.c static int find_compression_threshold(struct drm_device *dev, · line 163
Quoted 1 line
drivers/gpu/drm/i915/i915_gem_stolen.c +1
-0+WARN_ON (!mutex_is_locked (&dev->struct_mutex));I'm not a huge fan of vague mutex warnings that don't even check the owner. I'm espcially not a fan of adding a WARN and not handling the error.
But then, what exactly is your proposal? What would you like to see here?
We can discard this patch if you want. But I hope you're not advocating for
lockdep_assert_held(), because if I switch to lockdep, then Daniel is going to deny it again. Also, this type ofWARN_ONis a common pattern on our codebase...Quoted 7 lines
Quoted 3 lines
I'm just trying to convince Daniel that blindly using this pattern is the wrong approach and encouraging a proliferation of unhandled
WARN_ONdoesn't improve driver robustness.I think they serve as useful documentation at the very least, whether in lockdep form, WARN form, or BUG form. It's not really something we can recover from either (maybe returning early before touching data?), so...
Not grabbing a lock is generally a harmless error since real races out there are rare with X being single-threaded and all that. Especially in stuff called from modeset code. Hence I think just
WARN_ONplus continuing on with blissful ignorance is the best approach.
Quoted 4 lines
I don't the lockdep versions personally since they don't work when lockdep is disabled, which is pretty much always the case. Might be useful to do an
assert_mutex_heldwhich always does the most paranoid check (i.e.WARN_ONwithout lockdep,lockdep_assert_heldwith lockdep).
Maybe we should add WARN_ONs to the lockdep_assert macros in the !CONFIG_LOCKDEP case. That would give us documentation, checking in both cases, and everyone would be happy, right?
Jesse
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
Quoted 49 lines · Jesse Barnes
Quoted 48 lines · Daniel Vetter
Quoted 34 lines · Jesse Barnes
Quoted 33 lines · Chris Wilson
Quoted 31 lines · Paulo Zanoni
2015-06-30 11:15 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 19 lines · Paulo Zanoni
Quoted 18 lines
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/gpu/drm/i915/i915_gem_stolen.c +0
-0@@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev,int compression_threshold =1 ;int ret;Quoted 4 lines
drivers/gpu/drm/i915/i915_gem_stolen.c static int find_compression_threshold(struct drm_device *dev, · line 163
Quoted 1 line
drivers/gpu/drm/i915/i915_gem_stolen.c +1
-0+WARN_ON (!mutex_is_locked (&dev->struct_mutex));I'm not a huge fan of vague mutex warnings that don't even check the owner. I'm espcially not a fan of adding a WARN and not handling the error.
But then, what exactly is your proposal? What would you like to see here?
We can discard this patch if you want. But I hope you're not advocating for
lockdep_assert_held(), because if I switch to lockdep, then Daniel is going to deny it again. Also, this type ofWARN_ONis a common pattern on our codebase...Quoted 7 lines
Quoted 3 lines
I'm just trying to convince Daniel that blindly using this pattern is the wrong approach and encouraging a proliferation of unhandled
WARN_ONdoesn't improve driver robustness.I think they serve as useful documentation at the very least, whether in lockdep form, WARN form, or BUG form. It's not really something we can recover from either (maybe returning early before touching data?), so...
Not grabbing a lock is generally a harmless error since real races out there are rare with X being single-threaded and all that. Especially in stuff called from modeset code. Hence I think just
WARN_ONplus continuing on with blissful ignorance is the best approach.
Quoted 8 lines
Quoted 4 lines
I don't the lockdep versions personally since they don't work when lockdep is disabled, which is pretty much always the case. Might be useful to do an
assert_mutex_heldwhich always does the most paranoid check (i.e.WARN_ONwithout lockdep,lockdep_assert_heldwith lockdep).Maybe we should add
WARN_ONsto thelockdep_assertmacros in the !CONFIG_LOCKDEP case. That would give us documentation, checking in both cases, and everyone would be happy, right?
tbh never tried that ;-)
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
In reply to Paulo ZanoniQuoted 4 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.
As I elluded to in patch 5, I think the stolen warns are a misstep. -Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
Quoted 7 lines · Chris Wilson
Quoted 4 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.As I elluded to in patch 5, I think the stolen warns are a misstep.
Imo switching to a separate stolen_mutex should be a separate patch, this just documents the current rules. Which seems fine to me. -Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
Quoted 11 lines · Daniel Vetter
Quoted 7 lines · Chris Wilson
Quoted 4 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.As I elluded to in patch 5, I think the stolen warns are a misstep.
Imo switching to a separate
stolen_mutexshould be a separate patch, this just documents the current rules. Which seems fine to me.
Introducing a stolen mutex won't be a very much larger patch, and the current locking rules are an impediment for use elsewhere. -Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
2015-07-01 11:02 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 8 lines · Daniel Vetter
Quoted 7 lines · Chris Wilson
Quoted 6 lines · Paulo Zanoni
Quoted 4 lines
From: Paulo Zanoni <[email protected]>
Let's make sure the future Paulos don't forget that we need
struct_mutexwhen touchingdev_priv->mm.stolen.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
As I elluded to in patch 5, I think the stolen warns are a misstep.
Imo switching to a separate
stolen_mutexshould be a separate patch, this just documents the current rules. Which seems fine to me.Introducing a stolen mutex won't be a very much larger patch, and the current locking rules are an impediment for use elsewhere.
I wrote the stolen_mutex patches yesterday, I'll send them soon.
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 7/8 drm/i915: reduce struct_mutex coverage at intel_crtc_page_flip()
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Neither intel_fbc_disable() nor intel_frontbuffer_flip_prepare() need that lock.
That intel_fbc_disable() call over there can also certainly be improved, but let's do it on its own commit.
--- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +1 -1
@@ -11445,11 +11445,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,i915_gem_track_fb (intel_fb_obj (work->old_fb), obj,to_intel_plane (primary)->frontbuffer_bit); +mutex_unlock (&dev->struct_mutex);intel_fbc_disable (dev);intel_frontbuffer_flip_prepare (dev,to_intel_plane (primary)->frontbuffer_bit); -mutex_unlock (&dev->struct_mutex);trace_i915_flip_request (intel_crtc->plane, obj);
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 8/8 drm/i915: remove struct_mutex lock from intel_modeset_cleanup()
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Neither intel_unregister_dsm_handler() nor intel_fbc_disable() need the lock.
--- drivers/gpu/drm/i915/intel_display.c | 4 ---- 1 file changed, 4 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -4
@@ -15550,14 +15550,10 @@ void intel_modeset_cleanup(struct drm_device *dev)*/ drm_kms_helper_poll_fini (dev); -mutex_lock (&dev->struct_mutex); -intel_unregister_dsm_handler ();intel_fbc_disable (dev); -mutex_unlock (&dev->struct_mutex); -/* flush any delayed tasks or pending work */ flush_scheduled_work ();
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx