On Sat, Jul 11, 2020 at 11:16:33AM -0700, Joe Perches wrote:
On Sat, 2020-07-11 at 20:41 +0530, Suraj Upadhyay wrote:
On Fri, Jul 10, 2020 at 07:56:43PM +0200, Sam Ravnborg wrote:
Hi Suraj.
On Tue, Jul 07, 2020 at 10:04:14PM +0530, Suraj Upadhyay wrote:
This patchset converts logging to drm_* functions in drm core.
The following functions have been converted to their respective DRM alternatives : dev_info() --> drm_info() dev_err() --> drm_err() dev_warn() --> drm_warn() dev_err_once() --> drm_err_once().
I would prefer that DRM_* logging in the same files are converted in the same patch. So we have one logging conversion patch for each file you touches and that we do not need to re-vist the files later to change another set of logging functions.
Agreed.
If possible WARN_* should also be converted to drm_WARN_* If patch is too large, then split them up but again lets have all logging updated when we touch a file.
Care to take a look at this approach?
Hii, The problem with WARN_* macros is that they are used without any drm device context. For example [this use here](https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_edid.c#n1...) in drm_edid.c, doesn't have a drm device context and only has one argument (namely !raw_edid). There are many more such use cases.
And also there were cases where dev_* logging functions didn't have any drm_device context.
Perhaps change the __drm_printk macro to not dereference the drm argument when NULL.
A trivial but perhaps inefficient way might be used like:
drm_<level>(NULL, fmt, ...)
include/drm/drm_print.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index 1c9417430d08..9323a8f46b3c 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -395,8 +395,8 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
/* Helper for struct drm_device based logging. */ #define __drm_printk(drm, level, type, fmt, ...) \
- dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
- dev_##level##type((drm) ? (drm)->dev : NULL, "[drm] " fmt, \
##__VA_ARGS__)#define drm_info(drm, fmt, ...) \ __drm_printk((drm), info,, fmt, ##__VA_ARGS__)
Hi Joe, Thanks for your input. But I don't think that this change would be a good idea as we are supposed to find or make a substitute of WARN_* macros which take a `condition` as an argument and check for its truth. And I guess passing a NULL to dev_<level> would cause a format warning.
Also, the WARN_* macros are doing their job fine, and passing a NULL value everytime you want to warn about a certain condition at a particular line, doesn't seem good to me.
Thus, I think that WARN_* macros should be untouched.
I would like to hear what the MAINTAINERS think.
Thanks and Cheers,
Suraj Upadhyay.