Re: [PATCH 2/2] drm/i915/uncore: rename i915_reg_read_ioctl intel_uncore_reg_read_ioctl

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 05/01/2022 10:32, Jani Nikula wrote:
> On Wed, 05 Jan 2022, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> > On 05/01/2022 10:05, Jani Nikula wrote:
> > > Follow the usual naming convention.
> >
> > But intel_uncore_ prefix usually means functions takes intel_uncore as
> > the first argument.
> >
> > Maybe solution here is that i915_reg_read_ioctl does not belong in
> > intel_uncore.c, it being the UAPI layer thing? I guess arguments could
> > be made for either way.
>
> My position is that the function and file prefixes go hand in
> hand. You'll always know where to place a function, and you'll always
> know where the function is to be found.
>
> If you can *also* make the context argument follow the pattern, it's
> obviously better, and indicates the division to files is working out
> nicely. However, in a lot of cases you'll need to pass struct
> drm_i915_private or similar as the first parameter to e.g. init
> functions. It can't be the rigid rule.
>
> I'm fine with moving the entire function somewhere else, as long as the
> declaration is not in i915_drv.h. There's no longer a i915_drv.c, and
> i915_drv.h should not have function declarations at all.

Yes I agree it cannot be a rigid rule. I just that it feels 
intel_uncore.[hc] is too low level to me to hold an ioctl 
implementation, and header actually feels wrong to have the declaration. 
Not least it is about _one_ of the uncores, while the ioctl is not 
operating on that level, albeit undefined at the moment how exactly it 
would work for multi-tile.

Would it be too early, or unwarranted at this point, to maybe consider 
adding i915_ioctls.[hc]?

I like the i915_ prefix of ioctls for consistency.. i915_getparam_ioctl, 
i915_query_ioctl, i915_perf_..., i915_gem_....

Regards,

Tvrtko

> BR,
> Jani.
>
> > Regards,
> >
> > Tvrtko
> >
> > > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/i915/i915_driver.c  | 2 +-
> > >    drivers/gpu/drm/i915/intel_uncore.c | 4 ++--
> > >    drivers/gpu/drm/i915/intel_uncore.h | 4 ++--
> > >    3 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > index 95174938b160..f9a494e159dc 100644
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -1805,7 +1805,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
> > >    	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_RENDER_ALLOW),
> > >    	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE_EXT, i915_gem_context_create_ioctl, DRM_RENDER_ALLOW),
> > >    	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_RENDER_ALLOW),
> > > -	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_RENDER_ALLOW),
> > > +	DRM_IOCTL_DEF_DRV(I915_REG_READ, intel_uncore_reg_read_ioctl, DRM_RENDER_ALLOW),
> > >    	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_gem_context_reset_stats_ioctl, DRM_RENDER_ALLOW),
> > >    	DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_RENDER_ALLOW),
> > >    	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index fc25ebf1a593..33f95bb2d3d5 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -2269,8 +2269,8 @@ static const struct reg_whitelist {
> > >    	.size = 8
> > >    } };
> > >    
> > > -int i915_reg_read_ioctl(struct drm_device *dev,
> > > -			void *data, struct drm_file *file)
> > > +int intel_uncore_reg_read_ioctl(struct drm_device *dev,
> > > +				void *data, struct drm_file *file)
> > >    {
> > >    	struct drm_i915_private *i915 = to_i915(dev);
> > >    	struct intel_uncore *uncore = &i915->uncore;
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> > > index 3a87bbd906f8..697ac4586159 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.h
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.h
> > > @@ -457,7 +457,7 @@ static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
> > >    #define raw_reg_write(base, reg, value) \
> > >    	writel(value, base + i915_mmio_reg_offset(reg))
> > >    
> > > -int i915_reg_read_ioctl(struct drm_device *dev, void *data,
> > > -			struct drm_file *file);
> > > +int intel_uncore_reg_read_ioctl(struct drm_device *dev, void *data,
> > > +				struct drm_file *file);
> > >    
> > >    #endif /* !__INTEL_UNCORE_H__ */