Re: [PATCH 17/22] drm/i915: Use atomics to manipulate obj->frontbuffer_bits

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ke, 2016-07-27 at 12:14 +0100, Chris Wilson wrote:
>  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)

Should we have a BUILD_BUG_ON to make sure we have a fit?

>  #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

'bitfield' refers to specific C construct, so not the appropriate term
here now that it is removed. In this commit it is readable, but for
future I think just confusing.

>  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);
>  }
>  

These unrelated changes should not be squashed, me thinks. I expect
less intermingling in the future.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx