drm: strict type checking for drm_device based logging helpers

From: Gustavo Sousa <[email protected]>

The header drm_print.h uses members of struct drm_device pointers, as such, it should include drm_device.h to let the compiler know the full type definition.

Without such include, users of drm_print.h that don't explicitly need drm_device.h would bump into build errors and be forced to include the latter.

Signed-off-by Gustavo Sousa <[email protected]>

Reviewed-by Jani Nikula <[email protected]>

---

Including here as a dependency. May be merged independently.

---
 include/drm/drm_print.h | 1 +
 1 file changed, 1 insertion(+)

include/drm/drm_print.h +1 -0

index f77fe1531cf8..9732f514566d 100644
@@ -32,6 +32,7 @@
 #include <linux/dynamic_debug.h>
 
 #include <drm/drm.h>
+#include <drm/drm_device.h>
 
 struct debugfs_regset32;
 struct drm_device;

Quoted 6 lines

From: Gustavo Sousa <[email protected]>

The header drm_print.h uses members of struct drm_device pointers, as such, it should include drm_device.h to let the compiler know the full type definition.

Quoted 6 lines

Without such include, users of drm_print.h that don't explicitly need drm_device.h would bump into build errors and be forced to include the latter.

Signed-off-by Gustavo Sousa <[email protected]>

Reviewed-by Jani Nikula <[email protected]>

This posting should have had

Signed-off-by Jani Nikula <[email protected]>

but obviously not needed if the original [1] is merged instead.

[1] https://lore.kernel.org/r/[email protected]

Quoted 21 lines

---

Including here as a dependency. May be merged independently.

---
 include/drm/drm_print.h | 1 +
 1 file changed, 1 insertion(+)

include/drm/drm_print.h +1 -0

index f77fe1531cf8..9732f514566d 100644
@@ -32,6 +32,7 @@
 #include <linux/dynamic_debug.h>
 
 #include <drm/drm.h>
+#include <drm/drm_device.h>
 
 struct debugfs_regset32;
 struct drm_device;

Signature · 1 line

Jani Nikula, Intel

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Convert drm_err(sched, ...) to dev_err(sched->dev, ...) and similar. This matches current usage, as struct drm_device is not available, but drops "[drm]" or "[drm] *ERROR*" prefix from logging.

Unfortunately, there's no dev_WARN_ON(), so the conversion is not exactly the same.

Signed-off-by Jani Nikula <[email protected]>

---

Cc Matthew Brost <[email protected]>

Cc Danilo Krummrich <[email protected]>

Cc Philipp Stanner <[email protected]>

Cc "Christian König" <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/scheduler/sched_entity.c |  2 +-
 drivers/gpu/drm/scheduler/sched_main.c   | 20 +++++++++++---------
 2 files changed, 12 insertions(+), 10 deletions(-)

drivers/gpu/drm/scheduler/sched_entity.c +1 -1

index 69bcf0e99d57..e29af71d4b5c 100644
@@ -92,7 +92,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 		 * the lowest priority available.
 		 */
 		if (entity->priority >= sched_list[0]->num_rqs) {
-			drm_err(sched_list[0], "entity with out-of-bounds priority:%u num_rqs:%u\n",
+			dev_err(sched_list[0]->dev, "entity with out-of-bounds priority:%u num_rqs:%u\n",
 				entity->priority, sched_list[0]->num_rqs);
 			entity->priority = max_t(s32, (s32) sched_list[0]->num_rqs - 1,
 						 (s32) DRM_SCHED_PRIORITY_KERNEL);

drivers/gpu/drm/scheduler/sched_main.c +11 -9

index a48be16ab84f..d1c1f22fd1db 100644
@@ -103,9 +103,9 @@ static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
 {
 	u32 credits;
 
-	drm_WARN_ON(sched, check_sub_overflow(sched->credit_limit,
-					      atomic_read(&sched->credit_count),
-					      &credits));
+	WARN_ON(check_sub_overflow(sched->credit_limit,
+				   atomic_read(&sched->credit_count),
+				   &credits));
 
 	return credits;
 }

@@ -130,9 +130,11 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
 	/* If a job exceeds the credit limit, truncate it to the credit limit
 	 * itself to guarantee forward progress.
 	 */
-	if (drm_WARN(sched, s_job->credits > sched->credit_limit,
-		     "Jobs may not exceed the credit limit, truncate.\n"))
+	if (s_job->credits > sched->credit_limit) {
+		dev_WARN(sched->dev,
+			 "Jobs may not exceed the credit limit, truncate.\n");
 		s_job->credits = sched->credit_limit;
+	}
 
 	return drm_sched_available_credits(sched) >= s_job->credits;
 }

@@ -790,7 +792,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
 		 * or worse--a blank screen--leave a trail in the
 		 * logs, so this can be debugged easier.
 		 */
-		drm_err(job->sched, "%s: entity has no rq!\n", __func__);
+		dev_err(job->sched->dev, "%s: entity has no rq!\n", __func__);
 		return -ENOENT;
 	}
 

@@ -1280,7 +1282,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 	if (num_rqs > DRM_SCHED_PRIORITY_COUNT) {
 		/* This is a gross violation--tell drivers what the  problem is.
 		 */
-		drm_err(sched, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
+		dev_err(sched->dev, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
 			__func__);
 		return -EINVAL;
 	} else if (sched->sched_rq) {

@@ -1288,7 +1290,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 		 * fine-tune their DRM calling order, and return all
 		 * is good.
 		 */
-		drm_warn(sched, "%s: scheduler already initialized!\n", __func__);
+		dev_warn(sched->dev, "%s: scheduler already initialized!\n", __func__);
 		return 0;
 	}
 

@@ -1343,7 +1345,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 Out_check_own:
 	if (sched->own_submit_wq)
 		destroy_workqueue(sched->submit_wq);
-	drm_err(sched, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
+	dev_err(sched->dev, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
 	return -ENOMEM;
 }
 EXPORT_SYMBOL(drm_sched_init);

Signature · 1 line

2.39.5

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches current usage, but drops "[drm] *ERROR*" prefix from logging.

Signed-off-by Jani Nikula <[email protected]>

---

Looks like it's possible to hunt down the struct drm_device in most of these cases, if that's desired. This was the simplest change.

Cc Sandy Huang <[email protected]>

Cc "Heiko Stübner" <[email protected]>

Cc Andy Yan <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 16 ++++++++--------
drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------

2 files changed, 16 insertions(+), 16 deletions(-)

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +6 -6

index e7a6669c46b0..f737e7d46e66 100644
@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 

@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->ref_clk)) {
 		ret = PTR_ERR(hdmi->ref_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get reference clock\n");
+			dev_err(hdmi->dev, "failed to get reference clock\n");
 		return ret;
 	}
 

@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->grf_clk)) {
 		ret = PTR_ERR(hdmi->grf_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get grf clock\n");
+			dev_err(hdmi->dev, "failed to get grf clock\n");
 		return ret;
 	}
 

@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 
 	ret = clk_prepare_enable(hdmi->grf_clk);
 	if (ret < 0) {
-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
 		return;
 	}
 
 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
 	if (ret != 0)
-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
 
 	clk_disable_unprepare(hdmi->grf_clk);
-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
 }
 

