On 10/11/2022 10:35, Michal Wajdeczko wrote:
On 10.11.2022 10:55, Tvrtko Ursulin wrote:
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));
Okay lets go with this then since we have allowed some time for comments
and no strict objections have been raised. Probably limit to to
GT/GuC/CT space for not, ie. not adding i915_ loggers.
My preference is just to have a colon in the GT identifier, lowercase or
uppercase I don't mind.
Regards,
Tvrtko
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.
If there is a choice between having ugly code all over the place and few
more lines with helpers then without any doubts I would pick the latter.
And this seems to be option already used elsewhere, see:
#define dev_err(dev, fmt, ...) \
dev_printk_index_wrap ...
#define pci_err(pdev, fmt, arg...) \
dev_err(&(pdev)->dev, fmt, ##arg)
#define drm_err(drm, fmt, ...) \
__drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
#define drbd_err(obj, fmt, args...) \
drbd_printk(KERN_ERR, obj, fmt, ## args)
#define ch7006_err(client, format, ...) \
dev_err(&client->dev, format, __VA_ARGS__)
#define mthca_err(mdev, format, arg...) \
dev_err(&mdev->pdev->dev, format, ## arg)
#define ctx_err(ctx, fmt, arg...) \
cal_err((ctx)->cal, "ctx%u: " fmt, (ctx)->dma_ctx, ##arg)
#define mlx4_err(mdev, format, ...) \
dev_err(&(mdev)->persist->pdev->dev, format, ##__VA_ARGS__)
...
Michal
[1]
https://elixir.bootlin.com/linux/v6.1-rc4/source/include/linux/dev_printk.h#L143
[2]
https://elixir.bootlin.com/linux/v6.1-rc4/source/include/linux/pci.h#L2485
[3]
https://elixir.bootlin.com/linux/v6.1-rc4/source/include/drm/drm_print.h#L468
[4]
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/block/drbd/drbd_int.h#L113
[5]
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/gpu/drm/i2c/ch7006_priv.h#L139
[6]
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/infiniband/hw/mthca/mthca_dev.h#L377
[7]
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/media/platform/ti/cal/cal.h#L279
[8]
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/net/ethernet/mellanox/mlx4/mlx4.h#L225
Perhaps macro could be called something other than XX_LOG to make it
more readable, don't know.
Regards,
Tvrtko
