Sorry for late reply,
On Thu, 2022-04-14 at 17:13 +0100, Robert Beckett wrote:
>
>
> On 14/04/2022 15:05, Thomas Hellström wrote:
> > On Tue, 2022-04-12 at 15:18 +0000, Robert Beckett wrote:
> > > stolen/kernel buffers should not be mmapable by userland.
> > > do not provide callbacks to facilitate this for these buffers.
> > >
> > > Signed-off-by: Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 32
> > > +++++++++++++++++++++--
> > > --
> > > 1 file changed, 27 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > > index a878910a563c..b20f81836c54 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > > @@ -1092,8 +1092,8 @@ static void i915_ttm_unmap_virtual(struct
> > > drm_i915_gem_object *obj)
> > > ttm_bo_unmap_virtual(i915_gem_to_ttm(obj));
> > > }
> > >
> > > -static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops
> > > = {
> > > - .name = "i915_gem_object_ttm",
> > > +static const struct drm_i915_gem_object_ops
> > > i915_gem_ttm_user_obj_ops = {
> > > + .name = "i915_gem_object_ttm_user",
> > > .flags = I915_GEM_OBJECT_IS_SHRINKABLE |
> > > I915_GEM_OBJECT_SELF_MANAGED_SHRINK_LIST,
> > >
> > > @@ -1111,6 +1111,21 @@ static const struct
> > > drm_i915_gem_object_ops
> > > i915_gem_ttm_obj_ops = {
> > > .mmap_ops = &vm_ops_ttm,
> > > };
> > >
> > > +static const struct drm_i915_gem_object_ops
> > > i915_gem_ttm_kern_obj_ops = {
> > > + .name = "i915_gem_object_ttm_kern",
> > > + .flags = I915_GEM_OBJECT_IS_SHRINKABLE |
> > > + I915_GEM_OBJECT_SELF_MANAGED_SHRINK_LIST,
> > > +
> > > + .get_pages = i915_ttm_get_pages,
> > > + .put_pages = i915_ttm_put_pages,
> > > + .truncate = i915_ttm_truncate,
> > > + .shrink = i915_ttm_shrink,
> > > +
> > > + .adjust_lru = i915_ttm_adjust_lru,
> > > + .delayed_free = i915_ttm_delayed_free,
> > > + .migrate = i915_ttm_migrate,
> > > +};
> >
> > Do we really need two different ops here?
> >
> > Since if we don't have mmap ops, basically that tells GEM it should
> > do
> > the mmapping rather than TTM.
> >
> > That might of course come in handy for the shmem backend, but I
> > don't
> > fully follow why we need this for stolen.
>
> the main rationale for doing this was to avoid
> drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c:can_mmap()
> presuming
> that is can use I915_MMAP_TYPE_FIXED
>
> As the original backend also did not have mmap_offset handlers for
> stolen, this seemed like a reasonable design.
>
> If desired, we could add a special case for the testing logic, but
> those
> special cases have tendency to multiply.
>
> >
> > Also for the framebuffer handed over from BIOS to fbdev, Does that
> > need
> > mmapping and if so, how do we handle that?
> >
>
> I'm not sure of the usecase there. Do you know of any igt test that
> tests this? I can investigate further if you do not.
It would be if we the fbdev driver at startup inherits some image that
bios has preloaded into stolen, and then a client tries to write into
it. Not sure that this is a real use case though, or whether, in that
case, that takes a separate path for user-space mappings.
/Thomas
>
> >
> > /Thomas
> >
> >
> >
> >
> > > +
> > > void i915_ttm_bo_destroy(struct ttm_buffer_object *bo)
> > > {
> > > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
> > > @@ -1165,10 +1180,19 @@ int __i915_gem_ttm_object_init(struct
> > > intel_memory_region *mem,
> > > .no_wait_gpu = false,
> > > };
> > > enum ttm_bo_type bo_type;
> > > + const struct drm_i915_gem_object_ops *ops;
> > > int ret;
> > >
> > > drm_gem_private_object_init(&i915->drm, &obj->base,
> > > size);
> > > - i915_gem_object_init(obj, &i915_gem_ttm_obj_ops,
> > > &lock_class,
> > > flags);
> > > +
> > > + if (flags & I915_BO_ALLOC_USER &&
> > > intel_region_to_ttm_type(mem) != I915_PL_STOLEN) {
> > > + bo_type = ttm_bo_type_device;
> > > + ops = &i915_gem_ttm_user_obj_ops;
> > > + } else {
> > > + bo_type = ttm_bo_type_kernel;
> > > + ops = &i915_gem_ttm_kern_obj_ops;
> > > + }
> > > + i915_gem_object_init(obj, ops, &lock_class, flags);
> > >
> > > obj->bo_offset = offset;
> > >
> > > @@ -1178,8 +1202,6 @@ int __i915_gem_ttm_object_init(struct
> > > intel_memory_region *mem,
> > >
> > > INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL
> > > |
> > > __GFP_NOWARN);
> > > mutex_init(&obj->ttm.get_io_page.lock);
> > > - bo_type = (obj->flags & I915_BO_ALLOC_USER) ?
> > > ttm_bo_type_device :
> > > - ttm_bo_type_kernel;
> > >
> > > obj->base.vma_node.driver_private =
> > > i915_gem_to_ttm(obj);
> > >
> >
> >
