[PATCH v2 0/3] drm/panel-edp: Debugfs for panel-edp

[PATCH v2 0/3] drm/panel-edp: Debugfs for panel-edp

[Douglas Anderson]

The main goal of this series is the final patch in the series: to
allow test code to reliably find out if we ended up hitting the
"fallback" case of the generic edp-panel driver where we don't
recognize a panel and choose to use super conservative timing.

Version 1 of the patch actually landed but was quickly reverted since
it was pointed out that it should have been done in debugfs, not
sysfs.

As discussed on IRC [1], we want this support to be under the
"connector" directory of debugfs but there was no existing way to do
that. Thus patch #2 in the series was born to try to plumb this
through. It was asserted that it would be OK to rely on a fairly
modern display pipeline for this plumbing and perhaps fail to populate
the debugfs file if we're using older/deprecated pipelines.

Patch #1 in the series was born because the bridge chip I was using
was still using an older/deprecated pipeline. While this doesn't get
us fully to a modern pipeline for ti-sn65dsi86 (it still doesn't move
to "NO_CONNECTOR") it hopefully moves us in the right direction.

This was tested on sc7180-trogdor devices with _both_ the ti-sn65dsi86
and the parade-ps8640 bridge chips (since some devices have one, some
the other). The parade-ps8640 already uses supports "NO_CONNECTOR",
luckily.

[1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2022-02-02

Changes in v2:
- ("ti-sn65dsi86: Use drm_bridge_connector") new for v2.
- ("drm: Plumb debugfs_init through to panels") new for v2.
- Now using debugfs, not sysfs

Douglas Anderson (3):
  drm/bridge: ti-sn65dsi86: Use drm_bridge_connector
  drm: Plumb debugfs_init through to panels
  drm/panel-edp: Allow querying the detected panel via debugfs

 drivers/gpu/drm/bridge/panel.c         | 12 +++++
 drivers/gpu/drm/bridge/ti-sn65dsi86.c  | 72 +++++---------------------
 drivers/gpu/drm/drm_bridge_connector.c | 15 ++++++
 drivers/gpu/drm/drm_debugfs.c          |  3 ++
 drivers/gpu/drm/panel/panel-edp.c      | 37 +++++++++++--
 include/drm/drm_bridge.h               |  7 +++
 include/drm/drm_connector.h            |  7 +++
 include/drm/drm_panel.h                |  8 +++
 8 files changed, 98 insertions(+), 63 deletions(-)

-- 
2.35.0.263.gb82422642f-goog

[PATCH v2 1/3] drm/bridge: ti-sn65dsi86: Use drm_bridge_connector

[Douglas Anderson]

The ti-sn65dsi86 driver shouldn't hand-roll its own bridge
connector. It should use the normal drm_bridge_connector. Let's switch
to do that, removing all of the custom code.

NOTE: this still _doesn't_ implement DRM_BRIDGE_ATTACH_NO_CONNECTOR
support for ti-sn65dsi86 and that would still be a useful thing to do
in the future. It was attempted in the past [1] but put on the back
burner. However, unless we instantly change ti-sn65dsi86 fully from
not supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR at all to _only_
supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR then we'll still need a bit
of time when we support both. This is a better way to support the old
way where the driver hand rolls things itself.

A new notes about the implementation here:
* When using the drm_bridge_connector the connector should be created
  after all the bridges, so we change the ordering a bit.
* I'm reasonably certain that we don't need to do anything to "free"
  the new drm_bridge_connector. If drm_bridge_connector_init() returns
  success then we know drm_connector_init() was called with the
  `drm_bridge_connector_funcs`. The `drm_bridge_connector_funcs` has a
  .destroy() that does all the cleanup. drm_connector_init() calls
  __drm_mode_object_add() with a drm_connector_free() that will call
  the .destroy().
* I'm also reasonably certain that I don't need to "undo" the
  drm_bridge_attach() if drm_bridge_connector_init() fails. The
  "detach" function is private and other similar code doesn't try to
  undo the drm_bridge_attach() in error cases. There's also a comment
  indicating the lack of balance at the top of drm_bridge_attach().

[1] https://lore.kernel.org/r/20210920225801.227211-4-robdclark@gmail.com

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- ("ti-sn65dsi86: Use drm_bridge_connector") new for v2.

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 72 ++++++---------------------
 1 file changed, 14 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ba136a188be7..38616aab12ac 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -26,6 +26,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/dp/drm_dp_aux_bus.h>
 #include <drm/dp/drm_dp_helper.h>
 #include <drm/drm_mipi_dsi.h>
@@ -174,7 +175,7 @@ struct ti_sn65dsi86 {
 	struct regmap			*regmap;
 	struct drm_dp_aux		aux;
 	struct drm_bridge		bridge;
-	struct drm_connector		connector;
+	struct drm_connector		*connector;
 	struct device_node		*host_node;
 	struct mipi_dsi_device		*dsi;
 	struct clk			*refclk;
@@ -646,54 +647,6 @@ static struct auxiliary_driver ti_sn_aux_driver = {
 	.id_table = ti_sn_aux_id_table,
 };
 
-/* -----------------------------------------------------------------------------
- * DRM Connector Operations
- */
-
-static struct ti_sn65dsi86 *
-connector_to_ti_sn65dsi86(struct drm_connector *connector)
-{
-	return container_of(connector, struct ti_sn65dsi86, connector);
-}
-
-static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
-{
-	struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector);
-
-	return drm_bridge_get_modes(pdata->next_bridge, connector);
-}
-
-static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
-	.get_modes = ti_sn_bridge_connector_get_modes,
-};
-
-static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static int ti_sn_bridge_connector_init(struct ti_sn65dsi86 *pdata)
-{
-	int ret;
-
-	ret = drm_connector_init(pdata->bridge.dev, &pdata->connector,
-				 &ti_sn_bridge_connector_funcs,
-				 DRM_MODE_CONNECTOR_eDP);
-	if (ret) {
-		DRM_ERROR("Failed to initialize connector with drm\n");
-		return ret;
-	}
-
-	drm_connector_helper_add(&pdata->connector,
-				 &ti_sn_bridge_connector_helper_funcs);
-	drm_connector_attach_encoder(&pdata->connector, pdata->bridge.encoder);
-
-	return 0;
-}
-
 /*------------------------------------------------------------------------------
  * DRM Bridge
  */
