Re: [PATCH 06/11] drm/i915: Store owning file on the i915_address_space

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

On 14/12/15 11:36, Chris Wilson wrote:
> For the global GTT (and aliasing GTT), the address space is owned by the
> device (it is a global resource) and so the per-file owner field is
> NULL. For per-process GTT (where we create an address space per
> context), each is owned by the opening file. We can use this ownership
> information to both distinguish GGTT and ppGTT address spaces, as well
> as occasionally inspect the owner.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
>   drivers/gpu/drm/i915/i915_drv.h         |  1 -
>   drivers/gpu/drm/i915/i915_gem_context.c |  3 ++-
>   drivers/gpu/drm/i915/i915_gem_gtt.c     | 25 +++++++++++++------------
>   drivers/gpu/drm/i915/i915_gem_gtt.h     | 13 ++++++-------
>   5 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9ec133f5af00..179e3c5c5022 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -352,7 +352,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>   				= container_of(vma->vm,
>   					       struct i915_hw_ppgtt,
>   					       base);
> -			if (ppgtt->file_priv != stats->file_priv)
> +			if (ppgtt->base.file != stats->file_priv)
>   				continue;
>   		}
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6ce163a681f2..b32a00f60e98 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2924,7 +2924,6 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
>   	return container_of(vm, struct i915_hw_ppgtt, base);
>   }
>
> -
>   static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
>   {
>   	return i915_gem_obj_ggtt_bound_view(obj, &i915_ggtt_view_normal);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 25a8498f0b5a..dcb4603a7f03 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -296,7 +296,8 @@ i915_gem_create_context(struct drm_device *dev,
>   	}
>
>   	if (USES_FULL_PPGTT(dev)) {
> -		struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv);
> +		struct i915_hw_ppgtt *ppgtt =
> +			i915_ppgtt_create(to_i915(dev), file_priv);
>
>   		if (IS_ERR_OR_NULL(ppgtt)) {
>   			DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index da150c27a76c..130ccefb2491 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2112,11 +2112,12 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>   	return 0;
>   }
>
> -static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> +static int __hw_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
> +			   struct drm_i915_private *dev_priv)
>   {
> -	ppgtt->base.dev = dev;
> +	ppgtt->base.dev = dev_priv->dev;
>
> -	if (INTEL_INFO(dev)->gen < 8)
> +	if (INTEL_INFO(dev_priv)->gen < 8)
>   		return gen6_ppgtt_init(ppgtt);
>   	else
>   		return gen8_ppgtt_init(ppgtt);
> @@ -2132,15 +2133,17 @@ static void i915_address_space_init(struct i915_address_space *vm,
>   	list_add_tail(&vm->global_link, &dev_priv->vm_list);
>   }
>
> -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
> +int i915_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
> +		    struct drm_i915_private *dev_priv,
> +		    struct drm_i915_file_private *file_priv)
>   {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>   	int ret = 0;

Since you are using the opportunity to tidy whitespace maybe also tidy 
this needless init. ;)

>
> -	ret = __hw_ppgtt_init(dev, ppgtt);
> +	ret = __hw_ppgtt_init(ppgtt, dev_priv);
>   	if (ret == 0) {
>   		kref_init(&ppgtt->ref);
>   		i915_address_space_init(&ppgtt->base, dev_priv);
> +		ppgtt->base.file = file_priv;

I would keep using file_priv since that's what's it's called all over 
the place but whatever.

>   	}
>
>   	return ret;
> @@ -2183,7 +2186,8 @@ int i915_ppgtt_init_ring(struct drm_i915_gem_request *req)
>   }
>
>   struct i915_hw_ppgtt *
> -i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv)
> +i915_ppgtt_create(struct drm_i915_private *dev_priv,
> +		  struct drm_i915_file_private *fpriv)
>   {
>   	struct i915_hw_ppgtt *ppgtt;
>   	int ret;
> @@ -2192,14 +2196,12 @@ i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv)
>   	if (!ppgtt)
>   		return ERR_PTR(-ENOMEM);
>
> -	ret = i915_ppgtt_init(dev, ppgtt);
> +	ret = i915_ppgtt_init(ppgtt, dev_priv, fpriv);
>   	if (ret) {
>   		kfree(ppgtt);
>   		return ERR_PTR(ret);
>   	}
>
> -	ppgtt->file_priv = fpriv;
> -
>   	trace_i915_ppgtt_create(&ppgtt->base);
>
>   	return ppgtt;
> @@ -2717,7 +2719,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>   		if (!ppgtt)
>   			return -ENOMEM;
>
> -		ret = __hw_ppgtt_init(dev, ppgtt);
> +		ret = __hw_ppgtt_init(ppgtt, dev_priv);
>   		if (ret) {
>   			ppgtt->base.cleanup(&ppgtt->base);
>   			kfree(ppgtt);
> @@ -3134,7 +3136,6 @@ int i915_gem_gtt_init(struct drm_device *dev)
>   	}
>
>   	gtt->base.dev = dev;
> -	gtt->base.is_ggtt = true;
>
>   	ret = gtt->gtt_probe(dev, &gtt->base.total, &gtt->stolen_size,
>   			     &gtt->mappable_base, &gtt->mappable_end);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index bae005a62cfc..4e9553ace33f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -273,12 +273,11 @@ struct i915_pml4 {
>   struct i915_address_space {
>   	struct drm_mm mm;
>   	struct drm_device *dev;
> +	struct drm_i915_file_private *file;

Suggest putting a comment documenting when it is NULL and when it is 
valid. Commit says so, but I think comment is also needed.

With that,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko


>   	struct list_head global_link;
>   	u64 start;		/* Start offset always 0 for dri2 */
>   	u64 total;		/* size addr space maps (ex. 2GB for ggtt) */
>
> -	bool is_ggtt;
> -
>   	struct i915_page_scratch *scratch_page;
>   	struct i915_page_table *scratch_pt;
>   	struct i915_page_directory *scratch_pd;
> @@ -334,7 +333,7 @@ struct i915_address_space {
>   			u32 flags);
>   };
>
> -#define i915_is_ggtt(V) ((V)->is_ggtt)
> +#define i915_is_ggtt(V) ((V)->file == NULL)
>
>   /* The Graphics Translation Table is the way in which GEN hardware translates a
>    * Graphics Virtual Address into a Physical Address. In addition to the normal
> @@ -376,8 +375,6 @@ struct i915_hw_ppgtt {
>   		struct i915_page_directory pd;		/* GEN6-7 */
>   	};
>
> -	struct drm_i915_file_private *file_priv;
> -
>   	gen6_pte_t __iomem *pd_addr;
>
>   	int (*enable)(struct i915_hw_ppgtt *ppgtt);
> @@ -522,11 +519,13 @@ void i915_gem_init_global_gtt(struct drm_device *dev);
>   void i915_global_gtt_cleanup(struct drm_device *dev);
>
>
> -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt);
> +int i915_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
> +		    struct drm_i915_private *dev_priv,
> +		    struct drm_i915_file_private *file_priv);
>   int i915_ppgtt_init_hw(struct drm_device *dev);
>   int i915_ppgtt_init_ring(struct drm_i915_gem_request *req);
>   void i915_ppgtt_release(struct kref *kref);
> -struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev,
> +struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_i915_private *dev_priv,
>   					struct drm_i915_file_private *fpriv);
>   static inline void i915_ppgtt_get(struct i915_hw_ppgtt *ppgtt)
>   {
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx