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

[Arkadiusz Hiler]

On Wed, Apr 22, 2020 at 11:48:53AM +0300, Jani Nikula wrote:
> On Wed, 22 Apr 2020, Petri Latvala <petri.latvala at 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 at intel.com> wrote:
> >> > On Tue, Apr 21, 2020 at 07:17:19PM +0300, Juha-Pekka Heikkila wrote:
> >> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> >> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at 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 at 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