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 <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> 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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