static int

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +2 -2

@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	ret = rockchip_hdmi_parse_dt(hdmi);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "Unable to parse OF data\n");
+			dev_err(hdmi->dev, "Unable to parse OF data\n");
 		return ret;
 	}
 

@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy\n");
+			dev_err(hdmi->dev, "failed to get phy\n");
 		return ret;
 	}
 

drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c +8 -8

index f41151d49fca..3d1dddb34603 100644
@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
 	if (drm) {
 		changed = drm_helper_hpd_irq_event(drm);
 		if (changed)
-			drm_dbg(hdmi, "connector status changed\n");
+			dev_dbg(hdmi->dev, "connector status changed\n");
 	}
 }
 

@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		}
 	}
 	if (hdmi->port_id < 0) {
-		drm_err(hdmi, "Failed to match HDMI port ID\n");
+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
 		return hdmi->port_id;
 	}
 

@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 
 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,vo-grf");
 	if (IS_ERR(hdmi->vo_regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
 		return PTR_ERR(hdmi->vo_regmap);
 	}
 
 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
 	if (ret < 0) {
-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
 		return ret;
 	}
 

@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(hdmi->enable_gpio)) {
 		ret = PTR_ERR(hdmi->enable_gpio);
-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
 		return ret;
 	}
 

@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy: %d\n", ret);
+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
 		return ret;
 	}
 

@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
 		return ret;
 	}
 

Signature · 7 lines

2.39.5


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Hi，

Quoted 4 lines · Jani Nikula

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Quoted 2 lines

Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches current usage, but drops "[drm] *ERROR*" prefix from logging.

Frankly, I prefer the original version of the log.
It is a platform driver, so it should use its own device.
It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging

Quoted 170 lines

Signed-off-by Jani Nikula <[email protected]>

---

Looks like it's possible to hunt down the struct drm_device in most of these cases, if that's desired. This was the simplest change.

Cc Sandy Huang <[email protected]>

Cc "Heiko Stübner" <[email protected]>

Cc Andy Yan <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 16 ++++++++--------
drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------

2 files changed, 16 insertions(+), 16 deletions(-)

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +6 -6

index e7a6669c46b0..f737e7d46e66 100644
@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 

@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->ref_clk)) {
 		ret = PTR_ERR(hdmi->ref_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get reference clock\n");
+			dev_err(hdmi->dev, "failed to get reference clock\n");
 		return ret;
 	}
 

@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->grf_clk)) {
 		ret = PTR_ERR(hdmi->grf_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get grf clock\n");
+			dev_err(hdmi->dev, "failed to get grf clock\n");
 		return ret;
 	}
 

@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 
 	ret = clk_prepare_enable(hdmi->grf_clk);
 	if (ret < 0) {
-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
 		return;
 	}
 
 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
 	if (ret != 0)
-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
 
 	clk_disable_unprepare(hdmi->grf_clk);
-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
 }
 

static int

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +2 -2

@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	ret = rockchip_hdmi_parse_dt(hdmi);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "Unable to parse OF data\n");
+			dev_err(hdmi->dev, "Unable to parse OF data\n");
 		return ret;
 	}
 

@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy\n");
+			dev_err(hdmi->dev, "failed to get phy\n");
 		return ret;
 	}
 

drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c +8 -8

index f41151d49fca..3d1dddb34603 100644
@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
 	if (drm) {
 		changed = drm_helper_hpd_irq_event(drm);
 		if (changed)
-			drm_dbg(hdmi, "connector status changed\n");
+			dev_dbg(hdmi->dev, "connector status changed\n");
 	}
 }
 

@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		}
 	}
 	if (hdmi->port_id < 0) {
-		drm_err(hdmi, "Failed to match HDMI port ID\n");
+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
 		return hdmi->port_id;
 	}
 

@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 
 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,vo-grf");
 	if (IS_ERR(hdmi->vo_regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
 		return PTR_ERR(hdmi->vo_regmap);
 	}
 
 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
 	if (ret < 0) {
-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
 		return ret;
 	}
 

@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(hdmi->enable_gpio)) {
 		ret = PTR_ERR(hdmi->enable_gpio);
-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
 		return ret;
 	}
 

@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy: %d\n", ret);
+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
 		return ret;
 	}
 

@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
 		return ret;
 	}
 

Signature · 7 lines

2.39.5


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Quoted 11 lines · Jani Nikula

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Convert drm_err(sched, ...) to dev_err(sched->dev, ...) and similar. This matches current usage, as struct drm_device is not available, but drops "[drm]" or "[drm] *ERROR*" prefix from logging.

Unfortunately, there's no dev_WARN_ON(), so the conversion is not exactly the same.

Quoted 1 line

Signed-off-by Jani Nikula <[email protected]>

For the two previous patches just dev_ makes sense since they're just platform drivers, but for drm/sched I wonder whether it wouldn't be better to switch from struct device * to struct drm_device * instead. I guess might be best to leave that decision to scheduler folks.

Anyway on the series and with that caveat:

Reviewed-by Simona Vetter <[email protected]>

Quoted 98 lines

---

Cc Matthew Brost <[email protected]>

Cc Danilo Krummrich <[email protected]>

Cc Philipp Stanner <[email protected]>

Cc "Christian König" <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/scheduler/sched_entity.c |  2 +-
 drivers/gpu/drm/scheduler/sched_main.c   | 20 +++++++++++---------
 2 files changed, 12 insertions(+), 10 deletions(-)

drivers/gpu/drm/scheduler/sched_entity.c +1 -1

index 69bcf0e99d57..e29af71d4b5c 100644
@@ -92,7 +92,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 		 * the lowest priority available.
 		 */
 		if (entity->priority >= sched_list[0]->num_rqs) {
-			drm_err(sched_list[0], "entity with out-of-bounds priority:%u num_rqs:%u\n",
+			dev_err(sched_list[0]->dev, "entity with out-of-bounds priority:%u num_rqs:%u\n",
 				entity->priority, sched_list[0]->num_rqs);
 			entity->priority = max_t(s32, (s32) sched_list[0]->num_rqs - 1,
 						 (s32) DRM_SCHED_PRIORITY_KERNEL);

drivers/gpu/drm/scheduler/sched_main.c +11 -9

index a48be16ab84f..d1c1f22fd1db 100644
@@ -103,9 +103,9 @@ static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
 {
 	u32 credits;
 
-	drm_WARN_ON(sched, check_sub_overflow(sched->credit_limit,
-					      atomic_read(&sched->credit_count),
-					      &credits));
+	WARN_ON(check_sub_overflow(sched->credit_limit,
+				   atomic_read(&sched->credit_count),
+				   &credits));
 
 	return credits;
 }

@@ -130,9 +130,11 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
 	/* If a job exceeds the credit limit, truncate it to the credit limit
 	 * itself to guarantee forward progress.
 	 */
-	if (drm_WARN(sched, s_job->credits > sched->credit_limit,
-		     "Jobs may not exceed the credit limit, truncate.\n"))
+	if (s_job->credits > sched->credit_limit) {
+		dev_WARN(sched->dev,
+			 "Jobs may not exceed the credit limit, truncate.\n");
 		s_job->credits = sched->credit_limit;
+	}
 
 	return drm_sched_available_credits(sched) >= s_job->credits;
 }

@@ -790,7 +792,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
 		 * or worse--a blank screen--leave a trail in the
 		 * logs, so this can be debugged easier.
 		 */
-		drm_err(job->sched, "%s: entity has no rq!\n", __func__);
+		dev_err(job->sched->dev, "%s: entity has no rq!\n", __func__);
 		return -ENOENT;
 	}
 

