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