Re: [PATCH 3/3] drm/prime: Use anon_drm_getfile() for an internal drm struct file

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 6, 2019 at 11:45 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Daniel Vetter (2019-11-06 10:21:57)
> > On Wed, Nov 06, 2019 at 10:07:16AM +0000, Chris Wilson wrote:
> > > Currently the drm_prime mmap fallback uses a mock struct file to provide
> > > the file pointer into the backend mmap routine. Now that we can create
> > > fully fledged anonymous struct file around the drm device, put it to
> > > use.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/drm_prime.c | 26 ++++++++------------------
> > >  1 file changed, 8 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > > index 0814211b0f3f..5faa63713ec8 100644
> > > --- a/drivers/gpu/drm/drm_prime.c
> > > +++ b/drivers/gpu/drm/drm_prime.c
> > > @@ -709,8 +709,7 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vunmap);
> > >   */
> > >  int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> > >  {
> > > -     struct drm_file *priv;
> > > -     struct file *fil;
> > > +     struct file *file;
> > >       int ret;
> > >
> > >       if (obj->funcs && obj->funcs->mmap) {
> >
> > obj->funcs->mmap is the new way of doing this (and hopefully finally
> > something clean), I'd really like to retire the below hack outright.
> >
> > Plus I'm not sure why you need an anon inode here? If a driver needs this
> > for unmap_mapping_range or similar I think it'd be better to try and make
> > something work cleanly for obj->funcs->mmap.
>
> It's faking one currently. If the fake is not good enough, you are
> playing whack-a-mole until you finally do create a fully fledged file.
>
> If you are going to the trouble of having to create a struct file to
> provide to the fallback routines, might as well avoid stinky code :)

We're currently not faking the inode at all, we're just using the one
that comes with the dma-buf. So distinct from the drm_device file, and
hence unmap_mapping_range won't work (or at least doing that on the
drm_device inode wont shoot down the ptes for redirected dma-buf
mmaps). obj->funcs->mmap has the same issue.

But since all current users of this don't expect unmap_mapping_range
to work correctly, it's not an real issue. If that changes then imo we
should fix up the obj->funcs->mmap path to have the correct inode, not
the deprecated path you're updating here. But since there's no patch 4
in this series to start using this for i915 or someone else, I'm not
seeing the point.

Or am I blind? At least slightly confused,
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel