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

[Tvrtko Ursulin]

On 19/01/2022 11:12, Jani Nikula wrote:
> On Wed, 05 Jan 2022, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
>> 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.
> 
> So this got stalled a bit, and sidestepped from the main goal of just
> cleaning up i915_drv.h from the clutter that absolutely does not belong
> there.
> 
> Can we just merge patch 1, leave further cleanup to follow-up, and move
> on?

Sorry but I don't think the ioctl belongs in gt/intel_uncore.h either so 
I can't make myself ack it. But I am not nacking it either, as said 
before, if you find someone else to support it.

I would add i915_ioctls.[ch] even if they hold just this one.

Regards,

Tvrtko