Re: [PATCH 1/2] drm/i915/gt: Add GT oriented dmesg output

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

 




On 09/11/2022 19:57, Michal Wajdeczko wrote:

[snip]

Is it really a problem to merge this patch now to get the process
started? And other sub-components get updated as and when people get the
time to do them? You could maybe even help rather than posting
completely conflicting patch sets that basically duplicate all the
effort for no actual benefit.

Instead of merging this patch now, oriented on GT only, I would rather
wait until we discuss and plan solution for the all sub-components.

Yes, agreed.

Once that's done (with agreement on naming and output) we can start
converting exiting messages.

My proposal would be:
  - use wrappers per component

This is passable to me but Jani has raised a concern on IRC that it leads to a lot of macro duplication. Which is I think a valid point, but which does not have a completely nice solution. Best I heard so far was a suggestion from Joonas to add just a single component formatter macro and use the existing drm_xxx helpers.

  - use lower case names

I prefer this as well. Even though usual argument is for macros to be upper case, I find the improved readability of lower case trumps that.

  - don't add colon

Not sure, when I look at it below it looks a bit not structured enough without the colon, but maybe it is just me.

#define i915_xxx(_i915, _fmt, ...) \
	drm_xxx(&(_i915)->drm, _fmt, ##__VA_ARGS__)

#define gt_xxx(_gt, _fmt, ...) \
	i915_xxx((_gt)->i915, "GT%u " _fmt, (_gt)->info.id, ..

#define guc_xxx(_guc, _fmt, ...) \
	gt_xxx(guc_to_gt(_guc), "GuC " _fmt, ..

#define ct_xxx(_ct, _fmt, ...) \
	guc_xxx(ct_to_guc(_ct), "CTB " _fmt, ..

where
	xxx = { err, warn, notice, info, dbg }

and then for calls like:

	i915_err(i915, "Foo failed (%pe)\n", ERR_PTR(err));
	  gt_err(gt,   "Foo failed (%pe)\n", ERR_PTR(err));
	 guc_err(guc,  "Foo failed (%pe)\n", ERR_PTR(err));
	  ct_err(ct,   "Foo failed (%pe)\n", ERR_PTR(err));

So the macro idea would be like this:

  drm_err(I915_LOG("Foo failed (%pe)\n", i915), ERR_PTR(err));
  drm_err(GT_LOG("Foo failed (%pe)\n", gt), ERR_PTR(err));
  drm_err(GUC_LOG("Foo failed (%pe)\n", guc), ERR_PTR(err));
  drm_err(CT_LOG("Foo failed (%pe)\n", ct), ERR_PTR(err));

Each component would just need to define a single macro and not have to duplicate all the err, info, warn, notice, ratelimited, once, whatever versions. Which is a benefit but it's a quite a bit uglier to read in the code.

Perhaps macro could be called something other than XX_LOG to make it more readable, don't know.

Regards,

Tvrtko



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux