| Message ID | 20170922142726.44532-2-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michal Wajdeczko (2017-09-22 15:27:25) > By combining default value into helper macro we can initialize > modparams struct in the same automatic way as it was declared. > This will initialize members in the same order as declared > and additionally will disallow declaring new member without > proper default value for it. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Overall, I think this is a positive change. I'm not completely happy that the param() macro is more readable than the struct assignment, but that is offset by the reduction in duplication. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Fri, 22 Sep 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2017-09-22 15:27:25) >> By combining default value into helper macro we can initialize >> modparams struct in the same automatic way as it was declared. >> This will initialize members in the same order as declared >> and additionally will disallow declaring new member without >> proper default value for it. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Overall, I think this is a positive change. I'm not completely happy > that the param() macro is more readable than the struct assignment, but > that is offset by the reduction in duplication. I'm also not completely happy that the default values get moved away from the param descriptions. (Hmm, what next, putting the permissions and descriptions in I915_PARAMS_FOR_EACH too?! :o) There's also the benefit of being able to highlight the changed values and displaying the defaults in debugfs if desired. On the series, Acked-by: Jani Nikula <jani.nikula@intel.com>
On Fri, 2017-09-22 at 22:05 +0300, Jani Nikula wrote: > On Fri, 22 Sep 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Michal Wajdeczko (2017-09-22 15:27:25) > > > By combining default value into helper macro we can initialize > > > modparams struct in the same automatic way as it was declared. > > > This will initialize members in the same order as declared > > > and additionally will disallow declaring new member without > > > proper default value for it. > > > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > > Overall, I think this is a positive change. I'm not completely happy > > that the param() macro is more readable than the struct assignment, but > > that is offset by the reduction in duplication. > > I'm also not completely happy that the default values get moved away > from the param descriptions. (Hmm, what next, putting the permissions > and descriptions in I915_PARAMS_FOR_EACH too?! :o) > > There's also the benefit of being able to highlight the changed values > and displaying the defaults in debugfs if desired. Yes, I think this would be useful going forward. Regards, Joonas
On Fri, 2017-09-22 at 14:27 +0000, Michal Wajdeczko wrote: > By combining default value into helper macro we can initialize > modparams struct in the same automatic way as it was declared. > This will initialize members in the same order as declared > and additionally will disallow declaring new member without > proper default value for it. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Ha, seems like you beat me to it :) (https://lists.freedesktop.org/arch ives/intel-gfx/2017-September/139268.html) <SNIP> > @@ -33,45 +33,9 @@ > MODULE_PARM_DESC(name, desc) > > struct i915_params i915_modparams __read_mostly = { > - .modeset = -1, > - .panel_ignore_lid = 1, > - .semaphores = -1, > - .lvds_channel_mode = 0, > - .panel_use_ssc = -1, > - .vbt_sdvo_panel_type = -1, > - .enable_rc6 = -1, > - .enable_dc = -1, > - .enable_fbc = -1, > - .enable_execlists = -1, > - .enable_hangcheck = true, > - .enable_ppgtt = -1, > - .enable_psr = -1, > - .alpha_support = IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT), > - .disable_power_well = -1, > - .enable_ips = 1, > - .fastboot = 0, > - .prefault_disable = 0, > - .load_detect_test = 0, > - .force_reset_modeset_test = 0, > - .reset = 2, > - .error_capture = true, > - .invert_brightness = 0, > - .disable_display = 0, > - .enable_cmd_parser = true, > - .use_mmio_flip = 0, > - .mmio_debug = 0, > - .verbose_state_checks = 1, > - .nuclear_pageflip = 0, > - .edp_vswing = 0, > - .enable_guc_loading = 0, > - .enable_guc_submission = 0, > - .guc_log_level = -1, > - .guc_firmware_path = NULL, > - .huc_firmware_path = NULL, > - .enable_dp_mst = true, > - .inject_load_failure = 0, > - .enable_dpcd_backlight = false, > - .enable_gvt = false, > +#define MEMBER(T, member, value) .member = value, Should be .member = (value), With that; Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
On Fri, 22 Sep 2017 21:05:11 +0200, Jani Nikula <jani.nikula@intel.com> wrote: > On Fri, 22 Sep 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> Quoting Michal Wajdeczko (2017-09-22 15:27:25) >>> By combining default value into helper macro we can initialize >>> modparams struct in the same automatic way as it was declared. >>> This will initialize members in the same order as declared >>> and additionally will disallow declaring new member without >>> proper default value for it. >>> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> >> Overall, I think this is a positive change. I'm not completely happy >> that the param() macro is more readable than the struct assignment, but >> that is offset by the reduction in duplication. > > I'm also not completely happy that the default values get moved away > from the param descriptions. (Hmm, what next, putting the permissions > and descriptions in I915_PARAMS_FOR_EACH too?! :o) > In fact, I was already trying that ;) https://patchwork.freedesktop.org/patch/162037/ > There's also the benefit of being able to highlight the changed values > and displaying the defaults in debugfs if desired. And such highlight was also there https://patchwork.freedesktop.org/patch/162038/ Michal > > On the series, > > Acked-by: Jani Nikula <jani.nikula@intel.com> >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 13fc259..29c2299 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -67,7 +67,7 @@ static int i915_capabilities(struct seq_file *m, void *data) #undef PRINT_FLAG kernel_param_lock(THIS_MODULE); -#define PRINT_PARAM(T, x) seq_print_param(m, #x, #T, &i915_modparams.x); +#define PRINT_PARAM(T, x, ...) seq_print_param(m, #x, #T, &i915_modparams.x); I915_PARAMS_FOR_EACH(PRINT_PARAM); #undef PRINT_PARAM kernel_param_unlock(THIS_MODULE); diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index c7aaf62..c76b036 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -567,7 +567,7 @@ static __always_inline void err_print_param(struct drm_i915_error_state_buf *m, static void err_print_params(struct drm_i915_error_state_buf *m, const struct i915_params *p) { -#define PRINT(T, x) err_print_param(m, #x, #T, &p->x); +#define PRINT(T, x, ...) err_print_param(m, #x, #T, &p->x); I915_PARAMS_FOR_EACH(PRINT); #undef PRINT } @@ -861,7 +861,7 @@ void __i915_gpu_state_free(struct kref *error_ref) kfree(error->overlay); kfree(error->display); -#define FREE(T, x) free_param(#T, &error->params.x); +#define FREE(T, x, ...) free_param(#T, &error->params.x); I915_PARAMS_FOR_EACH(FREE); #undef FREE @@ -1697,7 +1697,7 @@ static int capture(void *data) error->i915->gt.last_init_time)); error->params = i915_modparams; -#define DUP(T, x) dup_param(#T, &error->params.x); +#define DUP(T, x, ...) dup_param(#T, &error->params.x); I915_PARAMS_FOR_EACH(DUP); #undef DUP diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index ec65341..46eab92 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -33,45 +33,9 @@ MODULE_PARM_DESC(name, desc) struct i915_params i915_modparams __read_mostly = { - .modeset = -1, - .panel_ignore_lid = 1, - .semaphores = -1, - .lvds_channel_mode = 0, - .panel_use_ssc = -1, - .vbt_sdvo_panel_type = -1, - .enable_rc6 = -1, - .enable_dc = -1, - .enable_fbc = -1, - .enable_execlists = -1, - .enable_hangcheck = true, - .enable_ppgtt = -1, - .enable_psr = -1, - .alpha_support = IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT), - .disable_power_well = -1, - .enable_ips = 1, - .fastboot = 0, - .prefault_disable = 0, - .load_detect_test = 0, - .force_reset_modeset_test = 0, - .reset = 2, - .error_capture = true, - .invert_brightness = 0, - .disable_display = 0, - .enable_cmd_parser = true, - .use_mmio_flip = 0, - .mmio_debug = 0, - .verbose_state_checks = 1, - .nuclear_pageflip = 0, - .edp_vswing = 0, - .enable_guc_loading = 0, - .enable_guc_submission = 0, - .guc_log_level = -1, - .guc_firmware_path = NULL, - .huc_firmware_path = NULL, - .enable_dp_mst = true, - .inject_load_failure = 0, - .enable_dpcd_backlight = false, - .enable_gvt = false, +#define MEMBER(T, member, value) .member = value, + I915_PARAMS_FOR_EACH(MEMBER) +#undef MEMBER }; i915_param_named(modeset, int, 0400, diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 0116bb9..da59939 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -28,49 +28,49 @@ #include <linux/cache.h> /* for __read_mostly */ #define I915_PARAMS_FOR_EACH(param) \ - param(char *, vbt_firmware) \ - param(int, modeset) \ - param(int, panel_ignore_lid) \ - param(int, semaphores) \ - param(int, lvds_channel_mode) \ - param(int, panel_use_ssc) \ - param(int, vbt_sdvo_panel_type) \ - param(int, enable_rc6) \ - param(int, enable_dc) \ - param(int, enable_fbc) \ - param(int, enable_ppgtt) \ - param(int, enable_execlists) \ - param(int, enable_psr) \ - param(int, disable_power_well) \ - param(int, enable_ips) \ - param(int, invert_brightness) \ - param(int, enable_guc_loading) \ - param(int, enable_guc_submission) \ - param(int, guc_log_level) \ - param(char *, guc_firmware_path) \ - param(char *, huc_firmware_path) \ - param(int, use_mmio_flip) \ - param(int, mmio_debug) \ - param(int, edp_vswing) \ - param(int, reset) \ - param(unsigned int, inject_load_failure) \ + param(char *, vbt_firmware, NULL) \ + param(int, modeset, -1) \ + param(int, panel_ignore_lid, 1) \ + param(int, semaphores, -1) \ + param(int, lvds_channel_mode, 0) \ + param(int, panel_use_ssc, -1) \ + param(int, vbt_sdvo_panel_type, -1) \ + param(int, enable_rc6, -1) \ + param(int, enable_dc, -1) \ + param(int, enable_fbc, -1) \ + param(int, enable_ppgtt, -1) \ + param(int, enable_execlists, -1) \ + param(int, enable_psr, -1) \ + param(int, disable_power_well, -1) \ + param(int, enable_ips, 1) \ + param(int, invert_brightness, 0) \ + param(int, enable_guc_loading, 0) \ + param(int, enable_guc_submission, 0) \ + param(int, guc_log_level, -1) \ + param(char *, guc_firmware_path, NULL) \ + param(char *, huc_firmware_path, NULL) \ + param(int, use_mmio_flip, 0) \ + param(int, mmio_debug, 0) \ + param(int, edp_vswing, 0) \ + param(int, reset, 2) \ + param(unsigned int, inject_load_failure, 0) \ /* leave bools at the end to not create holes */ \ - param(bool, alpha_support) \ - param(bool, enable_cmd_parser) \ - param(bool, enable_hangcheck) \ - param(bool, fastboot) \ - param(bool, prefault_disable) \ - param(bool, load_detect_test) \ - param(bool, force_reset_modeset_test) \ - param(bool, error_capture) \ - param(bool, disable_display) \ - param(bool, verbose_state_checks) \ - param(bool, nuclear_pageflip) \ - param(bool, enable_dp_mst) \ - param(bool, enable_dpcd_backlight) \ - param(bool, enable_gvt) + param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \ + param(bool, enable_cmd_parser, true) \ + param(bool, enable_hangcheck, true) \ + param(bool, fastboot, 0) \ + param(bool, prefault_disable, 0) \ + param(bool, load_detect_test, 0) \ + param(bool, force_reset_modeset_test, 0) \ + param(bool, error_capture, true) \ + param(bool, disable_display, 0) \ + param(bool, verbose_state_checks, 1) \ + param(bool, nuclear_pageflip, 0) \ + param(bool, enable_dp_mst, true) \ + param(bool, enable_dpcd_backlight, false) \ + param(bool, enable_gvt, false) -#define MEMBER(T, member) T member; +#define MEMBER(T, member, ...) T member; struct i915_params { I915_PARAMS_FOR_EACH(MEMBER); };
By combining default value into helper macro we can initialize modparams struct in the same automatic way as it was declared. This will initialize members in the same order as declared and additionally will disallow declaring new member without proper default value for it. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c | 6 +-- drivers/gpu/drm/i915/i915_params.c | 42 ++---------------- drivers/gpu/drm/i915/i915_params.h | 82 +++++++++++++++++------------------ 4 files changed, 48 insertions(+), 84 deletions(-)