Re: [Freedreno] [PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops

[Sam Ravnborg]

Hi Rob,

> > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > index 35e7f44c2a75..987e78b18244 100644
> > > --- a/include/drm/drm_gem.h
> > > +++ b/include/drm/drm_gem.h
> > > @@ -327,7 +327,7 @@ struct drm_gem_object {
> > >   * 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) \
> > > +#define DEFINE_DRM_GEM_FOPS(name, ...) \
> > >   static const struct file_operations name = {\
> > >   .owner  = THIS_MODULE,\
> > >   .open   = drm_open,\
> > > @@ -338,6 +338,7 @@ struct drm_gem_object {
> > >   .read   = drm_read,\
> > >   .llseek = noop_llseek,\
> > >   .mmap   = drm_gem_mmap,\
> > > + ##__VA_ARGS__\
> > >   }
> >
> > Would it not be less convoluted to make the macro only provide
> > the initializers? So you'd get something like:
> >
> > static const struct file_operations foo = {
> > DRM_GEM_FOPS,
> > .my_stuff = whatever,
> > };
> >
> 
> Hmm, I like my color of the bikeshed, but I guess it is a matter of opinion 
> ;-)
Or less surprise. Most similar macros provides initializers only.

Try "git grep DRM_.*OPS  | grep define" in include/drm
and take a look at the hits.

Sam

Re: [Freedreno] [PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops

[Rob Clark]

On Fri, Feb 25, 2022 at 12:36 PM Ville Syrjälä
 wrote:
>
> On Fri, Feb 25, 2022 at 12:26:12PM -0800, Rob Clark wrote:
> > From: Rob Clark 
> >
> > Extend the helper macro so we don't have to throw it away if driver adds
> > support for optional fops, like show_fdinfo().
> >
> > Signed-off-by: Rob Clark 
> > ---
> >  include/drm/drm_gem.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 35e7f44c2a75..987e78b18244 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -327,7 +327,7 @@ struct drm_gem_object {
> >   * 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) \
> > +#define DEFINE_DRM_GEM_FOPS(name, ...) \
> >   static const struct file_operations name = {\
> >   .owner  = THIS_MODULE,\
> >   .open   = drm_open,\
> > @@ -338,6 +338,7 @@ struct drm_gem_object {
> >   .read   = drm_read,\
> >   .llseek = noop_llseek,\
> >   .mmap   = drm_gem_mmap,\
> > + ##__VA_ARGS__\
> >   }
>
> Would it not be less convoluted to make the macro only provide
> the initializers? So you'd get something like:
>
> static const struct file_operations foo = {
> DRM_GEM_FOPS,
> .my_stuff = whatever,
> };
>

Hmm, I like my color of the bikeshed, but I guess it is a matter of opinion ;-)

BR,
-R

Re: [Freedreno] [PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops

[Ville Syrjälä]

On Fri, Feb 25, 2022 at 12:26:12PM -0800, Rob Clark wrote:
> From: Rob Clark 
> 
> Extend the helper macro so we don't have to throw it away if driver adds
> support for optional fops, like show_fdinfo().
> 
> Signed-off-by: Rob Clark 
> ---
>  include/drm/drm_gem.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 35e7f44c2a75..987e78b18244 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -327,7 +327,7 @@ struct drm_gem_object {
>   * 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) \
> +#define DEFINE_DRM_GEM_FOPS(name, ...) \
>   static const struct file_operations name = {\
>   .owner  = THIS_MODULE,\
>   .open   = drm_open,\
> @@ -338,6 +338,7 @@ struct drm_gem_object {
>   .read   = drm_read,\
>   .llseek = noop_llseek,\
>   .mmap   = drm_gem_mmap,\
> + ##__VA_ARGS__\
>   }

Would it not be less convoluted to make the macro only provide
the initializers? So you'd get something like:

static const struct file_operations foo = {
DRM_GEM_FOPS,
.my_stuff = whatever,
};

>  
>  void drm_gem_object_release(struct drm_gem_object *obj);
> -- 
> 2.35.1

-- 
Ville Syrjälä
Intel

[Freedreno] [PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops

[Rob Clark]

From: Rob Clark 

Extend the helper macro so we don't have to throw it away if driver adds
support for optional fops, like show_fdinfo().

Signed-off-by: Rob Clark 
---
 include/drm/drm_gem.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 35e7f44c2a75..987e78b18244 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -327,7 +327,7 @@ struct drm_gem_object {
  * 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) \
+#define DEFINE_DRM_GEM_FOPS(name, ...) \
static const struct file_operations name = {\
.owner  = THIS_MODULE,\
.open   = drm_open,\
@@ -338,6 +338,7 @@ struct drm_gem_object {
.read   = drm_read,\
.llseek = noop_llseek,\
.mmap   = drm_gem_mmap,\
+   ##__VA_ARGS__\
}
 
 void drm_gem_object_release(struct drm_gem_object *obj);
-- 
2.35.1