@@ -757,10 +710,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 		return ret;
 	}
 
-	ret = ti_sn_bridge_connector_init(pdata);
-	if (ret < 0)
-		goto err_conn_init;
-
 	/* We never want the next bridge to *also* create a connector: */
 	flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
 
@@ -768,13 +717,20 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
 				&pdata->bridge, flags);
 	if (ret < 0)
-		goto err_dsi_host;
+		goto err_initted_aux;
+
+	pdata->connector = drm_bridge_connector_init(pdata->bridge.dev,
+						     pdata->bridge.encoder);
+	if (IS_ERR(pdata->connector)) {
+		ret = PTR_ERR(pdata->connector);
+		goto err_initted_aux;
+	}
+
+	drm_connector_attach_encoder(pdata->connector, pdata->bridge.encoder);
 
 	return 0;
 
-err_dsi_host:
-	drm_connector_cleanup(&pdata->connector);
-err_conn_init:
+err_initted_aux:
 	drm_dp_aux_unregister(&pdata->aux);
 	return ret;
 }
@@ -824,7 +780,7 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
 
 static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
 {
-	if (pdata->connector.display_info.bpc <= 6)
+	if (pdata->connector->display_info.bpc <= 6)
 		return 18;
 	else
 		return 24;
-- 
2.35.0.263.gb82422642f-goog

[PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Douglas Anderson]

We'd like panels to be able to add things to debugfs underneath the
connector's directory. Let's plumb it through. A panel will be able to
put things in a "panel" directory under the connector's
directory. Note that debugfs is not ABI and so it's always possible
that the location that the panel gets for its debugfs could change in
the future.

NOTE: this currently only works if you're using a modern
architecture. Specifically the plumbing relies on _both_
drm_bridge_connector and drm_panel_bridge. If you're not using one or
both of these things then things won't be plumbed through.

As a side effect of this change, drm_bridges can also get callbacks to
put stuff underneath the connector's debugfs directory. At the moment
all bridges in the chain have their debugfs_init() called with the
connector's root directory.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- ("drm: Plumb debugfs_init through to panels") new for v2.

 drivers/gpu/drm/bridge/panel.c         | 12 ++++++++++++
 drivers/gpu/drm/drm_bridge_connector.c | 15 +++++++++++++++
 drivers/gpu/drm/drm_debugfs.c          |  3 +++
 include/drm/drm_bridge.h               |  7 +++++++
 include/drm/drm_connector.h            |  7 +++++++
 include/drm/drm_panel.h                |  8 ++++++++
 6 files changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index b32295abd9e7..5be057575183 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -138,6 +138,17 @@ static int panel_bridge_get_modes(struct drm_bridge *bridge,
 	return drm_panel_get_modes(panel_bridge->panel, connector);
 }
 
+static void panel_bridge_debugfs_init(struct drm_bridge *bridge,
+				      struct dentry *root)
+{
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+	struct drm_panel *panel = panel_bridge->panel;
+
+	root = debugfs_create_dir("panel", root);
+	if (panel->funcs->debugfs_init)
+		panel->funcs->debugfs_init(panel, root);
+}
+
 static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
 	.attach = panel_bridge_attach,
 	.detach = panel_bridge_detach,
@@ -150,6 +161,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
+	.debugfs_init = panel_bridge_debugfs_init,
 };
 
 /**
diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index 791379816837..60923cdfe8e1 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -216,6 +216,20 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector)
 	kfree(bridge_connector);
 }
 
+static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
+					      struct dentry *root)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_encoder *encoder = bridge_connector->encoder;
+	struct drm_bridge *bridge;
+
+	list_for_each_entry(bridge, &encoder->bridge_chain, chain_node) {
+		if (bridge->funcs->debugfs_init)
+			bridge->funcs->debugfs_init(bridge, root);
+	}
+}
+
 static const struct drm_connector_funcs drm_bridge_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.detect = drm_bridge_connector_detect,
@@ -223,6 +237,7 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
 	.destroy = drm_bridge_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.debugfs_init = drm_bridge_connector_debugfs_init,
 };
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index b0a826489488..7f1b82dbaebb 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -436,6 +436,9 @@ void drm_debugfs_connector_add(struct drm_connector *connector)
 	/* vrr range */
 	debugfs_create_file("vrr_range", S_IRUGO, root, connector,
 			    &vrr_range_fops);
+
+	if (connector->funcs->debugfs_init)
+		connector->funcs->debugfs_init(connector, root);
 }
 
 void drm_debugfs_connector_remove(struct drm_connector *connector)
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 061d87313fac..f27b4060faa2 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -649,6 +649,13 @@ struct drm_bridge_funcs {
 	 * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops.
 	 */
 	void (*hpd_disable)(struct drm_bridge *bridge);
+
+	/**
+	 * @debugfs_init:
+	 *
+	 * Allows bridges to create bridge-specific debugfs files.
+	 */
+	void (*debugfs_init)(struct drm_bridge *bridge, struct dentry *root);
 };
 
 /**
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 64cf5f88c05b..54429dde744a 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1142,6 +1142,13 @@ struct drm_connector_funcs {
 	 * has been received from a source outside the display driver / device.
 	 */
 	void (*oob_hotplug_event)(struct drm_connector *connector);