@@ -1280,7 +1282,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 	if (num_rqs > DRM_SCHED_PRIORITY_COUNT) {
 		/* This is a gross violation--tell drivers what the  problem is.
 		 */
-		drm_err(sched, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
+		dev_err(sched->dev, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
 			__func__);
 		return -EINVAL;
 	} else if (sched->sched_rq) {

@@ -1288,7 +1290,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 		 * fine-tune their DRM calling order, and return all
 		 * is good.
 		 */
-		drm_warn(sched, "%s: scheduler already initialized!\n", __func__);
+		dev_warn(sched->dev, "%s: scheduler already initialized!\n", __func__);
 		return 0;
 	}
 

@@ -1343,7 +1345,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 Out_check_own:
 	if (sched->own_submit_wq)
 		destroy_workqueue(sched->submit_wq);
-	drm_err(sched, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
+	dev_err(sched->dev, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
 	return -ENOMEM;
 }
 EXPORT_SYMBOL(drm_sched_init);

Signature · 1 line

2.39.5

Quoting Jani Nikula (2025-01-23 12:14:31-03:00)

Quoted 21 lines · Jani Nikula

Quoted 6 lines

From: Gustavo Sousa <[email protected]>

The header drm_print.h uses members of struct drm_device pointers, as such, it should include drm_device.h to let the compiler know the full type definition.

Quoted 6 lines

Without such include, users of drm_print.h that don't explicitly need drm_device.h would bump into build errors and be forced to include the latter.

Signed-off-by Gustavo Sousa <[email protected]>

Reviewed-by Jani Nikula <[email protected]>

This posting should have had

Signed-off-by Jani Nikula <[email protected]>

but obviously not needed if the original [1] is merged instead.

Quoted 1 line

[1] https://lore.kernel.org/r/[email protected]

Hm. Since that's in the upper drm layer, I thought I was not supposed to merge it myself.

Am I? In that case, is it okay to merge it via drm-intel-next?

--
Gustavo Sousa

Quoted 24 lines

Quoted 21 lines

---

Including here as a dependency. May be merged independently.

---
 include/drm/drm_print.h | 1 +
 1 file changed, 1 insertion(+)

include/drm/drm_print.h +1 -0

index f77fe1531cf8..9732f514566d 100644
@@ -32,6 +32,7 @@
 #include <linux/dynamic_debug.h>
 
 #include <drm/drm.h>
+#include <drm/drm_device.h>
 
 struct debugfs_regset32;
 struct drm_device;

Signature · 1 line

Jani Nikula, Intel

Quoted 22 lines

Quoting Jani Nikula (2025-01-23 12:14:31-03:00)

Quoted 21 lines · Jani Nikula

Quoted 6 lines

From: Gustavo Sousa <[email protected]>

The header drm_print.h uses members of struct drm_device pointers, as such, it should include drm_device.h to let the compiler know the full type definition.

Quoted 6 lines

Without such include, users of drm_print.h that don't explicitly need drm_device.h would bump into build errors and be forced to include the latter.

Signed-off-by Gustavo Sousa <[email protected]>

Reviewed-by Jani Nikula <[email protected]>

This posting should have had

Signed-off-by Jani Nikula <[email protected]>

but obviously not needed if the original [1] is merged instead.

Quoted 6 lines

Quoted 1 line

[1] https://lore.kernel.org/r/[email protected]

Hm. Since that's in the upper drm layer, I thought I was not supposed to merge it myself.

Am I? In that case, is it okay to merge it via drm-intel-next?

It's generally not okay to merge non-i915 stuff via drm-intel-next, because only i915 is under our maintenance. We'd need rationale and acks for that, which we occasionally do, but there's no reason here.

But you can apply for drm-misc commit rights, similar to drm-intel, and apply the patches to drm-misc-next yourself. See the maintainer tools documentation.

BR,
Jani.

Quoted 28 lines

--
Gustavo Sousa

Quoted 24 lines

Quoted 21 lines

---

Including here as a dependency. May be merged independently.

---
 include/drm/drm_print.h | 1 +
 1 file changed, 1 insertion(+)

include/drm/drm_print.h +1 -0

index f77fe1531cf8..9732f514566d 100644
@@ -32,6 +32,7 @@
 #include <linux/dynamic_debug.h>
 
 #include <drm/drm.h>
+#include <drm/drm_device.h>
 
 struct debugfs_regset32;
 struct drm_device;

Signature · 1 line

Jani Nikula, Intel

Signature · 1 line

Jani Nikula, Intel

Quoting Jani Nikula (2025-01-24 08:50:14-03:00)

Quoted 30 lines · Gustavo Sousa

Quoted 22 lines

Quoting Jani Nikula (2025-01-23 12:14:31-03:00)

Quoted 21 lines · Jani Nikula

Quoted 6 lines

From: Gustavo Sousa <[email protected]>

The header drm_print.h uses members of struct drm_device pointers, as such, it should include drm_device.h to let the compiler know the full type definition.

Quoted 6 lines

Without such include, users of drm_print.h that don't explicitly need drm_device.h would bump into build errors and be forced to include the latter.

Signed-off-by Gustavo Sousa <[email protected]>

Reviewed-by Jani Nikula <[email protected]>

This posting should have had

Signed-off-by Jani Nikula <[email protected]>

but obviously not needed if the original [1] is merged instead.

Quoted 6 lines

Quoted 1 line

[1] https://lore.kernel.org/r/[email protected]

Hm. Since that's in the upper drm layer, I thought I was not supposed to merge it myself.

Am I? In that case, is it okay to merge it via drm-intel-next?

Quoted 7 lines

It's generally not okay to merge non-i915 stuff via drm-intel-next, because only i915 is under our maintenance. We'd need rationale and acks for that, which we occasionally do, but there's no reason here.

But you can apply for drm-misc commit rights, similar to drm-intel, and apply the patches to drm-misc-next yourself. See the maintainer tools documentation.

The first bullet point in the list of criteria for drm-misc[2] says:

"Submitted a few (5-10 as a rule of thumb) non-trivial (not just simple spelling fixes and whitespace adjustment) patches that have been merged already."

I believe those patches should be targeted at drm-misc, right? (meaning not stuff that have their own tree (e.g. drm-xe drm-intel)).

If that's the case, since this is my first patch targeting core drm, I believe I would not fit into the criteria right now.

Decided to ask here just to make sure I don't raise a request in vain.

[2] https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#drm-misc

--
Gustavo Sousa

Quoted 37 lines

BR,
Jani.

Quoted 28 lines

--
Gustavo Sousa

Quoted 24 lines

Quoted 21 lines

---

Including here as a dependency. May be merged independently.

---
 include/drm/drm_print.h | 1 +
 1 file changed, 1 insertion(+)

include/drm/drm_print.h +1 -0

index f77fe1531cf8..9732f514566d 100644
@@ -32,6 +32,7 @@
 #include <linux/dynamic_debug.h>
 
 #include <drm/drm.h>
+#include <drm/drm_device.h>
 
 struct debugfs_regset32;
 struct drm_device;

Signature · 1 line

Jani Nikula, Intel

Signature · 1 line

Jani Nikula, Intel

Quoted 7 lines · Andy Yan

Hi，

Quoted 4 lines · Jani Nikula

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Quoted 6 lines

Quoted 2 lines

Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches current usage, but drops "[drm] *ERROR*" prefix from logging.

Frankly, I prefer the original version of the log.
It is a platform driver, so it should use its own device.
It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging

If you need to do struct device based logging that is not the same device as the struct drm_device dev member, you need to use dev_err() and friends. You can't and must not use drm_err() and friends.

It's as simple as that.

The current drm_err(hdmi, ...) usage is simply abuse of the macros, and must stop.

BR,
Jani.

Quoted 170 lines

Quoted 170 lines

Signed-off-by Jani Nikula <[email protected]>

---

Looks like it's possible to hunt down the struct drm_device in most of these cases, if that's desired. This was the simplest change.

Cc Sandy Huang <[email protected]>

Cc "Heiko Stübner" <[email protected]>

Cc Andy Yan <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 16 ++++++++--------
drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------

2 files changed, 16 insertions(+), 16 deletions(-)

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +6 -6

index e7a6669c46b0..f737e7d46e66 100644
@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 

@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->ref_clk)) {
 		ret = PTR_ERR(hdmi->ref_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get reference clock\n");
+			dev_err(hdmi->dev, "failed to get reference clock\n");
 		return ret;
 	}
 

@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->grf_clk)) {
 		ret = PTR_ERR(hdmi->grf_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get grf clock\n");
+			dev_err(hdmi->dev, "failed to get grf clock\n");
 		return ret;
 	}
 

@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 
 	ret = clk_prepare_enable(hdmi->grf_clk);
 	if (ret < 0) {
-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
 		return;
 	}
 
 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
 	if (ret != 0)
-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
 
 	clk_disable_unprepare(hdmi->grf_clk);
-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
 }
 

