Re: [igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

Juha-Pekka Heikkila
2020-05-05T18:44:58Z

On 5.5.2020 10.16, Petri Latvala wrote:
> On Tue, Apr 28, 2020 at 11:22:53PM +0300, Juha-Pekka Heikkila wrote:
>> From: Jani Nikula <jani.nikula@intel.com>
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>> lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
>> lib/igt_params.h | 3 +++
>> 2 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/igt_params.c b/lib/igt_params.c
>> index fe4b1df3..d9cf986c 100644
>> --- a/lib/igt_params.c
>> +++ b/lib/igt_params.c
>> @@ -153,6 +153,29 @@ int igt_params_open(int device)
>> return __igt_params_open(device, NULL);
>> }
>>
>> +__attribute__((format(printf, 3, 0)))
>> +static bool __igt_params_set(int device, const char *parameter,
>> + const char *fmt, va_list ap, bool save)
>> +{
>> + char *path = NULL;
>> + int dir;
>> + int ret;
>> +
>> + dir = __igt_params_open(device, save ? &path : NULL);
>> + if (dir < 0)
>> + return false;
>> +
>> + if (save)
>> + igt_params_save(dir, path, parameter);
>> +
>> + ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
>> +
>> + close(dir);
>> + free(path);
>> +
>> + return ret > 0;
>> +}
>> +
>> /**
>> * igt_params_set:
>> * @device: fd of the device
>> @@ -164,20 +187,36 @@ int igt_params_open(int device)
>> bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
>> {
>> va_list ap;
>> - int dir;
>> - int ret;
>> -
>> - dir = igt_params_open(device);
>> - if (dir < 0)
>> - return false;
>> + bool ret;
>>
>> va_start(ap, fmt);
>> - ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
>> + ret = __igt_params_set(device, parameter, fmt, ap, false);
>> va_end(ap);
>>
>> - close(dir);
>> + return ret;
>> +}
>>
>> - return ret > 0;
>> +/**
>> + * igt_params_set_save:
>> + * @device: fd of the device or -1 to default to first matching starting from
>> + * zero. See __igt_params_open()
>
>
> This documentation is still only understandable if you already know what it does.
>
> "-- to default to the first /dev/dri/cardX device, starting from zero" ?
>
> "See <a function without documentation>" in generated documentation is
> less than useful as well... Maybe just drop that sentence altogether
> from docs, point to the function in a normal comment? Or link to
> igt_debugfs_path() that will do the actual -1 handling...
>
> With better docs,
> Reviewed-by: Petri Latvala <petri.latvala@intel.com>

These were generally in line with how sysfs was documented but I agree
it's more fun to read code where there are more hints on what's going
on. I'll checkout those comment blocks what's all the stuff that could
be said there.

>
>
>
>> + * @parameter: the name of the parameter to set
>> + * @fmt: printf-esque format string
>> + *
>> + * Save the original value to be restored.
>> + *
>> + * Returns true on success
>> + */
>> +bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...)
>> +{
>> + va_list ap;
>> + bool ret;
>> +
>> + va_start(ap, fmt);
>> + ret = __igt_params_set(device, parameter, fmt, ap, true);
>> + va_end(ap);
>> +
>> + return ret;
>> }
>>
>> /**
>> diff --git a/lib/igt_params.h b/lib/igt_params.h
>> index 52eed77f..1b750d23 100644
>> --- a/lib/igt_params.h
>> +++ b/lib/igt_params.h
>> @@ -31,6 +31,9 @@ int igt_params_open(int device);
>> __attribute__((format(printf, 3, 4)))
>> bool igt_params_set(int device, const char *parameter, const char *fmt, ...);
>>
>> +__attribute__((format(printf, 3, 4)))
>> +bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...);
>> +
>> void igt_set_module_param(const char *name, const char *val);
>> void igt_set_module_param_int(const char *name, int val);
>>
>> --
>> 2.26.0
>>
>> _______________________________________________
>> igt-dev mailing list
>> igt-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/igt-dev