* [PATCH] drm/i915: Add fault injection support @ 2016-03-11 23:15 Imre Deak 2016-03-12 7:40 ` ✗ Fi.CI.BAT: failure for " Patchwork ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Imre Deak @ 2016-03-11 23:15 UTC (permalink / raw) To: intel-gfx Add support for forcing an error at selected places in the driver. As an example add 3 options to fail during driver loading. Requested by Chris. CC: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Imre Deak <imre.deak@intel.com> --- [This depends on https://lists.freedesktop.org/archives/intel-gfx/2016-March/089597.html] drivers/gpu/drm/i915/i915_dma.c | 9 +++++++++ drivers/gpu/drm/i915/i915_drv.h | 4 ++++ drivers/gpu/drm/i915/i915_params.c | 6 ++++++ drivers/gpu/drm/i915/i915_params.h | 1 + 4 files changed, 20 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index b1dbf1b..d12ae0b 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -950,6 +950,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, struct intel_device_info *device_info; int ret = 0; + if (i915.inject_fault & I915_FAULT_INIT_EARLY) + return -ENODEV; + dev_priv->dev = dev; /* Setup the write-once "constant" device info */ @@ -1064,6 +1067,9 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; int ret; + if (i915.inject_fault & I915_FAULT_INIT_MMIO) + return -ENODEV; + if (i915_get_bridge_dev(dev)) return -EIO; @@ -1107,6 +1113,9 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) uint32_t aperture_size; int ret; + if (i915.inject_fault & I915_FAULT_INIT_HW) + return -ENODEV; + intel_device_info_runtime_init(dev); ret = i915_gem_gtt_init(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f62b6d1..5122ed9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -98,6 +98,10 @@ #define I915_STATE_WARN_ON(x) \ I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")") +#define I915_FAULT_INIT_EARLY 0x0001 +#define I915_FAULT_INIT_MMIO 0x0002 +#define I915_FAULT_INIT_HW 0x0004 + static inline const char *yesno(bool v) { return v ? "yes" : "no"; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 278c9c4..21f8dd3 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = { .edp_vswing = 0, .enable_guc_submission = false, .guc_log_level = -1, + .inject_fault = 0, }; module_param_named(modeset, i915.modeset, int, 0400); @@ -201,3 +202,8 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)") module_param_named(guc_log_level, i915.guc_log_level, int, 0400); MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); + +module_param_named(inject_fault, i915.inject_fault, uint, 0600); +MODULE_PARM_DESC(inject_fault, + "Force an error at selected points " + "(0:disabled, 0x1:INIT_EARLY, 0x2:INIT_MMIO, 0x4:INIT_HW)"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index bd5026b..2989a48 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -59,6 +59,7 @@ struct i915_params { bool enable_guc_submission; bool verbose_state_checks; bool nuclear_pageflip; + unsigned int inject_fault; }; extern struct i915_params i915 __read_mostly; -- 2.5.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support 2016-03-11 23:15 [PATCH] drm/i915: Add fault injection support Imre Deak @ 2016-03-12 7:40 ` Patchwork 2016-03-14 9:20 ` [PATCH v2] " Imre Deak ` (2 subsequent siblings) 3 siblings, 0 replies; 19+ messages in thread From: Patchwork @ 2016-03-12 7:40 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx == Series Details == Series: drm/i915: Add fault injection support URL : https://patchwork.freedesktop.org/series/4385/ State : failure == Summary == Series 4385v1 drm/i915: Add fault injection support 2016-03-11T22:53:14.152284 http://patchwork.freedesktop.org/api/1.0/series/4385/revisions/1/mbox/ Applying: drm/i915: Add fault injection support Using index info to reconstruct a base tree... M drivers/gpu/drm/i915/i915_dma.c M drivers/gpu/drm/i915/i915_drv.h Falling back to patching base and 3-way merge... Auto-merging drivers/gpu/drm/i915/i915_drv.h Auto-merging drivers/gpu/drm/i915/i915_dma.c CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_dma.c Patch failed at 0001 drm/i915: Add fault injection support _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] drm/i915: Add fault injection support 2016-03-11 23:15 [PATCH] drm/i915: Add fault injection support Imre Deak 2016-03-12 7:40 ` ✗ Fi.CI.BAT: failure for " Patchwork @ 2016-03-14 9:20 ` Imre Deak 2016-03-14 14:59 ` [PATCH v3] " Imre Deak 2016-03-14 10:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev2) Patchwork 2016-03-14 14:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev3) Patchwork 3 siblings, 1 reply; 19+ messages in thread From: Imre Deak @ 2016-03-14 9:20 UTC (permalink / raw) To: intel-gfx Add support for forcing an error at selected places in the driver. As an example add 4 options to fail during driver loading. Requested by Chris. v2: - Add fault point for modeset initialization - Print debug message when injecting an error CC: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 12 ++++++++++++ drivers/gpu/drm/i915/i915_drv.h | 16 ++++++++++++++++ drivers/gpu/drm/i915/i915_params.c | 5 +++++ drivers/gpu/drm/i915/i915_params.h | 1 + 4 files changed, 34 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index b1dbf1b..65be671 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -370,6 +370,9 @@ static int i915_load_modeset_init(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int ret; + if (i915_inject_fault(I915_FAULT_INIT_MODESET)) + return -ENODEV; + ret = intel_bios_init(dev_priv); if (ret) DRM_INFO("failed to find VBIOS tables\n"); @@ -950,6 +953,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, struct intel_device_info *device_info; int ret = 0; + if (i915_inject_fault(I915_FAULT_INIT_EARLY)) + return -ENODEV; + dev_priv->dev = dev; /* Setup the write-once "constant" device info */ @@ -1064,6 +1070,9 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; int ret; + if (i915_inject_fault(I915_FAULT_INIT_MMIO)) + return -ENODEV; + if (i915_get_bridge_dev(dev)) return -EIO; @@ -1107,6 +1116,9 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) uint32_t aperture_size; int ret; + if (i915_inject_fault(I915_FAULT_INIT_HW)) + return -ENODEV; + intel_device_info_runtime_init(dev); ret = i915_gem_gtt_init(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f62b6d1..33bcc6e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -98,6 +98,22 @@ #define I915_STATE_WARN_ON(x) \ I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")") +#define I915_FAULT_INIT_EARLY 1 +#define I915_FAULT_INIT_MMIO 2 +#define I915_FAULT_INIT_HW 4 +#define I915_FAULT_INIT_MODESET 8 + +static inline bool i915_inject_fault(unsigned int fault_mask) +{ + if (i915.inject_fault & fault_mask) { + DRM_DEBUG_DRIVER("Injecting fault %08x\n", fault_mask); + + return true; + } + + return false; +} + static inline const char *yesno(bool v) { return v ? "yes" : "no"; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 278c9c4..d96c2c0 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = { .edp_vswing = 0, .enable_guc_submission = false, .guc_log_level = -1, + .inject_fault = 0, }; module_param_named(modeset, i915.modeset, int, 0400); @@ -201,3 +202,7 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)") module_param_named(guc_log_level, i915.guc_log_level, int, 0400); MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); + +module_param_named(inject_fault, i915.inject_fault, uint, 0600); +MODULE_PARM_DESC(inject_fault, + "Force an error at selected points (0:disabled, 1:INIT_EARLY, 2:INIT_MMIO, 4:INIT_HW)"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index bd5026b..2989a48 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -59,6 +59,7 @@ struct i915_params { bool enable_guc_submission; bool verbose_state_checks; bool nuclear_pageflip; + unsigned int inject_fault; }; extern struct i915_params i915 __read_mostly; -- 2.5.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v3] drm/i915: Add fault injection support 2016-03-14 9:20 ` [PATCH v2] " Imre Deak @ 2016-03-14 14:59 ` Imre Deak 2016-03-15 8:34 ` Joonas Lahtinen 2016-03-15 8:56 ` Daniel Vetter 0 siblings, 2 replies; 19+ messages in thread From: Imre Deak @ 2016-03-14 14:59 UTC (permalink / raw) To: intel-gfx Add support for forcing an error at selected places in the driver. As an example add 4 options to fail during driver loading. Requested by Chris. v2: - Add fault point for modeset initialization - Print debug message when injecting an error v3: - Rename inject_fault to inject_load_failure, rename the related macros and helper accordingly (Chris) CC: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Imre Deak <imre.deak@intel.com> --- [This depends on https://lists.freedesktop.org/archives/intel-gfx/2016-March/089597.html] drivers/gpu/drm/i915/i915_dma.c | 12 ++++++++++++ drivers/gpu/drm/i915/i915_drv.h | 16 ++++++++++++++++ drivers/gpu/drm/i915/i915_params.c | 5 +++++ drivers/gpu/drm/i915/i915_params.h | 1 + 4 files changed, 34 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index a5121cd..7490307 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -370,6 +370,9 @@ static int i915_load_modeset_init(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int ret; + if (i915_inject_load_failure(I915_FAIL_INIT_MODESET)) + return -ENODEV; + ret = intel_bios_init(dev_priv); if (ret) DRM_INFO("failed to find VBIOS tables\n"); @@ -950,6 +953,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, struct intel_device_info *device_info; int ret = 0; + if (i915_inject_load_failure(I915_FAIL_INIT_EARLY)) + return -ENODEV; + dev_priv->dev = dev; /* Setup the write-once "constant" device info */ @@ -1064,6 +1070,9 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) struct drm_device *dev = dev_priv->dev; int ret; + if (i915_inject_load_failure(I915_FAIL_INIT_MMIO)) + return -ENODEV; + if (i915_get_bridge_dev(dev)) return -EIO; @@ -1107,6 +1116,9 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) uint32_t aperture_size; int ret; + if (i915_inject_load_failure(I915_FAIL_INIT_HW)) + return -ENODEV; + intel_device_info_runtime_init(dev); ret = i915_gem_gtt_init(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 25274e1..e2d21d5 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -98,6 +98,22 @@ #define I915_STATE_WARN_ON(x) \ I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")") +#define I915_FAIL_INIT_EARLY BIT(0) +#define I915_FAIL_INIT_MMIO BIT(1) +#define I915_FAIL_INIT_HW BIT(2) +#define I915_FAIL_INIT_MODESET BIT(3) + +static inline bool i915_inject_load_failure(unsigned int fail_mask) +{ + if (i915.inject_load_failure & fail_mask) { + DRM_DEBUG_DRIVER("Injecting failure %08x\n", fail_mask); + + return true; + } + + return false; +} + static inline const char *yesno(bool v) { return v ? "yes" : "no"; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 278c9c4..4faeeed 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = { .edp_vswing = 0, .enable_guc_submission = false, .guc_log_level = -1, + .inject_load_failure = 0, }; module_param_named(modeset, i915.modeset, int, 0400); @@ -201,3 +202,7 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)") module_param_named(guc_log_level, i915.guc_log_level, int, 0400); MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); + +module_param_named(inject_load_failure, i915.inject_load_failure, uint, 0600); +MODULE_PARM_DESC(inject_load_failure, + "Force an error at selected points (0:disabled, 0x1:INIT_EARLY, 0x2:INIT_MMIO, 0x4:INIT_HW, 0x8:INIT_MODESET)"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index bd5026b..b691026 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -59,6 +59,7 @@ struct i915_params { bool enable_guc_submission; bool verbose_state_checks; bool nuclear_pageflip; + unsigned int inject_load_failure; }; extern struct i915_params i915 __read_mostly; -- 2.5.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-14 14:59 ` [PATCH v3] " Imre Deak @ 2016-03-15 8:34 ` Joonas Lahtinen 2016-03-15 9:28 ` Chris Wilson 2016-03-15 14:08 ` Imre Deak 2016-03-15 8:56 ` Daniel Vetter 1 sibling, 2 replies; 19+ messages in thread From: Joonas Lahtinen @ 2016-03-15 8:34 UTC (permalink / raw) To: Imre Deak, intel-gfx On ma, 2016-03-14 at 16:59 +0200, Imre Deak wrote: > Add support for forcing an error at selected places in the driver. As > an > example add 4 options to fail during driver loading. > > Requested by Chris. > > v2: > - Add fault point for modeset initialization > - Print debug message when injecting an error > v3: > - Rename inject_fault to inject_load_failure, rename the related > macros > and helper accordingly (Chris) > > CC: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > > [This depends on > https://lists.freedesktop.org/archives/intel-gfx/2016-March/089597.h > tml] > > drivers/gpu/drm/i915/i915_dma.c | 12 ++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 16 ++++++++++++++++ > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > drivers/gpu/drm/i915/i915_params.h | 1 + > 4 files changed, 34 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c > b/drivers/gpu/drm/i915/i915_dma.c > index a5121cd..7490307 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -370,6 +370,9 @@ static int i915_load_modeset_init(struct > drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > + if (i915_inject_load_failure(I915_FAIL_INIT_MODESET)) > + return -ENODEV; > + > ret = intel_bios_init(dev_priv); > if (ret) > DRM_INFO("failed to find VBIOS tables\n"); > @@ -950,6 +953,9 @@ static int i915_driver_init_early(struct > drm_i915_private *dev_priv, > struct intel_device_info *device_info; > int ret = 0; > > + if (i915_inject_load_failure(I915_FAIL_INIT_EARLY)) > + return -ENODEV; > + > dev_priv->dev = dev; > > /* Setup the write-once "constant" device info */ > @@ -1064,6 +1070,9 @@ static int i915_driver_init_mmio(struct > drm_i915_private *dev_priv) > struct drm_device *dev = dev_priv->dev; > int ret; > > + if (i915_inject_load_failure(I915_FAIL_INIT_MMIO)) > + return -ENODEV; > + > if (i915_get_bridge_dev(dev)) > return -EIO; > > @@ -1107,6 +1116,9 @@ static int i915_driver_init_hw(struct > drm_i915_private *dev_priv) > uint32_t aperture_size; > int ret; > > + if (i915_inject_load_failure(I915_FAIL_INIT_HW)) > + return -ENODEV; > + > intel_device_info_runtime_init(dev); > > ret = i915_gem_gtt_init(dev); > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index 25274e1..e2d21d5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -98,6 +98,22 @@ > #define I915_STATE_WARN_ON(x) > \ > I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")") > > +#define I915_FAIL_INIT_EARLY BIT(0) > +#define I915_FAIL_INIT_MMIO BIT(1) > +#define I915_FAIL_INIT_HW BIT(2) > +#define I915_FAIL_INIT_MODESET BIT(3) > + > +static inline bool i915_inject_load_failure(unsigned int fail_mask) > +{ > + if (i915.inject_load_failure & fail_mask) { > + DRM_DEBUG_DRIVER("Injecting failure %08x\n", > fail_mask); > + > + return true; > + } > + > + return false; > +} > + > static inline const char *yesno(bool v) > { > return v ? "yes" : "no"; > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index 278c9c4..4faeeed 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = { > .edp_vswing = 0, > .enable_guc_submission = false, > .guc_log_level = -1, > + .inject_load_failure = 0, > }; > > module_param_named(modeset, i915.modeset, int, 0400); > @@ -201,3 +202,7 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable > GuC submission (default:false)") > module_param_named(guc_log_level, i915.guc_log_level, int, 0400); > MODULE_PARM_DESC(guc_log_level, > "GuC firmware logging level (-1:disabled (default), 0- > 3:enabled)"); > + > +module_param_named(inject_load_failure, i915.inject_load_failure, > uint, 0600); This most definitely should be module_param_named_unsafe. I think I'd also hope to see some kind of compile time option to enable this. I don't think average user wants this by default. > +MODULE_PARM_DESC(inject_load_failure, > + "Force an error at selected points (0:disabled, > 0x1:INIT_EARLY, 0x2:INIT_MMIO, 0x4:INIT_HW, 0x8:INIT_MODESET)"); > diff --git a/drivers/gpu/drm/i915/i915_params.h > b/drivers/gpu/drm/i915/i915_params.h > index bd5026b..b691026 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -59,6 +59,7 @@ struct i915_params { > bool enable_guc_submission; > bool verbose_state_checks; > bool nuclear_pageflip; > + unsigned int inject_load_failure; Duh, add it above the bools, this struct was cleaned once already :P Regards, Joonas > }; > > extern struct i915_params i915 __read_mostly; -- Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-15 8:34 ` Joonas Lahtinen @ 2016-03-15 9:28 ` Chris Wilson 2016-03-15 13:17 ` Daniel Vetter 2016-03-15 14:08 ` Imre Deak 1 sibling, 1 reply; 19+ messages in thread From: Chris Wilson @ 2016-03-15 9:28 UTC (permalink / raw) To: Joonas Lahtinen; +Cc: intel-gfx On Tue, Mar 15, 2016 at 10:34:02AM +0200, Joonas Lahtinen wrote: > On ma, 2016-03-14 at 16:59 +0200, Imre Deak wrote: > > +module_param_named(inject_load_failure, i915.inject_load_failure, > > uint, 0600); > > This most definitely should be module_param_named_unsafe. > > I think I'd also hope to see some kind of compile time option to enable > this. I don't think average user wants this by default. That's not a bad idea. We can get rid of most of the dangerous params this way. On the other hand, we need to keep the debug/quirk params around so that we can test options in the wild (bug reporters). -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-15 9:28 ` Chris Wilson @ 2016-03-15 13:17 ` Daniel Vetter 0 siblings, 0 replies; 19+ messages in thread From: Daniel Vetter @ 2016-03-15 13:17 UTC (permalink / raw) To: Chris Wilson, Joonas Lahtinen, Imre Deak, intel-gfx On Tue, Mar 15, 2016 at 09:28:34AM +0000, Chris Wilson wrote: > On Tue, Mar 15, 2016 at 10:34:02AM +0200, Joonas Lahtinen wrote: > > On ma, 2016-03-14 at 16:59 +0200, Imre Deak wrote: > > > +module_param_named(inject_load_failure, i915.inject_load_failure, > > > uint, 0600); > > > > This most definitely should be module_param_named_unsafe. > > > > I think I'd also hope to see some kind of compile time option to enable > > this. I don't think average user wants this by default. > > That's not a bad idea. We can get rid of most of the dangerous params > this way. On the other hand, we need to keep the debug/quirk params > around so that we can test options in the wild (bug reporters). Yeah, most of the _unsafe stuff is _really_ useful for quick bug triage. Definitely don't want to disable those by default. Maybe an i915.fry_my_kernel option? But even that is too confusing I think for testers. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-15 8:34 ` Joonas Lahtinen 2016-03-15 9:28 ` Chris Wilson @ 2016-03-15 14:08 ` Imre Deak 1 sibling, 0 replies; 19+ messages in thread From: Imre Deak @ 2016-03-15 14:08 UTC (permalink / raw) To: Joonas Lahtinen, intel-gfx On ti, 2016-03-15 at 10:34 +0200, Joonas Lahtinen wrote: > On ma, 2016-03-14 at 16:59 +0200, Imre Deak wrote: > > Add support for forcing an error at selected places in the driver. > > As > > an > > example add 4 options to fail during driver loading. > > > > Requested by Chris. > > > > v2: > > - Add fault point for modeset initialization > > - Print debug message when injecting an error > > v3: > > - Rename inject_fault to inject_load_failure, rename the related > > macros > > and helper accordingly (Chris) > > > > CC: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > > > [This depends on > > https://lists.freedesktop.org/archives/intel-gfx/2016-March/089597 > > .h > > tml] > > > > drivers/gpu/drm/i915/i915_dma.c | 12 ++++++++++++ > > drivers/gpu/drm/i915/i915_drv.h | 16 ++++++++++++++++ > > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > > drivers/gpu/drm/i915/i915_params.h | 1 + > > 4 files changed, 34 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c > > b/drivers/gpu/drm/i915/i915_dma.c > > index a5121cd..7490307 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -370,6 +370,9 @@ static int i915_load_modeset_init(struct > > drm_device *dev) > > struct drm_i915_private *dev_priv = dev->dev_private; > > int ret; > > > > + if (i915_inject_load_failure(I915_FAIL_INIT_MODESET)) > > + return -ENODEV; > > + > > ret = intel_bios_init(dev_priv); > > if (ret) > > DRM_INFO("failed to find VBIOS tables\n"); > > @@ -950,6 +953,9 @@ static int i915_driver_init_early(struct > > drm_i915_private *dev_priv, > > struct intel_device_info *device_info; > > int ret = 0; > > > > + if (i915_inject_load_failure(I915_FAIL_INIT_EARLY)) > > + return -ENODEV; > > + > > dev_priv->dev = dev; > > > > /* Setup the write-once "constant" device info */ > > @@ -1064,6 +1070,9 @@ static int i915_driver_init_mmio(struct > > drm_i915_private *dev_priv) > > struct drm_device *dev = dev_priv->dev; > > int ret; > > > > + if (i915_inject_load_failure(I915_FAIL_INIT_MMIO)) > > + return -ENODEV; > > + > > if (i915_get_bridge_dev(dev)) > > return -EIO; > > > > @@ -1107,6 +1116,9 @@ static int i915_driver_init_hw(struct > > drm_i915_private *dev_priv) > > uint32_t aperture_size; > > int ret; > > > > + if (i915_inject_load_failure(I915_FAIL_INIT_HW)) > > + return -ENODEV; > > + > > intel_device_info_runtime_init(dev); > > > > ret = i915_gem_gtt_init(dev); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 25274e1..e2d21d5 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -98,6 +98,22 @@ > > #define I915_STATE_WARN_ON(x) > > \ > > I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")") > > > > +#define I915_FAIL_INIT_EARLY BIT(0) > > +#define I915_FAIL_INIT_MMIO BIT(1) > > +#define I915_FAIL_INIT_HW BIT(2) > > +#define I915_FAIL_INIT_MODESET BIT(3) > > + > > +static inline bool i915_inject_load_failure(unsigned int > > fail_mask) > > +{ > > + if (i915.inject_load_failure & fail_mask) { > > + DRM_DEBUG_DRIVER("Injecting failure %08x\n", > > fail_mask); > > + > > + return true; > > + } > > + > > + return false; > > +} > > + > > static inline const char *yesno(bool v) > > { > > return v ? "yes" : "no"; > > diff --git a/drivers/gpu/drm/i915/i915_params.c > > b/drivers/gpu/drm/i915/i915_params.c > > index 278c9c4..4faeeed 100644 > > --- a/drivers/gpu/drm/i915/i915_params.c > > +++ b/drivers/gpu/drm/i915/i915_params.c > > @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = { > > .edp_vswing = 0, > > .enable_guc_submission = false, > > .guc_log_level = -1, > > + .inject_load_failure = 0, > > }; > > > > module_param_named(modeset, i915.modeset, int, 0400); > > @@ -201,3 +202,7 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable > > GuC submission (default:false)") > > module_param_named(guc_log_level, i915.guc_log_level, int, 0400); > > MODULE_PARM_DESC(guc_log_level, > > "GuC firmware logging level (-1:disabled (default), 0- > > 3:enabled)"); > > + > > +module_param_named(inject_load_failure, i915.inject_load_failure, > > uint, 0600); > > This most definitely should be module_param_named_unsafe. Ok, will change that. > > I think I'd also hope to see some kind of compile time option to > enable > this. I don't think average user wants this by default. I agree with Chris and Daniel, so I'll leave this out for now. It can be added later in any case. > > > +MODULE_PARM_DESC(inject_load_failure, > > + "Force an error at selected points (0:disabled, > > 0x1:INIT_EARLY, 0x2:INIT_MMIO, 0x4:INIT_HW, 0x8:INIT_MODESET)"); > > diff --git a/drivers/gpu/drm/i915/i915_params.h > > b/drivers/gpu/drm/i915/i915_params.h > > index bd5026b..b691026 100644 > > --- a/drivers/gpu/drm/i915/i915_params.h > > +++ b/drivers/gpu/drm/i915/i915_params.h > > @@ -59,6 +59,7 @@ struct i915_params { > > bool enable_guc_submission; > > bool verbose_state_checks; > > bool nuclear_pageflip; > > + unsigned int inject_load_failure; > > Duh, add it above the bools, this struct was cleaned once already :P Ah, missed the code comment about this. Will move it thanks. --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-14 14:59 ` [PATCH v3] " Imre Deak 2016-03-15 8:34 ` Joonas Lahtinen @ 2016-03-15 8:56 ` Daniel Vetter 2016-03-15 14:01 ` Imre Deak 1 sibling, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2016-03-15 8:56 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx On Mon, Mar 14, 2016 at 04:59:20PM +0200, Imre Deak wrote: > Add support for forcing an error at selected places in the driver. As an > example add 4 options to fail during driver loading. > > Requested by Chris. > > v2: > - Add fault point for modeset initialization > - Print debug message when injecting an error > v3: > - Rename inject_fault to inject_load_failure, rename the related macros > and helper accordingly (Chris) > > CC: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > > [This depends on > https://lists.freedesktop.org/archives/intel-gfx/2016-March/089597.html] > > drivers/gpu/drm/i915/i915_dma.c | 12 ++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 16 ++++++++++++++++ > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > drivers/gpu/drm/i915/i915_params.h | 1 + > 4 files changed, 34 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index a5121cd..7490307 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -370,6 +370,9 @@ static int i915_load_modeset_init(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > + if (i915_inject_load_failure(I915_FAIL_INIT_MODESET)) > + return -ENODEV; > + > ret = intel_bios_init(dev_priv); > if (ret) > DRM_INFO("failed to find VBIOS tables\n"); > @@ -950,6 +953,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, > struct intel_device_info *device_info; > int ret = 0; > > + if (i915_inject_load_failure(I915_FAIL_INIT_EARLY)) > + return -ENODEV; > + > dev_priv->dev = dev; > > /* Setup the write-once "constant" device info */ > @@ -1064,6 +1070,9 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > struct drm_device *dev = dev_priv->dev; > int ret; > > + if (i915_inject_load_failure(I915_FAIL_INIT_MMIO)) > + return -ENODEV; > + > if (i915_get_bridge_dev(dev)) > return -EIO; Ok, thought a bit more about this, and if we really want to check all the load failure paths then this will become extremely verbose. Since we'd need to have a bitflag for every return -EFAIL. So maybe another look at lib/fault-inject.c is warranted, plus then some macro magic that we could to wrap _all_ the checks in our load code like this: if (i915_load_failure(i915_get_bridge_dev(dev))) return -EIO; i915_load_failure ofc needs to fail if it's argument is false, but it could also increment a counter every time it's called and then use that counter to inquire the fault injection framework with should_fail(i915_load_failures, counter). Abusing a counter for the size would allow us to easily restrict fault injection to a certain range of faults. We could also expose the final value of that counter in debugfs, so that the igt can tune itself. Anyway, this is kinda a big plan, but the part I think we should ponder is how to make the fault injection less noise and intrusive. A macro to wrap existing checks seems like a good idea. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-15 8:56 ` Daniel Vetter @ 2016-03-15 14:01 ` Imre Deak 2016-03-15 14:14 ` Chris Wilson 0 siblings, 1 reply; 19+ messages in thread From: Imre Deak @ 2016-03-15 14:01 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On ti, 2016-03-15 at 09:56 +0100, Daniel Vetter wrote: > On Mon, Mar 14, 2016 at 04:59:20PM +0200, Imre Deak wrote: > > Add support for forcing an error at selected places in the driver. > > As an > > example add 4 options to fail during driver loading. > > > > Requested by Chris. > > > > v2: > > - Add fault point for modeset initialization > > - Print debug message when injecting an error > > v3: > > - Rename inject_fault to inject_load_failure, rename the related > > macros > > and helper accordingly (Chris) > > > > CC: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > > > [This depends on > > https://lists.freedesktop.org/archives/intel-gfx/2016-March/089597 > > .html] > > > > drivers/gpu/drm/i915/i915_dma.c | 12 ++++++++++++ > > drivers/gpu/drm/i915/i915_drv.h | 16 ++++++++++++++++ > > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > > drivers/gpu/drm/i915/i915_params.h | 1 + > > 4 files changed, 34 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c > > b/drivers/gpu/drm/i915/i915_dma.c > > index a5121cd..7490307 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -370,6 +370,9 @@ static int i915_load_modeset_init(struct > > drm_device *dev) > > struct drm_i915_private *dev_priv = dev->dev_private; > > int ret; > > > > + if (i915_inject_load_failure(I915_FAIL_INIT_MODESET)) > > + return -ENODEV; > > + > > ret = intel_bios_init(dev_priv); > > if (ret) > > DRM_INFO("failed to find VBIOS tables\n"); > > @@ -950,6 +953,9 @@ static int i915_driver_init_early(struct > > drm_i915_private *dev_priv, > > struct intel_device_info *device_info; > > int ret = 0; > > > > + if (i915_inject_load_failure(I915_FAIL_INIT_EARLY)) > > + return -ENODEV; > > + > > dev_priv->dev = dev; > > > > /* Setup the write-once "constant" device info */ > > @@ -1064,6 +1070,9 @@ static int i915_driver_init_mmio(struct > > drm_i915_private *dev_priv) > > struct drm_device *dev = dev_priv->dev; > > int ret; > > > > + if (i915_inject_load_failure(I915_FAIL_INIT_MMIO)) > > + return -ENODEV; > > + > > if (i915_get_bridge_dev(dev)) > > return -EIO; > > Ok, thought a bit more about this, and if we really want to check all the > load failure paths then this will become extremely verbose. Since we'd > need to have a bitflag for every return -EFAIL. I'm not sure if you want to check all failure paths, I think for that the existing failslab etc. mechanisms are better suited. This new option would be used at relatively few well defined places. The option is a mask since Chris wanted the possibility to mix failures (which makes sense when injecting a non-fatal failure somewhere). If he doesn't insist on that possibility I can convert the mask option to a counter based one identifying a single failure spot instead as you suggest. Chris? > So maybe another look at > lib/fault-inject.c is warranted, plus then some macro magic that we could > to wrap _all_ the checks in our load code like this: > > if (i915_load_failure(i915_get_bridge_dev(dev))) > return -EIO; The above will not work when we want to propagate the error and I haven't found a nice way to do that with such a helper taking the function call as argument. I could move the check to the call site instead of having it inside the called function, if that's what you'd prefer: if ((ret = i915_load_failure(...)) < 0 || (ret = i915_driver_init_mmio(...)) < 0) return ret; Although I'm not sure if this makes things clearer. Other suggestions are welcome. > i915_load_failure ofc needs to fail if it's argument is false, but it > could also increment a counter every time it's called and then use that > counter to inquire the fault injection framework with > should_fail(i915_load_failures, counter). Abusing a counter for the size > would allow us to easily restrict fault injection to a certain range of > faults. We could also expose the final value of that counter in debugfs, > so that the igt can tune itself. Yes, I could do this provided Chris doesn't oppose the idea of the counter based approach. > Anyway, this is kinda a big plan, but the part I think we should ponder is > how to make the fault injection less noise and intrusive. A macro to wrap > existing checks seems like a good idea. I can switch to using a counter instead of the mask and if there is a good suggestion about the macro I can use that instead. --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-15 14:01 ` Imre Deak @ 2016-03-15 14:14 ` Chris Wilson 2016-03-16 9:18 ` Joonas Lahtinen 0 siblings, 1 reply; 19+ messages in thread From: Chris Wilson @ 2016-03-15 14:14 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote: > I'm not sure if you want to check all failure paths, I think for that > the existing failslab etc. mechanisms are better suited. This new > option would be used at relatively few well defined places. The option > is a mask since Chris wanted the possibility to mix failures (which > makes sense when injecting a non-fatal failure somewhere). If he > doesn't insist on that possibility I can convert the mask option to a > counter based one identifying a single failure spot instead as you > suggest. Chris? We can extend the counter mechanism by having multiple counters behind i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1) so extensibility for more testing seems fine. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-15 14:14 ` Chris Wilson @ 2016-03-16 9:18 ` Joonas Lahtinen 2016-03-16 9:24 ` Chris Wilson 0 siblings, 1 reply; 19+ messages in thread From: Joonas Lahtinen @ 2016-03-16 9:18 UTC (permalink / raw) To: Chris Wilson, Imre Deak; +Cc: intel-gfx On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote: > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote: > > > > I'm not sure if you want to check all failure paths, I think for that > > the existing failslab etc. mechanisms are better suited. This new > > option would be used at relatively few well defined places. The option > > is a mask since Chris wanted the possibility to mix failures (which > > makes sense when injecting a non-fatal failure somewhere). If he > > doesn't insist on that possibility I can convert the mask option to a > > counter based one identifying a single failure spot instead as you > > suggest. Chris? > We can extend the counter mechanism by having multiple counters behind > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1) Now that there's a series to split down the init functions nicely, one could use the function names directly. By stripping parts of it if needed to shorten them. Regards, Joonas > so extensibility for more testing seems fine. > -Chris > -- Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-16 9:18 ` Joonas Lahtinen @ 2016-03-16 9:24 ` Chris Wilson 2016-03-16 9:43 ` Imre Deak 0 siblings, 1 reply; 19+ messages in thread From: Chris Wilson @ 2016-03-16 9:24 UTC (permalink / raw) To: Joonas Lahtinen; +Cc: intel-gfx On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote: > On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote: > > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote: > > > > > > I'm not sure if you want to check all failure paths, I think for that > > > the existing failslab etc. mechanisms are better suited. This new > > > option would be used at relatively few well defined places. The option > > > is a mask since Chris wanted the possibility to mix failures (which > > > makes sense when injecting a non-fatal failure somewhere). If he > > > doesn't insist on that possibility I can convert the mask option to a > > > counter based one identifying a single failure spot instead as you > > > suggest. Chris? > > We can extend the counter mechanism by having multiple counters behind > > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1) > > Now that there's a series to split down the init functions nicely, one > could use the function names directly. By stripping parts of it if > needed to shorten them. That's nice. Advantage for using counters is that we can write a test to iterate over the first thousand and have it run against future faultpointers. Advantage for using names is that it is readable and easily extensible. What we could do for testing is record the available fault injection points for debugfs and so have the tests automatically adjust (given a working start point). Overkill? -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-16 9:24 ` Chris Wilson @ 2016-03-16 9:43 ` Imre Deak 2016-03-16 10:04 ` Chris Wilson 0 siblings, 1 reply; 19+ messages in thread From: Imre Deak @ 2016-03-16 9:43 UTC (permalink / raw) To: Chris Wilson, Joonas Lahtinen; +Cc: intel-gfx On Wed, 2016-03-16 at 09:24 +0000, Chris Wilson wrote: > On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote: > > On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote: > > > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote: > > > > > > > > I'm not sure if you want to check all failure paths, I think > > > > for that > > > > the existing failslab etc. mechanisms are better suited. This > > > > new > > > > option would be used at relatively few well defined places. The > > > > option > > > > is a mask since Chris wanted the possibility to mix failures > > > > (which > > > > makes sense when injecting a non-fatal failure somewhere). If > > > > he > > > > doesn't insist on that possibility I can convert the mask > > > > option to a > > > > counter based one identifying a single failure spot instead as > > > > you > > > > suggest. Chris? > > > We can extend the counter mechanism by having multiple counters > > > behind > > > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1) > > > > Now that there's a series to split down the init functions nicely, > > one > > could use the function names directly. By stripping parts of it if > > needed to shorten them. > > That's nice. Advantage for using counters is that we can write a test > to > iterate over the first thousand and have it run against future > faultpointers. Advantage for using names is that it is readable and > easily extensible. > > What we could do for testing is record the available fault injection > points for debugfs and so have the tests automatically adjust (given > a > working start point). Overkill? I'd prefer if we could fine grain things once we have more failure points. Atm the ones we defined cause driver init to fail, so separate counters wouldn't bring us much. I suggest that we keep the module option as a simple integer for now and extend it later to support the above tagged counter mechanism and debugfs interface. Here is what I planned to submit to the list: https://github.com/ideak/linux/commits/driver_init_refactor Comments? --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-16 9:43 ` Imre Deak @ 2016-03-16 10:04 ` Chris Wilson 2016-03-16 10:17 ` Imre Deak 0 siblings, 1 reply; 19+ messages in thread From: Chris Wilson @ 2016-03-16 10:04 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx On Wed, Mar 16, 2016 at 11:43:14AM +0200, Imre Deak wrote: > On Wed, 2016-03-16 at 09:24 +0000, Chris Wilson wrote: > > On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote: > > > On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote: > > > > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote: > > > > > > > > > > I'm not sure if you want to check all failure paths, I think > > > > > for that > > > > > the existing failslab etc. mechanisms are better suited. This > > > > > new > > > > > option would be used at relatively few well defined places. The > > > > > option > > > > > is a mask since Chris wanted the possibility to mix failures > > > > > (which > > > > > makes sense when injecting a non-fatal failure somewhere). If > > > > > he > > > > > doesn't insist on that possibility I can convert the mask > > > > > option to a > > > > > counter based one identifying a single failure spot instead as > > > > > you > > > > > suggest. Chris? > > > > We can extend the counter mechanism by having multiple counters > > > > behind > > > > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1) > > > > > > Now that there's a series to split down the init functions nicely, > > > one > > > could use the function names directly. By stripping parts of it if > > > needed to shorten them. > > > > That's nice. Advantage for using counters is that we can write a test > > to > > iterate over the first thousand and have it run against future > > faultpointers. Advantage for using names is that it is readable and > > easily extensible. > > > > What we could do for testing is record the available fault injection > > points for debugfs and so have the tests automatically adjust (given > > a > > working start point). Overkill? > > I'd prefer if we could fine grain things once we have more failure > points. Atm the ones we defined cause driver init to fail, so separate > counters wouldn't bring us much. I suggest that we keep the module > option as a simple integer for now and extend it later to support the > above tagged counter mechanism and debugfs interface. Here is what I > planned to submit to the list: > > https://github.com/ideak/linux/commits/driver_init_refactor Move the inline into __i915_inject_load_failure, and yes I am happy with the simple first step. Please use __func__ or __FUNCTION__ though as that is more descriptive than file, especially when they move. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-16 10:04 ` Chris Wilson @ 2016-03-16 10:17 ` Imre Deak 2016-03-16 10:26 ` Chris Wilson 0 siblings, 1 reply; 19+ messages in thread From: Imre Deak @ 2016-03-16 10:17 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On Wed, 2016-03-16 at 10:04 +0000, Chris Wilson wrote: > On Wed, Mar 16, 2016 at 11:43:14AM +0200, Imre Deak wrote: > > On Wed, 2016-03-16 at 09:24 +0000, Chris Wilson wrote: > > > On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote: > > > > On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote: > > > > > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote: > > > > > > > > > > > > I'm not sure if you want to check all failure paths, I > > > > > > think > > > > > > for that > > > > > > the existing failslab etc. mechanisms are better suited. > > > > > > This > > > > > > new > > > > > > option would be used at relatively few well defined places. > > > > > > The > > > > > > option > > > > > > is a mask since Chris wanted the possibility to mix > > > > > > failures > > > > > > (which > > > > > > makes sense when injecting a non-fatal failure somewhere). > > > > > > If > > > > > > he > > > > > > doesn't insist on that possibility I can convert the mask > > > > > > option to a > > > > > > counter based one identifying a single failure spot instead > > > > > > as > > > > > > you > > > > > > suggest. Chris? > > > > > We can extend the counter mechanism by having multiple > > > > > counters > > > > > behind > > > > > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1) > > > > > > > > Now that there's a series to split down the init functions > > > > nicely, > > > > one > > > > could use the function names directly. By stripping parts of it > > > > if > > > > needed to shorten them. > > > > > > That's nice. Advantage for using counters is that we can write a > > > test > > > to > > > iterate over the first thousand and have it run against future > > > faultpointers. Advantage for using names is that it is readable > > > and > > > easily extensible. > > > > > > What we could do for testing is record the available fault > > > injection > > > points for debugfs and so have the tests automatically adjust > > > (given > > > a > > > working start point). Overkill? > > > > I'd prefer if we could fine grain things once we have more failure > > points. Atm the ones we defined cause driver init to fail, so > > separate > > counters wouldn't bring us much. I suggest that we keep the module > > option as a simple integer for now and extend it later to support > > the > > above tagged counter mechanism and debugfs interface. Here is what > > I > > planned to submit to the list: > > > > https://github.com/ideak/linux/commits/driver_init_refactor > > Move the inline into __i915_inject_load_failure hm, I have the macro in the header file to optimize for the failure_count=0 case. Or did you mean to bring __i915_inject_load_failure to the header file too/merge it into the macro? > , and yes I am happy with > the simple first step. Please use __func__ or __FUNCTION__ though as > that is more descriptive than file, especially when they move. Ok. --Imre > -Chris > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] drm/i915: Add fault injection support 2016-03-16 10:17 ` Imre Deak @ 2016-03-16 10:26 ` Chris Wilson 0 siblings, 0 replies; 19+ messages in thread From: Chris Wilson @ 2016-03-16 10:26 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx On Wed, Mar 16, 2016 at 12:17:51PM +0200, Imre Deak wrote: > On Wed, 2016-03-16 at 10:04 +0000, Chris Wilson wrote: > > On Wed, Mar 16, 2016 at 11:43:14AM +0200, Imre Deak wrote: > > > On Wed, 2016-03-16 at 09:24 +0000, Chris Wilson wrote: > > > > On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote: > > > > > On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote: > > > > > > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote: > > > > > > > > > > > > > > I'm not sure if you want to check all failure paths, I > > > > > > > think > > > > > > > for that > > > > > > > the existing failslab etc. mechanisms are better suited. > > > > > > > This > > > > > > > new > > > > > > > option would be used at relatively few well defined places. > > > > > > > The > > > > > > > option > > > > > > > is a mask since Chris wanted the possibility to mix > > > > > > > failures > > > > > > > (which > > > > > > > makes sense when injecting a non-fatal failure somewhere). > > > > > > > If > > > > > > > he > > > > > > > doesn't insist on that possibility I can convert the mask > > > > > > > option to a > > > > > > > counter based one identifying a single failure spot instead > > > > > > > as > > > > > > > you > > > > > > > suggest. Chris? > > > > > > We can extend the counter mechanism by having multiple > > > > > > counters > > > > > > behind > > > > > > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1) > > > > > > > > > > Now that there's a series to split down the init functions > > > > > nicely, > > > > > one > > > > > could use the function names directly. By stripping parts of it > > > > > if > > > > > needed to shorten them. > > > > > > > > That's nice. Advantage for using counters is that we can write a > > > > test > > > > to > > > > iterate over the first thousand and have it run against future > > > > faultpointers. Advantage for using names is that it is readable > > > > and > > > > easily extensible. > > > > > > > > What we could do for testing is record the available fault > > > > injection > > > > points for debugfs and so have the tests automatically adjust > > > > (given > > > > a > > > > working start point). Overkill? > > > > > > I'd prefer if we could fine grain things once we have more failure > > > points. Atm the ones we defined cause driver init to fail, so > > > separate > > > counters wouldn't bring us much. I suggest that we keep the module > > > option as a simple integer for now and extend it later to support > > > the > > > above tagged counter mechanism and debugfs interface. Here is what > > > I > > > planned to submit to the list: > > > > > > https://github.com/ideak/linux/commits/driver_init_refactor > > > > Move the inline into __i915_inject_load_failure > > hm, I have the macro in the header file to optimize for the > failure_count=0 case. Or did you mean to > bring __i915_inject_load_failure to the header file too/merge it into > the macro? Just the extra inlined conditional need not be in the header. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev2) 2016-03-11 23:15 [PATCH] drm/i915: Add fault injection support Imre Deak 2016-03-12 7:40 ` ✗ Fi.CI.BAT: failure for " Patchwork 2016-03-14 9:20 ` [PATCH v2] " Imre Deak @ 2016-03-14 10:40 ` Patchwork 2016-03-14 14:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev3) Patchwork 3 siblings, 0 replies; 19+ messages in thread From: Patchwork @ 2016-03-14 10:40 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx == Series Details == Series: drm/i915: Add fault injection support (rev2) URL : https://patchwork.freedesktop.org/series/4385/ State : failure == Summary == Series 4385v2 drm/i915: Add fault injection support 2016-03-14T08:58:17.234391 http://patchwork.freedesktop.org/api/1.0/series/4385/revisions/2/mbox/ Applying: drm/i915: Add fault injection support Using index info to reconstruct a base tree... M drivers/gpu/drm/i915/i915_dma.c M drivers/gpu/drm/i915/i915_drv.h Falling back to patching base and 3-way merge... Auto-merging drivers/gpu/drm/i915/i915_drv.h Auto-merging drivers/gpu/drm/i915/i915_dma.c CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_dma.c Patch failed at 0001 drm/i915: Add fault injection support _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev3) 2016-03-11 23:15 [PATCH] drm/i915: Add fault injection support Imre Deak ` (2 preceding siblings ...) 2016-03-14 10:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev2) Patchwork @ 2016-03-14 14:40 ` Patchwork 3 siblings, 0 replies; 19+ messages in thread From: Patchwork @ 2016-03-14 14:40 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx == Series Details == Series: drm/i915: Add fault injection support (rev3) URL : https://patchwork.freedesktop.org/series/4385/ State : failure == Summary == Series 4385v3 drm/i915: Add fault injection support 2016-03-14T14:37:24.542491 http://patchwork.freedesktop.org/api/1.0/series/4385/revisions/3/mbox/ Applying: drm/i915: Add fault injection support Using index info to reconstruct a base tree... M drivers/gpu/drm/i915/i915_dma.c M drivers/gpu/drm/i915/i915_drv.h Falling back to patching base and 3-way merge... Auto-merging drivers/gpu/drm/i915/i915_drv.h Auto-merging drivers/gpu/drm/i915/i915_dma.c CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_dma.c Patch failed at 0001 drm/i915: Add fault injection support _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-03-16 10:27 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-11 23:15 [PATCH] drm/i915: Add fault injection support Imre Deak 2016-03-12 7:40 ` ✗ Fi.CI.BAT: failure for " Patchwork 2016-03-14 9:20 ` [PATCH v2] " Imre Deak 2016-03-14 14:59 ` [PATCH v3] " Imre Deak 2016-03-15 8:34 ` Joonas Lahtinen 2016-03-15 9:28 ` Chris Wilson 2016-03-15 13:17 ` Daniel Vetter 2016-03-15 14:08 ` Imre Deak 2016-03-15 8:56 ` Daniel Vetter 2016-03-15 14:01 ` Imre Deak 2016-03-15 14:14 ` Chris Wilson 2016-03-16 9:18 ` Joonas Lahtinen 2016-03-16 9:24 ` Chris Wilson 2016-03-16 9:43 ` Imre Deak 2016-03-16 10:04 ` Chris Wilson 2016-03-16 10:17 ` Imre Deak 2016-03-16 10:26 ` Chris Wilson 2016-03-14 10:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev2) Patchwork 2016-03-14 14:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev3) Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.