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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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