| Message ID | 1476144064-5314-1-git-send-email-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> == Summary == > > Series 13358v2 drm/i915/guc: Sanitory checks for platform that dont have > GuC > https://patchwork.freedesktop.org/api/1.0/series/13358/revisions/2/mbox/ > > Test kms_pipe_crc_basic: > Subgroup bad-nb-words-3: > pass -> DMESG-WARN (fi-ilk-650) Stdout IGT-Version: 1.16-g48a9e1e (x86_64) (Linux: 4.8.0-CI-Patchwork_2668+ x86_64) Subtest bad-nb-words-3: SUCCESS (0.000s) Stderr Environment PIGLIT_SOURCE_DIR="/opt/igt/piglit" PIGLIT_PLATFORM="mixed_glx_egl" Command /opt/igt/tests/kms_pipe_crc_basic --run-subtest bad-nb-words-3 dmesg [ 401.433161] [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR* PCH transcoder B FIFO underrun [ 401.433188] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe B FIFO underrun > fi-ilk-650 total:248 pass:184 dwarn:1 dfail:0 fail:2 skip:61 --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Tue, 11 Oct 2016, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote: > i915.enable_guc_loading/submission=2 forces the usage of GuC. > For platforms that do not have a GuC, asking the kernel to > use a GuC should not result in an error state. Do extra checks > to see if the platform even has a GuC or not, regardless of the > kernel parameter. > > Based on Rodriogo's <vivi.rodrigo@intel.com> patch. > Have considered Paulo Zanoni's<paulo.r.zanoni@intel.com> > suggestions on the implementation. There's a bug for this, please find it and reference it. > take on the implemenation. > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_loader.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 7ace96b..98718db 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -718,12 +718,17 @@ void intel_guc_init(struct drm_device *dev) > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > const char *fw_path; > > + if (!HAS_GUC(dev)) { > + i915.enable_guc_loading = 0; > + i915.enable_guc_submission = 0; > + } else { > /* A negative value means "use platform default" */ > if (i915.enable_guc_loading < 0) > i915.enable_guc_loading = HAS_GUC_UCODE(dev); > if (i915.enable_guc_submission < 0) > i915.enable_guc_submission = HAS_GUC_SCHED(dev); > > + } Indentation? BR, Jani. > if (!HAS_GUC_UCODE(dev)) { > fw_path = NULL; > } else if (IS_SKYLAKE(dev)) {
Em Ter, 2016-10-11 às 10:36 +0300, Jani Nikula escreveu: > On Tue, 11 Oct 2016, Anusha Srivatsa <anusha.srivatsa@intel.com> > wrote: > > > > i915.enable_guc_loading/submission=2 forces the usage of GuC. > > For platforms that do not have a GuC, asking the kernel to > > use a GuC should not result in an error state. Do extra checks > > to see if the platform even has a GuC or not, regardless of the > > kernel parameter. > > > > Based on Rodriogo's <vivi.rodrigo@intel.com> patch. > > Have considered Paulo Zanoni's<paulo.r.zanoni@intel.com> > > suggestions on the implementation. The correct way to give credit to reviewers is by adding a patch version changelog, and later including any reviewed-by tags that the reviewer may give. No need for a sentence like the one above. Just take a small look at the git log and see how people do the changelogs. Yours would be something like: v2: change indentation, add back blank lines (Paulo) (but I do have to add that the way you changed it was not the way I had in mind, please see below) > > There's a bug for this, please find it and reference it. > > > > > take on the implemenation. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > --- > > drivers/gpu/drm/i915/intel_guc_loader.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > > b/drivers/gpu/drm/i915/intel_guc_loader.c > > index 7ace96b..98718db 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > > @@ -718,12 +718,17 @@ void intel_guc_init(struct drm_device *dev) > > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > const char *fw_path; > > > > + if (!HAS_GUC(dev)) { > > + i915.enable_guc_loading = 0; > > + i915.enable_guc_submission = 0; > > + } else { > > /* A negative value means "use platform default" */ > > if (i915.enable_guc_loading < 0) > > i915.enable_guc_loading = HAS_GUC_UCODE(dev); > > if (i915.enable_guc_submission < 0) > > i915.enable_guc_submission = HAS_GUC_SCHED(dev); > > > > + } The blank line is usually inserted _after_ the line containing the closing bracket char. Please see the many examples in the rest of our code. > > Indentation? I had already complained about indentation before. It's different now, but it's still not exactly what I was expecting... The idea was indeed to align the comment indentation with the rest of the "else" case, but not the way it was done here. Please see the many examples in the rest of our code....... > > BR, > Jani. > > > > > if (!HAS_GUC_UCODE(dev)) { > > fw_path = NULL; > > } else if (IS_SKYLAKE(dev)) { >
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 7ace96b..98718db 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -718,12 +718,17 @@ void intel_guc_init(struct drm_device *dev) struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; const char *fw_path; + if (!HAS_GUC(dev)) { + i915.enable_guc_loading = 0; + i915.enable_guc_submission = 0; + } else { /* A negative value means "use platform default" */ if (i915.enable_guc_loading < 0) i915.enable_guc_loading = HAS_GUC_UCODE(dev); if (i915.enable_guc_submission < 0) i915.enable_guc_submission = HAS_GUC_SCHED(dev); + } if (!HAS_GUC_UCODE(dev)) { fw_path = NULL; } else if (IS_SKYLAKE(dev)) {
i915.enable_guc_loading/submission=2 forces the usage of GuC. For platforms that do not have a GuC, asking the kernel to use a GuC should not result in an error state. Do extra checks to see if the platform even has a GuC or not, regardless of the kernel parameter. Based on Rodriogo's <vivi.rodrigo@intel.com> patch. Have considered Paulo Zanoni's<paulo.r.zanoni@intel.com> suggestions on the implementation. take on the implemenation. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 5 +++++ 1 file changed, 5 insertions(+)