Re: [PATCH v2] drm/dp: Move panel DP AUX backlight support to drm_dp_helper

rajeevny@xxxxxxxxxxxxxx · Tue, 13 Jul 2021 12:57:42 +0530

Hi Doug,

On 12-07-2021 20:30, Douglas Anderson wrote:
We were getting a depmod error:
  depmod: ERROR: Cycle detected: drm_kms_helper -> drm -> 
drm_kms_helper

It looks like the rule is that drm_kms_helper can call into drm, but
drm can't call into drm_kms_helper. That means we've got to move the
DP AUX backlight support into drm_dp_helper.

NOTE: as part of this, I didn't try to do any renames of the main
registration function. Even though it's in the drm_dp_helper, it still
feels very parallel to drm_panel_of_backlight().

Fixes: 10f7b40e4f30 ("drm/panel: add basic DP AUX backlight support")
Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Reported-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
---
Note that I've compile tested this, but I don't have a device setup
yet that uses this code. Since the code is basically the same as it
was this should be OK, but if Rajeev could confirm that nothing is
broken that'd be nice.


Reviewed-by: Rajeev Nandan <rajeevny@xxxxxxxxxxxxxx>

Thanks,
Rajeev


Changes in v2:
- Guard new functions by the proper configs.

 drivers/gpu/drm/drm_dp_helper.c | 113 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_panel.c     | 108 ------------------------------
 include/drm/drm_dp_helper.h     |  16 +++++
 include/drm/drm_panel.h         |   8 ---
 4 files changed, 129 insertions(+), 116 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c 
b/drivers/gpu/drm/drm_dp_helper.c
index 24bbc710c825..e8eec20ab364 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -33,9 +33,17 @@
 #include <drm/drm_print.h>
 #include <drm/drm_vblank.h>
 #include <drm/drm_dp_mst_helper.h>
+#include <drm/drm_panel.h>

 #include "drm_crtc_helper_internal.h"

