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

[Sean Paul <seanpaul@chromium.org>]

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.

Reviewed-by: Sean Paul <seanpaul@chromium.org>
> 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx