On Wed, Jul 27, 2016 at 12:14:55PM +0100, Chris Wilson wrote: > The individual bits inside obj->frontbuffer_bits are protected by each > plane->mutex, but the whole bitfield may be accessed by multiple KMS > operations simultaneously and so the RMW need to be under atomics. > However, for updating the single field we do not need to mandate that it > be under the struct_mutex, one more step towards its removal as the de > facto BKL. > > Signed-off-by: Chris Wilson <[email protected]> > Cc: Daniel Vetter <[email protected]>
Reviewed-by: Daniel Vetter <[email protected]> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 6 ++++-- > drivers/gpu/drm/i915/i915_drv.h | 4 +--- > drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++------- > drivers/gpu/drm/i915/intel_display.c | 18 ++++++------------ > drivers/gpu/drm/i915/intel_drv.h | 20 ++++++++++++++------ > drivers/gpu/drm/i915/intel_frontbuffer.c | 23 +++++++++-------------- > 6 files changed, 45 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index fcfa9ca6b50a..10a346237795 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -138,6 +138,7 @@ describe_obj(struct seq_file *m, struct > drm_i915_gem_object *obj) > struct drm_i915_private *dev_priv = to_i915(obj->base.dev); > struct intel_engine_cs *engine; > struct i915_vma *vma; > + unsigned int frontbuffer_bits; > int pin_count = 0; > enum intel_engine_id id; > > @@ -204,8 +205,9 @@ describe_obj(struct seq_file *m, struct > drm_i915_gem_object *obj) > if (engine) > seq_printf(m, " (%s)", engine->name); > > - if (obj->frontbuffer_bits) > - seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits); > + frontbuffer_bits = atomic_read(&obj->frontbuffer_bits); > + if (frontbuffer_bits) > + seq_printf(m, " (frontbuffer: 0x%03x)", frontbuffer_bits); > } > > static int i915_gem_object_list_info(struct seq_file *m, void *data) > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a24d31e3e014..b6b9a1f78238 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2127,8 +2127,6 @@ struct drm_i915_gem_object_ops { > */ > #define INTEL_MAX_SPRITE_BITS_PER_PIPE 5 > #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8 > -#define INTEL_FRONTBUFFER_BITS \ > - (INTEL_FRONTBUFFER_BITS_PER_PIPE * I915_MAX_PIPES) > #define INTEL_FRONTBUFFER_PRIMARY(pipe) \ > (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))) > #define INTEL_FRONTBUFFER_CURSOR(pipe) \ > @@ -2216,7 +2214,7 @@ struct drm_i915_gem_object { > unsigned int cache_level:3; > unsigned int cache_dirty:1; > > - unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS; > + atomic_t frontbuffer_bits; > > unsigned int has_wc_mmap; > /** Count of VMA actually bound by this object */ > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 7db0808f6961..bc5bc5ccdde0 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4031,7 +4031,7 @@ void i915_gem_free_object(struct drm_gem_object > *gem_obj) > if (obj->stolen) > i915_gem_object_unpin_pages(obj); > > - WARN_ON(obj->frontbuffer_bits); > + WARN_ON(atomic_read(&obj->frontbuffer_bits)); > > if (obj->pages && obj->madv == I915_MADV_WILLNEED && > dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES && > @@ -4549,16 +4549,20 @@ void i915_gem_track_fb(struct drm_i915_gem_object > *old, > struct drm_i915_gem_object *new, > unsigned frontbuffer_bits) > { > + /* Control of individual bits within the bitfield are guarded by > + * the owning plane->mutex, i.e. we can never see concurrent > + * manipulation of individual bits. But since the bitfield as a whole > + * is updated using RMW, we need to use atomics in order to update > + * the bits. > + */ > if (old) { > - WARN_ON(!mutex_is_locked(&old->base.dev->struct_mutex)); > - WARN_ON(!(old->frontbuffer_bits & frontbuffer_bits)); > - old->frontbuffer_bits &= ~frontbuffer_bits; > + WARN_ON(!(atomic_read(&old->frontbuffer_bits) & > frontbuffer_bits)); > + atomic_andnot(frontbuffer_bits, &old->frontbuffer_bits); > } > > if (new) { > - WARN_ON(!mutex_is_locked(&new->base.dev->struct_mutex)); > - WARN_ON(new->frontbuffer_bits & frontbuffer_bits); > - new->frontbuffer_bits |= frontbuffer_bits; > + WARN_ON(atomic_read(&new->frontbuffer_bits) & frontbuffer_bits); > + atomic_or(frontbuffer_bits, &new->frontbuffer_bits); > } > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index ed2069c56036..1c70f68328b4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2600,7 +2600,8 @@ valid_fb: > primary->fb = primary->state->fb = fb; > primary->crtc = primary->state->crtc = &intel_crtc->base; > intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary)); > - obj->frontbuffer_bits |= to_intel_plane(primary)->frontbuffer_bit; > + atomic_or(to_intel_plane(primary)->frontbuffer_bit, > + &obj->frontbuffer_bits); > } > > static void i9xx_update_primary_plane(struct drm_plane *primary, > @@ -13807,19 +13808,12 @@ static void intel_atomic_track_fbs(struct > drm_atomic_state *state) > { > struct drm_plane_state *old_plane_state; > struct drm_plane *plane; > - struct drm_i915_gem_object *obj, *old_obj; > - struct intel_plane *intel_plane; > int i; > > - mutex_lock(&state->dev->struct_mutex); > - for_each_plane_in_state(state, plane, old_plane_state, i) { > - obj = intel_fb_obj(plane->state->fb); > - old_obj = intel_fb_obj(old_plane_state->fb); > - intel_plane = to_intel_plane(plane); > - > - i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit); > - } > - mutex_unlock(&state->dev->struct_mutex); > + for_each_plane_in_state(state, plane, old_plane_state, i) > + i915_gem_track_fb(intel_fb_obj(old_plane_state->fb), > + intel_fb_obj(plane->state->fb), > + to_intel_plane(plane)->frontbuffer_bit); > } > > /** > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 01056ce8d461..5294039cf238 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1147,27 +1147,35 @@ unsigned int intel_fb_align_height(struct drm_device > *dev, > uint64_t fb_format_modifier); > > void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, > - enum fb_op_origin origin); > + enum fb_op_origin origin, > + unsigned int frontbuffer_bits); > static inline void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, > enum fb_op_origin origin) > { > - if (!obj->frontbuffer_bits) > + unsigned int frontbuffer_bits; > + > + frontbuffer_bits = atomic_read(&obj->frontbuffer_bits); > + if (!frontbuffer_bits) > return; > > - __intel_fb_obj_invalidate(obj, origin); > + __intel_fb_obj_invalidate(obj, origin, frontbuffer_bits); > } > > void __intel_fb_obj_flush(struct drm_i915_gem_object *obj, > bool retire, > - enum fb_op_origin origin); > + enum fb_op_origin origin, > + unsigned int frontbuffer_tibst); > static inline void intel_fb_obj_flush(struct drm_i915_gem_object *obj, > bool retire, > enum fb_op_origin origin) > { > - if (!obj->frontbuffer_bits) > + unsigned int frontbuffer_bits; > + > + frontbuffer_bits = atomic_read(&obj->frontbuffer_bits); > + if (!frontbuffer_bits) > return; > > - __intel_fb_obj_flush(obj, retire, origin); > + __intel_fb_obj_flush(obj, retire, origin, frontbuffer_bits); > } > > u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv, > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c > b/drivers/gpu/drm/i915/intel_frontbuffer.c > index a38ccfe4894a..636324da21c2 100644 > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c > @@ -77,23 +77,22 @@ > * scheduled. > */ > void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, > - enum fb_op_origin origin) > + enum fb_op_origin origin, > + unsigned int frontbuffer_bits) > { > struct drm_device *dev = obj->base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > > - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > - > if (origin == ORIGIN_CS) { > spin_lock(&dev_priv->fb_tracking.lock); > - dev_priv->fb_tracking.busy_bits |= obj->frontbuffer_bits; > - dev_priv->fb_tracking.flip_bits &= ~obj->frontbuffer_bits; > + dev_priv->fb_tracking.busy_bits |= frontbuffer_bits; > + dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits; > spin_unlock(&dev_priv->fb_tracking.lock); > } > > - intel_psr_invalidate(dev, obj->frontbuffer_bits); > - intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits); > - intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin); > + intel_psr_invalidate(dev, frontbuffer_bits); > + intel_edp_drrs_invalidate(dev, frontbuffer_bits); > + intel_fbc_invalidate(dev_priv, frontbuffer_bits, origin); > } > > /** > @@ -139,15 +138,11 @@ static void intel_frontbuffer_flush(struct drm_device > *dev, > */ > void __intel_fb_obj_flush(struct drm_i915_gem_object *obj, > bool retire, > - enum fb_op_origin origin) > + enum fb_op_origin origin, > + unsigned int frontbuffer_bits) > { > struct drm_device *dev = obj->base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > - unsigned frontbuffer_bits; > - > - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > - > - frontbuffer_bits = obj->frontbuffer_bits; > > if (retire) { > spin_lock(&dev_priv->fb_tracking.lock); > -- > 2.8.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