+struct dp_aux_backlight {
+	struct backlight_device *base;
+	struct drm_dp_aux *aux;
+	struct drm_edp_backlight_info info;
+	bool enabled;
+};
+
 /**
  * DOC: dp helpers
  *
@@ -3462,3 +3470,108 @@ drm_edp_backlight_init(struct drm_dp_aux *aux,
struct drm_edp_backlight_info *bl
 	return 0;
 }
 EXPORT_SYMBOL(drm_edp_backlight_init);
+
+#if IS_BUILTIN(CONFIG_BACKLIGHT_CLASS_DEVICE) || \
+	(IS_MODULE(CONFIG_DRM_KMS_HELPER) && 
IS_MODULE(CONFIG_BACKLIGHT_CLASS_DEVICE))
+
+static int dp_aux_backlight_update_status(struct backlight_device *bd)
+{
+	struct dp_aux_backlight *bl = bl_get_data(bd);
+	u16 brightness = backlight_get_brightness(bd);
+	int ret = 0;
+
+	if (!backlight_is_blank(bd)) {
+		if (!bl->enabled) {
+			drm_edp_backlight_enable(bl->aux, &bl->info, brightness);
+			bl->enabled = true;
+			return 0;
+		}
+		ret = drm_edp_backlight_set_level(bl->aux, &bl->info, brightness);
+	} else {
+		if (bl->enabled) {
+			drm_edp_backlight_disable(bl->aux, &bl->info);
+			bl->enabled = false;
+		}
+	}
+
+	return ret;
+}
+
+static const struct backlight_ops dp_aux_bl_ops = {
+	.update_status = dp_aux_backlight_update_status,
+};
+
+/**
+ * drm_panel_dp_aux_backlight - create and use DP AUX backlight
+ * @panel: DRM panel
+ * @aux: The DP AUX channel to use
+ *
+ * Use this function to create and handle backlight if your panel
+ * supports backlight control over DP AUX channel using DPCD
+ * registers as per VESA's standard backlight control interface.
+ *
+ * When the panel is enabled backlight will be enabled after a
+ * successful call to &drm_panel_funcs.enable()
+ *
+ * When the panel is disabled backlight will be disabled before the
+ * call to &drm_panel_funcs.disable().
+ *
+ * A typical implementation for a panel driver supporting backlight
+ * control over DP AUX will call this function at probe time.
+ * Backlight will then be handled transparently without requiring
+ * any intervention from the driver.
+ *
+ * drm_panel_dp_aux_backlight() must be called after the call to
drm_panel_init().
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct 
drm_dp_aux *aux)
+{
+	struct dp_aux_backlight *bl;
+	struct backlight_properties props = { 0 };
+	u16 current_level;
+	u8 current_mode;
+	u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
+	int ret;
+
+	if (!panel || !panel->dev || !aux)
+		return -EINVAL;
+
+	ret = drm_dp_dpcd_read(aux, DP_EDP_DPCD_REV, edp_dpcd,
+			       EDP_DISPLAY_CTL_CAP_SIZE);
+	if (ret < 0)
+		return ret;
+
+	if (!drm_edp_backlight_supported(edp_dpcd)) {
+		DRM_DEV_INFO(panel->dev, "DP AUX backlight is not supported\n");
+		return 0;
+	}
+
+	bl = devm_kzalloc(panel->dev, sizeof(*bl), GFP_KERNEL);
+	if (!bl)
+		return -ENOMEM;
+
+	bl->aux = aux;
+
+	ret = drm_edp_backlight_init(aux, &bl->info, 0, edp_dpcd,
+				     &current_level, &current_mode);
+	if (ret < 0)
+		return ret;
+
+	props.type = BACKLIGHT_RAW;
+	props.brightness = current_level;
+	props.max_brightness = bl->info.max;
+
+	bl->base = devm_backlight_device_register(panel->dev, 
"dp_aux_backlight",
+						  panel->dev, bl,
+						  &dp_aux_bl_ops, &props);
+	if (IS_ERR(bl->base))
+		return PTR_ERR(bl->base);
+
+	panel->backlight = bl->base;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
+
+#endif
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 4fa1e3bb1b78..f634371c717a 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -26,20 +26,12 @@
 #include <linux/module.h>

 #include <drm/drm_crtc.h>
-#include <drm/drm_dp_helper.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>

 static DEFINE_MUTEX(panel_lock);
 static LIST_HEAD(panel_list);

-struct dp_aux_backlight {
-	struct backlight_device *base;
-	struct drm_dp_aux *aux;
-	struct drm_edp_backlight_info info;
-	bool enabled;
-};
-
 /**
  * DOC: drm panel
  *
@@ -350,106 +342,6 @@ int drm_panel_of_backlight(struct drm_panel 
*panel)
 	return 0;
 }
 EXPORT_SYMBOL(drm_panel_of_backlight);
-
-static int dp_aux_backlight_update_status(struct backlight_device *bd)
-{
-	struct dp_aux_backlight *bl = bl_get_data(bd);
-	u16 brightness = backlight_get_brightness(bd);
-	int ret = 0;
-
-	if (!backlight_is_blank(bd)) {
-		if (!bl->enabled) {
-			drm_edp_backlight_enable(bl->aux, &bl->info, brightness);
-			bl->enabled = true;
-			return 0;
-		}
-		ret = drm_edp_backlight_set_level(bl->aux, &bl->info, brightness);
-	} else {
-		if (bl->enabled) {
-			drm_edp_backlight_disable(bl->aux, &bl->info);
-			bl->enabled = false;
-		}
-	}
-
-	return ret;
-}
-
-static const struct backlight_ops dp_aux_bl_ops = {
-	.update_status = dp_aux_backlight_update_status,
-};
-
-/**
- * drm_panel_dp_aux_backlight - create and use DP AUX backlight
- * @panel: DRM panel
- * @aux: The DP AUX channel to use
- *
- * Use this function to create and handle backlight if your panel
- * supports backlight control over DP AUX channel using DPCD
- * registers as per VESA's standard backlight control interface.
- *
- * When the panel is enabled backlight will be enabled after a
- * successful call to &drm_panel_funcs.enable()
- *
- * When the panel is disabled backlight will be disabled before the
- * call to &drm_panel_funcs.disable().
- *
- * A typical implementation for a panel driver supporting backlight
- * control over DP AUX will call this function at probe time.
- * Backlight will then be handled transparently without requiring
- * any intervention from the driver.
- *
- * drm_panel_dp_aux_backlight() must be called after the call to
drm_panel_init().
- *
- * Return: 0 on success or a negative error code on failure.
- */
-int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct 
drm_dp_aux *aux)
-{
-	struct dp_aux_backlight *bl;
-	struct backlight_properties props = { 0 };
-	u16 current_level;
-	u8 current_mode;
-	u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
-	int ret;
-
-	if (!panel || !panel->dev || !aux)
-		return -EINVAL;
-
-	ret = drm_dp_dpcd_read(aux, DP_EDP_DPCD_REV, edp_dpcd,
-			       EDP_DISPLAY_CTL_CAP_SIZE);
-	if (ret < 0)
-		return ret;
-
-	if (!drm_edp_backlight_supported(edp_dpcd)) {
-		DRM_DEV_INFO(panel->dev, "DP AUX backlight is not supported\n");
-		return 0;
-	}
-
-	bl = devm_kzalloc(panel->dev, sizeof(*bl), GFP_KERNEL);
-	if (!bl)
-		return -ENOMEM;
-
-	bl->aux = aux;
-
-	ret = drm_edp_backlight_init(aux, &bl->info, 0, edp_dpcd,
-				     &current_level, &current_mode);
-	if (ret < 0)
-		return ret;
-
-	props.type = BACKLIGHT_RAW;
-	props.brightness = current_level;
-	props.max_brightness = bl->info.max;
-
-	bl->base = devm_backlight_device_register(panel->dev, 
"dp_aux_backlight",
-						  panel->dev, bl,
-						  &dp_aux_bl_ops, &props);
-	if (IS_ERR(bl->base))
-		return PTR_ERR(bl->base);
-
-	panel->backlight = bl->base;
-
-	return 0;
-}
-EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
 #endif

 MODULE_AUTHOR("Thierry Reding <treding@xxxxxxxxxx>");
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 729d5d82475e..a1b2d945def6 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -30,6 +30,7 @@

 struct drm_device;
 struct drm_dp_aux;
