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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 05/01/2022 10:32, Jani Nikula wrote:
> On Wed, 05 Jan 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> 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@intel.com>
>>> ---
>>>    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__ */
>>>
>