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

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 05 Jan 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 05/01/2022 13:18, Jani Nikula wrote:
>> On Wed, 05 Jan 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> 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]?
>> 
>> 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?


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center