+
+	/**
+	 * @debugfs_init:
+	 *
+	 * Allows connectors to create connector-specific debugfs files.
+	 */
+	void (*debugfs_init)(struct drm_connector *connector, struct dentry *root);
 };
 
 /**
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 4602f833eb51..1ba2d424a53f 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -29,6 +29,7 @@
 #include <linux/list.h>
 
 struct backlight_device;
+struct dentry;
 struct device_node;
 struct drm_connector;
 struct drm_device;
@@ -125,6 +126,13 @@ struct drm_panel_funcs {
 	 */
 	int (*get_timings)(struct drm_panel *panel, unsigned int num_timings,
 			   struct display_timing *timings);
+
+	/**
+	 * @debugfs_init:
+	 *
+	 * Allows panels to create panels-specific debugfs files.
+	 */
+	void (*debugfs_init)(struct drm_panel *panel, struct dentry *root);
 };
 
 /**
-- 
2.35.0.263.gb82422642f-goog

[PATCH v2 3/3] drm/panel-edp: Allow querying the detected panel via debugfs

[Douglas Anderson]

Recently we added generic "edp-panel"s probed by EDID. To support
panels in this way we look at the panel ID in the EDID and look up the
panel in a table that has power sequence timings. If we find a panel
that's not in the table we will still attempt to use it but we'll use
conservative timings. While it's likely that these conservative
timings will work for most nearly all panels, the performance of
turning the panel off and on suffers.

We'd like to be able to reliably detect the case that we're using the
hardcoded timings without relying on parsing dmesg. This allows us to
implement tests that ensure that no devices get shipped that are
relying on the conservative timings.

Let's add a new debugfs entry to panel devices. It will have one of:
* UNKNOWN - We tried to detect a panel but it wasn't in our table.
* HARDCODED - We're not using generic "edp-panel" probed by EDID.
* A panel name - This is the name of the panel from our table.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Now using debugfs, not sysfs

 drivers/gpu/drm/panel/panel-edp.c | 37 ++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index a394a15dc3fb..0fda1eb7b690 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -21,6 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/iopoll.h>
@@ -222,6 +223,8 @@ struct panel_edp {
 	struct gpio_desc *enable_gpio;
 	struct gpio_desc *hpd_gpio;
 
+	const struct edp_panel_entry *detected_panel;
+
 	struct edid *edid;
 
 	struct drm_display_mode override_mode;
@@ -606,6 +609,28 @@ static int panel_edp_get_timings(struct drm_panel *panel,
 	return p->desc->num_timings;
 }
 
+static int detected_panel_show(struct seq_file *s, void *data)
+{
+	struct drm_panel *panel = s->private;
+	struct panel_edp *p = to_panel_edp(panel);
+
+	if (IS_ERR(p->detected_panel))
+		seq_puts(s, "UNKNOWN\n");
+	else if (!p->detected_panel)
+		seq_puts(s, "HARDCODED\n");
+	else
+		seq_printf(s, "%s\n", p->detected_panel->name);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(detected_panel);
+
+static void panel_edp_debugfs_init(struct drm_panel *panel, struct dentry *root)
+{
+	debugfs_create_file("detected_panel", 0600, root, panel, &detected_panel_fops);
+}
+
 static const struct drm_panel_funcs panel_edp_funcs = {
 	.disable = panel_edp_disable,
 	.unprepare = panel_edp_unprepare,
@@ -613,6 +638,7 @@ static const struct drm_panel_funcs panel_edp_funcs = {
 	.enable = panel_edp_enable,
 	.get_modes = panel_edp_get_modes,
 	.get_timings = panel_edp_get_timings,
+	.debugfs_init = panel_edp_debugfs_init,
 };
 
 #define PANEL_EDP_BOUNDS_CHECK(to_check, bounds, field) \
@@ -666,7 +692,6 @@ static const struct edp_panel_entry *find_edp_panel(u32 panel_id);
 
 static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
 {
-	const struct edp_panel_entry *edp_panel;
 	struct panel_desc *desc;
 	u32 panel_id;
 	char vend[4];
@@ -705,14 +730,14 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
 	}
 	drm_edid_decode_panel_id(panel_id, vend, &product_id);
 
-	edp_panel = find_edp_panel(panel_id);
+	panel->detected_panel = find_edp_panel(panel_id);
 
 	/*
 	 * We're using non-optimized timings and want it really obvious that
 	 * someone needs to add an entry to the table, so we'll do a WARN_ON
 	 * splat.
 	 */
