| Message ID | 1475711459-5904-1-git-send-email-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Qua, 2016-10-05 às 16:50 -0700, Anusha Srivatsa escreveu: > 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. I'm not exactly sure who did what here, but I think Rodrigo deserves some credit for his initial work (and possibly debug). Maybe add: Based-on-patch-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Zanoni Paulo <paulo.r.zanoni@intel.com> I see the Cc tag here but no Cc mail header with my email on it, so this email wasn't colored blue on my intel-gfx folder. Why didn't I actually get Cc'd here? Did you use git-send-email? This may be worth investigating since it may happen again in the future, and you want to make sure people you Cc actually get Cc'd. > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > b/drivers/gpu/drm/i915/intel_guc_loader.c > index 7ace96b..15d2d53 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -717,13 +717,16 @@ void intel_guc_init(struct drm_device *dev) > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > const char *fw_path; > - Please do not remove the blank line above. > + if (!HAS_GUC(dev)) { > + i915.enable_guc_loading = 0; > + i915.enable_guc_submission = 0; > + } else { > /* A negative value means "use platform default" */ Bad indentation here. > - 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); > - Please also do not remove the blank line above. Otherwise, looks good. > + 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)) {
On Fri, 2016-10-07 at 10:41 -0300, Paulo Zanoni wrote: > Em Qua, 2016-10-05 às 16:50 -0700, Anusha Srivatsa escreveu: > > 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. > > I'm not exactly sure who did what here, but I think Rodrigo deserves > some credit for his initial work (and possibly debug). Maybe add: > Based-on-patch-by: Rodrigo Vivi <rodrigo.vivi@intel.com> I just drop a quickly patch that day, but she did the work, specially debugging it after. > > > > Cc: Zanoni Paulo <paulo.r.zanoni@intel.com> > > I see the Cc tag here but no Cc mail header with my email on it, so > this email wasn't colored blue on my intel-gfx folder. Why didn't I > actually get Cc'd here? Did you use git-send-email? This may be worth > investigating since it may happen again in the future, and you want to > make sure people you Cc actually get Cc'd. > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > --- > > drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > > b/drivers/gpu/drm/i915/intel_guc_loader.c > > index 7ace96b..15d2d53 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > > @@ -717,13 +717,16 @@ void intel_guc_init(struct drm_device *dev) > > struct drm_i915_private *dev_priv = to_i915(dev); > > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > const char *fw_path; > > - > > Please do not remove the blank line above. > > > + if (!HAS_GUC(dev)) { > > + i915.enable_guc_loading = 0; > > + i915.enable_guc_submission = 0; > > + } else { > > /* A negative value means "use platform default" */ > > Bad indentation here. > > > - 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); > > - > > Please also do not remove the blank line above. > > Otherwise, looks good. > > > + 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)) {
>-----Original Message----- >From: Vivi, Rodrigo >Sent: Friday, October 7, 2016 7:06 AM >To: Zanoni, Paulo R <paulo.r.zanoni@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Srivatsa, Anusha ><anusha.srivatsa@intel.com> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/guc: Sanitory checks for platform that >dont have GuC > >On Fri, 2016-10-07 at 10:41 -0300, Paulo Zanoni wrote: >> Em Qua, 2016-10-05 às 16:50 -0700, Anusha Srivatsa escreveu: >> > 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. >> >> I'm not exactly sure who did what here, but I think Rodrigo deserves >> some credit for his initial work (and possibly debug). Maybe add: >> Based-on-patch-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >I just drop a quickly patch that day, but she did the work, specially debugging it >after. I agree with you Paulo. In fact I was thinking of an exact way of giving credit to both you and Rodrigo since the patch is inspired by his patch and I have taken your suggestion as well. Will update the patch with "Based-on". >> >> >> > Cc: Zanoni Paulo <paulo.r.zanoni@intel.com> >> >> I see the Cc tag here but no Cc mail header with my email on it, so >> this email wasn't colored blue on my intel-gfx folder. Why didn't I >> actually get Cc'd here? Did you use git-send-email? This may be worth >> investigating since it may happen again in the future, and you want to >> make sure people you Cc actually get Cc'd. I used git send-email. Will investigate it. >> >> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++------ >> > 1 file changed, 9 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >> > b/drivers/gpu/drm/i915/intel_guc_loader.c >> > index 7ace96b..15d2d53 100644 >> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> > @@ -717,13 +717,16 @@ void intel_guc_init(struct drm_device *dev) >> > struct drm_i915_private *dev_priv = to_i915(dev); >> > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; >> > const char *fw_path; >> > - >> >> Please do not remove the blank line above. >> >> > + if (!HAS_GUC(dev)) { >> > + i915.enable_guc_loading = 0; >> > + i915.enable_guc_submission = 0; >> > + } else { >> > /* A negative value means "use platform default" */ >> >> Bad indentation here. >> >> > - 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); >> > - >> >> Please also do not remove the blank line above. >> >> Otherwise, looks good. Will fix indentation issue. Thank you. >> > + 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)) {
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 7ace96b..15d2d53 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -717,13 +717,16 @@ void intel_guc_init(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(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 (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. Cc: Zanoni Paulo <paulo.r.zanoni@intel.com> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)