Re: [PATCH 24/24] drm/gem: Add DEFINE_DRM_GEM_FOPS

From: Daniel Vetter <daniel@ffwll.ch>
To: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 24/24] drm/gem: Add DEFINE_DRM_GEM_FOPS
Date: Tue, 14 Mar 2017 14:31:23 +0100	[thread overview]
Message-ID: <20170314133123.pgvoccqfcvpiuhwz@phenom.ffwll.local> (raw)
In-Reply-To: <20170313193543.GV20329@art_vandelay>

On Mon, Mar 13, 2017 at 03:35:43PM -0400, Sean Paul wrote:
> On Wed, Mar 08, 2017 at 03:12:57PM +0100, Daniel Vetter wrote:
> > Sadly there's only 1 driver which can use it, everyone else is special
> > for some reason:
> > 
> > - gma500 has a horrible runtime PM ioctl wrapper that probably doesn't
> >   really work but meh.
> > - i915 needs special compat_ioctl handler because regrets.
> > - arcgpu needs to fixup the pgprot because (no idea why it can't do
> >   that in the fault handler like everyone else).
> > - tegra does even worse stuff with pgprot
> > - udl does something with vm_flags too ...
> > - cma helpers, etnaviv, mtk, msm, rockchip, omap all implement some
> >   variation on prefaulting.
> > - exynos is exynos, I got lost in the midlayers.
> > - vc4 has to reinvent half of cma helpers because those are too much
> >   midlayer, plus vm_flags dances.
> > - vgem also seems unhappy with the default vm_flags.
> 
> I wonder whether a todo entry to clean this up would be appropriate.

I would have, if I actually had an idea what we should be doing here. All
the reasons are kinda semi-legit: Adjusting flags to get the right caching
mode, prefaulting, subclassing for special needs are all reasonable things
to do. I have honestly not enough clue about cross-platform issues to come
up with a good suggestion, especially about the flags stuff.

> Reviewed-by: Sean Paul <seanpaul@chromium.org>

Merged the last two patches too, thx a lot for review.
-Daniel

> > 
> > So pretty sad divergence and I'm sure we could do better, but not
> > really an idea. Oh well, maybe this macro here helps to encourage more
> > consistency at least going forward.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/armada/armada_drv.c | 11 +----------
> >  drivers/gpu/drm/drm_file.c          |  8 +++-----
> >  include/drm/drm_gem.h               | 26 ++++++++++++++++++++++++++
> >  3 files changed, 30 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
> > index 1952e8748fea..f986e9ff093d 100644
> > --- a/drivers/gpu/drm/armada/armada_drv.c
> > +++ b/drivers/gpu/drm/armada/armada_drv.c
> > @@ -60,16 +60,7 @@ static void armada_drm_lastclose(struct drm_device *dev)
> >  	armada_fbdev_lastclose(dev);
> >  }
> >  
> > -static const struct file_operations armada_drm_fops = {
> > -	.owner			= THIS_MODULE,
> > -	.llseek			= no_llseek,
> > -	.read			= drm_read,
> > -	.poll			= drm_poll,
> > -	.unlocked_ioctl		= drm_ioctl,
> > -	.mmap			= drm_gem_mmap,
> > -	.open			= drm_open,
> > -	.release		= drm_release,
> > -};
> > +DEFINE_DRM_GEM_FOPS(armada_drm_fops);
> >  
> >  static struct drm_driver armada_drm_driver = {
> >  	.lastclose		= armada_drm_lastclose,
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 260a83563976..0701362dcca1 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -89,11 +89,9 @@ DEFINE_MUTEX(drm_global_mutex);
> >   *             .mmap = drm_gem_mmap,
> >   *     };
> >   *
> > - * For CMA based drivers there is the DEFINE_DRM_GEM_CMA_FOPS() macro to make
> > - * this simpler.
> > - *
> > - * FIXME: We should have a macro for this (and the CMA version) so that drivers
> > - * don't have to repeat it all the time.
> > + * For plain GEM based drivers there is the DEFINE_DRM_GEM_FOPS() macro, and for
> > + * CMA based drivers there is the DEFINE_DRM_GEM_CMA_FOPS() macro to make this
> > + * simpler.
> >   */
> >  
> >  static int drm_open_helper(struct file *filp, struct drm_minor *minor);
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index b9ade75ecd82..663d80358057 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -178,6 +178,32 @@ struct drm_gem_object {
> >  	struct dma_buf_attachment *import_attach;
> >  };
> >  
> > +/**
> > + * DEFINE_DRM_GEM_FOPS() - macro to generate file operations for GEM drivers
> > + * @name: name for the generated structure
> > + *
> > + * This macro autogenerates a suitable &struct file_operations for GEM based
> > + * drivers, which can be assigned to &drm_driver.fops. Note that this structure
> > + * cannot be shared between drivers, because it contains a reference to the
> > + * current module using THIS_MODULE.
> > + *
> > + * Note that the declaration is already marked as static - if you need a
> > + * non-static version of this you're probably doing it wrong and will break the
> > + * THIS_MODULE reference by accident.
> > + */
> > +#define DEFINE_DRM_GEM_FOPS(name) \
> > +	static const struct file_operations name = {\
> > +		.owner		= THIS_MODULE,\
> > +		.open		= drm_open,\
> > +		.release	= drm_release,\
> > +		.unlocked_ioctl	= drm_ioctl,\
> > +		.compat_ioctl	= drm_compat_ioctl,\
> > +		.poll		= drm_poll,\
> > +		.read		= drm_read,\
> > +		.llseek		= noop_llseek,\
> > +		.mmap		= drm_gem_mmap,\
> > +	}
> > +
> >  void drm_gem_object_release(struct drm_gem_object *obj);
> >  void drm_gem_object_free(struct kref *kref);
> >  int drm_gem_object_init(struct drm_device *dev,
> > -- 
> > 2.11.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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