On Tue, Jul 07, 2020 at 09:58:48PM +0530, Suraj Upadhyay wrote:

Convert logging errors from dev_err() to drm_err().

Signed-off-by: Suraj Upadhyay usuraj35@gmail.com

Thanks. Applied to drm-misc-next as there was no more logging conversion to do in this file.

Sam

---

drivers/gpu/drm/drm_mipi_dsi.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 07102d8da58f..5dd475e82995 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -34,6 +34,7 @@ #include <linux/slab.h>

#include <drm/drm_dsc.h> +#include <drm/drm_print.h> #include <video/mipi_display.h>

/** @@ -155,19 +156,18 @@ static int mipi_dsi_device_add(struct mipi_dsi_device *dsi) static struct mipi_dsi_device * of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node) {

• struct device *dev = host->dev; struct mipi_dsi_device_info info = { }; int ret; u32 reg;

  if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {

• dev_err(dev, "modalias failure on %pOF\n", node);
  

• drm_err(host, "modalias failure on %pOF\n", node);
  

  return ERR_PTR(-EINVAL); }

  ret = of_property_read_u32(node, "reg", &reg); if (ret) {

• dev_err(dev, "device node %pOF has no valid reg property: %d\n",
  

• drm_err(host, "device node %pOF has no valid reg property: %d\n",
  node, ret);
  

  return ERR_PTR(-EINVAL); }

@@ -202,22 +202,21 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, const struct mipi_dsi_device_info *info) { struct mipi_dsi_device *dsi;

• struct device *dev = host->dev; int ret;

  if (!info) {

• dev_err(dev, "invalid mipi_dsi_device_info pointer\n");
  

• drm_err(host, "invalid mipi_dsi_device_info pointer\n");
  

  return ERR_PTR(-EINVAL); }

  if (info->channel > 3) {

• dev_err(dev, "invalid virtual channel: %u\n", info->channel);
  

• drm_err(host, "invalid virtual channel: %u\n", info->channel);
  

  return ERR_PTR(-EINVAL); }

  dsi = mipi_dsi_device_alloc(host); if (IS_ERR(dsi)) {

• dev_err(dev, "failed to allocate DSI device %ld\n",
  

• drm_err(host, "failed to allocate DSI device %ld\n",
  PTR_ERR(dsi));
  

  return dsi; }

@@ -228,7 +227,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,

ret = mipi_dsi_device_add(dsi); if (ret) {

• dev_err(dev, "failed to add DSI device %d\n", ret);
  

• drm_err(host, "failed to add DSI device %d\n", ret);
  

  kfree(dsi); return ERR_PTR(ret); }

-- 2.17.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Jul 07, 2020 at 10:07:21PM +0530, Suraj Upadhyay wrote:

Change logging information from dev_info() to drm_info().

Signed-off-by: Suraj Upadhyay usuraj35@gmail.com

Thanks. Applied to drm-misc-next.

Also, because there was no more logging functions that needed an update.

Sam

---

drivers/gpu/drm/drm_fb_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 5609e164805f..88146f7245c5 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1819,7 +1819,7 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper, if (ret < 0) return ret;

• dev_info(dev->dev, "fb%d: %s frame buffer device\n",

• drm_info(dev, "fb%d: %s frame buffer device\n", info->node, info->fix.id);

  mutex_lock(&kernel_fb_helper_lock);

-- 2.17.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

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.

On Sun, Jul 12, 2020 at 12:07:45PM -0700, Joe Perches wrote:

On Mon, 2020-07-13 at 00:24 +0530, Suraj Upadhyay wrote:

...

On Sat, Jul 11, 2020 at 11:16:33AM -0700, Joe Perches wrote:

[]

...

...

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, ...)

[]

...

...

diff --git 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.

So do I but the suggestion was not about WARN macros only about drm_<level> macros and possibly unnecessary conversions to dev_<level> when a drm_device context is unavailable.

Also, you don't have to guess, the code is there for you to inspect.

dev_<level> when a NULL is used as the first argument emits "(NULL device *)" instead of dev_driver_string(dev) and dev_name(dev).

See: drivers/base/core.c::__dev_printk()

Yes, Thanks my bad. But the dev_<level> usages in drm/* always have a context and doesn't need NULL to be passed, i.e. some of them have only a `struct device` context which cannot be simply converted into drm_<level> since they take a struct pointer with a `dev` member and not a `dev` itself. What we can convert is calls to dev_<level> with a drm_device context or with a struct pointer which has a dev member.

And, I really want the MAINTAINERS to look into it.

Thanks and Cheers,

Suraj Upadhyay.