Re: [Intel-gfx] [PATCH v3] drm/i915: Partial abandonment of legacy DRM logging macros

Thu, 10 Nov 2022 03:09:26 -0800 

On 09.11.2022 11:46, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin <tvrtko.ursu...@intel.com>

Convert some usages of legacy DRM logging macros into versions which tell
us on which device have the events occurred.

v2:
  * Don't have struct drm_device as local. (Jani, Ville)

v3:
  * Store gt, not i915, in workaround list. (John)



Neither gt neither i915 does fit into wa list IMHO.
The best solution would be provide context (i915/gt/whatever)
as a function parameter, every time it is necessary.
On the other side it should not block the patch.
More below.



Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
Reviewed-by: Andrzej Hajda <andrzej.ha...@intel.com> # v2
Acked-by: Jani Nikula <jani.nik...@intel.com>
Cc: Jani Nikula <jani.nik...@intel.com>
Cc: John Harrison <john.c.harri...@intel.com>
Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
---
  drivers/gpu/drm/i915/gem/i915_gem_context.c   |  2 +-
  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 26 ++++++++----
  .../drm/i915/gt/intel_execlists_submission.c  | 13 +++---
  drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c  |  4 +-
  drivers/gpu/drm/i915/gt/intel_gt.c            |  4 +-
  drivers/gpu/drm/i915/gt/intel_gt_irq.c        |  8 ++--
  drivers/gpu/drm/i915/gt/intel_rps.c           |  6 ++-
  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 42 +++++++++++--------
  .../gpu/drm/i915/gt/intel_workarounds_types.h |  3 ++
  .../gpu/drm/i915/gt/selftest_workarounds.c    |  4 +-
  drivers/gpu/drm/i915/i915_debugfs.c           |  4 +-
  drivers/gpu/drm/i915/i915_gem.c               |  2 +-
  drivers/gpu/drm/i915/i915_getparam.c          |  2 +-
  drivers/gpu/drm/i915/i915_irq.c               | 12 +++---
  drivers/gpu/drm/i915/i915_perf.c              | 14 ++++---
  drivers/gpu/drm/i915/i915_query.c             | 12 +++---
  drivers/gpu/drm/i915/i915_sysfs.c             |  3 +-
  drivers/gpu/drm/i915/i915_vma.c               | 16 +++----
  drivers/gpu/drm/i915/intel_uncore.c           | 21 ++++++----
  19 files changed, 117 insertions(+), 81 deletions(-)



(...)


@@ -1749,7 +1755,7 @@ wa_list_apply(struct intel_gt *gt, const struct 
i915_wa_list *wal)
                                intel_gt_mcr_read_any_fw(gt, wa->mcr_reg) :
                                intel_uncore_read_fw(uncore, wa->reg);
- wa_verify(wa, val, wal->name, "application"); 
+                       wa_verify(wal->gt, wa, val, wal->name, "application");
This looks confusing at 1st sight, why wa_verify(wal->gt,...) and not wa_verify(gt,...). Can they differ? and similar questions as in case of redundant vars. 
The same apply to wal->engine_name, which is almost unused anyway?
Also AFAIK there is always sequence:
1. wa_init_start
2. *init_workarounds*
3. wa_init_finish - btw funny name.
Why not 1 and 3 embed in 2? Do we need this sequence.
Anyway all these comments are for wa handling, which should be addressed in other patch. So my r-b still holds, either with wal->i915, either with wal->gt. 

Reviewed-by: Andrzej Hajda <andrzej.ha...@intel.com>

Regards
Andrzej






                }
        }
@@ -1779,7 +1785,7 @@ static bool wa_list_verify(struct intel_gt *gt, 
        intel_uncore_forcewake_get__locked(uncore, fw);
for (i = 0, wa = wal->list; i < wal->count; i++, wa++) 
-               ok &= wa_verify(wa, wa->is_mcr ?
+               ok &= wa_verify(wal->gt, wa, wa->is_mcr ?
                                intel_gt_mcr_read_any_fw(gt, wa->mcr_reg) :
                                intel_uncore_read_fw(uncore, wa->reg),
                                wal->name, from);
@@ -2127,7 +2133,7 @@ void intel_engine_init_whitelist(struct intel_engine_cs 
*engine)
        struct drm_i915_private *i915 = engine->i915;
        struct i915_wa_list *w = &engine->whitelist;
- wa_init_start(w, "whitelist", engine->name); 
+       wa_init_start(w, engine->gt, "whitelist", engine->name);
if (IS_PONTEVECCHIO(i915)) 
                pvc_whitelist_build(engine);
@@ -3012,7 +3018,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs 
*engine)
        if (GRAPHICS_VER(engine->i915) < 4)
                return;
- wa_init_start(wal, "engine", engine->name); 
+       wa_init_start(wal, engine->gt, "engine", engine->name);
        engine_init_workarounds(engine, wal);
        wa_init_finish(wal);
  }
