| Message ID | 1508280649-26705-4-git-send-email-sujaritha.sundaresan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/18/2017 4:20 AM, Sujaritha Sundaresan wrote: > Updating GuC and HuC firmware select function to support removing > i915_modparams.enable_guc_loading module parameter. > > v2: Clarifying the commit message (Anusha) > > v3: Unify seq_puts messages, Re-factoring code as per review (Michal) > > v4: Rebase > > v5: Separating message unification into a separate patch > > v6: Re-factoring code (Sagar, Michal) > Rebase > > v7: Separating from previuos patch (Sagar) > Rebase > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++----- > drivers/gpu/drm/i915/intel_guc_fw.h | 2 +- > drivers/gpu/drm/i915/intel_huc.c | 4 +++- > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c > index ef67a36..5bffeef 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fw.c > +++ b/drivers/gpu/drm/i915/intel_guc_fw.c > @@ -63,7 +63,7 @@ > * > * Return: zero when we know firmware, non-zero in other case Update this documentation about return. You have dropped call to fw_select in this series. Can you please check. > */ > -int intel_guc_fw_select(struct intel_guc *guc) > +void intel_guc_fw_select(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > @@ -90,11 +90,10 @@ int intel_guc_fw_select(struct intel_guc *guc) > guc->fw.major_ver_wanted = GLK_FW_MAJOR; > guc->fw.minor_ver_wanted = GLK_FW_MINOR; > } else { > - DRM_ERROR("No GuC firmware known for platform with GuC!\n"); > - return -ENOENT; > + if (!HAS_GUC(dev_priv)) This should be HAS_GUC > + DRM_ERROR("No GuC FW known for platform with GuC!\n"); > + return; > } > - > - return 0; > } > > /* > diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h b/drivers/gpu/drm/i915/intel_guc_fw.h > index 023f5ba..7f6ccaf 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fw.h > +++ b/drivers/gpu/drm/i915/intel_guc_fw.h > @@ -27,7 +27,7 @@ > > struct intel_guc; > > -int intel_guc_fw_select(struct intel_guc *guc); > +void intel_guc_fw_select(struct intel_guc *guc); > int intel_guc_fw_upload(struct intel_guc *guc); > > #endif > diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c > index c8a48cb..fc61779 100644 > --- a/drivers/gpu/drm/i915/intel_huc.c > +++ b/drivers/gpu/drm/i915/intel_huc.c > @@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc) > huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR; > huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR; > } else { > - DRM_ERROR("No HuC firmware known for platform with HuC!\n"); > + /* For now, everything with a GuC also has a HuC */ > + if (HAS_GUC(dev_priv)) > + DRM_ERROR("No HuC FW known for platform with HuC!\n"); > return; > } > }
On Wed, 18 Oct 2017 12:29:13 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > > > On 10/18/2017 4:20 AM, Sujaritha Sundaresan wrote: >> Updating GuC and HuC firmware select function to support removing >> i915_modparams.enable_guc_loading module parameter. >> >> v2: Clarifying the commit message (Anusha) >> >> v3: Unify seq_puts messages, Re-factoring code as per review (Michal) >> >> v4: Rebase >> >> v5: Separating message unification into a separate patch >> >> v6: Re-factoring code (Sagar, Michal) >> Rebase >> >> v7: Separating from previuos patch (Sagar) >> Rebase >> >> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++----- >> drivers/gpu/drm/i915/intel_guc_fw.h | 2 +- >> drivers/gpu/drm/i915/intel_huc.c | 4 +++- >> 3 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c >> b/drivers/gpu/drm/i915/intel_guc_fw.c >> index ef67a36..5bffeef 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fw.c >> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c >> @@ -63,7 +63,7 @@ >> * >> * Return: zero when we know firmware, non-zero in other case > Update this documentation about return. > You have dropped call to fw_select in this series. Can you please check. >> */ >> -int intel_guc_fw_select(struct intel_guc *guc) >> +void intel_guc_fw_select(struct intel_guc *guc) >> { >> struct drm_i915_private *dev_priv = guc_to_i915(guc); >> @@ -90,11 +90,10 @@ int intel_guc_fw_select(struct intel_guc *guc) >> guc->fw.major_ver_wanted = GLK_FW_MAJOR; >> guc->fw.minor_ver_wanted = GLK_FW_MINOR; >> } else { >> - DRM_ERROR("No GuC firmware known for platform with GuC!\n"); >> - return -ENOENT; >> + if (!HAS_GUC(dev_priv)) > This should be HAS_GUC Hmm, I'm not sure that we really need such check here at all. We can do proper check *before* calling this function and cover both Huc and Guc at once. >> + DRM_ERROR("No GuC FW known for platform with GuC!\n"); >> + return; >> } >> - >> - return 0; >> } >> /* >> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h >> b/drivers/gpu/drm/i915/intel_guc_fw.h >> index 023f5ba..7f6ccaf 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fw.h >> +++ b/drivers/gpu/drm/i915/intel_guc_fw.h >> @@ -27,7 +27,7 @@ >> struct intel_guc; >> -int intel_guc_fw_select(struct intel_guc *guc); >> +void intel_guc_fw_select(struct intel_guc *guc); >> int intel_guc_fw_upload(struct intel_guc *guc); >> #endif >> diff --git a/drivers/gpu/drm/i915/intel_huc.c >> b/drivers/gpu/drm/i915/intel_huc.c >> index c8a48cb..fc61779 100644 >> --- a/drivers/gpu/drm/i915/intel_huc.c >> +++ b/drivers/gpu/drm/i915/intel_huc.c >> @@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc) >> huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR; >> huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR; >> } else { >> - DRM_ERROR("No HuC firmware known for platform with HuC!\n"); >> + /* For now, everything with a GuC also has a HuC */ >> + if (HAS_GUC(dev_priv)) >> + DRM_ERROR("No HuC FW known for platform with HuC!\n"); >> return; >> } >> }
On 10/18/2017 03:29 AM, Sagar Arun Kamble wrote: > > > On 10/18/2017 4:20 AM, Sujaritha Sundaresan wrote: >> Updating GuC and HuC firmware select function to support removing >> i915_modparams.enable_guc_loading module parameter. >> >> v2: Clarifying the commit message (Anusha) >> >> v3: Unify seq_puts messages, Re-factoring code as per review (Michal) >> >> v4: Rebase >> >> v5: Separating message unification into a separate patch >> >> v6: Re-factoring code (Sagar, Michal) >> Rebase >> >> v7: Separating from previuos patch (Sagar) >> Rebase >> >> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++----- >> drivers/gpu/drm/i915/intel_guc_fw.h | 2 +- >> drivers/gpu/drm/i915/intel_huc.c | 4 +++- >> 3 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c >> b/drivers/gpu/drm/i915/intel_guc_fw.c >> index ef67a36..5bffeef 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fw.c >> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c >> @@ -63,7 +63,7 @@ >> * >> * Return: zero when we know firmware, non-zero in other case > Update this documentation about return. > You have dropped call to fw_select in this series. Can you please check. I will check and update this. >> */ >> -int intel_guc_fw_select(struct intel_guc *guc) >> +void intel_guc_fw_select(struct intel_guc *guc) >> { >> struct drm_i915_private *dev_priv = guc_to_i915(guc); >> @@ -90,11 +90,10 @@ int intel_guc_fw_select(struct intel_guc *guc) >> guc->fw.major_ver_wanted = GLK_FW_MAJOR; >> guc->fw.minor_ver_wanted = GLK_FW_MINOR; >> } else { >> - DRM_ERROR("No GuC firmware known for platform with GuC!\n"); >> - return -ENOENT; >> + if (!HAS_GUC(dev_priv)) > This should be HAS_GUC I have decided to drop this check based on Michal's review. >> + DRM_ERROR("No GuC FW known for platform with GuC!\n"); >> + return; >> } >> - >> - return 0; >> } >> /* >> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h >> b/drivers/gpu/drm/i915/intel_guc_fw.h >> index 023f5ba..7f6ccaf 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fw.h >> +++ b/drivers/gpu/drm/i915/intel_guc_fw.h >> @@ -27,7 +27,7 @@ >> struct intel_guc; >> -int intel_guc_fw_select(struct intel_guc *guc); >> +void intel_guc_fw_select(struct intel_guc *guc); >> int intel_guc_fw_upload(struct intel_guc *guc); >> #endif >> diff --git a/drivers/gpu/drm/i915/intel_huc.c >> b/drivers/gpu/drm/i915/intel_huc.c >> index c8a48cb..fc61779 100644 >> --- a/drivers/gpu/drm/i915/intel_huc.c >> +++ b/drivers/gpu/drm/i915/intel_huc.c >> @@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc) >> huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR; >> huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR; >> } else { >> - DRM_ERROR("No HuC firmware known for platform with HuC!\n"); >> + /* For now, everything with a GuC also has a HuC */ >> + if (HAS_GUC(dev_priv)) >> + DRM_ERROR("No HuC FW known for platform with HuC!\n"); >> return; >> } >> } > Thanks for the review, Regards, Sujaritha
On 10/18/2017 02:17 PM, Michal Wajdeczko wrote: > On Wed, 18 Oct 2017 12:29:13 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> >> >> On 10/18/2017 4:20 AM, Sujaritha Sundaresan wrote: >>> Updating GuC and HuC firmware select function to support removing >>> i915_modparams.enable_guc_loading module parameter. >>> >>> v2: Clarifying the commit message (Anusha) >>> >>> v3: Unify seq_puts messages, Re-factoring code as per review (Michal) >>> >>> v4: Rebase >>> >>> v5: Separating message unification into a separate patch >>> >>> v6: Re-factoring code (Sagar, Michal) >>> Rebase >>> >>> v7: Separating from previuos patch (Sagar) >>> Rebase >>> >>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++----- >>> drivers/gpu/drm/i915/intel_guc_fw.h | 2 +- >>> drivers/gpu/drm/i915/intel_huc.c | 4 +++- >>> 3 files changed, 8 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c >>> b/drivers/gpu/drm/i915/intel_guc_fw.c >>> index ef67a36..5bffeef 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_fw.c >>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c >>> @@ -63,7 +63,7 @@ >>> * >>> * Return: zero when we know firmware, non-zero in other case >> Update this documentation about return. >> You have dropped call to fw_select in this series. Can you please check. >>> */ >>> -int intel_guc_fw_select(struct intel_guc *guc) >>> +void intel_guc_fw_select(struct intel_guc *guc) >>> { >>> struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> @@ -90,11 +90,10 @@ int intel_guc_fw_select(struct intel_guc *guc) >>> guc->fw.major_ver_wanted = GLK_FW_MAJOR; >>> guc->fw.minor_ver_wanted = GLK_FW_MINOR; >>> } else { >>> - DRM_ERROR("No GuC firmware known for platform with GuC!\n"); >>> - return -ENOENT; >>> + if (!HAS_GUC(dev_priv)) >> This should be HAS_GUC > > Hmm, I'm not sure that we really need such check here at all. > We can do proper check *before* calling this function and cover both > Huc and Guc at once. I will drop the check in the next revision. > >>> + DRM_ERROR("No GuC FW known for platform with GuC!\n"); >>> + return; >>> } >>> - >>> - return 0; >>> } >>> /* >>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h >>> b/drivers/gpu/drm/i915/intel_guc_fw.h >>> index 023f5ba..7f6ccaf 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_fw.h >>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.h >>> @@ -27,7 +27,7 @@ >>> struct intel_guc; >>> -int intel_guc_fw_select(struct intel_guc *guc); >>> +void intel_guc_fw_select(struct intel_guc *guc); >>> int intel_guc_fw_upload(struct intel_guc *guc); >>> #endif >>> diff --git a/drivers/gpu/drm/i915/intel_huc.c >>> b/drivers/gpu/drm/i915/intel_huc.c >>> index c8a48cb..fc61779 100644 >>> --- a/drivers/gpu/drm/i915/intel_huc.c >>> +++ b/drivers/gpu/drm/i915/intel_huc.c >>> @@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc) >>> huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR; >>> huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR; >>> } else { >>> - DRM_ERROR("No HuC firmware known for platform with HuC!\n"); >>> + /* For now, everything with a GuC also has a HuC */ >>> + if (HAS_GUC(dev_priv)) >>> + DRM_ERROR("No HuC FW known for platform with HuC!\n"); >>> return; >>> } >>> } Thanks for the review. Regards, Sujaritha
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c index ef67a36..5bffeef 100644 --- a/drivers/gpu/drm/i915/intel_guc_fw.c +++ b/drivers/gpu/drm/i915/intel_guc_fw.c @@ -63,7 +63,7 @@ * * Return: zero when we know firmware, non-zero in other case */ -int intel_guc_fw_select(struct intel_guc *guc) +void intel_guc_fw_select(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); @@ -90,11 +90,10 @@ int intel_guc_fw_select(struct intel_guc *guc) guc->fw.major_ver_wanted = GLK_FW_MAJOR; guc->fw.minor_ver_wanted = GLK_FW_MINOR; } else { - DRM_ERROR("No GuC firmware known for platform with GuC!\n"); - return -ENOENT; + if (!HAS_GUC(dev_priv)) + DRM_ERROR("No GuC FW known for platform with GuC!\n"); + return; } - - return 0; } /* diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h b/drivers/gpu/drm/i915/intel_guc_fw.h index 023f5ba..7f6ccaf 100644 --- a/drivers/gpu/drm/i915/intel_guc_fw.h +++ b/drivers/gpu/drm/i915/intel_guc_fw.h @@ -27,7 +27,7 @@ struct intel_guc; -int intel_guc_fw_select(struct intel_guc *guc); +void intel_guc_fw_select(struct intel_guc *guc); int intel_guc_fw_upload(struct intel_guc *guc); #endif diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c index c8a48cb..fc61779 100644 --- a/drivers/gpu/drm/i915/intel_huc.c +++ b/drivers/gpu/drm/i915/intel_huc.c @@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc) huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR; huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR; } else { - DRM_ERROR("No HuC firmware known for platform with HuC!\n"); + /* For now, everything with a GuC also has a HuC */ + if (HAS_GUC(dev_priv)) + DRM_ERROR("No HuC FW known for platform with HuC!\n"); return; } }
Updating GuC and HuC firmware select function to support removing i915_modparams.enable_guc_loading module parameter. v2: Clarifying the commit message (Anusha) v3: Unify seq_puts messages, Re-factoring code as per review (Michal) v4: Rebase v5: Separating message unification into a separate patch v6: Re-factoring code (Sagar, Michal) Rebase v7: Separating from previuos patch (Sagar) Rebase Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> --- drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++----- drivers/gpu/drm/i915/intel_guc_fw.h | 2 +- drivers/gpu/drm/i915/intel_huc.c | 4 +++- 3 files changed, 8 insertions(+), 7 deletions(-)