-	if (WARN_ON(!edp_panel)) {
+	if (WARN_ON(!panel->detected_panel)) {
 		dev_warn(dev,
 			 "Unknown panel %s %#06x, using conservative timings\n",
 			 vend, product_id);
@@ -734,12 +759,14 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
 		 */
 		desc->delay.unprepare = 2000;
 		desc->delay.enable = 200;
+
+		panel->detected_panel = ERR_PTR(-EINVAL);
 	} else {
 		dev_info(dev, "Detected %s %s (%#06x)\n",
-			 vend, edp_panel->name, product_id);
+			 vend, panel->detected_panel->name, product_id);
 
 		/* Update the delay; everything else comes from EDID */
-		desc->delay = *edp_panel->delay;
+		desc->delay = *panel->detected_panel->delay;
 	}
 
 	ret = 0;
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH v2 1/3] drm/bridge: ti-sn65dsi86: Use drm_bridge_connector

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Feb 04, 2022 at 04:13:40PM -0800, Douglas Anderson wrote:
> The ti-sn65dsi86 driver shouldn't hand-roll its own bridge
> connector. It should use the normal drm_bridge_connector. Let's switch
> to do that, removing all of the custom code.
> 
> NOTE: this still _doesn't_ implement DRM_BRIDGE_ATTACH_NO_CONNECTOR
> support for ti-sn65dsi86 and that would still be a useful thing to do
> in the future. It was attempted in the past [1] but put on the back
> burner. However, unless we instantly change ti-sn65dsi86 fully from
> not supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR at all to _only_
> supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR then we'll still need a bit
> of time when we support both. This is a better way to support the old
> way where the driver hand rolls things itself.
> 
> A new notes about the implementation here:
> * When using the drm_bridge_connector the connector should be created
>   after all the bridges, so we change the ordering a bit.
> * I'm reasonably certain that we don't need to do anything to "free"
>   the new drm_bridge_connector. If drm_bridge_connector_init() returns
>   success then we know drm_connector_init() was called with the
>   `drm_bridge_connector_funcs`. The `drm_bridge_connector_funcs` has a
>   .destroy() that does all the cleanup. drm_connector_init() calls
>   __drm_mode_object_add() with a drm_connector_free() that will call
>   the .destroy().
> * I'm also reasonably certain that I don't need to "undo" the
>   drm_bridge_attach() if drm_bridge_connector_init() fails. The
>   "detach" function is private and other similar code doesn't try to
>   undo the drm_bridge_attach() in error cases. There's also a comment
>   indicating the lack of balance at the top of drm_bridge_attach().
> 
> [1] https://lore.kernel.org/r/20210920225801.227211-4-robdclark@gmail.com
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> Changes in v2:
> - ("ti-sn65dsi86: Use drm_bridge_connector") new for v2.
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 72 ++++++---------------------
>  1 file changed, 14 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ba136a188be7..38616aab12ac 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -26,6 +26,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/dp/drm_dp_aux_bus.h>
>  #include <drm/dp/drm_dp_helper.h>
>  #include <drm/drm_mipi_dsi.h>
> @@ -174,7 +175,7 @@ struct ti_sn65dsi86 {
>  	struct regmap			*regmap;
>  	struct drm_dp_aux		aux;
>  	struct drm_bridge		bridge;
> -	struct drm_connector		connector;
> +	struct drm_connector		*connector;
>  	struct device_node		*host_node;
>  	struct mipi_dsi_device		*dsi;
>  	struct clk			*refclk;
> @@ -646,54 +647,6 @@ static struct auxiliary_driver ti_sn_aux_driver = {
>  	.id_table = ti_sn_aux_id_table,
>  };
>  
> -/* -----------------------------------------------------------------------------
> - * DRM Connector Operations
> - */
> -
> -static struct ti_sn65dsi86 *
> -connector_to_ti_sn65dsi86(struct drm_connector *connector)
> -{
> -	return container_of(connector, struct ti_sn65dsi86, connector);
> -}
> -
> -static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> -{
> -	struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector);
> -
> -	return drm_bridge_get_modes(pdata->next_bridge, connector);
> -}
> -
> -static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
> -	.get_modes = ti_sn_bridge_connector_get_modes,
> -};
> -
> -static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_connector_cleanup,
> -	.reset = drm_atomic_helper_connector_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
> -static int ti_sn_bridge_connector_init(struct ti_sn65dsi86 *pdata)
> -{
> -	int ret;
> -
> -	ret = drm_connector_init(pdata->bridge.dev, &pdata->connector,
> -				 &ti_sn_bridge_connector_funcs,
> -				 DRM_MODE_CONNECTOR_eDP);
> -	if (ret) {
> -		DRM_ERROR("Failed to initialize connector with drm\n");
> -		return ret;
> -	}
> -
> -	drm_connector_helper_add(&pdata->connector,
> -				 &ti_sn_bridge_connector_helper_funcs);
> -	drm_connector_attach_encoder(&pdata->connector, pdata->bridge.encoder);
> -
> -	return 0;
> -}
> -
>  /*------------------------------------------------------------------------------
>   * DRM Bridge
>   */
> @@ -757,10 +710,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>  		return ret;
>  	}
>  
> -	ret = ti_sn_bridge_connector_init(pdata);
> -	if (ret < 0)
> -		goto err_conn_init;
> -
>  	/* We never want the next bridge to *also* create a connector: */
>  	flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
>  
> @@ -768,13 +717,20 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>  	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
>  				&pdata->bridge, flags);
>  	if (ret < 0)
> -		goto err_dsi_host;
> +		goto err_initted_aux;
> +
> +	pdata->connector = drm_bridge_connector_init(pdata->bridge.dev,
> +						     pdata->bridge.encoder);
> +	if (IS_ERR(pdata->connector)) {
> +		ret = PTR_ERR(pdata->connector);
> +		goto err_initted_aux;
> +	}
> +
> +	drm_connector_attach_encoder(pdata->connector, pdata->bridge.encoder);
>  
>  	return 0;
>  
> -err_dsi_host:
> -	drm_connector_cleanup(&pdata->connector);
> -err_conn_init:
> +err_initted_aux:
>  	drm_dp_aux_unregister(&pdata->aux);
>  	return ret;
>  }
> @@ -824,7 +780,7 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
>  
>  static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
>  {
> -	if (pdata->connector.display_info.bpc <= 6)
> +	if (pdata->connector->display_info.bpc <= 6)
>  		return 18;
>  	else
>  		return 24;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Laurent Pinchart]

Hi Douglas,