@@ -3193,7 +3199,7 @@ static int engine_wa_list_verify(struct intel_context *ce,
                if (mcr_range(rq->engine->i915, i915_mmio_reg_offset(wa->reg)))
                        continue;
- if (!wa_verify(wa, results[i], wal->name, from)) 
+               if (!wa_verify(wal->gt, wa, results[i], wal->name, from))
                        err = -ENXIO;
        }
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds_types.h b/drivers/gpu/drm/i915/gt/intel_workarounds_types.h 
index 7c8b01d00043..e14188120e66 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds_types.h
@@ -10,6 +10,8 @@
#include "i915_reg_defs.h" +struct intel_gt; 
+
  struct i915_wa {
        union {
                i915_reg_t      reg;
@@ -24,6 +26,7 @@ struct i915_wa {
  };
struct i915_wa_list { 
+       struct intel_gt *gt;
        const char      *name;
        const char      *engine_name;
        struct i915_wa  *list;
diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c 
b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index 21b1edc052f8..711014bb53d9 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -66,14 +66,14 @@ reference_lists_init(struct intel_gt *gt, struct wa_lists 
*lists)
memset(lists, 0, sizeof(*lists)); - wa_init_start(&lists->gt_wa_list, "GT_REF", "global"); 
+       wa_init_start(&lists->gt_wa_list, gt, "GT_REF", "global");
        gt_init_workarounds(gt, &lists->gt_wa_list);
        wa_init_finish(&lists->gt_wa_list);
for_each_engine(engine, gt, id) { 
                struct i915_wa_list *wal = &lists->engine[id].wa_list;
- wa_init_start(wal, "REF", engine->name); 
+               wa_init_start(wal, gt, "REF", engine->name);
                engine_init_workarounds(engine, wal);
                wa_init_finish(wal);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c 
index ae987e92251d..6c7ac73b69a5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -688,8 +688,8 @@ i915_drop_caches_set(void *data, u64 val)
        unsigned int flags;
        int ret;
- DRM_DEBUG("Dropping caches: 0x%08llx [0x%08llx]\n", 
-                 val, val & DROP_ALL);
+       drm_dbg(&i915->drm, "Dropping caches: 0x%08llx [0x%08llx]\n",
+               val, val & DROP_ALL);
ret = gt_drop_caches(to_gt(i915), val); 
        if (ret)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 299f94a9fb87..8132743ca87e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1286,7 +1286,7 @@ int i915_gem_open(struct drm_i915_private *i915, struct 
drm_file *file)
        struct i915_drm_client *client;
        int ret = -ENOMEM;
- DRM_DEBUG("\n"); 
+       drm_dbg(&i915->drm, "\n");
file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL); 
        if (!file_priv)
diff --git a/drivers/gpu/drm/i915/i915_getparam.c 
b/drivers/gpu/drm/i915/i915_getparam.c
index 3047e80e1163..61ef2d9cfa62 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -179,7 +179,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
                value = i915_perf_oa_timestamp_frequency(i915);
                break;
        default:
-               DRM_DEBUG("Unknown parameter %d\n", param->param);
+               drm_dbg(&i915->drm, "Unknown parameter %d\n", param->param);
                return -EINVAL;
        }
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c 
index b0180ea38de0..6c20817f8967 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1086,8 +1086,9 @@ static void ivb_parity_work(struct work_struct *work)
                kobject_uevent_env(&dev_priv->drm.primary->kdev->kobj,
                                   KOBJ_CHANGE, parity_event);
- DRM_DEBUG("Parity error: Slice = %d, Row = %d, Bank = %d, Sub bank = %d.\n", 
-                         slice, row, bank, subbank);
+               drm_dbg(&dev_priv->drm,
+                       "Parity error: Slice = %d, Row = %d, Bank = %d, Sub bank = 
%d.\n",
+                       slice, row, bank, subbank);
kfree(parity_event[4]); 
                kfree(parity_event[3]);
@@ -2774,7 +2775,8 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
                master_ctl = raw_reg_read(regs, GEN11_GFX_MSTR_IRQ);
                raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, master_ctl);
        } else {
-               DRM_ERROR("Tile not supported: 0x%08x\n", master_tile_ctl);
+               drm_err(&i915->drm, "Tile not supported: 0x%08x\n",
+                       master_tile_ctl);
                dg1_master_intr_enable(regs);
                return IRQ_NONE;
        }