+struct drm_panel;

 /*
  * Unless otherwise noted, all values are from the DP 1.1a spec.  Note 
that
@@ -2200,6 +2201,21 @@ int drm_edp_backlight_enable(struct drm_dp_aux
*aux, const struct drm_edp_backli
 			     u16 level);
 int drm_edp_backlight_disable(struct drm_dp_aux *aux, const struct
drm_edp_backlight_info *bl);

+#if IS_ENABLED(CONFIG_DRM_KMS_HELPER) &&
(IS_BUILTIN(CONFIG_BACKLIGHT_CLASS_DEVICE) || \
+	(IS_MODULE(CONFIG_DRM_KMS_HELPER) &&
IS_MODULE(CONFIG_BACKLIGHT_CLASS_DEVICE)))
+
+int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct
drm_dp_aux *aux);
+
+#else
+
+static inline int drm_panel_dp_aux_backlight(struct drm_panel *panel,
+					     struct drm_dp_aux *aux)
+{
+	return 0;
+}
+
+#endif
+
 #ifdef CONFIG_DRM_DP_CEC
 void drm_dp_cec_irq(struct drm_dp_aux *aux);
 void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 71aac751a032..4602f833eb51 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -32,7 +32,6 @@ struct backlight_device;
 struct device_node;
 struct drm_connector;
 struct drm_device;
-struct drm_dp_aux;
 struct drm_panel;
 struct display_timing;

@@ -209,18 +208,11 @@ static inline int
of_drm_get_panel_orientation(const struct device_node *np,
 #if IS_ENABLED(CONFIG_DRM_PANEL) &&
(IS_BUILTIN(CONFIG_BACKLIGHT_CLASS_DEVICE) || \
 	(IS_MODULE(CONFIG_DRM) && IS_MODULE(CONFIG_BACKLIGHT_CLASS_DEVICE)))
 int drm_panel_of_backlight(struct drm_panel *panel);
-int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct
drm_dp_aux *aux);
 #else
 static inline int drm_panel_of_backlight(struct drm_panel *panel)
 {
 	return 0;
 }
-
-static inline int drm_panel_dp_aux_backlight(struct drm_panel *panel,
-					     struct drm_dp_aux *aux)
-{
-	return 0;
-}
 #endif

 #endif