Thank you for the patch.

On Fri, Feb 04, 2022 at 04:13:41PM -0800, Douglas Anderson wrote:
> We'd like panels to be able to add things to debugfs underneath the
> connector's directory. Let's plumb it through. A panel will be able to
> put things in a "panel" directory under the connector's
> directory. Note that debugfs is not ABI and so it's always possible
> that the location that the panel gets for its debugfs could change in
> the future.
> 
> NOTE: this currently only works if you're using a modern
> architecture. Specifically the plumbing relies on _both_
> drm_bridge_connector and drm_panel_bridge. If you're not using one or
> both of these things then things won't be plumbed through.
> 
> As a side effect of this change, drm_bridges can also get callbacks to
> put stuff underneath the connector's debugfs directory. At the moment
> all bridges in the chain have their debugfs_init() called with the
> connector's root directory.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> Changes in v2:
> - ("drm: Plumb debugfs_init through to panels") new for v2.
> 
>  drivers/gpu/drm/bridge/panel.c         | 12 ++++++++++++
>  drivers/gpu/drm/drm_bridge_connector.c | 15 +++++++++++++++
>  drivers/gpu/drm/drm_debugfs.c          |  3 +++
>  include/drm/drm_bridge.h               |  7 +++++++
>  include/drm/drm_connector.h            |  7 +++++++
>  include/drm/drm_panel.h                |  8 ++++++++
>  6 files changed, 52 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index b32295abd9e7..5be057575183 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -138,6 +138,17 @@ static int panel_bridge_get_modes(struct drm_bridge *bridge,
>  	return drm_panel_get_modes(panel_bridge->panel, connector);
>  }
>  
> +static void panel_bridge_debugfs_init(struct drm_bridge *bridge,
> +				      struct dentry *root)
> +{
> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> +	struct drm_panel *panel = panel_bridge->panel;
> +
> +	root = debugfs_create_dir("panel", root);
> +	if (panel->funcs->debugfs_init)
> +		panel->funcs->debugfs_init(panel, root);
> +}
> +
>  static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
>  	.attach = panel_bridge_attach,
>  	.detach = panel_bridge_detach,
> @@ -150,6 +161,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  	.atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
> +	.debugfs_init = panel_bridge_debugfs_init,
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> index 791379816837..60923cdfe8e1 100644
> --- a/drivers/gpu/drm/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -216,6 +216,20 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector)
>  	kfree(bridge_connector);
>  }
>  
> +static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
> +					      struct dentry *root)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_encoder *encoder = bridge_connector->encoder;
> +	struct drm_bridge *bridge;
> +
> +	list_for_each_entry(bridge, &encoder->bridge_chain, chain_node) {
> +		if (bridge->funcs->debugfs_init)
> +			bridge->funcs->debugfs_init(bridge, root);
> +	}
> +}
> +
>  static const struct drm_connector_funcs drm_bridge_connector_funcs = {
>  	.reset = drm_atomic_helper_connector_reset,
>  	.detect = drm_bridge_connector_detect,
> @@ -223,6 +237,7 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
>  	.destroy = drm_bridge_connector_destroy,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +	.debugfs_init = drm_bridge_connector_debugfs_init,
>  };
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index b0a826489488..7f1b82dbaebb 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -436,6 +436,9 @@ void drm_debugfs_connector_add(struct drm_connector *connector)
>  	/* vrr range */
>  	debugfs_create_file("vrr_range", S_IRUGO, root, connector,
>  			    &vrr_range_fops);
> +
> +	if (connector->funcs->debugfs_init)
> +		connector->funcs->debugfs_init(connector, root);
>  }
>  
>  void drm_debugfs_connector_remove(struct drm_connector *connector)
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 061d87313fac..f27b4060faa2 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -649,6 +649,13 @@ struct drm_bridge_funcs {
>  	 * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops.
>  	 */
>  	void (*hpd_disable)(struct drm_bridge *bridge);
> +
> +	/**
> +	 * @debugfs_init:
> +	 *
> +	 * Allows bridges to create bridge-specific debugfs files.
> +	 */
> +	void (*debugfs_init)(struct drm_bridge *bridge, struct dentry *root);
>  };
>  
>  /**
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 64cf5f88c05b..54429dde744a 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1142,6 +1142,13 @@ struct drm_connector_funcs {
>  	 * has been received from a source outside the display driver / device.
>  	 */
>  	void (*oob_hotplug_event)(struct drm_connector *connector);
> +
> +	/**
> +	 * @debugfs_init:
> +	 *
> +	 * Allows connectors to create connector-specific debugfs files.
> +	 */
> +	void (*debugfs_init)(struct drm_connector *connector, struct dentry *root);
>  };
>  
>  /**
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 4602f833eb51..1ba2d424a53f 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -29,6 +29,7 @@
>  #include <linux/list.h>
>  
>  struct backlight_device;
> +struct dentry;
>  struct device_node;
>  struct drm_connector;
>  struct drm_device;
> @@ -125,6 +126,13 @@ struct drm_panel_funcs {
>  	 */
>  	int (*get_timings)(struct drm_panel *panel, unsigned int num_timings,
>  			   struct display_timing *timings);
> +
> +	/**
> +	 * @debugfs_init:
> +	 *
> +	 * Allows panels to create panels-specific debugfs files.
> +	 */
> +	void (*debugfs_init)(struct drm_panel *panel, struct dentry *root);
>  };
>  
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Javier Martinez Canillas]

Hello Doug,

On 2/5/22 01:13, Douglas Anderson wrote:

[snip]