static int

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +2 -2

@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	ret = rockchip_hdmi_parse_dt(hdmi);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "Unable to parse OF data\n");
+			dev_err(hdmi->dev, "Unable to parse OF data\n");
 		return ret;
 	}
 

@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy\n");
+			dev_err(hdmi->dev, "failed to get phy\n");
 		return ret;
 	}
 

drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c +8 -8

index f41151d49fca..3d1dddb34603 100644
@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
 	if (drm) {
 		changed = drm_helper_hpd_irq_event(drm);
 		if (changed)
-			drm_dbg(hdmi, "connector status changed\n");
+			dev_dbg(hdmi->dev, "connector status changed\n");
 	}
 }
 

@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		}
 	}
 	if (hdmi->port_id < 0) {
-		drm_err(hdmi, "Failed to match HDMI port ID\n");
+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
 		return hdmi->port_id;
 	}
 

@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 
 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,vo-grf");
 	if (IS_ERR(hdmi->vo_regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
 		return PTR_ERR(hdmi->vo_regmap);
 	}
 
 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
 	if (ret < 0) {
-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
 		return ret;
 	}
 

@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(hdmi->enable_gpio)) {
 		ret = PTR_ERR(hdmi->enable_gpio);
-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
 		return ret;
 	}
 

@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy: %d\n", ret);
+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
 		return ret;
 	}
 

@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
 		return ret;
 	}
 

Signature · 7 lines

2.39.5


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Signature · 1 line

Jani Nikula, Intel

Quoted 17 lines · Simona Vetter

Quoted 11 lines · Jani Nikula

The expectation is that the struct drm_device based logging helpers get
passed an actual struct drm_device pointer rather than some random
struct pointer where you can dereference the ->dev member.

Convert drm_err(sched, ...) to dev_err(sched->dev, ...) and similar. This matches current usage, as struct drm_device is not available, but drops "[drm]" or "[drm] *ERROR*" prefix from logging.

Quoted 4 lines

Unfortunately, there's no dev_WARN_ON(), so the conversion is not exactly the same.

Signed-off-by Jani Nikula <[email protected]>

Quoted 7 lines

For the two previous patches just dev_ makes sense since they're just
platform drivers, but for drm/sched I wonder whether it wouldn't be better
to switch from struct device * to struct drm_device * instead. I guess
might be best to leave that decision to scheduler folks.

I had a very brief look, and it seemed like struct drm_device isn't passed around in sched. If use of struct drm_device is preferred, I'm not the best person to figure out how to do that.

But the abuse of the
drm_err() and friends macros needs to stop.

Quoted 3 lines

Anyway on the series and with that caveat:

Reviewed-by Simona Vetter <[email protected]>

Thanks,
Jani.

Quoted 16 lines

Quoted 16 lines

---

Cc Matthew Brost <[email protected]>

Cc Danilo Krummrich <[email protected]>

Cc Philipp Stanner <[email protected]>

Cc "Christian König" <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/scheduler/sched_entity.c |  2 +- drivers/gpu/drm/scheduler/sched_main.c   | 20 +++++++++++------- --

 2 files changed, 12 insertions(+), 10 deletions(-)

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/scheduler/sched_entity.c +0 -0

