[Intel-gfx] [PATCH 02/18] drm/i915: introduce drm_i915_gem_object page_size members

[Daniel Vetter]

On Wed, Apr 05, 2017 at 11:07:55AM +0100, Matthew Auld wrote:
> 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 at 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.

But if you already handle mixed pages, why do we need this still? Viz.
this thread, it does seem to cause quite a bit of confusion.

And I have no idea why we need the cpu_page_size, we kmap at the page
level anyway, and shmem handles the cpu mmap stuff for us.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch