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

[Tvrtko Ursulin]

On 05/01/2022 13:18, Jani Nikula wrote:
> On Wed, 05 Jan 2022, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
>> On 05/01/2022 10:32, Jani Nikula wrote:
>>> On Wed, 05 Jan 2022, Tvrtko Ursulin <tvrtko.ursulin at 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]?
> 
> Then the conversation would be about putting together a ton of unrelated
> functions where the only thing in common is that they're an ioctl
> implementation. Arguably many of them would have less in common than the
> reg read ioctl has with uncore!

I imagined it as a place for ioctls which don't fit anywhere else, like 
it this case it is not a family of ioctls but and odd one out. So yes, 
first "problem" would be there is only one to put there and no line of 
sight for others.

> And when is it okay to put an ioctl in the i915_ioctls.c file and when
> is it warranted to put it somewhere else? It's just a different set of
> problems.

When it does not fit anywhere else?

>> I like the i915_ prefix of ioctls for consistency.. i915_getparam_ioctl,
>> i915_query_ioctl, i915_perf_..., i915_gem_....
> 
> The display ioctls have intel_ prefix anyway. It's the _ioctl suffix
> that we use.
> 
> Again, my main driver here is cleaning up i915_drv.h. I can shove the
> reg read ioctl somewhere other than intel_uncore.[ch] too. But as it
> stands, the only alternative that seems better than intel_uncore.[ch] at
> the moment is adding a dedicated file for a 60-line function.

I understand your motivation and I wouldn't nack your efforts, but I 
also cannot yet make myself ack it. Is 60 lines so bad? Lets see..

$ find . -name "*.c" -print0 | xargs -0 wc -l | sort -nr
...
      59 ./selftests/mock_request.c
      59 ./gt/uc/intel_uc_debugfs.c
      59 ./gem/i915_gemfs.c
      52 ./selftests/igt_mmap.c
      51 ./selftests/igt_reset.c
      49 ./selftests/mock_uncore.c
      47 ./selftests/igt_atomic.c
      36 ./gt/uc/intel_huc_debugfs.c
      36 ./gt/intel_gt_engines_debugfs.c
      35 ./selftests/igt_flush_test.c
      34 ./selftests/librapl.c
      34 ./gvt/trace_points.c
      29 ./gt/selftests/mock_timeline.c
      27 ./gt/selftest_engine.c
      26 ./gt/uc/intel_huc_fw.c
      15 ./i915_config.c
      14 ./i915_trace_points.c
       9 ./display/intel_display_trace.c

So kind of meh, wouldn't be first. I'd add a dedicated file just for the 
benefit of being able to legitimately keep the i915_reg_read_ioctl name. 
Come multi-tile it may get company. Even though at the moment I am not 
aware anyone is trying to add multi-tile aware reg read, but I expect 
there will be need as long as need for the existing one exists.

Regards,

Tvrtko