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
