On Fri, 01 Feb 2019, Sam Ravnborg sam@ravnborg.org wrote:
Hi Thierry.
I personally like the DRM_DEV_* variants better because of the additional information that they provide. That can be useful when grepping logs etc.
I'm slightly on the fence about this patch. The unwritten, and admittedly fuzzy, rules that I've been using so far are that dev_*() are used or messages that have to do with the panel device itself, whereas DRM_* variants are used for things that are actually related to DRM. So typically this would mean that roughly everything in ->probe() or ->remove() would be dev_*(), while the rest would be DRM_DEV_*().
For a rookie like me it is much simpler if one can use the same logging primitives all over or at least the rules when to use what is simple. It is simple to say that everything that exists below drivers/gpu/drm/ relates to drm.
Suggested set of rules to follow:
- If in drm core, use DRM_XXX where XXX represent the core functionality
- If in a driver use DRM_DEV* if a struct device is available
- If in a driver and no struct device, use plain DRM_ERROR/INFO
Core and drivers are already pretty conflated:
http://patchwork.freedesktop.org/patch/msgid/20181227162310.13023-1-jani.nik...
---
Side note, I'd like to switch i915 to dev based debugs, but I absolutely hate the idea of changing:
DRM_DEBUG_KMS("...")
to:
DRM_DEV_DEBUG_KMS(dev_priv->drm.dev, "...")
I think the dev based macros are way too long, and would serve *most* (though not all) drivers better by having struct drm_device * rather than struct device * as the first param. In the above, just the boilerplate consumes half the line.
Basically I'd like to see drm_ prefixed analogues to all the dev_ based logging functions, e.g. drm_dbg that takes drm_device. But it's so much churn that I'm contemplating just making i915 specific wrappers instead. :(
BR, Jani.
If there is a need to distingush before/after one has a drm_device, the best way would be to have a set of logging primitives that take a drm_device. So we could extend the rule set:
- If in a driver use DRM_DRM* if a struct drm_device is available (This rule would take precedence over a struct device)
DRM_DRM*, or DRM_DDEV* or ... But you get the idea.
But this is not where we are today.
Shall I redo the patch-set so we go back to dev_*() in probe() / remove()?
Sam _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel