Re: [PATCH v8 1/6] drm/i915 : Unifying seq_puts messages for feature support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxx> 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@xxxxxxxxx>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@xxxxxxxxx>
Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
---
 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Thanks for the review,

Regards,
Sujaritha
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux