Re: [igt-dev] [PATCH i-g-t 2/8] lib/params: start renaming functions igt_params_*

Arkadiusz Hiler
2020-04-22T09:04:29Z

On Wed, Apr 22, 2020 at 11:48:53AM +0300, Jani Nikula wrote:
> On Wed, 22 Apr 2020, Petri Latvala <petri.latvala@intel.com> wrote:
> > On Wed, Apr 22, 2020 at 11:34:44AM +0300, Jani Nikula wrote:
> >> On Wed, 22 Apr 2020, Petri Latvala <petri.latvala@intel.com> wrote:
> >> > On Tue, Apr 21, 2020 at 07:17:19PM +0300, Juha-Pekka Heikkila wrote:
> >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> >> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> >> >> ---
> >> >> lib/drmtest.c | 2 +-
> >> >> lib/i915/gem_submission.c | 2 +-
> >> >> lib/igt_aux.c | 3 +--
> >> >> lib/igt_gt.c | 2 +-
> >> >> lib/igt_params.c | 12 +++++-------
> >> >> lib/igt_params.h | 5 ++---
> >> >> tests/i915/gem_ctx_exec.c | 2 +-
> >> >> tests/i915/gem_ctx_persistence.c | 9 ++++-----
> >> >> tests/i915/gem_mmap_gtt.c | 2 +-
> >> >> tests/i915/gem_reset_stats.c | 6 ++----
> >> >> tests/i915/sysfs_heartbeat_interval.c | 3 ++-
> >> >> tests/i915/sysfs_preempt_timeout.c | 3 ++-
> >> >> tests/i915/sysfs_timeslice_duration.c | 3 ++-
> >> >> 13 files changed, 25 insertions(+), 29 deletions(-)
> >> >>
> >> >> diff --git a/lib/drmtest.c b/lib/drmtest.c
> >> >> index 1fc39925..17067843 100644
> >> >> --- a/lib/drmtest.c
> >> >> +++ b/lib/drmtest.c
> >> >> @@ -393,7 +393,7 @@ static void __cancel_work_at_exit(int fd)
> >> >> {
> >> >> igt_terminate_spins(); /* for older kernels */
> >> >>
> >> >> - igt_sysfs_set_parameter(fd, "reset", "%x", -1u /* any method */);
> >> >> + igt_params_set(fd, "reset", "%x", -1u /* any method */);
> >> >
> >> >
> >> > Hmm. Now that we're renaming anyway, we have a good chance to consider
> >> > if the name is good.
> >> >
> >> > igt_module_params_set()? No, the point of this series is to make the
> >> > params not module-specific. igt_device_params_set()?
> >> >
> >> > I don't have a strong opinion on this and I can live with
> >> > igt_params_set() as well. Just calling out for opinions! Arek?
> >> >
> >> > As for the patch, whatever the name, straight up renames all around.
> >> > Reviewed-by: Petri Latvala <petri.latvala@intel.com>
> >>
> >> The way I envisioned it is that it would look at the debugfs for device
> >> params, and if they don't exist, it should fall back to the module
> >> params for older kernels. Hence just igt_params_set to avoid being
> >> specific about device vs. module. Also, shorter.
> >
> > I was more concerned with the confusion between params for the kernel
> > vs params for other operations. But I'm well prepared to accept that
> > the confusion exists only in my own head.
>
> Ah. I'll leave it up to your discretion if the confusion is bad
> enough. At least I'm not hung up on the naming, so long as we unify the
> naming, something this series tries to do as a side effect.

igt_params() looks a bit like IGT itself is parametrizable in some
way... But the confusion did not last long, and I think this should be
easy enough to internalize. I would prefer using something like
igt_dev_params to both keep it short and make it a bit more obvious as
of what it does, but no strong opinions here.

--
Cheers,
Arek