[Intel-gfx] [PATCH 1/2] drm/i915/gt: Add GT oriented dmesg output

[Tvrtko Ursulin]

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