@@ -3940,7 +3942,7 @@ static void i8xx_error_irq_ack(struct drm_i915_private 
*i915,
  static void i8xx_error_irq_handler(struct drm_i915_private *dev_priv,
                                   u16 eir, u16 eir_stuck)
  {
-       DRM_DEBUG("Master Error: EIR 0x%04x\n", eir);
+       drm_dbg(&dev_priv->drm, "Master Error: EIR 0x%04x\n", eir);
if (eir_stuck) 
                drm_dbg(&dev_priv->drm, "EIR stuck: 0x%04x, masked\n",
@@ -3975,7 +3977,7 @@ static void i9xx_error_irq_ack(struct drm_i915_private 
*dev_priv,
  static void i9xx_error_irq_handler(struct drm_i915_private *dev_priv,
                                   u32 eir, u32 eir_stuck)
  {
-       DRM_DEBUG("Master Error, EIR 0x%08x\n", eir);
+       drm_dbg(&dev_priv->drm, "Master Error, EIR 0x%08x\n", eir);
if (eir_stuck) 
                drm_dbg(&dev_priv->drm, "EIR stuck: 0x%08x, masked\n",
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 0dd597a7a11f..9e6f060592d8 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -530,9 +530,9 @@ static bool oa_buffer_check_unlocked(struct 
i915_perf_stream *stream)
if (OA_TAKEN(hw_tail, tail) > report_size && 
                    __ratelimit(&stream->perf->tail_pointer_race))
-                       DRM_NOTE("unlanded report(s) head=0x%x "
-                                "tail=0x%x hw_tail=0x%x\n",
-                                head, tail, hw_tail);
+                       drm_notice(&stream->uncore->i915->drm,
+                                  "unlanded report(s) head=0x%x tail=0x%x 
hw_tail=0x%x\n",
+                                  head, tail, hw_tail);
stream->oa_buffer.tail = gtt_offset + tail; 
                stream->oa_buffer.aging_tail = gtt_offset + hw_tail;
@@ -1015,7 +1015,8 @@ static int gen7_append_oa_reports(struct i915_perf_stream 
*stream,
                 */
                if (report32[0] == 0) {
                        if (__ratelimit(&stream->perf->spurious_report_rs))
-                               DRM_NOTE("Skipping spurious, invalid OA 
report\n");
+                               drm_notice(&uncore->i915->drm,
+                                          "Skipping spurious, invalid OA 
report\n");
                        continue;
                }
@@ -1602,8 +1603,9 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream) 
        free_noa_wait(stream);
if (perf->spurious_report_rs.missed) { 
-               DRM_NOTE("%d spurious OA report notices suppressed due to 
ratelimiting\n",
-                        perf->spurious_report_rs.missed);
+               drm_notice(&gt->i915->drm,
+                          "%d spurious OA report notices suppressed due to 
ratelimiting\n",
+                          perf->spurious_report_rs.missed);
        }
  }
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c 
index 6ec9c9fb7b0d..00871ef99792 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -250,8 +250,9 @@ static int query_perf_config_data(struct drm_i915_private 
*i915,
                return total_size;
if (query_item->length < total_size) { 
-               DRM_DEBUG("Invalid query config data item size=%u 
expected=%u\n",
-                         query_item->length, total_size);
+               drm_dbg(&i915->drm,
+                       "Invalid query config data item size=%u expected=%u\n",
+                       query_item->length, total_size);
                return -EINVAL;
        }
@@ -418,9 +419,10 @@ static int query_perf_config_list(struct drm_i915_private *i915, 
        } while (n_configs > alloc);
if (query_item->length < sizeof_perf_config_list(n_configs)) { 
-               DRM_DEBUG("Invalid query config list item size=%u 
expected=%zu\n",
-                         query_item->length,
-                         sizeof_perf_config_list(n_configs));
+               drm_dbg(&i915->drm,
+                       "Invalid query config list item size=%u expected=%zu\n",
+                       query_item->length,
+                       sizeof_perf_config_list(n_configs));
                kfree(oa_config_ids);
                return -EINVAL;
        }
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
b/drivers/gpu/drm/i915/i915_sysfs.c
index 1e2750210831..595e8b574990 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -218,7 +218,8 @@ static const struct bin_attribute error_state_attr = {
  static void i915_setup_error_capture(struct device *kdev)
  {
        if (sysfs_create_bin_file(&kdev->kobj, &error_state_attr))
-               DRM_ERROR("error_state sysfs setup failed\n");
+               drm_err(&kdev_minor_to_i915(kdev)->drm,
+                       "error_state sysfs setup failed\n");
  }
static void i915_teardown_error_capture(struct device *kdev) 
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index c39488eb9eeb..3b969d679c1e 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -73,14 +73,16 @@ static void vma_print_allocator(struct i915_vma *vma, const 
char *reason)
        char buf[512];
if (!vma->node.stack) { 
-               DRM_DEBUG_DRIVER("vma.node [%08llx + %08llx] %s: unknown 
owner\n",
-                                vma->node.start, vma->node.size, reason);
+               drm_dbg(&to_i915(vma->obj->base.dev)->drm
+                       "vma.node [%08llx + %08llx] %s: unknown owner\n",
+                       vma->node.start, vma->node.size, reason);
                return;
        }
stack_depot_snprint(vma->node.stack, buf, sizeof(buf), 0); 
-       DRM_DEBUG_DRIVER("vma.node [%08llx + %08llx] %s: inserted at %s\n",
-                        vma->node.start, vma->node.size, reason, buf);
+       drm_dbg(&to_i915(vma->obj->base.dev)->drm,
+               "vma.node [%08llx + %08llx] %s: inserted at %s\n",
+               vma->node.start, vma->node.size, reason, buf);
  }
#else 
@@ -782,9 +784,9 @@ i915_vma_insert(struct i915_vma *vma, struct 
i915_gem_ww_ctx *ww,
         * attempt to find space.
         */
        if (size > end) {
-               DRM_DEBUG("Attempting to bind an object larger than the aperture: 
request=%llu > %s aperture=%llu\n",
-                         size, flags & PIN_MAPPABLE ? "mappable" : "total",
-                         end);
+               drm_dbg(&to_i915(vma->obj->base.dev)->drm,
+                       "Attempting to bind an object larger than the aperture: 
request=%llu > %s aperture=%llu\n",
+                       size, flags & PIN_MAPPABLE ? "mappable" : "total", end);
                return -ENOSPC;
        }
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c 
index 2a3e2869fe71..6c25c9e7090a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -178,8 +178,9 @@ static inline void
  fw_domain_wait_ack_clear(const struct intel_uncore_forcewake_domain *d)
  {
        if (wait_ack_clear(d, FORCEWAKE_KERNEL)) {
-               DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n",
-                         intel_uncore_forcewake_domain_to_str(d->id));
+               drm_err(&d->uncore->i915->drm,
+                       "%s: timed out waiting for forcewake ack to clear.\n",
+                       intel_uncore_forcewake_domain_to_str(d->id));
                add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI now 
unreliable */
        }
  }
@@ -226,11 +227,12 @@ fw_domain_wait_ack_with_fallback(const struct 
intel_uncore_forcewake_domain *d,
                fw_clear(d, FORCEWAKE_KERNEL_FALLBACK);
        } while (!ack_detected && pass++ < 10);
- DRM_DEBUG_DRIVER("%s had to use fallback to %s ack, 0x%x (passes %u)\n", 
-                        intel_uncore_forcewake_domain_to_str(d->id),
-                        type == ACK_SET ? "set" : "clear",
-                        fw_ack(d),
-                        pass);
+       drm_dbg(&d->uncore->i915->drm,
+               "%s had to use fallback to %s ack, 0x%x (passes %u)\n",
+               intel_uncore_forcewake_domain_to_str(d->id),
+               type == ACK_SET ? "set" : "clear",
+               fw_ack(d),
+               pass);
return ack_detected ? 0 : -ETIMEDOUT; 
  }
@@ -255,8 +257,9 @@ static inline void
  fw_domain_wait_ack_set(const struct intel_uncore_forcewake_domain *d)
  {
        if (wait_ack_set(d, FORCEWAKE_KERNEL)) {
-               DRM_ERROR("%s: timed out waiting for forcewake ack request.\n",
-                         intel_uncore_forcewake_domain_to_str(d->id));
+               drm_err(&d->uncore->i915->drm,
+                       "%s: timed out waiting for forcewake ack request.\n",
+                       intel_uncore_forcewake_domain_to_str(d->id));
                add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI now 
unreliable */
        }
  }

Reply via email to