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@xxxxxxxxxxxx>
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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
