Re: [Intel-gfx] [PATCH] drm/i915/guc: Sanitory checks for platform that dont have GuC
Em Qua, 2016-10-12 às 16:13 -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. > > v2: Based on Rodrigo's patch and Paulo's suggestion(Paulo, Rodrigo) > v3: Correct the Indentation(Jani, Paulo) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > Cc: Rodrigo Vivi > Cc: Zanoni Paulo > Cc: Jani Nikula > > Signed-off-by: Anusha Srivatsa > --- > drivers/gpu/drm/i915/intel_guc_loader.c | 16 ++-- > 1 file changed, 10 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..811080f 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -718,12 +718,16 @@ void intel_guc_init(struct drm_device *dev) > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > const char *fw_path; > > - /* 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(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); > + } Well, the indentation is correct now, but this patch is still removing the blank line that was supposed to be at this spot, despite the fact that I pointed this problem in my two previous reviews... Please always pay attention to the reviews and try to make sure you're actually following them when submit new versions. It's a good idea if you can learn the patch format and look at the patch files directly in order to spot problems like this before submitting. I know this sounds super annoying since I'm just complaining about blank lines, but since I expect you to start contributing much more, I think it's worth trying to teach the coding style that's used by everybody so the next patches all come in the expected format. Anyway, I suppose we could amend this fix while applying the patch? If someone fixes that while applying, we can add: Reviewed-by: Paulo Zanoni > if (!HAS_GUC_UCODE(dev)) { > fw_path = NULL; > } else if (IS_SKYLAKE(dev)) { ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/guc: Sanitory checks for platform that dont have GuC
Em Ter, 2016-10-11 às 10:36 +0300, Jani Nikula escreveu: > On Tue, 11 Oct 2016, Anusha Srivatsa > 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 patch. > > Have considered Paulo Zanoni's > > 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 > > Signed-off-by: Anusha Srivatsa > > --- > > 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)) { > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/guc: Sanitory checks for platform that dont have GuC
On Tue, 11 Oct 2016, Anusha Srivatsa 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 patch. > Have considered Paulo Zanoni's > suggestions on the implementation. There's a bug for this, please find it and reference it. > take on the implemenation. > Cc: Paulo Zanoni > Signed-off-by: Anusha Srivatsa > --- > 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)) { -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/guc: Sanitory checks for platform that dont have GuC
>-Original Message- >From: Vivi, Rodrigo >Sent: Friday, October 7, 2016 7:06 AM >To: Zanoni, Paulo R >Cc: intel-gfx@lists.freedesktop.org; Srivatsa, Anusha > >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 > >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 >> >> 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 >> > --- >> > 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)) { ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 I just drop a quickly patch that day, but she did the work, specially debugging it after. > > > > Cc: Zanoni Paulo > > 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 > > --- > > 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)) { ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/guc: Sanitory checks for platform that dont have GuC
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 > Cc: Zanoni Paulo 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 > --- > 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)) { ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx