Re: [PATCH 02/18] drm/i915: introduce drm_i915_gem_object page_size members

[Matthew Auld <matthew.william.auld@gmail.com>]

On 04/05, Chris Wilson wrote:
> On Wed, Apr 05, 2017 at 08:49:17AM +0200, Daniel Vetter wrote:
> > On Tue, Apr 04, 2017 at 11:11:12PM +0100, Matthew Auld wrote:
> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c        | 5 +++++
> > >  drivers/gpu/drm/i915/i915_gem_object.h | 3 +++
> > >  2 files changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 4ca88f2539c0..cbf97f4bbb72 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -2441,6 +2441,8 @@ static int ____i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> > >  	struct sg_table *pages;
> > >  
> > >  	GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> > > +	GEM_BUG_ON(!is_valid_gtt_page_size(obj->page_size));
> > > +	GEM_BUG_ON(!is_valid_gtt_page_size(obj->gtt_page_size));
> > >  
> > >  	if (unlikely(obj->mm.madv != I915_MADV_WILLNEED)) {
> > >  		DRM_DEBUG("Attempting to obtain a purgeable object\n");
> > > @@ -4159,6 +4161,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> > >  
> > >  	obj->ops = ops;
> > >  
> > > +	obj->page_size = PAGE_SIZE;
> > > +	obj->gtt_page_size = I915_GTT_PAGE_SIZE;
> > > +
> > >  	reservation_object_init(&obj->__builtin_resv);
> > >  	obj->resv = &obj->__builtin_resv;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
> > > index 174cf923c236..b1dacbfe5173 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_object.h
> > > +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> > > @@ -107,6 +107,9 @@ struct drm_i915_gem_object {
> > >  	unsigned int cache_level:3;
> > >  	unsigned int cache_dirty:1;
> > >  
> > > +	unsigned int page_size; /* CPU pov - 4K(default), 2M, 1G */
> > > +	unsigned int gtt_page_size; /* GPU pov - 4K(default), 64K, 2M, 1G */
> > 
> > Just kinda archecture review, with a long-term view: Is the plan to
> > eventually become more flexible here, i.e. allow mixed mode?
> 
> Simply put we can not support obj->page_size. Every object will be
> composed of a mixture of page sizes, often outside of our control and
> those page sizes may vary over the lifetime of the object.
> 
> Trying to design around an a priori static page_size is a bad idea, imo.

I think I've misrepresented the intention of obj->page_size, it merely
serves as a hint to get pages, thereafter it represents the minimum page
size in the mapping and is just bookkeeping, so mixed pages are totally
fine and expected. I mostly wanted it to make it clear to the reader
that we have a gtt and cpu page size. I also wanted to know if an object
is entirely composed of huge-pages for debugfs purposes. I'll try to
rework this to make it less terrible.

> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx