[Intel-gfx] [RFC v2 0/6] DRM logging tidy

[Tvrtko Ursulin]

On 25/01/2018 11:32, Jani Nikula wrote:
> On Wed, 24 Jan 2018, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
>> On 24/01/2018 16:23, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-01-24 16:18:15)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> This series tries to solve a few issues in the current DRM logging code to
>>>> primarily make it clearer which messages belong to which driver.
>>>>
>>>> Main problem is that currently some logging functions allow individual drivers
>>>> to override the log prefix (since they are defined as macros, or static
>>>> inlines), while other hardcode the "drm" prefix into them due being situated in
>>>> the DRM core modules.
>>>>
>>>> Another thing is that I noticed the DRM_NAME macro which is used for this is
>>>> defined in the uAPI header and had a comment which looked outdated.
>>>>
>>>> Therefore I introduce a new define, called, DRM_LOG_NAME, this time defined
>>>> internally in the kernel headers and not exported in the uAPI.
>>>>
>>>> I also refactored some logging functions to take this string as a parameter
>>>> instead of hardcoding it.
>>>>
>>>> Individual drivers can then override this define to make DRM logging functions
>>>> prefix their message with the respective driver prefix.
>>>>
>>>> End result in the case of the i915 driver looks like this:
>>>>
>>>> Old log:
>>>>
>>>>    [drm] Found 128MB of eDRAM
>>>>    [drm:skl_enable_dc6 [i915]] Enabling DC6
>>>>
>>>> New log:
>>>>
>>>>    [i915] Found 128MB of eDRAM
>>>>    [i915:skl_enable_dc6 [i915]] Enabling DC6
>>>
>>> And still not conforming to the standard logging string. DRM_LOG should
>>
>> What is the standard logging string? the dev_ one?
>>
>>> be killed off as an anachronistic OS compat layer.
>>
>> You mean only dev variants should be kept?
> 
> I think the DRM_LOG_NAME override mechanism is too fragile, as it
> depends on #include ordering. For our driver, I think it basically means
> always including one of our headers (i915_drv.h) first everywhere (to
> have a single point of truth for DRM_LOG_NAME), and including
> drm_print.h first from there. That's not currently true, and I don't
> want to see a massive #include reordering patchset to make it so.
> 
> This is like pr_fmt() which I think has been a mistake and should not be
> repeated.
> 
> I think the direction to go is using dev_printk, dev_dbg, dev_err,
> dev_warn, and friends, which use dev_driver_string internally. We
> already have some drm wrappers for those. The problem with them is
> passing dev, and I think that's the problem we should think about.
> 
> We also seem to have opted to use drm_dev_printk (which calls dev_printk
> or printk) for DRM_DEV_DEBUG and friends. This is arguably a bad choice,
> because using dev_dbg would let us make use of dynamic debug.

I agree the current state is messy and that a better improvement could 
be made. And that the macro string approach is ugly.

To my defence, it sounded like a smaller amount of work to make it at 
least a little bit better. The way I implemented it, as long as the 
define is before any include directives it will work as expected. And on 
my T460p which has both nouveau and i915 it makes the kernel log a bit 
less confusing. But yeah, it is a marginal user base.

Scratch this then, previous posting was only two years ago so it can 
wait some more for a more thorough etc rework.

Regards,

Tvrtko