Re: [PATCH 1/4] drm/i915: add gen6 ppgtt dummy creation function

[Robert Beckett]

On 11/05/2022 11:13, Thomas Hellström wrote:
> Hi,
>
> On Tue, 2022-05-03 at 19:13 +0000, Robert Beckett wrote:
> > Internal gem objects will soon just be volatile system memory region
> > objects.
> > To enable this, create a separate dummy object creation function
> > for gen6 ppgtt
>
>
> It's not clear from the commit message why we need a special case for
> this. Could you describe more in detail?

it always was a special case, that used the internal backend but 
provided it's own ops, so actually had no benefit from using the 
internal backend. See b0b0f2d225da6fe58417fae37e3f797e2db27b62

I'll add some further explanation in the commit message for v2.

> Thanks,
> Thomas
>
>
> > Signed-off-by: Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 43
> > ++++++++++++++++++++++++++--
> >   1 file changed, 40 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > index 1bb766c79dcb..f3b660cfeb7f 100644
> > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > @@ -372,6 +372,45 @@ static const struct drm_i915_gem_object_ops
> > pd_dummy_obj_ops = {
> >          .put_pages = pd_dummy_obj_put_pages,
> >   };
> >   
> > +static struct drm_i915_gem_object *
> > +i915_gem_object_create_dummy(struct drm_i915_private *i915,
> > phys_addr_t size)
> > +{
> > +       static struct lock_class_key lock_class;
> > +       struct drm_i915_gem_object *obj;
> > +       unsigned int cache_level;
> > +
> > +       GEM_BUG_ON(!size);
> > +       GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE));
> > +
> > +       if (overflows_type(size, obj->base.size))
> > +               return ERR_PTR(-E2BIG);
> > +
> > +       obj = i915_gem_object_alloc();
> > +       if (!obj)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       drm_gem_private_object_init(&i915->drm, &obj->base, size);
> > +       i915_gem_object_init(obj, &pd_dummy_obj_ops, &lock_class, 0);
> > +       obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
> > +
> > +       /*
> > +        * Mark the object as volatile, such that the pages are
> > marked as
> > +        * dontneed whilst they are still pinned. As soon as they are
> > unpinned
> > +        * they are allowed to be reaped by the shrinker, and the
> > caller is
> > +        * expected to repopulate - the contents of this object are
> > only valid
> > +        * whilst active and pinned.
> > +        */
> > +       i915_gem_object_set_volatile(obj);
> > +
> > +       obj->read_domains = I915_GEM_DOMAIN_CPU;
> > +       obj->write_domain = I915_GEM_DOMAIN_CPU;
> > +
> > +       cache_level = HAS_LLC(i915) ? I915_CACHE_LLC :
> > I915_CACHE_NONE;
> > +       i915_gem_object_set_cache_coherency(obj, cache_level);
> > +
> > +       return obj;
> > +}
> > +
> >   static struct i915_page_directory *
> >   gen6_alloc_top_pd(struct gen6_ppgtt *ppgtt)
> >   {
> > @@ -383,9 +422,7 @@ gen6_alloc_top_pd(struct gen6_ppgtt *ppgtt)
> >          if (unlikely(!pd))
> >                  return ERR_PTR(-ENOMEM);
> >   
> > -       pd->pt.base = __i915_gem_object_create_internal(ppgtt-
> > > base.vm.gt->i915,
> > -
> >                                                         &pd_dummy_obj_o
> > ps,
> > -                                                       I915_PDES *
> > SZ_4K);
> > +       pd->pt.base = i915_gem_object_create_dummy(ppgtt->base.vm.gt-
> > > i915, I915_PDES * SZ_4K);
> >          if (IS_ERR(pd->pt.base)) {
> >                  err = PTR_ERR(pd->pt.base);
> >                  pd->pt.base = NULL;