| Message ID | 20190306003011.12724-1-sujaritha.sundaresan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/guc: Fixing error code for WOPCM initialization | expand |
On Wed, 06 Mar 2019 01:30:11 +0100, Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> wrote: > Replacing the -E2BIG error code return for WOPCM > initialization with -ENODEV. This will prevent the pci from s/pci/CI ? > picking this up as a warning during fault injection testing. > > v2: change the final return code in i915_pci_probe() to ENODEV > instead of the specific wopcm change. - Daniele s/wopcm/WOPCM > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > --- > drivers/gpu/drm/i915/i915_pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c > b/drivers/gpu/drm/i915/i915_pci.c > index c42c5ccf38fe..f962b5c0b3c1 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -778,7 +778,7 @@ static int i915_pci_probe(struct pci_dev *pdev, > const struct pci_device_id *ent) > err = i915_driver_load(pdev, ent); > if (err) > - return err; > + return i915_error_injected() ? -ENODEV : err; > if (i915_inject_load_failure()) { > i915_pci_remove(pdev); Hmm, since this is quite big deviation from original approach, maybe you should sent it as new patch (with updated title and commit message) as now this covers all injected errors (not just E2BIG from WOPCM) Anyway, with typos fixed this is: Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Thanks, ~Michal
Quoting Michal Wajdeczko (2019-03-06 08:41:20) > On Wed, 06 Mar 2019 01:30:11 +0100, Sujaritha Sundaresan > <sujaritha.sundaresan@intel.com> wrote: > > > Replacing the -E2BIG error code return for WOPCM > > initialization with -ENODEV. This will prevent the pci from > > s/pci/CI ? > > > picking this up as a warning during fault injection testing. > > > > v2: change the final return code in i915_pci_probe() to ENODEV > > instead of the specific wopcm change. - Daniele > > s/wopcm/WOPCM > > > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_pci.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c > > b/drivers/gpu/drm/i915/i915_pci.c > > index c42c5ccf38fe..f962b5c0b3c1 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -778,7 +778,7 @@ static int i915_pci_probe(struct pci_dev *pdev, > > const struct pci_device_id *ent) > > err = i915_driver_load(pdev, ent); > > if (err) > > - return err; > > + return i915_error_injected() ? -ENODEV : err; > > if (i915_inject_load_failure()) { > > i915_pci_remove(pdev); > > Hmm, since this is quite big deviation from original approach, maybe > you should sent it as new patch (with updated title and commit message) > as now this covers all injected errors (not just E2BIG from WOPCM) And I'm not sure we want to do that. When we do failure injection, we pick the errno and part of the test is making sure that gets reported unadulterated. Now doing an if (i915_error_injected() && !err) err = -EINVAL; makes sense to catch places where we've eaten that error and so breaking the test. -Chris
On Wed, 06 Mar 2019 09:45:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2019-03-06 08:41:20) >> On Wed, 06 Mar 2019 01:30:11 +0100, Sujaritha Sundaresan >> <sujaritha.sundaresan@intel.com> wrote: >> >> > Replacing the -E2BIG error code return for WOPCM >> > initialization with -ENODEV. This will prevent the pci from >> >> s/pci/CI ? >> >> > picking this up as a warning during fault injection testing. >> > >> > v2: change the final return code in i915_pci_probe() to ENODEV >> > instead of the specific wopcm change. - Daniele >> >> s/wopcm/WOPCM >> >> > >> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_pci.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_pci.c >> > b/drivers/gpu/drm/i915/i915_pci.c >> > index c42c5ccf38fe..f962b5c0b3c1 100644 >> > --- a/drivers/gpu/drm/i915/i915_pci.c >> > +++ b/drivers/gpu/drm/i915/i915_pci.c >> > @@ -778,7 +778,7 @@ static int i915_pci_probe(struct pci_dev *pdev, >> > const struct pci_device_id *ent) >> > err = i915_driver_load(pdev, ent); >> > if (err) >> > - return err; >> > + return i915_error_injected() ? -ENODEV : err; >> > if (i915_inject_load_failure()) { >> > i915_pci_remove(pdev); >> >> Hmm, since this is quite big deviation from original approach, maybe >> you should sent it as new patch (with updated title and commit message) >> as now this covers all injected errors (not just E2BIG from WOPCM) > > And I'm not sure we want to do that. When we do failure injection, we > pick the errno and part of the test is making sure that gets reported > unadulterated. > > Now doing an if (i915_error_injected() && !err) err = -EINVAL; makes > sense to catch places where we've eaten that error and so breaking the > test. This will not work today, as at least in one place - i915_gem_init - we inject -EIO which later we try to replace with success. And for that case we may rather want to add if (i915_error_injected() && !err) i915_pci_remove(pdev); return -ENODEV; } ~Michal
Quoting Michal Wajdeczko (2019-03-06 09:01:09) > On Wed, 06 Mar 2019 09:45:17 +0100, Chris Wilson > > Now doing an if (i915_error_injected() && !err) err = -EINVAL; makes > > sense to catch places where we've eaten that error and so breaking the > > test. > > This will not work today, as at least in one place - i915_gem_init - we > inject -EIO which later we try to replace with success. And for that case > we may rather want to add I think we may want to move that particular test to one side, something along the lines of i915.disable_gpu=1 ? -Chris
On 3/6/19 1:08 AM, Chris Wilson wrote: > Quoting Michal Wajdeczko (2019-03-06 09:01:09) >> On Wed, 06 Mar 2019 09:45:17 +0100, Chris Wilson >>> Now doing an if (i915_error_injected() && !err) err = -EINVAL; makes >>> sense to catch places where we've eaten that error and so breaking the >>> test. >> This will not work today, as at least in one place - i915_gem_init - we >> inject -EIO which later we try to replace with success. And for that case >> we may rather want to add > I think we may want to move that particular test to one side, something > along the lines of i915.disable_gpu=1 ? > -Chris Just to clarify on how to proceed with the fix. I'm not sure if the suggestion is to go back to the previous version of the fix or to go for a different approach. Also Arek has said in the internal mailing list that there are some changes being made to the -piglit-style-dmesg, so looks like this issue will be flagging up in the public CI as well. -Sujaritha <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p><br> </p> <div class="moz-cite-prefix">On 3/6/19 1:08 AM, Chris Wilson wrote:<br> </div> <blockquote type="cite" cite="mid:155186330557.27405.944800209177609752@skylake-alporthouse-com"> <pre class="moz-quote-pre" wrap="">Quoting Michal Wajdeczko (2019-03-06 09:01:09) </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">On Wed, 06 Mar 2019 09:45:17 +0100, Chris Wilson </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Now doing an if (i915_error_injected() && !err) err = -EINVAL; makes sense to catch places where we've eaten that error and so breaking the test. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> This will not work today, as at least in one place - i915_gem_init - we inject -EIO which later we try to replace with success. And for that case we may rather want to add </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> I think we may want to move that particular test to one side, something along the lines of i915.disable_gpu=1 ? -Chris </pre> </blockquote> <p><font size="-1">Just to clarify on how to proceed with the fix. I'm not sure if the <br> </font></p> <p><font size="-1">suggestion is to go back to the previous version of the fix or to</font></p> <p><font size="-1">go for a different approach. Also Arek has said in the internal <br> </font></p> <p><font size="-1">mailing list that there are some changes being made to the <br> </font></p> <p><font size="-1">-piglit-style-dmesg, </font><font size="-1"><font size="-1">so looks like this issue will be</font> flagging up in <br> </font></p> <p><font size="-1">the public CI as well. <br> </font></p> <p><font size="-1">-Sujaritha<br> </font></p> </body> </html>
-----Original Message----- From: Chris Wilson <chris@chris-wilson.co.uk> Sent: Wednesday, March 6, 2019 1:08 AM To: Wajdeczko, Michal <Michal.Wajdeczko@intel.com>; Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/guc: Fixing error code for WOPCM initialization Quoting Michal Wajdeczko (2019-03-06 09:01:09) > On Wed, 06 Mar 2019 09:45:17 +0100, Chris Wilson > > Now doing an if (i915_error_injected() && !err) err = -EINVAL; makes > > sense to catch places where we've eaten that error and so breaking > > the test. > > This will not work today, as at least in one place - i915_gem_init - > we inject -EIO which later we try to replace with success. And for > that case we may rather want to add I think we may want to move that particular test to one side, something along the lines of i915.disable_gpu=1 ? -Chris Just to clarify on how to proceed with the fix. I'm not sure if the suggestion is to go back to the previous version of the patch or to go for a different approach. It also looks like this issue will be flaggin up on the public CI, following the changes Arek has said will be made to the dmesg style. -Sujaritha
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index c42c5ccf38fe..f962b5c0b3c1 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -778,7 +778,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) err = i915_driver_load(pdev, ent); if (err) - return err; + return i915_error_injected() ? -ENODEV : err; if (i915_inject_load_failure()) { i915_pci_remove(pdev);
Replacing the -E2BIG error code return for WOPCM initialization with -ENODEV. This will prevent the pci from picking this up as a warning during fault injection testing. v2: change the final return code in i915_pci_probe() to ENODEV instead of the specific wopcm change. - Daniele Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> --- drivers/gpu/drm/i915/i915_pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)