Re: [RFC v2 0/6] DRM logging tidy

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 24 Jan 2018, Tvrtko Ursulin <tvrtko.ursulin@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@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.

BR,
Jani.



>
> Regards,
>
> Tvrtko
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx