| Message ID | 1508865685-7722-2-git-send-email-sujaritha.sundaresan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> wrote: > Unifying the various seq_puts messages in debugfs to the simplest one for > feature support. > > v2: Clarifying the commit message (Anusha) > > v3: Re-factoring code as per review (Michal) > > v4: Rebase > > v5: Split from following patch > > v6: Re-factoring code (Michal, Sagar) > Clarifying commit message (Sagar) > > v7: Generalizing subject to drm/i915 (Sagar) > > v8: Omitting DRRS seq_puts unification (Michal) > > Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index c65e381..8edd029 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1641,7 +1641,7 @@ static int i915_fbc_status(struct seq_file *m, > void *unused) > struct drm_i915_private *dev_priv = node_to_i915(m->private); > if (!HAS_FBC(dev_priv)) { > - seq_puts(m, "FBC unsupported on this chipset\n"); > + seq_puts(m, "not supported\n"); > return 0; > } > @@ -1809,7 +1809,7 @@ static int i915_ring_freq_table(struct seq_file > *m, void *unused) > unsigned int max_gpu_freq, min_gpu_freq; > if (!HAS_LLC(dev_priv)) { > - seq_puts(m, "unsupported on this chipset\n"); > + seq_puts(m, "not supported\n"); > return 0; > } > @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct > seq_file *m, void *data) > struct drm_i915_private *dev_priv = node_to_i915(m->private); > struct drm_printer p; > - if (!HAS_HUC_UCODE(dev_priv)) > + if (!HAS_GUC(dev_priv)) { Hmm, I think that in above code we should use HAS_HUC defined as: /* HuC is inherent part of the GuC ... */ #define HAS_HUC(dev_priv) HAS_GUC(dev_priv) to make it clear that code checks HuC sub-feature (not other part of the GuC or GuC itself). And additionally we can use above define to explicitly document relation between GuC and HuC. Michal > + seq_puts(m, "not supported\n"); > return 0; > + } > p = drm_seq_file_printer(m); > intel_uc_fw_dump(&dev_priv->huc.fw, &p); > @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct > seq_file *m, void *data) > struct drm_printer p; > u32 tmp, i; > - if (!HAS_GUC_UCODE(dev_priv)) > + if (!HAS_GUC(dev_priv)) { > + seq_puts(m, "not supported\n"); > return 0; > + } > p = drm_seq_file_printer(m); > intel_uc_fw_dump(&dev_priv->guc.fw, &p); > @@ -2650,7 +2654,7 @@ static int i915_edp_psr_status(struct seq_file *m, > void *data) > bool enabled = false; > if (!HAS_PSR(dev_priv)) { > - seq_puts(m, "PSR not supported\n"); > + seq_puts(m, "not supported\n"); > return 0; > }
On 25/10/17 06:31, Michal Wajdeczko wrote: > On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan > <sujaritha.sundaresan@intel.com> wrote: > >> Unifying the various seq_puts messages in debugfs to the simplest one for >> feature support. >> >> v2: Clarifying the commit message (Anusha) >> >> v3: Re-factoring code as per review (Michal) >> >> v4: Rebase >> >> v5: Split from following patch >> >> v6: Re-factoring code (Michal, Sagar) >> Clarifying commit message (Sagar) >> >> v7: Generalizing subject to drm/i915 (Sagar) >> >> v8: Omitting DRRS seq_puts unification (Michal) >> >> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> b/drivers/gpu/drm/i915/i915_debugfs.c >> index c65e381..8edd029 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -1641,7 +1641,7 @@ static int i915_fbc_status(struct seq_file *m, >> void *unused) >> struct drm_i915_private *dev_priv = node_to_i915(m->private); >> if (!HAS_FBC(dev_priv)) { >> - seq_puts(m, "FBC unsupported on this chipset\n"); >> + seq_puts(m, "not supported\n"); >> return 0; >> } >> @@ -1809,7 +1809,7 @@ static int i915_ring_freq_table(struct seq_file >> *m, void *unused) >> unsigned int max_gpu_freq, min_gpu_freq; >> if (!HAS_LLC(dev_priv)) { >> - seq_puts(m, "unsupported on this chipset\n"); >> + seq_puts(m, "not supported\n"); >> return 0; >> } >> @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct >> seq_file *m, void *data) >> struct drm_i915_private *dev_priv = node_to_i915(m->private); >> struct drm_printer p; >> - if (!HAS_HUC_UCODE(dev_priv)) >> + if (!HAS_GUC(dev_priv)) { > > Hmm, I think that in above code we should use HAS_HUC defined as: > > /* HuC is inherent part of the GuC ... */ > #define HAS_HUC(dev_priv) HAS_GUC(dev_priv) > > to make it clear that code checks HuC sub-feature (not other part > of the GuC or GuC itself). And additionally we can use above define > to explicitly document relation between GuC and HuC. > > Michal > The suggested comment feels confusing to me. HuC is a different microcontroller and not a part of GuC. The only tie the 2 have is that GuC needs to do the authentication. It is however true that any platform that has a GuC also has a HuC so the suggested define itself is fine. Daniele >> + seq_puts(m, "not supported\n"); >> return 0; >> + } >> p = drm_seq_file_printer(m); >> intel_uc_fw_dump(&dev_priv->huc.fw, &p); >> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct >> seq_file *m, void *data) >> struct drm_printer p; >> u32 tmp, i; >> - if (!HAS_GUC_UCODE(dev_priv)) >> + if (!HAS_GUC(dev_priv)) { >> + seq_puts(m, "not supported\n"); >> return 0; >> + } >> p = drm_seq_file_printer(m); >> intel_uc_fw_dump(&dev_priv->guc.fw, &p); >> @@ -2650,7 +2654,7 @@ static int i915_edp_psr_status(struct seq_file >> *m, void *data) >> bool enabled = false; >> if (!HAS_PSR(dev_priv)) { >> - seq_puts(m, "PSR not supported\n"); >> + seq_puts(m, "not supported\n"); >> return 0; >> } > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 10/26/2017 11:24 PM, Daniele Ceraolo Spurio wrote: > > > On 25/10/17 06:31, Michal Wajdeczko wrote: >> On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan >> <sujaritha.sundaresan@intel.com> wrote: >> >>> Unifying the various seq_puts messages in debugfs to the simplest >>> one for >>> feature support. As Michal noted in the v7 review, if the goal is to unification of consistent output then I see some more in the debugfs that might need to be updated: *_wm_latency_open, i915_ipc_status, i915_runtime_pm_status(returning early could be done later) i915_llc (add change to return early). Also, I think this patch can be separated from this series as it has very little dependency. >>> >>> v2: Clarifying the commit message (Anusha) >>> >>> v3: Re-factoring code as per review (Michal) >>> >>> v4: Rebase >>> >>> v5: Split from following patch >>> >>> v6: Re-factoring code (Michal, Sagar) >>> Clarifying commit message (Sagar) >>> >>> v7: Generalizing subject to drm/i915 (Sagar) >>> >>> v8: Omitting DRRS seq_puts unification (Michal) >>> >>> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++----- >>> 1 file changed, 9 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>> b/drivers/gpu/drm/i915/i915_debugfs.c >>> index c65e381..8edd029 100644 >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>> @@ -1641,7 +1641,7 @@ static int i915_fbc_status(struct seq_file *m, >>> void *unused) >>> struct drm_i915_private *dev_priv = node_to_i915(m->private); >>> if (!HAS_FBC(dev_priv)) { >>> - seq_puts(m, "FBC unsupported on this chipset\n"); >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> } >>> @@ -1809,7 +1809,7 @@ static int i915_ring_freq_table(struct >>> seq_file *m, void *unused) >>> unsigned int max_gpu_freq, min_gpu_freq; >>> if (!HAS_LLC(dev_priv)) { >>> - seq_puts(m, "unsupported on this chipset\n"); >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> } >>> @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct >>> seq_file *m, void *data) >>> struct drm_i915_private *dev_priv = node_to_i915(m->private); >>> struct drm_printer p; >>> - if (!HAS_HUC_UCODE(dev_priv)) >>> + if (!HAS_GUC(dev_priv)) { >> >> Hmm, I think that in above code we should use HAS_HUC defined as: >> >> /* HuC is inherent part of the GuC ... */ >> #define HAS_HUC(dev_priv) HAS_GUC(dev_priv) >> >> to make it clear that code checks HuC sub-feature (not other part >> of the GuC or GuC itself). And additionally we can use above define >> to explicitly document relation between GuC and HuC. >> >> Michal >> > > The suggested comment feels confusing to me. HuC is a different > microcontroller and not a part of GuC. The only tie the 2 have is that > GuC needs to do the authentication. It is however true that any > platform that has a GuC also has a HuC so the suggested define itself > is fine. > > Daniele > >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> + } >>> p = drm_seq_file_printer(m); >>> intel_uc_fw_dump(&dev_priv->huc.fw, &p); >>> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct >>> seq_file *m, void *data) >>> struct drm_printer p; >>> u32 tmp, i; >>> - if (!HAS_GUC_UCODE(dev_priv)) >>> + if (!HAS_GUC(dev_priv)) { >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> + } >>> p = drm_seq_file_printer(m); >>> intel_uc_fw_dump(&dev_priv->guc.fw, &p); >>> @@ -2650,7 +2654,7 @@ static int i915_edp_psr_status(struct seq_file >>> *m, void *data) >>> bool enabled = false; >>> if (!HAS_PSR(dev_priv)) { >>> - seq_puts(m, "PSR not supported\n"); >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> } >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 10/29/2017 09:49 PM, Sagar Arun Kamble wrote: > > > On 10/26/2017 11:24 PM, Daniele Ceraolo Spurio wrote: >> >> >> On 25/10/17 06:31, Michal Wajdeczko wrote: >>> On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan >>> <sujaritha.sundaresan@intel.com> wrote: >>> >>>> Unifying the various seq_puts messages in debugfs to the simplest >>>> one for >>>> feature support. > As Michal noted in the v7 review, if the goal is to unification of > consistent output then I see some more in the debugfs that > might need to be updated: *_wm_latency_open, i915_ipc_status, > i915_runtime_pm_status(returning early could be done later) > i915_llc (add change to return early). > Also, I think this patch can be separated from this series as it has > very little dependency. Sure, I will include the other updates to the patch. But I feel that it might be better to keep the patch with series, since I'm including the change to HAS_GUC here. >>>> >>>> v2: Clarifying the commit message (Anusha) >>>> >>>> v3: Re-factoring code as per review (Michal) >>>> >>>> v4: Rebase >>>> >>>> v5: Split from following patch >>>> >>>> v6: Re-factoring code (Michal, Sagar) >>>> Clarifying commit message (Sagar) >>>> >>>> v7: Generalizing subject to drm/i915 (Sagar) >>>> >>>> v8: Omitting DRRS seq_puts unification (Michal) >>>> >>>> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++----- >>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>>> b/drivers/gpu/drm/i915/i915_debugfs.c >>>> index c65e381..8edd029 100644 >>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>>> @@ -1641,7 +1641,7 @@ static int i915_fbc_status(struct seq_file >>>> *m, void *unused) >>>> struct drm_i915_private *dev_priv = node_to_i915(m->private); >>>> if (!HAS_FBC(dev_priv)) { >>>> - seq_puts(m, "FBC unsupported on this chipset\n"); >>>> + seq_puts(m, "not supported\n"); >>>> return 0; >>>> } >>>> @@ -1809,7 +1809,7 @@ static int i915_ring_freq_table(struct >>>> seq_file *m, void *unused) >>>> unsigned int max_gpu_freq, min_gpu_freq; >>>> if (!HAS_LLC(dev_priv)) { >>>> - seq_puts(m, "unsupported on this chipset\n"); >>>> + seq_puts(m, "not supported\n"); >>>> return 0; >>>> } >>>> @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct >>>> seq_file *m, void *data) >>>> struct drm_i915_private *dev_priv = node_to_i915(m->private); >>>> struct drm_printer p; >>>> - if (!HAS_HUC_UCODE(dev_priv)) >>>> + if (!HAS_GUC(dev_priv)) { >>> >>> Hmm, I think that in above code we should use HAS_HUC defined as: >>> >>> /* HuC is inherent part of the GuC ... */ >>> #define HAS_HUC(dev_priv) HAS_GUC(dev_priv) >>> >>> to make it clear that code checks HuC sub-feature (not other part >>> of the GuC or GuC itself). And additionally we can use above define >>> to explicitly document relation between GuC and HuC. >>> >>> Michal >>> >> >> The suggested comment feels confusing to me. HuC is a different >> microcontroller and not a part of GuC. The only tie the 2 have is >> that GuC needs to do the authentication. It is however true that any >> platform that has a GuC also has a HuC so the suggested define itself >> is fine. >> >> Daniele >> >>>> + seq_puts(m, "not supported\n"); >>>> return 0; >>>> + } >>>> p = drm_seq_file_printer(m); >>>> intel_uc_fw_dump(&dev_priv->huc.fw, &p); >>>> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct >>>> seq_file *m, void *data) >>>> struct drm_printer p; >>>> u32 tmp, i; >>>> - if (!HAS_GUC_UCODE(dev_priv)) >>>> + if (!HAS_GUC(dev_priv)) { >>>> + seq_puts(m, "not supported\n"); >>>> return 0; >>>> + } >>>> p = drm_seq_file_printer(m); >>>> intel_uc_fw_dump(&dev_priv->guc.fw, &p); >>>> @@ -2650,7 +2654,7 @@ static int i915_edp_psr_status(struct >>>> seq_file *m, void *data) >>>> bool enabled = false; >>>> if (!HAS_PSR(dev_priv)) { >>>> - seq_puts(m, "PSR not supported\n"); >>>> + seq_puts(m, "not supported\n"); >>>> return 0; >>>> } >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > Thanks for the review, Regards, Sujaritha
On 10/25/2017 06:31 AM, Michal Wajdeczko wrote: > On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan > <sujaritha.sundaresan@intel.com> wrote: > >> Unifying the various seq_puts messages in debugfs to the simplest one >> for >> feature support. >> >> v2: Clarifying the commit message (Anusha) >> >> v3: Re-factoring code as per review (Michal) >> >> v4: Rebase >> >> v5: Split from following patch >> >> v6: Re-factoring code (Michal, Sagar) >> Clarifying commit message (Sagar) >> >> v7: Generalizing subject to drm/i915 (Sagar) >> >> v8: Omitting DRRS seq_puts unification (Michal) >> >> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> b/drivers/gpu/drm/i915/i915_debugfs.c >> index c65e381..8edd029 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -1641,7 +1641,7 @@ static int i915_fbc_status(struct seq_file *m, >> void *unused) >> struct drm_i915_private *dev_priv = node_to_i915(m->private); >> if (!HAS_FBC(dev_priv)) { >> - seq_puts(m, "FBC unsupported on this chipset\n"); >> + seq_puts(m, "not supported\n"); >> return 0; >> } >> @@ -1809,7 +1809,7 @@ static int i915_ring_freq_table(struct seq_file >> *m, void *unused) >> unsigned int max_gpu_freq, min_gpu_freq; >> if (!HAS_LLC(dev_priv)) { >> - seq_puts(m, "unsupported on this chipset\n"); >> + seq_puts(m, "not supported\n"); >> return 0; >> } >> @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct >> seq_file *m, void *data) >> struct drm_i915_private *dev_priv = node_to_i915(m->private); >> struct drm_printer p; >> - if (!HAS_HUC_UCODE(dev_priv)) >> + if (!HAS_GUC(dev_priv)) { > > Hmm, I think that in above code we should use HAS_HUC defined as: > > /* HuC is inherent part of the GuC ... */ > #define HAS_HUC(dev_priv) HAS_GUC(dev_priv) > > to make it clear that code checks HuC sub-feature (not other part > of the GuC or GuC itself). And additionally we can use above define > to explicitly document relation between GuC and HuC. > > Michal Sure, I will include the HAS_HUC condition and a comment that clarifies the HuC and GuC tie in the next revision. > >> + seq_puts(m, "not supported\n"); >> return 0; >> + } >> p = drm_seq_file_printer(m); >> intel_uc_fw_dump(&dev_priv->huc.fw, &p); >> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct >> seq_file *m, void *data) >> struct drm_printer p; >> u32 tmp, i; >> - if (!HAS_GUC_UCODE(dev_priv)) >> + if (!HAS_GUC(dev_priv)) { >> + seq_puts(m, "not supported\n"); >> return 0; >> + } >> p = drm_seq_file_printer(m); >> intel_uc_fw_dump(&dev_priv->guc.fw, &p); >> @@ -2650,7 +2654,7 @@ static int i915_edp_psr_status(struct seq_file >> *m, void *data) >> bool enabled = false; >> if (!HAS_PSR(dev_priv)) { >> - seq_puts(m, "PSR not supported\n"); >> + seq_puts(m, "not supported\n"); >> return 0; >> } Thanks for the review. Regards, Sujaritha
On 10/26/2017 10:54 AM, Daniele Ceraolo Spurio wrote: > > > On 25/10/17 06:31, Michal Wajdeczko wrote: >> On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan >> <sujaritha.sundaresan@intel.com> wrote: >> >>> Unifying the various seq_puts messages in debugfs to the simplest >>> one for >>> feature support. >>> >>> v2: Clarifying the commit message (Anusha) >>> >>> v3: Re-factoring code as per review (Michal) >>> >>> v4: Rebase >>> >>> v5: Split from following patch >>> >>> v6: Re-factoring code (Michal, Sagar) >>> Clarifying commit message (Sagar) >>> >>> v7: Generalizing subject to drm/i915 (Sagar) >>> >>> v8: Omitting DRRS seq_puts unification (Michal) >>> >>> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++----- >>> 1 file changed, 9 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>> b/drivers/gpu/drm/i915/i915_debugfs.c >>> index c65e381..8edd029 100644 >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>> @@ -1641,7 +1641,7 @@ static int i915_fbc_status(struct seq_file *m, >>> void *unused) >>> struct drm_i915_private *dev_priv = node_to_i915(m->private); >>> if (!HAS_FBC(dev_priv)) { >>> - seq_puts(m, "FBC unsupported on this chipset\n"); >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> } >>> @@ -1809,7 +1809,7 @@ static int i915_ring_freq_table(struct >>> seq_file *m, void *unused) >>> unsigned int max_gpu_freq, min_gpu_freq; >>> if (!HAS_LLC(dev_priv)) { >>> - seq_puts(m, "unsupported on this chipset\n"); >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> } >>> @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct >>> seq_file *m, void *data) >>> struct drm_i915_private *dev_priv = node_to_i915(m->private); >>> struct drm_printer p; >>> - if (!HAS_HUC_UCODE(dev_priv)) >>> + if (!HAS_GUC(dev_priv)) { >> >> Hmm, I think that in above code we should use HAS_HUC defined as: >> >> /* HuC is inherent part of the GuC ... */ >> #define HAS_HUC(dev_priv) HAS_GUC(dev_priv) >> >> to make it clear that code checks HuC sub-feature (not other part >> of the GuC or GuC itself). And additionally we can use above define >> to explicitly document relation between GuC and HuC. >> >> Michal >> > > The suggested comment feels confusing to me. HuC is a different > microcontroller and not a part of GuC. The only tie the 2 have is that > GuC needs to do the authentication. It is however true that any > platform that has a GuC also has a HuC so the suggested define itself > is fine. > > Daniele > I agree. I will include the condition and a comment that clearly mentions the GuC and HuC tie. >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> + } >>> p = drm_seq_file_printer(m); >>> intel_uc_fw_dump(&dev_priv->huc.fw, &p); >>> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct >>> seq_file *m, void *data) >>> struct drm_printer p; >>> u32 tmp, i; >>> - if (!HAS_GUC_UCODE(dev_priv)) >>> + if (!HAS_GUC(dev_priv)) { >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> + } >>> p = drm_seq_file_printer(m); >>> intel_uc_fw_dump(&dev_priv->guc.fw, &p); >>> @@ -2650,7 +2654,7 @@ static int i915_edp_psr_status(struct seq_file >>> *m, void *data) >>> bool enabled = false; >>> if (!HAS_PSR(dev_priv)) { >>> - seq_puts(m, "PSR not supported\n"); >>> + seq_puts(m, "not supported\n"); >>> return 0; >>> } >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx Thanks for the review. Regards, Sujaritha
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index c65e381..8edd029 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1641,7 +1641,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused) struct drm_i915_private *dev_priv = node_to_i915(m->private); if (!HAS_FBC(dev_priv)) { - seq_puts(m, "FBC unsupported on this chipset\n"); + seq_puts(m, "not supported\n"); return 0; } @@ -1809,7 +1809,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused) unsigned int max_gpu_freq, min_gpu_freq; if (!HAS_LLC(dev_priv)) { - seq_puts(m, "unsupported on this chipset\n"); + seq_puts(m, "not supported\n"); return 0; } @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data) struct drm_i915_private *dev_priv = node_to_i915(m->private); struct drm_printer p; - if (!HAS_HUC_UCODE(dev_priv)) + if (!HAS_GUC(dev_priv)) { + seq_puts(m, "not supported\n"); return 0; + } p = drm_seq_file_printer(m); intel_uc_fw_dump(&dev_priv->huc.fw, &p); @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data) struct drm_printer p; u32 tmp, i; - if (!HAS_GUC_UCODE(dev_priv)) + if (!HAS_GUC(dev_priv)) { + seq_puts(m, "not supported\n"); return 0; + } p = drm_seq_file_printer(m); intel_uc_fw_dump(&dev_priv->guc.fw, &p); @@ -2650,7 +2654,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) bool enabled = false; if (!HAS_PSR(dev_priv)) { - seq_puts(m, "PSR not supported\n"); + seq_puts(m, "not supported\n"); return 0; }
Unifying the various seq_puts messages in debugfs to the simplest one for feature support. v2: Clarifying the commit message (Anusha) v3: Re-factoring code as per review (Michal) v4: Rebase v5: Split from following patch v6: Re-factoring code (Michal, Sagar) Clarifying commit message (Sagar) v7: Generalizing subject to drm/i915 (Sagar) v8: Omitting DRRS seq_puts unification (Michal) Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)