> +static void panel_bridge_debugfs_init(struct drm_bridge *bridge,
> +				      struct dentry *root)
> +{
> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> +	struct drm_panel *panel = panel_bridge->panel;
> +
> +	root = debugfs_create_dir("panel", root);

This could return a ERR_PTR(-errno) if the function doesn't succeed.

I noticed that most kernel code doesn't check the return value though...

> +	if (panel->funcs->debugfs_init)

Probably if (!(IS_ERR(root) && panel->funcs->debugfs_init) ?

> +		panel->funcs->debugfs_init(panel, root);
> +}

[snip]

> @@ -436,6 +436,9 @@ void drm_debugfs_connector_add(struct drm_connector *connector)
>  	/* vrr range */
>  	debugfs_create_file("vrr_range", S_IRUGO, root, connector,
>  			    &vrr_range_fops);

Same here, wonder if the return value should be checked.

I leave it to you to decide, but regardless of that the patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 3/3] drm/panel-edp: Allow querying the detected panel via debugfs

[Javier Martinez Canillas]

On 2/5/22 01:13, Douglas Anderson wrote:
> Recently we added generic "edp-panel"s probed by EDID. To support
> panels in this way we look at the panel ID in the EDID and look up the
> panel in a table that has power sequence timings. If we find a panel
> that's not in the table we will still attempt to use it but we'll use
> conservative timings. While it's likely that these conservative
> timings will work for most nearly all panels, the performance of
> turning the panel off and on suffers.
> 
> We'd like to be able to reliably detect the case that we're using the
> hardcoded timings without relying on parsing dmesg. This allows us to
> implement tests that ensure that no devices get shipped that are
> relying on the conservative timings.
> 
> Let's add a new debugfs entry to panel devices. It will have one of:
> * UNKNOWN - We tried to detect a panel but it wasn't in our table.
> * HARDCODED - We're not using generic "edp-panel" probed by EDID.
> * A panel name - This is the name of the panel from our table.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Andrzej Hajda]