index 69bcf0e99d57..e29af71d4b5c 100644
@@ -92,7 +92,7 @@ int drm_sched_entity_init(struct
 drm_sched_entity *entity,

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/scheduler/sched_entity.c +1 -1

  		 * the lowest priority available.
  		 */
  		if (entity->priority >= sched_list[0]->num_rqs)
 {
-			drm_err(sched_list[0], "entity with out-
 of-bounds priority:%u num_rqs:%u\n",
+			dev_err(sched_list[0]->dev, "entity with

out-of-bounds priority:%u num_rqs:%u\n",

Quoted 102 lines

Quoted 102 lines

  entity->priority, sched_list[0]-

Quoted 1 line

num_rqs);

  entity->priority = max_t(s32, (s32) sched_list[0]->num_rqs - 1,
  (s32) DRM_SCHED_PRIORITY_KERNEL);

drivers/gpu/drm/scheduler/sched_main.c +0 -0

index a48be16ab84f..d1c1f22fd1db 100644
@@ -103,9 +103,9 @@ static u32 drm_sched_available_credits(struct
 drm_gpu_scheduler *sched)
  {
  	u32 credits;

drivers/gpu/drm/scheduler/sched_main.c +0 -1

-	drm_WARN_ON(sched, check_sub_overflow(sched-

Quoted 1 line

credit_limit,

drivers/gpu/drm/scheduler/sched_main.c +0 -1

-					     

atomic_read(&sched->credit_count),

drivers/gpu/drm/scheduler/sched_main.c +2 -1

-					      &credits));
+	WARN_ON(check_sub_overflow(sched->credit_limit,
+				   atomic_read(&sched-

Quoted 1 line

credit_count),

drivers/gpu/drm/scheduler/sched_main.c +1 -0

+				   &credits));

  return credits;
 }

drivers/gpu/drm/scheduler/sched_main.c +0 -0

@@ -130,9 +130,11 @@ static bool drm_sched_can_queue(struct
 drm_gpu_scheduler *sched,
  	/* If a job exceeds the credit limit, truncate it to the

credit limit
  * itself to guarantee forward progress.
  */

drivers/gpu/drm/scheduler/sched_main.c +0 -1

-	if (drm_WARN(sched, s_job->credits > sched-

Quoted 1 line

credit_limit,

drivers/gpu/drm/scheduler/sched_main.c +0 -1

-		     "Jobs may not exceed the credit limit,

truncate.\n"))

drivers/gpu/drm/scheduler/sched_main.c +3 -0

+	if (s_job->credits > sched->credit_limit) {
+		dev_WARN(sched->dev,
+			 "Jobs may not exceed the credit limit,

truncate.\n");
  s_job->credits = sched->credit_limit;

drivers/gpu/drm/scheduler/sched_main.c +1 -0

+	}

  return drm_sched_available_credits(sched) >= s_job-

Quoted 1 line

credits;

 }

drivers/gpu/drm/scheduler/sched_main.c +0 -0

@@ -790,7 +792,7 @@ int drm_sched_job_init(struct drm_sched_job
 *job,
  		 * or worse--a blank screen--leave a trail in

the
  * logs, so this can be debugged easier. */

drivers/gpu/drm/scheduler/sched_main.c +0 -1

-		drm_err(job->sched, "%s: entity has no rq!\n",

__func__);

drivers/gpu/drm/scheduler/sched_main.c +1 -0

+		dev_err(job->sched->dev, "%s: entity has no

rq!\n", __func__);
  return -ENOENT;
  }

drivers/gpu/drm/scheduler/sched_main.c +0 -0

@@ -1280,7 +1282,7 @@ int drm_sched_init(struct drm_gpu_scheduler
 *sched,
  	if (num_rqs > DRM_SCHED_PRIORITY_COUNT) {
  		/* This is a gross violation--tell drivers what

the  problem is.
  */

drivers/gpu/drm/scheduler/sched_main.c +0 -1

-		drm_err(sched, "%s: num_rqs cannot be greater

than DRM_SCHED_PRIORITY_COUNT\n",

drivers/gpu/drm/scheduler/sched_main.c +1 -0

+		dev_err(sched->dev, "%s: num_rqs cannot be

greater than DRM_SCHED_PRIORITY_COUNT\n",
  __func__);
  return -EINVAL;
  } else if (sched->sched_rq) {

drivers/gpu/drm/scheduler/sched_main.c +0 -0

@@ -1288,7 +1290,7 @@ int drm_sched_init(struct drm_gpu_scheduler
 *sched,
  		 * fine-tune their DRM calling order, and return

all
  * is good.
  */

drivers/gpu/drm/scheduler/sched_main.c +0 -1

-		drm_warn(sched, "%s: scheduler already

initialized!\n", __func__);

drivers/gpu/drm/scheduler/sched_main.c +1 -0

+		dev_warn(sched->dev, "%s: scheduler already

initialized!\n", __func__);
  return 0;
  }

drivers/gpu/drm/scheduler/sched_main.c +0 -0

@@ -1343,7 +1345,7 @@ int drm_sched_init(struct drm_gpu_scheduler
 *sched,

 Out_check_own:
  if (sched->own_submit_wq) destroy_workqueue(sched->submit_wq);

drivers/gpu/drm/scheduler/sched_main.c +0 -1

-	drm_err(sched, "%s: Failed to setup GPU scheduler--out

of memory\n", __func__);

drivers/gpu/drm/scheduler/sched_main.c +1 -0

+	dev_err(sched->dev, "%s: Failed to setup GPU scheduler--

out of memory\n", __func__);
  return -ENOMEM;
 }
 EXPORT_SYMBOL(drm_sched_init);

Signature · 1 line

2.39.5

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches current usage, but drops "[drm] *ERROR*" prefix from logging.

Signed-off-by Jani Nikula <[email protected]>

---

Looks like it's possible to hunt down the struct drm_device in most of these cases, if that's desired. This was the simplest change.

Cc Sandy Huang <[email protected]>

Cc "Heiko Stübner" <[email protected]>

Cc Andy Yan <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    | 16 ++++++++--------
  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------
  2 files changed, 16 insertions(+), 16 deletions(-)

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +8 -8

index e7a6669c46b0..f737e7d46e66 100644
@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 
  	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
  	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
  		return PTR_ERR(hdmi->regmap);
  	}
 

@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
  	if (IS_ERR(hdmi->ref_clk)) {
  		ret = PTR_ERR(hdmi->ref_clk);
  		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get reference clock\n");
+			dev_err(hdmi->dev, "failed to get reference clock\n");
  		return ret;
  	}
 

@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
  	if (IS_ERR(hdmi->grf_clk)) {
  		ret = PTR_ERR(hdmi->grf_clk);
  		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get grf clock\n");
+			dev_err(hdmi->dev, "failed to get grf clock\n");
  		return ret;
  	}
 

@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 
  	ret = clk_prepare_enable(hdmi->grf_clk);
  	if (ret < 0) {
-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
  		return;
  	}
 
  	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
  	if (ret != 0)
-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
 
  	clk_disable_unprepare(hdmi->grf_clk);
-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
  }
 
  static int

@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
  	ret = rockchip_hdmi_parse_dt(hdmi);
  	if (ret) {
  		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "Unable to parse OF data\n");
+			dev_err(hdmi->dev, "Unable to parse OF data\n");
  		return ret;
  	}
 

@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
  	if (IS_ERR(hdmi->phy)) {
  		ret = PTR_ERR(hdmi->phy);
  		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy\n");
+			dev_err(hdmi->dev, "failed to get phy\n");
  		return ret;
  	}
 

drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c +8 -8

index f41151d49fca..3d1dddb34603 100644
@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
  	if (drm) {
  		changed = drm_helper_hpd_irq_event(drm);
  		if (changed)
-			drm_dbg(hdmi, "connector status changed\n");
+			dev_dbg(hdmi->dev, "connector status changed\n");
  	}
  }
 

@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
  		}
  	}
  	if (hdmi->port_id < 0) {
-		drm_err(hdmi, "Failed to match HDMI port ID\n");
+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
  		return hdmi->port_id;
  	}
 

@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
  	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
  						       "rockchip,grf");
  	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
  		return PTR_ERR(hdmi->regmap);
  	}
 
  	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
  							  "rockchip,vo-grf");
  	if (IS_ERR(hdmi->vo_regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
  		return PTR_ERR(hdmi->vo_regmap);
  	}
 
  	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
  	if (ret < 0) {
-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
  		return ret;
  	}
 

@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
  						    GPIOD_OUT_HIGH);
  	if (IS_ERR(hdmi->enable_gpio)) {
  		ret = PTR_ERR(hdmi->enable_gpio);
-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
  		return ret;
  	}
 

@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
  	if (IS_ERR(hdmi->phy)) {
  		ret = PTR_ERR(hdmi->phy);
  		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy: %d\n", ret);
+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
  		return ret;
  	}
 

@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
  	connector = drm_bridge_connector_init(drm, encoder);
  	if (IS_ERR(connector)) {
  		ret = PTR_ERR(connector);
-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
  		return ret;
  	}
 

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Convert drm_err(sched, ...) to dev_err(sched->dev, ...) and similar. This matches current usage, as struct drm_device is not available, but drops "[drm]" or "[drm] *ERROR*" prefix from logging.

Unfortunately, there's no dev_WARN_ON(), so the conversion is not exactly the same.

Signed-off-by Jani Nikula <[email protected]>

---

Cc Matthew Brost <[email protected]>

Cc Danilo Krummrich <[email protected]>

Cc Philipp Stanner <[email protected]>

Cc "Christian König" <[email protected]>

Cc [email protected]

---
  drivers/gpu/drm/scheduler/sched_entity.c |  2 +-
  drivers/gpu/drm/scheduler/sched_main.c   | 20 +++++++++++---------
  2 files changed, 12 insertions(+), 10 deletions(-)

drivers/gpu/drm/scheduler/sched_entity.c +1 -1

index 69bcf0e99d57..e29af71d4b5c 100644
@@ -92,7 +92,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
  		 * the lowest priority available.
  		 */
  		if (entity->priority >= sched_list[0]->num_rqs) {
-			drm_err(sched_list[0], "entity with out-of-bounds priority:%u num_rqs:%u\n",
+			dev_err(sched_list[0]->dev, "entity with out-of-bounds priority:%u num_rqs:%u\n",
  				entity->priority, sched_list[0]->num_rqs);
  			entity->priority = max_t(s32, (s32) sched_list[0]->num_rqs - 1,
  						 (s32) DRM_SCHED_PRIORITY_KERNEL);

drivers/gpu/drm/scheduler/sched_main.c +11 -9

index a48be16ab84f..d1c1f22fd1db 100644
@@ -103,9 +103,9 @@ static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
  {
  	u32 credits;
 
-	drm_WARN_ON(sched, check_sub_overflow(sched->credit_limit,
-					      atomic_read(&sched->credit_count),
-					      &credits));
+	WARN_ON(check_sub_overflow(sched->credit_limit,
+				   atomic_read(&sched->credit_count),
+				   &credits));
 
  	return credits;
  }

@@ -130,9 +130,11 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
  	/* If a job exceeds the credit limit, truncate it to the credit limit
  	 * itself to guarantee forward progress.
  	 */
-	if (drm_WARN(sched, s_job->credits > sched->credit_limit,
-		     "Jobs may not exceed the credit limit, truncate.\n"))
+	if (s_job->credits > sched->credit_limit) {
+		dev_WARN(sched->dev,
+			 "Jobs may not exceed the credit limit, truncate.\n");
  		s_job->credits = sched->credit_limit;
+	}
 
  	return drm_sched_available_credits(sched) >= s_job->credits;
  }

@@ -790,7 +792,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
  		 * or worse--a blank screen--leave a trail in the
  		 * logs, so this can be debugged easier.
  		 */
-		drm_err(job->sched, "%s: entity has no rq!\n", __func__);
+		dev_err(job->sched->dev, "%s: entity has no rq!\n", __func__);
  		return -ENOENT;
  	}
 

@@ -1280,7 +1282,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
  	if (num_rqs > DRM_SCHED_PRIORITY_COUNT) {
  		/* This is a gross violation--tell drivers what the  problem is.
  		 */
-		drm_err(sched, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
+		dev_err(sched->dev, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
  			__func__);
  		return -EINVAL;
  	} else if (sched->sched_rq) {

@@ -1288,7 +1290,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
  		 * fine-tune their DRM calling order, and return all
  		 * is good.
  		 */
-		drm_warn(sched, "%s: scheduler already initialized!\n", __func__);
+		dev_warn(sched->dev, "%s: scheduler already initialized!\n", __func__);
  		return 0;
  	}
 

@@ -1343,7 +1345,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
  Out_check_own:
  	if (sched->own_submit_wq)
  		destroy_workqueue(sched->submit_wq);
-	drm_err(sched, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
+	dev_err(sched->dev, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
  	return -ENOMEM;
  }
  EXPORT_SYMBOL(drm_sched_init);

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Add a static inline helper to convert struct drm_device to struct device, with the main benefit being the type checking of the macro argument.

As a side effect, this also reduces macro argument double references.

Signed-off-by Jani Nikula <[email protected]>

---
  include/drm/drm_print.h | 41 +++++++++++++++++++++++------------------
  1 file changed, 23 insertions(+), 18 deletions(-)

include/drm/drm_print.h +23 -18

index 9732f514566d..f31eba1c7cab 100644
@@ -584,9 +584,15 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
   * Prefer drm_device based logging over device or prink based logging.
   */
 
+/* Helper to enforce struct drm_device type */
+static inline struct device *__drm_to_dev(const struct drm_device *drm)
+{
+	return drm ? drm->dev : NULL;
+}
+
  /* Helper for struct drm_device based logging. */
  #define __drm_printk(drm, level, type, fmt, ...)			\
-	dev_##level##type((drm) ? (drm)->dev : NULL, "[drm] " fmt, ##__VA_ARGS__)
+	dev_##level##type(__drm_to_dev(drm), "[drm] " fmt, ##__VA_ARGS__)
 
 
  #define drm_info(drm, fmt, ...)					\

@@ -620,25 +626,25 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
 
 
  #define drm_dbg_core(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_CORE, fmt, ##__VA_ARGS__)
-#define drm_dbg_driver(drm, fmt, ...)						\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_CORE, fmt, ##__VA_ARGS__)
+#define drm_dbg_driver(drm, fmt, ...)					\
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
  #define drm_dbg_kms(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_KMS, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_KMS, fmt, ##__VA_ARGS__)
  #define drm_dbg_prime(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_PRIME, fmt, ##__VA_ARGS__)
  #define drm_dbg_atomic(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
  #define drm_dbg_vbl(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_VBL, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_VBL, fmt, ##__VA_ARGS__)
  #define drm_dbg_state(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_STATE, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_STATE, fmt, ##__VA_ARGS__)
  #define drm_dbg_lease(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_LEASE, fmt, ##__VA_ARGS__)
  #define drm_dbg_dp(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DP, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_DP, fmt, ##__VA_ARGS__)
  #define drm_dbg_drmres(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRMRES, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(__drm_to_dev(drm), DRM_UT_DRMRES, fmt, ##__VA_ARGS__)
 
  #define drm_dbg(drm, fmt, ...)	drm_dbg_driver(drm, fmt, ##__VA_ARGS__)
 

@@ -727,10 +733,9 @@ void __drm_err(const char *format, ...);
  #define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...)					\
  ({												\
  	static DEFINE_RATELIMIT_STATE(rs_, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);\
-	const struct drm_device *drm_ = (drm);							\
  												\
  	if (drm_debug_enabled(DRM_UT_ ## category) && __ratelimit(&rs_))			\
-		drm_dev_printk(drm_ ? drm_->dev : NULL, KERN_DEBUG, fmt, ## __VA_ARGS__);	\
+		drm_dev_printk(__drm_to_dev(drm), KERN_DEBUG, fmt, ## __VA_ARGS__);		\
  })
 
  #define drm_dbg_ratelimited(drm, fmt, ...) \

@@ -752,13 +757,13 @@ void __drm_err(const char *format, ...);
  /* Helper for struct drm_device based WARNs */
  #define drm_WARN(drm, condition, format, arg...)			\
  	WARN(condition, "%s %s: [drm] " format,				\
-			dev_driver_string((drm)->dev),			\
-			dev_name((drm)->dev), ## arg)
+			dev_driver_string(__drm_to_dev(drm)),		\
+			dev_name(__drm_to_dev(drm)), ## arg)
 
  #define drm_WARN_ONCE(drm, condition, format, arg...)			\
  	WARN_ONCE(condition, "%s %s: [drm] " format,			\
-			dev_driver_string((drm)->dev),			\
-			dev_name((drm)->dev), ## arg)
+			dev_driver_string(__drm_to_dev(drm)),		\
+			dev_name(__drm_to_dev(drm)), ## arg)
 
  #define drm_WARN_ON(drm, x)						\
  	drm_WARN((drm), (x), "%s",					\

Quoted 12 lines · Simona Vetter

Quoted 11 lines · Jani Nikula

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Convert drm_err(sched, ...) to dev_err(sched->dev, ...) and similar. This matches current usage, as struct drm_device is not available, but drops "[drm]" or "[drm] *ERROR*" prefix from logging.

Unfortunately, there's no dev_WARN_ON(), so the conversion is not exactly the same.

Quoted 6 lines

Quoted 1 line

Signed-off-by Jani Nikula <[email protected]>

For the two previous patches just dev_ makes sense since they're just platform drivers, but for drm/sched I wonder whether it wouldn't be better to switch from struct device * to struct drm_device * instead. I guess might be best to leave that decision to scheduler folks.

I had a very brief look, and it seemed like struct drm_device isn't passed around in sched. If use of struct drm_device is preferred, I'm not the best person to figure out how to do that. But the abuse of the drm_err() and friends macros needs to stop.

Quoted 3 lines

Anyway on the series and with that caveat:

Reviewed-by Simona Vetter <[email protected]>

Thanks,
Jani.

Quoted 98 lines

Quoted 98 lines

---

Cc Matthew Brost <[email protected]>

Cc Danilo Krummrich <[email protected]>

Cc Philipp Stanner <[email protected]>

Cc "Christian König" <[email protected]>

Cc [email protected]

---
  drivers/gpu/drm/scheduler/sched_entity.c |  2 +-
  drivers/gpu/drm/scheduler/sched_main.c   | 20 +++++++++++---------
  2 files changed, 12 insertions(+), 10 deletions(-)

drivers/gpu/drm/scheduler/sched_entity.c +1 -1

index 69bcf0e99d57..e29af71d4b5c 100644
@@ -92,7 +92,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
  		 * the lowest priority available.
  		 */
  		if (entity->priority >= sched_list[0]->num_rqs) {
-			drm_err(sched_list[0], "entity with out-of-bounds priority:%u num_rqs:%u\n",
+			dev_err(sched_list[0]->dev, "entity with out-of-bounds priority:%u num_rqs:%u\n",
  				entity->priority, sched_list[0]->num_rqs);
  			entity->priority = max_t(s32, (s32) sched_list[0]->num_rqs - 1,
  						 (s32) DRM_SCHED_PRIORITY_KERNEL);

drivers/gpu/drm/scheduler/sched_main.c +11 -9

index a48be16ab84f..d1c1f22fd1db 100644
@@ -103,9 +103,9 @@ static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
  {
  	u32 credits;
 
-	drm_WARN_ON(sched, check_sub_overflow(sched->credit_limit,
-					      atomic_read(&sched->credit_count),
-					      &credits));
+	WARN_ON(check_sub_overflow(sched->credit_limit,
+				   atomic_read(&sched->credit_count),
+				   &credits));
 
  	return credits;
  }

@@ -130,9 +130,11 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
  	/* If a job exceeds the credit limit, truncate it to the credit limit
  	 * itself to guarantee forward progress.
  	 */
-	if (drm_WARN(sched, s_job->credits > sched->credit_limit,
-		     "Jobs may not exceed the credit limit, truncate.\n"))
+	if (s_job->credits > sched->credit_limit) {
+		dev_WARN(sched->dev,
+			 "Jobs may not exceed the credit limit, truncate.\n");
  		s_job->credits = sched->credit_limit;
+	}
 
  	return drm_sched_available_credits(sched) >= s_job->credits;
  }

@@ -790,7 +792,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
  		 * or worse--a blank screen--leave a trail in the
  		 * logs, so this can be debugged easier.
  		 */
-		drm_err(job->sched, "%s: entity has no rq!\n", __func__);
+		dev_err(job->sched->dev, "%s: entity has no rq!\n", __func__);
  		return -ENOENT;
  	}
 

@@ -1280,7 +1282,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
  	if (num_rqs > DRM_SCHED_PRIORITY_COUNT) {
  		/* This is a gross violation--tell drivers what the  problem is.
  		 */
-		drm_err(sched, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
+		dev_err(sched->dev, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
  			__func__);
  		return -EINVAL;
  	} else if (sched->sched_rq) {

@@ -1288,7 +1290,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
  		 * fine-tune their DRM calling order, and return all
  		 * is good.
  		 */
-		drm_warn(sched, "%s: scheduler already initialized!\n", __func__);
+		dev_warn(sched->dev, "%s: scheduler already initialized!\n", __func__);
  		return 0;
  	}
 

@@ -1343,7 +1345,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
  Out_check_own:
  	if (sched->own_submit_wq)
  		destroy_workqueue(sched->submit_wq);
-	drm_err(sched, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
+	dev_err(sched->dev, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
  	return -ENOMEM;
  }
  EXPORT_SYMBOL(drm_sched_init);

Signature · 1 line

2.39.5

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Convert drm_err(host, ...) to dev_err(host->dev, ...). This matches current usage, as struct drm_device is not available, but drops "[drm] *ERROR*" from logs.

Signed-off-by Jani Nikula <[email protected]>

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Add a static inline helper to convert struct drm_device to struct device, with the main benefit being the type checking of the macro argument.

As a side effect, this also reduces macro argument double references.

Signed-off-by Jani Nikula <[email protected]>

Quoted 15 lines · 在 2025-01-24 19:43:07，"Jani Nikula" <[email protected]> 写道：

Quoted 7 lines · Andy Yan

Hi，

Quoted 4 lines · Jani Nikula

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Quoted 6 lines

Quoted 2 lines

Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches current usage, but drops "[drm] *ERROR*" prefix from logging.

Frankly, I prefer the original version of the log.
It is a platform driver, so it should use its own device.
It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging

Quoted 8 lines

If you need to do struct device based logging that is not the same device as the struct drm_device dev member, you need to use dev_err() and friends. You can't and must not use drm_err() and friends.

It's as simple as that.

The current drm_err(hdmi, ...) usage is simply abuse of the macros, and must stop.

Perhaps when you initially designed this macros, you intended it to accept only drm_device, but your code implementation didn't enforce this restriction at the beginning.
If that's truly what you intended, I suggest just reverting this commit that converting to use these macros[0], as neither drm_err nor dev_err can maintain consistency with the original log of this driver. Alternatively, as suggested by Sam in the initial submission of your patch 5 years ago, there should also be a macro similar to drm_dev_info(device *, ..).[1]

[0]https://lore.kernel.org/linux-rockchip/20240813-dw-hdmi-rockchip-cleanup-v1-1-b3e73b5f4fd6@collabora.com/ [1]https://lore.kernel.org/dri-devel/[email protected]/

Quoted 179 lines

BR,
Jani.

Quoted 170 lines

Quoted 170 lines

Signed-off-by Jani Nikula <[email protected]>

---

Looks like it's possible to hunt down the struct drm_device in most of these cases, if that's desired. This was the simplest change.

Cc Sandy Huang <[email protected]>

Cc "Heiko Stübner" <[email protected]>

Cc Andy Yan <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 16 ++++++++--------
drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------

2 files changed, 16 insertions(+), 16 deletions(-)

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +6 -6

index e7a6669c46b0..f737e7d46e66 100644
@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 

@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->ref_clk)) {
 		ret = PTR_ERR(hdmi->ref_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get reference clock\n");
+			dev_err(hdmi->dev, "failed to get reference clock\n");
 		return ret;
 	}
 

@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->grf_clk)) {
 		ret = PTR_ERR(hdmi->grf_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get grf clock\n");
+			dev_err(hdmi->dev, "failed to get grf clock\n");
 		return ret;
 	}
 

@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 
 	ret = clk_prepare_enable(hdmi->grf_clk);
 	if (ret < 0) {
-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
 		return;
 	}
 
 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
 	if (ret != 0)
-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
 
 	clk_disable_unprepare(hdmi->grf_clk);
-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
 }
 

static int

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +2 -2

@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	ret = rockchip_hdmi_parse_dt(hdmi);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "Unable to parse OF data\n");
+			dev_err(hdmi->dev, "Unable to parse OF data\n");
 		return ret;
 	}
 

@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy\n");
+			dev_err(hdmi->dev, "failed to get phy\n");
 		return ret;
 	}
 

drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c +8 -8

index f41151d49fca..3d1dddb34603 100644
@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
 	if (drm) {
 		changed = drm_helper_hpd_irq_event(drm);
 		if (changed)
-			drm_dbg(hdmi, "connector status changed\n");
+			dev_dbg(hdmi->dev, "connector status changed\n");
 	}
 }
 

@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		}
 	}
 	if (hdmi->port_id < 0) {
-		drm_err(hdmi, "Failed to match HDMI port ID\n");
+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
 		return hdmi->port_id;
 	}
 

@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 
 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,vo-grf");
 	if (IS_ERR(hdmi->vo_regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
 		return PTR_ERR(hdmi->vo_regmap);
 	}
 
 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
 	if (ret < 0) {
-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
 		return ret;
 	}
 

@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(hdmi->enable_gpio)) {
 		ret = PTR_ERR(hdmi->enable_gpio);
-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
 		return ret;
 	}
 

@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy: %d\n", ret);
+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
 		return ret;
 	}
 

@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
 		return ret;
 	}
 

Signature · 7 lines

2.39.5


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Signature · 1 line

Jani Nikula, Intel

Quoted 17 lines · Andy Yan

Quoted 7 lines

Hi，

Quoted 4 lines · Jani Nikula

The expectation is that the struct drm_device based logging helpers get passed an actual struct drm_device pointer rather than some random struct pointer where you can dereference the ->dev member.

Quoted 5 lines

Quoted 2 lines

Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches current usage, but drops "[drm] *ERROR*" prefix from logging.

Frankly, I prefer the original version of the log.
It is a platform driver, so it should use its own device.
It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging

If you need to do struct device based logging that is not the same device as the struct drm_device dev member, you need to use dev_err() and friends. You can't and must not use drm_err() and friends.

Quoted 4 lines

It's as simple as that.

The current drm_err(hdmi, ...) usage is simply abuse of the macros, and must stop.

Perhaps when you initially designed this macros, you intended it to accept only drm_device, but your code implementation didn't enforce this restriction at the beginning.

If that's truly what you intended, I suggest just reverting this commit that converting to use these macros[0], as neither drm_err nor dev_err can maintain consistency with the original log of this driver. Alternatively, as suggested by Sam in the initial submission of your patch 5 years ago, there should also be a macro similar to drm_dev_info(device *, ..).[1]

[0]https://lore.kernel.org/linux-rockchip/20240813-dw-hdmi-rockchip-cleanup-v1-1-b3e73b5f4fd6@collabora.com/ [1]https://lore.kernel.org/dri-devel/[email protected]/

Quoted 175 lines

BR,
Jani.

Quoted 168 lines

Quoted 168 lines

Signed-off-by Jani Nikula <[email protected]>

---

Looks like it's possible to hunt down the struct drm_device in most of these cases, if that's desired. This was the simplest change.

Cc Sandy Huang <[email protected]>

Cc "Heiko Stübner" <[email protected]>

Cc Andy Yan <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 16 ++++++++--------
drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------

2 files changed, 16 insertions(+), 16 deletions(-)

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +6 -6

index e7a6669c46b0..f737e7d46e66 100644
@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 

@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->ref_clk)) {
 		ret = PTR_ERR(hdmi->ref_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get reference clock\n");
+			dev_err(hdmi->dev, "failed to get reference clock\n");
 		return ret;
 	}
 

@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->grf_clk)) {
 		ret = PTR_ERR(hdmi->grf_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get grf clock\n");
+			dev_err(hdmi->dev, "failed to get grf clock\n");
 		return ret;
 	}
 

@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 
 	ret = clk_prepare_enable(hdmi->grf_clk);
 	if (ret < 0) {
-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
 		return;
 	}
 
 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
 	if (ret != 0)
-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
 
 	clk_disable_unprepare(hdmi->grf_clk);
-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
 }
 

static int

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +2 -2

@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	ret = rockchip_hdmi_parse_dt(hdmi);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "Unable to parse OF data\n");
+			dev_err(hdmi->dev, "Unable to parse OF data\n");
 		return ret;
 	}
 

@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy\n");
+			dev_err(hdmi->dev, "failed to get phy\n");
 		return ret;
 	}
 

drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c +8 -8

index f41151d49fca..3d1dddb34603 100644
@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
 	if (drm) {
 		changed = drm_helper_hpd_irq_event(drm);
 		if (changed)
-			drm_dbg(hdmi, "connector status changed\n");
+			dev_dbg(hdmi->dev, "connector status changed\n");
 	}
 }
 

@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		}
 	}
 	if (hdmi->port_id < 0) {
-		drm_err(hdmi, "Failed to match HDMI port ID\n");
+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
 		return hdmi->port_id;
 	}
 

@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 
 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,vo-grf");
 	if (IS_ERR(hdmi->vo_regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
 		return PTR_ERR(hdmi->vo_regmap);
 	}
 
 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
 	if (ret < 0) {
-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
 		return ret;
 	}
 

@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(hdmi->enable_gpio)) {
 		ret = PTR_ERR(hdmi->enable_gpio);
-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
 		return ret;
 	}
 

@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy: %d\n", ret);
+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
 		return ret;
 	}
 

@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
 		return ret;
 	}
 

Signature · 7 lines

2.39.5


_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Signature · 1 line

Jani Nikula, Intel