[Intel-gfx] [PATCH 06/11] drm/i915: Store owning file on the i915_address_space
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Dec 17 03:52:06 PST 2015
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 at 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, >t->base.total, >t->stolen_size,
> >t->mappable_base, >t->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 at 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)
> {
>
More information about the Intel-gfx
mailing list