On 15.02.2022 23:09, Javier Martinez Canillas wrote:
> Hello Doug,
>
> On 2/5/22 01:13, Douglas Anderson wrote:
>
> [snip]
>
>> +static void panel_bridge_debugfs_init(struct drm_bridge *bridge,
>> +				      struct dentry *root)
>> +{
>> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>> +	struct drm_panel *panel = panel_bridge->panel;
>> +
>> +	root = debugfs_create_dir("panel", root);
> This could return a ERR_PTR(-errno) if the function doesn't succeed.
>
> I noticed that most kernel code doesn't check the return value though...
>
>> +	if (panel->funcs->debugfs_init)
> Probably if (!(IS_ERR(root) && panel->funcs->debugfs_init) ?
>
>> +		panel->funcs->debugfs_init(panel, root);
>> +}
> [snip]
>
>> @@ -436,6 +436,9 @@ void drm_debugfs_connector_add(struct drm_connector *connector)
>>   	/* vrr range */
>>   	debugfs_create_file("vrr_range", S_IRUGO, root, connector,
>>   			    &vrr_range_fops);
> Same here, wonder if the return value should be checked.

I've seen sometimes that file/dir was already created with the same 
name, reporting error in such case will be helpful.

Regards
Andrzej

>
> I leave it to you to decide, but regardless of that the patch looks good to me.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>
> Best regards,

Re: [PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Doug Anderson]

Hi,

On Tue, Feb 15, 2022 at 2:20 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>
> On 15.02.2022 23:09, Javier Martinez Canillas wrote:
> > Hello Doug,
> >
> > On 2/5/22 01:13, Douglas Anderson wrote:
> >
> > [snip]
> >
> >> +static void panel_bridge_debugfs_init(struct drm_bridge *bridge,
> >> +                                  struct dentry *root)
> >> +{
> >> +    struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> >> +    struct drm_panel *panel = panel_bridge->panel;
> >> +
> >> +    root = debugfs_create_dir("panel", root);
> > This could return a ERR_PTR(-errno) if the function doesn't succeed.
> >
> > I noticed that most kernel code doesn't check the return value though...
> >
> >> +    if (panel->funcs->debugfs_init)
> > Probably if (!(IS_ERR(root) && panel->funcs->debugfs_init) ?
> >
> >> +            panel->funcs->debugfs_init(panel, root);
> >> +}
> > [snip]
> >
> >> @@ -436,6 +436,9 @@ void drm_debugfs_connector_add(struct drm_connector *connector)
> >>      /* vrr range */
> >>      debugfs_create_file("vrr_range", S_IRUGO, root, connector,
> >>                          &vrr_range_fops);
> > Same here, wonder if the return value should be checked.

My plan (confirmed with Javier over IRC) is to land my patches and we
can address as needed with follow-up patches.

I actually wrote said follow-up patches and they were ready to go, but
when I was trying to come up with the right "Fixes" tag I found commit
b792e64021ec ("drm: no need to check return value of debugfs_create
functions"). So what's being requested is nearly the opposite of what
Greg did there.

I thought about perhaps only checking for directories but even that
type of check was removed by Greg's patch. Further checking shows that
start_creating() actually has:

if (IS_ERR(parent))
  return parent;

...so I guess that explains why it's fine to skip the check even for parents?

Sure enough I confirmed that if I pass `ERR_PTR(-EINVAL)` as the root
for `panel->funcs->debugfs_init()` that nothing bad seems to happen...


> I've seen sometimes that file/dir was already created with the same
> name, reporting error in such case will be helpful.

It sure looks like start_creating() already handles that type of
reporting... Sure enough, I tried to create the "force" file twice,
adding no error checking myself, and I see:

debugfs: File 'force' in directory 'eDP-1' already present!
debugfs: File 'force' in directory 'DP-1' already present!


So tl;dr is that I'm going to land the patches and now am _not_
planning on doing followup patches. However, if I'm confused about any
of the above then please let me know and I'll dig more / can send
follow-up patches.

-Doug

Re: [PATCH v2 0/3] drm/panel-edp: Debugfs for panel-edp

[Doug Anderson]

Hi,

On Fri, Feb 4, 2022 at 4:14 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> The main goal of this series is the final patch in the series: to
> allow test code to reliably find out if we ended up hitting the
> "fallback" case of the generic edp-panel driver where we don't
> recognize a panel and choose to use super conservative timing.
>
> Version 1 of the patch actually landed but was quickly reverted since
> it was pointed out that it should have been done in debugfs, not
> sysfs.
>
> As discussed on IRC [1], we want this support to be under the
> "connector" directory of debugfs but there was no existing way to do
> that. Thus patch #2 in the series was born to try to plumb this
> through. It was asserted that it would be OK to rely on a fairly
> modern display pipeline for this plumbing and perhaps fail to populate
> the debugfs file if we're using older/deprecated pipelines.
>
> Patch #1 in the series was born because the bridge chip I was using
> was still using an older/deprecated pipeline. While this doesn't get
> us fully to a modern pipeline for ti-sn65dsi86 (it still doesn't move
> to "NO_CONNECTOR") it hopefully moves us in the right direction.
>
> This was tested on sc7180-trogdor devices with _both_ the ti-sn65dsi86
> and the parade-ps8640 bridge chips (since some devices have one, some
> the other). The parade-ps8640 already uses supports "NO_CONNECTOR",
> luckily.
>
> [1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2022-02-02
>
> Changes in v2:
> - ("ti-sn65dsi86: Use drm_bridge_connector") new for v2.
> - ("drm: Plumb debugfs_init through to panels") new for v2.
> - Now using debugfs, not sysfs
>
> Douglas Anderson (3):
>   drm/bridge: ti-sn65dsi86: Use drm_bridge_connector
>   drm: Plumb debugfs_init through to panels
>   drm/panel-edp: Allow querying the detected panel via debugfs
>
>  drivers/gpu/drm/bridge/panel.c         | 12 +++++
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c  | 72 +++++---------------------
>  drivers/gpu/drm/drm_bridge_connector.c | 15 ++++++
>  drivers/gpu/drm/drm_debugfs.c          |  3 ++
>  drivers/gpu/drm/panel/panel-edp.c      | 37 +++++++++++--
>  include/drm/drm_bridge.h               |  7 +++
>  include/drm/drm_connector.h            |  7 +++
>  include/drm/drm_panel.h                |  8 +++
>  8 files changed, 98 insertions(+), 63 deletions(-)

Landed these three patches to drm-misc-next w/ accumulated tags:

$ git log --oneline -3
6ed19359d6bd drm/panel-edp: Allow querying the detected panel via debugfs
2509969a9862 drm: Plumb debugfs_init through to panels
e283820cbf80 drm/bridge: ti-sn65dsi86: Use drm_bridge_connector

If it turns out that we want to add more reporting when debugfs calls
return errors then I'm happy to submit follow-on patches. Discussion
about that can be found in:

https://lore.kernel.org/r/CAD=FV=Ut3N9syXbN7i939mNsx3h7-u9cU9j6=XFkz9vrh0Vseg@mail.gmail.com

Unless something changes, though, my current plan is not to do
follow-up patches and leave this as-is without any extra error
reporting.

-Doug

Re: [PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Jani Nikula]

On Tue, 15 Feb 2022, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Tue, Feb 15, 2022 at 2:20 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>>
>> On 15.02.2022 23:09, Javier Martinez Canillas wrote:
>> > Hello Doug,
>> >
>> > On 2/5/22 01:13, Douglas Anderson wrote:
>> >
>> > [snip]
>> >
>> >> +static void panel_bridge_debugfs_init(struct drm_bridge *bridge,
>> >> +                                  struct dentry *root)
>> >> +{
>> >> +    struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>> >> +    struct drm_panel *panel = panel_bridge->panel;
>> >> +
>> >> +    root = debugfs_create_dir("panel", root);
>> > This could return a ERR_PTR(-errno) if the function doesn't succeed.
>> >
>> > I noticed that most kernel code doesn't check the return value though...
>> >
>> >> +    if (panel->funcs->debugfs_init)
>> > Probably if (!(IS_ERR(root) && panel->funcs->debugfs_init) ?
>> >
>> >> +            panel->funcs->debugfs_init(panel, root);
>> >> +}
>> > [snip]
>> >
>> >> @@ -436,6 +436,9 @@ void drm_debugfs_connector_add(struct drm_connector *connector)
>> >>      /* vrr range */
>> >>      debugfs_create_file("vrr_range", S_IRUGO, root, connector,
>> >>                          &vrr_range_fops);
>> > Same here, wonder if the return value should be checked.
>
> My plan (confirmed with Javier over IRC) is to land my patches and we
> can address as needed with follow-up patches.
>
> I actually wrote said follow-up patches and they were ready to go, but
> when I was trying to come up with the right "Fixes" tag I found commit
> b792e64021ec ("drm: no need to check return value of debugfs_create
> functions"). So what's being requested is nearly the opposite of what
> Greg did there.
>
> I thought about perhaps only checking for directories but even that
> type of check was removed by Greg's patch. Further checking shows that
> start_creating() actually has:
>
> if (IS_ERR(parent))
>   return parent;
>
> ...so I guess that explains why it's fine to skip the check even for parents?
>
> Sure enough I confirmed that if I pass `ERR_PTR(-EINVAL)` as the root
> for `panel->funcs->debugfs_init()` that nothing bad seems to happen...

Yeah, the idea is that you don't need to check for debugfs function
return values and you can safely pass error pointers to debugfs
functions. The worst that can happen is you don't get the debugfs, but
hey, it's debugfs so you shouldn't fail anything else because of that
anyway.

BR,
Jani.

>
>
>> I've seen sometimes that file/dir was already created with the same
>> name, reporting error in such case will be helpful.
>
> It sure looks like start_creating() already handles that type of
> reporting... Sure enough, I tried to create the "force" file twice,
> adding no error checking myself, and I see:
>
> debugfs: File 'force' in directory 'eDP-1' already present!
> debugfs: File 'force' in directory 'DP-1' already present!
>
>
> So tl;dr is that I'm going to land the patches and now am _not_
> planning on doing followup patches. However, if I'm confused about any
> of the above then please let me know and I'll dig more / can send
> follow-up patches.
>
> -Doug

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Javier Martinez Canillas]

On 2/16/22 10:25, Jani Nikula wrote:

[snip]

>>
>> I actually wrote said follow-up patches and they were ready to go, but
>> when I was trying to come up with the right "Fixes" tag I found commit
>> b792e64021ec ("drm: no need to check return value of debugfs_create
>> functions"). So what's being requested is nearly the opposite of what
>> Greg did there.
>>
>> I thought about perhaps only checking for directories but even that
>> type of check was removed by Greg's patch. Further checking shows that
>> start_creating() actually has:
>>
>> if (IS_ERR(parent))
>>   return parent;
>>
>> ...so I guess that explains why it's fine to skip the check even for parents?
>>
>> Sure enough I confirmed that if I pass `ERR_PTR(-EINVAL)` as the root
>> for `panel->funcs->debugfs_init()` that nothing bad seems to happen...
> 
> Yeah, the idea is that you don't need to check for debugfs function
> return values and you can safely pass error pointers to debugfs
> functions. The worst that can happen is you don't get the debugfs, but
> hey, it's debugfs so you shouldn't fail anything else because of that
> anyway.
> 

Thanks a lot Doug and Jani for the explanations. That makes sense and it
explains why most code I looked was not checking for the return value.

I guess we should write something about this in the debugfs functions
kernel doc so it's mentioned explicitly and people don't have to guess. 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Andrzej Hajda]

On 16.02.2022 10:35, Javier Martinez Canillas wrote:
> On 2/16/22 10:25, Jani Nikula wrote:
>
> [snip]
>
>>> I actually wrote said follow-up patches and they were ready to go, but
>>> when I was trying to come up with the right "Fixes" tag I found commit
>>> b792e64021ec ("drm: no need to check return value of debugfs_create
>>> functions"). So what's being requested is nearly the opposite of what
>>> Greg did there.
>>>
>>> I thought about perhaps only checking for directories but even that
>>> type of check was removed by Greg's patch. Further checking shows that
>>> start_creating() actually has:
>>>
>>> if (IS_ERR(parent))
>>>    return parent;

>>>
>>> ...so I guess that explains why it's fine to skip the check even for parents?
>>>
>>> Sure enough I confirmed that if I pass `ERR_PTR(-EINVAL)` as the root
>>> for `panel->funcs->debugfs_init()` that nothing bad seems to happen...
>> Yeah, the idea is that you don't need to check for debugfs function
>> return values and you can safely pass error pointers to debugfs
>> functions. The worst that can happen is you don't get the debugfs, but
>> hey, it's debugfs so you shouldn't fail anything else because of that
>> anyway.
>>
> Thanks a lot Doug and Jani for the explanations. That makes sense and it
> explains why most code I looked was not checking for the return value.
>
> I guess we should write something about this in the debugfs functions
> kernel doc so it's mentioned explicitly and people don't have to guess.

Nice, didn't know debugfs started using this pattern. I hope the pattern 
will be used broader, as it allows to save lot of redundant checks.

Regards
Andrzej


>
> Best regards,

Re: [PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Doug Anderson]

Hi,

On Wed, Feb 16, 2022 at 1:36 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> On 2/16/22 10:25, Jani Nikula wrote:
>
> [snip]
>
> >>
> >> I actually wrote said follow-up patches and they were ready to go, but
> >> when I was trying to come up with the right "Fixes" tag I found commit
> >> b792e64021ec ("drm: no need to check return value of debugfs_create
> >> functions"). So what's being requested is nearly the opposite of what
> >> Greg did there.
> >>
> >> I thought about perhaps only checking for directories but even that
> >> type of check was removed by Greg's patch. Further checking shows that
> >> start_creating() actually has:
> >>
> >> if (IS_ERR(parent))
> >>   return parent;
> >>
> >> ...so I guess that explains why it's fine to skip the check even for parents?
> >>
> >> Sure enough I confirmed that if I pass `ERR_PTR(-EINVAL)` as the root
> >> for `panel->funcs->debugfs_init()` that nothing bad seems to happen...
> >
> > Yeah, the idea is that you don't need to check for debugfs function
> > return values and you can safely pass error pointers to debugfs
> > functions. The worst that can happen is you don't get the debugfs, but
> > hey, it's debugfs so you shouldn't fail anything else because of that
> > anyway.
> >
>
> Thanks a lot Doug and Jani for the explanations. That makes sense and it
> explains why most code I looked was not checking for the return value.
>
> I guess we should write something about this in the debugfs functions
> kernel doc so it's mentioned explicitly and people don't have to guess.

For anyone interested, I've taken Javier's suggestion and tried to
update the docs:

https://lore.kernel.org/r/20220222154555.1.I26d364db7a007f8995e8f0dac978673bc8e9f5e2@changeid