Re: [PATCH v2 1/8] drm: Add dummy page per device or GEM object

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Mon, Jun 22, 2020 at 3:18 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 21.06.20 um 08:03 schrieb Andrey Grodzovsky:
> > Will be used to reroute CPU mapped BO's page faults once
> > device is removed.
> >
> > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > ---
> >   drivers/gpu/drm/drm_file.c  |  8 ++++++++
> >   drivers/gpu/drm/drm_prime.c | 10 ++++++++++
> >   include/drm/drm_file.h      |  2 ++
> >   include/drm/drm_gem.h       |  2 ++
> >   4 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index c4c704e..67c0770 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -188,6 +188,12 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
> >                       goto out_prime_destroy;
> >       }
> >
> > +     file->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> > +     if (!file->dummy_page) {
> > +             ret = -ENOMEM;
> > +             goto out_prime_destroy;
> > +     }
> > +
> >       return file;
> >
> >   out_prime_destroy:
> > @@ -284,6 +290,8 @@ void drm_file_free(struct drm_file *file)
> >       if (dev->driver->postclose)
> >               dev->driver->postclose(dev, file);
> >
> > +     __free_page(file->dummy_page);
> > +
> >       drm_prime_destroy_file_private(&file->prime);
> >
> >       WARN_ON(!list_empty(&file->event_list));
> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > index 1de2cde..c482e9c 100644
> > --- a/drivers/gpu/drm/drm_prime.c
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -335,6 +335,13 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> >
> >       ret = drm_prime_add_buf_handle(&file_priv->prime,
> >                       dma_buf, *handle);
> > +
> > +     if (!ret) {
> > +             obj->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> > +             if (!obj->dummy_page)
> > +                     ret = -ENOMEM;
> > +     }
> > +
>
> While the per file case still looks acceptable this is a clear NAK since
> it will massively increase the memory needed for a prime exported object.
>
> I think that this is quite overkill in the first place and for the hot
> unplug case we can just use the global dummy page as well.

Imo we either don't bother with per-file dummy page, or we need this.
Half-way doesn't make much sense, since for anything you dma-buf
exported you have no idea whether it left a sandbox or not.

E.g. anything that's shared between client/compositor has a different
security context, so picking the dummy page of either is the wrong
thing.

If you're worried about the overhead we can also allocate the dummy
page on demand, and SIGBUS if we can't allocate the right one. Then we
just need to track whether a buffer has ever been exported.
-Daniel

>
> Christian.
>
> >       mutex_unlock(&file_priv->prime.lock);
> >       if (ret)
> >               goto fail;
> > @@ -1006,6 +1013,9 @@ void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
> >               dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
> >       dma_buf = attach->dmabuf;
> >       dma_buf_detach(attach->dmabuf, attach);
> > +
> > +     __free_page(obj->dummy_page);
> > +
> >       /* remove the reference */
> >       dma_buf_put(dma_buf);
> >   }
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index 19df802..349a658 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -335,6 +335,8 @@ struct drm_file {
> >        */
> >       struct drm_prime_file_private prime;
> >
> > +     struct page *dummy_page;
> > +
> >       /* private: */
> >   #if IS_ENABLED(CONFIG_DRM_LEGACY)
> >       unsigned long lock_count; /* DRI1 legacy lock count */
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 0b37506..47460d1 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -310,6 +310,8 @@ struct drm_gem_object {
> >        *
> >        */
> >       const struct drm_gem_object_funcs *funcs;
> > +
> > +     struct page *dummy_page;
> >   };
> >
> >   /**
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx