[PATCH v1 0/9] drm/bridge: ps8640 and ti-sn65dsi86 updates

[PATCH v1 0/9] drm/bridge: ps8640 and ti-sn65dsi86 updates

[Sam Ravnborg]

Add a new macro DRM_BRIDGE_STATE_OPS that can be used to
assign atomic_reset and atomic_{duplicate,destroy}_state to
the default implementations. They will be valid in most cases.

Add a drm variant of media-bus-format.h to hold a single
function to get the bpc from the bus format.

Also add a small drm_atomic_helper_bridge_dsi_input_bus_fmt helper.

Update ti-sn65dsi86 to support atomic and NO_CONNECTOR.
The NO_CONNECTOR support was from Rob Clark - I just rebased it.
To support NO_CONNECTOR use the newly introduced function to
lokk up bpc from the bus format.

Update parade-ps8640 to support atomic. To do this just migrate
to the atomic variants of the operations and add the few mandatry
missing callbacks.

A few of the patches are migrated from a patchset I posted several
months ago and I decided to add them here for now, which explains
why there is a v4 of a patch in a v1 submission.

For the output bus fmt stuff I did what I think is correct - but
as I have paged out all my memory of this it may be all wrong.

The code builds - but needs testing.

I was temped to move bridge helpers to a new drm_bridge_helper.c
but that will wait until next time.

	Sam

Rob Clark (1):
      drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

Sam Ravnborg (8):
      drm/bridge: add DRM_BRIDGE_STATE_OPS macro
      drm: add drm specific media-bus-format header file
      drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt
      drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs
      drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state
      drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs
      drm/bridge: ps8640: plug atomic_get_input_bus_fmts
      drm/bridge: Drop unused drm_bridge_chain functions

 drivers/gpu/drm/bridge/parade-ps8640.c |  18 ++++--
 drivers/gpu/drm/bridge/ti-sn65dsi86.c  |  57 +++++++++--------
 drivers/gpu/drm/drm_atomic_helper.c    |  41 ++++++++++++
 drivers/gpu/drm/drm_bridge.c           | 110 ---------------------------------
 include/drm/drm_atomic_helper.h        |   7 +++
 include/drm/drm_bridge.h               |  40 ++++--------
 include/drm/media-bus-format.h         |  53 ++++++++++++++++
 7 files changed, 157 insertions(+), 169 deletions(-)

[PATCH v1 1/9] drm/bridge: add DRM_BRIDGE_STATE_OPS macro

[Sam Ravnborg]

The DRM_BRIDGE_STATE_OPS can be used as shortcut for bridge drivers that
do not subclass drm_bridge_state to assign the default operations for
reset, duplicate and destroy of the state.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
---
 include/drm/drm_bridge.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 061d87313fac..fc00304be643 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -935,4 +935,16 @@ static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
 }
 #endif
 
+/**
+ * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs
+ *
+ * Bridge driver that do not subclass &drm_bridge_state can use the helpers
+ * for reset, duplicate, and destroy. This macro provides a shortcut for
+ * setting the helpers in the &drm_bridge_funcs structure.
+ */
+#define DRM_BRIDGE_STATE_OPS \
+	.atomic_reset = drm_atomic_helper_bridge_reset,				\
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,	\
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state
+
 #endif
-- 
2.32.0

[PATCH v1 2/9] drm: add drm specific media-bus-format header file

[Sam Ravnborg]

For now the header file includes a single method to retreive the bpc
from the bus format.
The supported MEDIA_BUS_* codes are the ones used for the current panels
in DRM. The list can be extended as there are a need for more formats.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 include/drm/media-bus-format.h

diff --git a/include/drm/media-bus-format.h b/include/drm/media-bus-format.h
new file mode 100644
index 000000000000..d4d18f19a70f
--- /dev/null
+++ b/include/drm/media-bus-format.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Sam Ravnborg
+ */
+
+#ifndef __LINUX_DRM_MEDIA_BUS_FORMAT
+#define __LINUX_DRM_MEDIA_BUS_FORMAT
+
+#include <linux/bug.h>
+#include <linux/media-bus-format.h>
+#include <linux/types.h>
+
+/**
+ * media_bus_format_to_bpc - The bits per color channel for the bus_format
+ *
+ * Based on the supplied bus_format return the maximum number of bits
+ * per color channel.
+ *
+ * RETURNS
+ * The number of bits per color channel, or -EINVAL if the bus_format
+ * is unknown.
+ */
+static inline int media_bus_format_to_bpc(u32 bus_format)
+{
+	switch (bus_format) {
+	/* DPI */
+	case MEDIA_BUS_FMT_RGB565_1X16:
+	case MEDIA_BUS_FMT_RGB666_1X18:
+		return 6;
+
+	/* DPI */
+	case MEDIA_BUS_FMT_RGB888_1X24:
+	case MEDIA_BUS_FMT_RGB888_3X8:
+	case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
+	case MEDIA_BUS_FMT_Y8_1X8:
+		return 8;
+
+     	/* LVDS */
+	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
+		return 6;
+
+     	/* LVDS */
+	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
+	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
+		return 8;
+
+	default:
+		WARN(1, "Unknown MEDIA_BUS format %d\n", bus_format);
+		return -EINVAL;
+	}
+}
+
+#endif
-- 
2.32.0

[PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt

[Sam Ravnborg]

There is a number of bridge drivers that supports a single media bus
format for DSI. Add a helper to avoid duplicating the code.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |  7 +++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a7a05e1e26bb..94f313dc196f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,
 	return input_fmts;
 }
 EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt);
+
+/**
+ * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format
+ *
+ * @bridge: bridge control structure
+ * @bridge_state: new bridge state
+ * @crtc_state: new CRTC state
+ * @conn_state: new connector state
+ * @output_fmt: tested output bus format
+ * @num_input_fmts: will contain the size of the returned array
+ *
+ * This helper is an implementation of the
+ * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports
+ * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
+ *
+ * RETURNS
+ * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
+ * or NULL if the allocation failed
+ */
+u32 *
+drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
+					   struct drm_bridge_state *bridge_state,
+					   struct drm_crtc_state *crtc_state,
+					   struct drm_connector_state *conn_state,
+					   u32 output_fmt,
+					   unsigned int *num_input_fmts)
+{
+	u32 *input_fmts;
+
+	*num_input_fmts = 0;
+
+	input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	/* This is the DSI-end bus format */
+	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+	*num_input_fmts = 1;
+
+	return input_fmts;
+}
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 4045e2507e11..0e81b6f637d6 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -231,4 +231,11 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,
 					u32 output_fmt,
 					unsigned int *num_input_fmts);
 
+u32 *
+drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
+					   struct drm_bridge_state *bridge_state,
+					   struct drm_crtc_state *crtc_state,
+					   struct drm_connector_state *conn_state,
+					   u32 output_fmt,
+					   unsigned int *num_input_fmts);
 #endif /* DRM_ATOMIC_HELPER_H_ */
-- 
2.32.0

[PATCH v1 4/9] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

[Sam Ravnborg]

Move away from the deprecated enable/diable operations in
drm_bridge_funcs and enable atomic use.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ba136a188be7..d681ab68205c 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -796,7 +796,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_bridge_state)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
@@ -1055,7 +1056,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 	return ret;
 }
 
-static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
+				       struct drm_bridge_state *old_bridge_state)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	const char *last_err_str = "No supported DP rate";
@@ -1124,7 +1126,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 			   VSTREAM_ENABLE);
 }
 
-static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_bridge_state)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
@@ -1137,7 +1140,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 	usleep_range(100, 110);
 }
 
-static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
+					     struct drm_bridge_state *old_bridge_state)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
@@ -1158,10 +1162,11 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.detach = ti_sn_bridge_detach,
 	.mode_valid = ti_sn_bridge_mode_valid,
-	.pre_enable = ti_sn_bridge_pre_enable,
-	.enable = ti_sn_bridge_enable,
-	.disable = ti_sn_bridge_disable,
-	.post_disable = ti_sn_bridge_post_disable,
+	.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
+	.atomic_enable = ti_sn_atomic_bridge_enable,
+	.atomic_disable = ti_sn_atomic_bridge_disable,
+	.atomic_post_disable = ti_sn_bridge_post_disable,
+	DRM_BRIDGE_STATE_OPS,
 };
 
 static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
-- 
2.32.0

[PATCH v1 5/9] drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state

[Sam Ravnborg]

To prepare for DRM_BRIDGE_ATTACH_NO_CONNECTOR support,
fix so the bpc is found using the output format.

This avoids the use of the connector stored in the private data.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index d681ab68205c..dc6ec40bc1ef 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -33,6 +33,7 @@
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/media-bus-format.h>
 
 #define SN_DEVICE_REV_REG			0x08
 #define SN_DPPLL_SRC_REG			0x0A
@@ -823,9 +824,11 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
 	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
 }
 
-static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
+static unsigned int ti_sn_bridge_get_bpp(struct drm_bridge_state *bridge_state)
 {
-	if (pdata->connector.display_info.bpc <= 6)
+	int bpc = media_bus_format_to_bpc(bridge_state->output_bus_cfg.format);
+
+	if (bpc <= 6)
 		return 18;
 	else
 		return 24;
@@ -840,7 +843,8 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
 	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
 };
 
-static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
+static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata,
+					     struct drm_bridge_state *bridge_state)
 {
 	unsigned int bit_rate_khz, dp_rate_mhz;
 	unsigned int i;
@@ -848,7 +852,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
 		&pdata->bridge.encoder->crtc->state->adjusted_mode;
 
 	/* Calculate minimum bit rate based on our pixel clock. */
-	bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
+	bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(bridge_state);
 
 	/* Calculate minimum DP data rate, taking 80% as per DP spec */
 	dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
@@ -1092,7 +1096,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 			   DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
 
 	/* Set the DP output format (18 bpp or 24 bpp) */
-	val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
+	val = (ti_sn_bridge_get_bpp(old_bridge_state) == 18) ? BPP_18_RGB : 0;
 	regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
 
 	/* DP lane config */
@@ -1103,7 +1107,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 	valid_rates = ti_sn_bridge_read_valid_rates(pdata);
 
 	/* Train until we run out of rates */
-	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
+	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, old_bridge_state);
 	     dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
 	     dp_rate_idx++) {
 		if (!(valid_rates & BIT(dp_rate_idx)))
-- 
2.32.0

[PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Sam Ravnborg]

From: Rob Clark <robdclark@chromium.org>

Slightly awkward to fish out the display_info when we aren't creating
own connector.  But I don't see an obvious better way.

v3:
 - Rebased and dropped the ti_sn_bridge_get_bpp() patch
   as this was solved in a different way (Sam)

v2:
 - Remove error return with NO_CONNECTOR flag (Rob)

Signed-off-by: Rob Clark <robdclark@chromium.org>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Rob Clark <robdclark@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index dc6ec40bc1ef..a9041dfd2ae5 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -746,11 +746,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	int ret;
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
-
 	pdata->aux.drm_dev = bridge->dev;
 	ret = drm_dp_aux_register(&pdata->aux);
 	if (ret < 0) {
@@ -758,12 +753,14 @@ 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;
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+		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;
+		/* We never want the next bridge to *also* create a connector: */
+		flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+	}
 
 	/* Attach the next bridge */
 	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
@@ -774,7 +771,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	return 0;
 
 err_dsi_host:
-	drm_connector_cleanup(&pdata->connector);
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
+		drm_connector_cleanup(&pdata->connector);
 err_conn_init:
 	drm_dp_aux_unregister(&pdata->aux);
 	return ret;
-- 
2.32.0

[PATCH v1 7/9] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Sam Ravnborg]

The atomic variants of enable/disable in drm_bridge_funcs are the
preferred operations - introduce these.

The ps8640 driver used the non-atomic variants of the drm_bridge_chain_pre_enable/
drm_bridge_chain_post_disable - convert these to the atomic variants.

v4:
  - Rebased

v3:
  - Init state operations in drm_bridge_funcs (Laurent)

v2:
  - Added a few more people to cc: (Jitao, Enric, Philip) to increase
    possibility to get test feedback

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Maxime Ripard <maxime@cerno.tech>
Cc: Jitao Shi <jitao.shi@mediatek.com>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc: Philip Chen <philipchen@chromium.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 3f17337ee389..d63c762fcd34 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -13,6 +13,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/dp/drm_dp_aux_bus.h>
 #include <drm/dp/drm_dp_helper.h>
@@ -398,7 +399,8 @@ static const struct dev_pm_ops ps8640_pm_ops = {
 				pm_runtime_force_resume)
 };
 
-static void ps8640_pre_enable(struct drm_bridge *bridge)
+static void ps8640_atomic_pre_enable(struct drm_bridge *bridge,
+				     struct drm_bridge_state *old_bridge_state)
 {
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
 	struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
@@ -430,7 +432,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
 	ps_bridge->pre_enabled = true;
 }
 
-static void ps8640_post_disable(struct drm_bridge *bridge)
+static void ps8640_atomic_post_disable(struct drm_bridge *bridge,
+				       struct drm_bridge_state *old_bridge_state)
 {
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
 
@@ -508,7 +511,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 	 * EDID, for this chip, we need to do a full poweron, otherwise it will
 	 * fail.
 	 */
-	drm_bridge_chain_pre_enable(bridge);
+	drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
 
 	edid = drm_get_edid(connector,
 			    ps_bridge->page[PAGE0_DP_CNTL]->adapter);
@@ -518,7 +521,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 	 * before, return the chip to its original power state.
 	 */
 	if (poweroff)
-		drm_bridge_chain_post_disable(bridge);
+		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
 
 	return edid;
 }
@@ -533,8 +536,9 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
 	.attach = ps8640_bridge_attach,
 	.detach = ps8640_bridge_detach,
 	.get_edid = ps8640_bridge_get_edid,
-	.post_disable = ps8640_post_disable,
-	.pre_enable = ps8640_pre_enable,
+	.atomic_post_disable = ps8640_atomic_post_disable,
+	.atomic_pre_enable = ps8640_atomic_pre_enable,
+	DRM_BRIDGE_STATE_OPS,
 };
 
 static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridge)
-- 
2.32.0

[PATCH v1 8/9] drm/bridge: ps8640: plug atomic_get_input_bus_fmts

[Sam Ravnborg]

Use the helper drm_atomic_helper_bridge_dsi_input_bus_fmt to
announce the DSI media bus format.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index d63c762fcd34..23af23522eee 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -13,6 +13,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/dp/drm_dp_aux_bus.h>
@@ -538,6 +539,7 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
 	.get_edid = ps8640_bridge_get_edid,
 	.atomic_post_disable = ps8640_atomic_post_disable,
 	.atomic_pre_enable = ps8640_atomic_pre_enable,
+	.atomic_get_input_bus_fmts = drm_atomic_helper_bridge_dsi_input_bus_fmt,
 	DRM_BRIDGE_STATE_OPS,
 };
 
-- 
2.32.0

[PATCH v1 9/9] drm/bridge: Drop unused drm_bridge_chain functions

[Sam Ravnborg]

The drm_bridge_chain_{pre_enable,enable,disable,post_disable} has no
users left and we have atomic variants that should be used.
Drop them so they do not gain new users.

Adjust a few comments to avoid references to the dropped functions.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Maxime Ripard <maxime@cerno.tech>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_bridge.c | 110 -----------------------------------
 include/drm/drm_bridge.h     |  28 ---------
 2 files changed, 138 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c96847fc0ebc..7a57d6816105 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -485,61 +485,6 @@ drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
 
-/**
- * drm_bridge_chain_disable - disables all bridges in the encoder chain
- * @bridge: bridge control structure
- *
- * Calls &drm_bridge_funcs.disable op for all the bridges in the encoder
- * chain, starting from the last bridge to the first. These are called before
- * calling the encoder's prepare op.
- *
- * Note: the bridge passed should be the one closest to the encoder
- */
-void drm_bridge_chain_disable(struct drm_bridge *bridge)
-{
-	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->disable)
-			iter->funcs->disable(iter);
-
-		if (iter == bridge)
-			break;
-	}
-}
-EXPORT_SYMBOL(drm_bridge_chain_disable);
-
-/**
- * drm_bridge_chain_post_disable - cleans up after disabling all bridges in the
- *				   encoder chain
- * @bridge: bridge control structure
- *
- * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
- * encoder chain, starting from the first bridge to the last. These are called
- * after completing the encoder's prepare op.
- *
- * Note: the bridge passed should be the one closest to the encoder
- */
-void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
-{
-	struct drm_encoder *encoder;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->post_disable)
-			bridge->funcs->post_disable(bridge);
-	}
-}
-EXPORT_SYMBOL(drm_bridge_chain_post_disable);
-
 /**
  * drm_bridge_chain_mode_set - set proposed mode for all bridges in the
  *			       encoder chain
@@ -569,61 +514,6 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_set);
 
-/**
- * drm_bridge_chain_pre_enable - prepares for enabling all bridges in the
- *				 encoder chain
- * @bridge: bridge control structure
- *
- * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
- * chain, starting from the last bridge to the first. These are called
- * before calling the encoder's commit op.
- *
- * Note: the bridge passed should be the one closest to the encoder
- */
-void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
-{
-	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->pre_enable)
-			iter->funcs->pre_enable(iter);
-
-		if (iter == bridge)
-			break;
-	}
-}
-EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
-
-/**
- * drm_bridge_chain_enable - enables all bridges in the encoder chain
- * @bridge: bridge control structure
- *
- * Calls &drm_bridge_funcs.enable op for all the bridges in the encoder
- * chain, starting from the first bridge to the last. These are called
- * after completing the encoder's commit op.
- *
- * Note that the bridge passed should be the one closest to the encoder
- */
-void drm_bridge_chain_enable(struct drm_bridge *bridge)
-{
-	struct drm_encoder *encoder;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->enable)
-			bridge->funcs->enable(bridge);
-	}
-}
-EXPORT_SYMBOL(drm_bridge_chain_enable);
-
 /**
  * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
  * @bridge: bridge control structure
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index fc00304be643..ed2e4a8fe510 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -297,12 +297,6 @@ struct drm_bridge_funcs {
 	 * not enable the display link feeding the next bridge in the chain (if
 	 * there is one) when this callback is called.
 	 *
-	 * Note that this function will only be invoked in the context of an
-	 * atomic commit. It will not be invoked from
-	 * &drm_bridge_chain_pre_enable. It would be prudent to also provide an
-	 * implementation of @pre_enable if you are expecting driver calls into
-	 * &drm_bridge_chain_pre_enable.
-	 *
 	 * The @atomic_pre_enable callback is optional.
 	 */
 	void (*atomic_pre_enable)(struct drm_bridge *bridge,
@@ -323,11 +317,6 @@ struct drm_bridge_funcs {
 	 * callback must enable the display link feeding the next bridge in the
 	 * chain if there is one.
 	 *
-	 * Note that this function will only be invoked in the context of an
-	 * atomic commit. It will not be invoked from &drm_bridge_chain_enable.
-	 * It would be prudent to also provide an implementation of @enable if
-	 * you are expecting driver calls into &drm_bridge_chain_enable.
-	 *
 	 * The @atomic_enable callback is optional.
 	 */
 	void (*atomic_enable)(struct drm_bridge *bridge,
@@ -345,12 +334,6 @@ struct drm_bridge_funcs {
 	 * The bridge can assume that the display pipe (i.e. clocks and timing
 	 * signals) feeding it is still running when this callback is called.
 	 *
-	 * Note that this function will only be invoked in the context of an
-	 * atomic commit. It will not be invoked from
-	 * &drm_bridge_chain_disable. It would be prudent to also provide an
-	 * implementation of @disable if you are expecting driver calls into
-	 * &drm_bridge_chain_disable.
-	 *
 	 * The @atomic_disable callback is optional.
 	 */
 	void (*atomic_disable)(struct drm_bridge *bridge,
@@ -370,13 +353,6 @@ struct drm_bridge_funcs {
 	 * signals) feeding it is no longer running when this callback is
 	 * called.
 	 *
-	 * Note that this function will only be invoked in the context of an
-	 * atomic commit. It will not be invoked from
-	 * &drm_bridge_chain_post_disable.
-	 * It would be prudent to also provide an implementation of
-	 * @post_disable if you are expecting driver calls into
-	 * &drm_bridge_chain_post_disable.
-	 *
 	 * The @atomic_post_disable callback is optional.
 	 */
 	void (*atomic_post_disable)(struct drm_bridge *bridge,
@@ -868,13 +844,9 @@ enum drm_mode_status
 drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
 			    const struct drm_display_info *info,
 			    const struct drm_display_mode *mode);
-void drm_bridge_chain_disable(struct drm_bridge *bridge);
-void drm_bridge_chain_post_disable(struct drm_bridge *bridge);
 void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
 			       const struct drm_display_mode *mode,
 			       const struct drm_display_mode *adjusted_mode);
-void drm_bridge_chain_pre_enable(struct drm_bridge *bridge);
-void drm_bridge_chain_enable(struct drm_bridge *bridge);
 
 int drm_atomic_bridge_chain_check(struct drm_bridge *bridge,
 				  struct drm_crtc_state *crtc_state,
-- 
2.32.0

Re: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt

[kernel test robot]

Hi Sam,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.17-rc2 next-20220204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: hexagon-buildonly-randconfig-r003-20220206 (https://download.01.org/0day-ci/archive/20220207/202202070113.dsmEk0pf-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 6daaf5a44925592c764c59219b0024ee06317028)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d72252f11e9d51bd1da85eb99299f3d8fec36bc2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638
        git checkout d72252f11e9d51bd1da85eb99299f3d8fec36bc2
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/gpu/drm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/drm_atomic_helper.c:3583:23: error: use of undeclared identifier 'MAX_INPUT_SEL_FORMATS'
           input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);
                                ^
   1 error generated.


vim +/MAX_INPUT_SEL_FORMATS +3583 drivers/gpu/drm/drm_atomic_helper.c

  3552	
  3553	/**
  3554	 * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format
  3555	 *
  3556	 * @bridge: bridge control structure
  3557	 * @bridge_state: new bridge state
  3558	 * @crtc_state: new CRTC state
  3559	 * @conn_state: new connector state
  3560	 * @output_fmt: tested output bus format
  3561	 * @num_input_fmts: will contain the size of the returned array
  3562	 *
  3563	 * This helper is an implementation of the
  3564	 * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports
  3565	 * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
  3566	 *
  3567	 * RETURNS
  3568	 * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
  3569	 * or NULL if the allocation failed
  3570	 */
  3571	u32 *
  3572	drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
  3573						   struct drm_bridge_state *bridge_state,
  3574						   struct drm_crtc_state *crtc_state,
  3575						   struct drm_connector_state *conn_state,
  3576						   u32 output_fmt,
  3577						   unsigned int *num_input_fmts)
  3578	{
  3579		u32 *input_fmts;
  3580	
  3581		*num_input_fmts = 0;
  3582	
> 3583		input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3506 bytes --]

Hi Sam,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.17-rc2 next-20220204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: hexagon-buildonly-randconfig-r003-20220206 (https://download.01.org/0day-ci/archive/20220207/202202070113.dsmEk0pf-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 6daaf5a44925592c764c59219b0024ee06317028)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d72252f11e9d51bd1da85eb99299f3d8fec36bc2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638
        git checkout d72252f11e9d51bd1da85eb99299f3d8fec36bc2
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/gpu/drm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/drm_atomic_helper.c:3583:23: error: use of undeclared identifier 'MAX_INPUT_SEL_FORMATS'
           input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);
                                ^
   1 error generated.


vim +/MAX_INPUT_SEL_FORMATS +3583 drivers/gpu/drm/drm_atomic_helper.c

  3552	
  3553	/**
  3554	 * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format
  3555	 *
  3556	 * @bridge: bridge control structure
  3557	 * @bridge_state: new bridge state
  3558	 * @crtc_state: new CRTC state
  3559	 * @conn_state: new connector state
  3560	 * @output_fmt: tested output bus format
  3561	 * @num_input_fmts: will contain the size of the returned array
  3562	 *
  3563	 * This helper is an implementation of the
  3564	 * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports
  3565	 * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
  3566	 *
  3567	 * RETURNS
  3568	 * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
  3569	 * or NULL if the allocation failed
  3570	 */
  3571	u32 *
  3572	drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
  3573						   struct drm_bridge_state *bridge_state,
  3574						   struct drm_crtc_state *crtc_state,
  3575						   struct drm_connector_state *conn_state,
  3576						   u32 output_fmt,
  3577						   unsigned int *num_input_fmts)
  3578	{
  3579		u32 *input_fmts;
  3580	
  3581		*num_input_fmts = 0;
  3582	
> 3583		input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH v1 4/9] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

[kernel test robot]

Hi Sam,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.17-rc2 next-20220204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: hexagon-buildonly-randconfig-r003-20220206 (https://download.01.org/0day-ci/archive/20220207/202202070105.4pH2BXwR-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 6daaf5a44925592c764c59219b0024ee06317028)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b4a6b21edb289882a1f838a231032110dd0f3d1e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638
        git checkout b4a6b21edb289882a1f838a231032110dd0f3d1e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/gpu/drm/bridge/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1166:19: error: use of undeclared identifier 'ti_sn_atomic_bridge_enable'
           .atomic_enable = ti_sn_atomic_bridge_enable,
                            ^
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1167:20: error: use of undeclared identifier 'ti_sn_atomic_bridge_disable'
           .atomic_disable = ti_sn_atomic_bridge_disable,
                             ^
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1168:25: error: use of undeclared identifier 'ti_sn_bridge_post_disable'; did you mean 'ti_sn_bridge_atomic_disable'?
           .atomic_post_disable = ti_sn_bridge_post_disable,
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~
                                  ti_sn_bridge_atomic_disable
   drivers/gpu/drm/bridge/ti-sn65dsi86.c:799:13: note: 'ti_sn_bridge_atomic_disable' declared here
   static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
               ^
   3 errors generated.


vim +/ti_sn_atomic_bridge_enable +1166 drivers/gpu/drm/bridge/ti-sn65dsi86.c

  1160	
  1161	static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
  1162		.attach = ti_sn_bridge_attach,
  1163		.detach = ti_sn_bridge_detach,
  1164		.mode_valid = ti_sn_bridge_mode_valid,
  1165		.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> 1166		.atomic_enable = ti_sn_atomic_bridge_enable,
> 1167		.atomic_disable = ti_sn_atomic_bridge_disable,
> 1168		.atomic_post_disable = ti_sn_bridge_post_disable,
  1169		DRM_BRIDGE_STATE_OPS,
  1170	};
  1171	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v1 4/9] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3423 bytes --]

Hi Sam,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.17-rc2 next-20220204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: hexagon-buildonly-randconfig-r003-20220206 (https://download.01.org/0day-ci/archive/20220207/202202070105.4pH2BXwR-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 6daaf5a44925592c764c59219b0024ee06317028)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b4a6b21edb289882a1f838a231032110dd0f3d1e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638
        git checkout b4a6b21edb289882a1f838a231032110dd0f3d1e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/gpu/drm/bridge/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1166:19: error: use of undeclared identifier 'ti_sn_atomic_bridge_enable'
           .atomic_enable = ti_sn_atomic_bridge_enable,
                            ^
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1167:20: error: use of undeclared identifier 'ti_sn_atomic_bridge_disable'
           .atomic_disable = ti_sn_atomic_bridge_disable,
                             ^
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1168:25: error: use of undeclared identifier 'ti_sn_bridge_post_disable'; did you mean 'ti_sn_bridge_atomic_disable'?
           .atomic_post_disable = ti_sn_bridge_post_disable,
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~
                                  ti_sn_bridge_atomic_disable
   drivers/gpu/drm/bridge/ti-sn65dsi86.c:799:13: note: 'ti_sn_bridge_atomic_disable' declared here
   static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
               ^
   3 errors generated.


vim +/ti_sn_atomic_bridge_enable +1166 drivers/gpu/drm/bridge/ti-sn65dsi86.c

  1160	
  1161	static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
  1162		.attach = ti_sn_bridge_attach,
  1163		.detach = ti_sn_bridge_detach,
  1164		.mode_valid = ti_sn_bridge_mode_valid,
  1165		.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> 1166		.atomic_enable = ti_sn_atomic_bridge_enable,
> 1167		.atomic_disable = ti_sn_atomic_bridge_disable,
> 1168		.atomic_post_disable = ti_sn_bridge_post_disable,
  1169		DRM_BRIDGE_STATE_OPS,
  1170	};
  1171	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH v1 0/9] drm/bridge: ps8640 and ti-sn65dsi86 updates

[Sam Ravnborg]

> 
> The code builds - but needs testing.

Hrmff, no it does not build. The fixes was by accident not included.
Will wait a bit for feedback before posting a v2.

	Sam

Re: [PATCH v1 1/9] drm/bridge: add DRM_BRIDGE_STATE_OPS macro

[Doug Anderson]

Hi,

On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> The DRM_BRIDGE_STATE_OPS can be used as shortcut for bridge drivers that
> do not subclass drm_bridge_state to assign the default operations for
> reset, duplicate and destroy of the state.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  include/drm/drm_bridge.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 061d87313fac..fc00304be643 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -935,4 +935,16 @@ static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
>  }
>  #endif
>
> +/**
> + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs
> + *
> + * Bridge driver that do not subclass &drm_bridge_state can use the helpers
> + * for reset, duplicate, and destroy. This macro provides a shortcut for
> + * setting the helpers in the &drm_bridge_funcs structure.
> + */
> +#define DRM_BRIDGE_STATE_OPS \
> +       .atomic_reset = drm_atomic_helper_bridge_reset,                         \
> +       .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,     \
> +       .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state

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

Re: [PATCH v1 2/9] drm: add drm specific media-bus-format header file

[Doug Anderson]

Hi,

On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> For now the header file includes a single method to retreive the bpc
> from the bus format.
> The supported MEDIA_BUS_* codes are the ones used for the current panels
> in DRM. The list can be extended as there are a need for more formats.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 include/drm/media-bus-format.h
>
> diff --git a/include/drm/media-bus-format.h b/include/drm/media-bus-format.h
> new file mode 100644
> index 000000000000..d4d18f19a70f
> --- /dev/null
> +++ b/include/drm/media-bus-format.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022 Sam Ravnborg
> + */
> +
> +#ifndef __LINUX_DRM_MEDIA_BUS_FORMAT
> +#define __LINUX_DRM_MEDIA_BUS_FORMAT
> +
> +#include <linux/bug.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/types.h>
> +
> +/**
> + * media_bus_format_to_bpc - The bits per color channel for the bus_format
> + *
> + * Based on the supplied bus_format return the maximum number of bits
> + * per color channel.
> + *
> + * RETURNS
> + * The number of bits per color channel, or -EINVAL if the bus_format
> + * is unknown.

kernel-doc doesn't like your syntax quite right. Try running:

./scripts/kernel-doc -rst -v include/drm/media-bus-format.h

It will tell you that you're missing a description of the parameter
and the way you're describing the return value isn't in a way that it
can parse...

> + */
> +static inline int media_bus_format_to_bpc(u32 bus_format)
> +{
> +       switch (bus_format) {
> +       /* DPI */
> +       case MEDIA_BUS_FMT_RGB565_1X16:
> +       case MEDIA_BUS_FMT_RGB666_1X18:
> +               return 6;
> +
> +       /* DPI */
> +       case MEDIA_BUS_FMT_RGB888_1X24:
> +       case MEDIA_BUS_FMT_RGB888_3X8:
> +       case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
> +       case MEDIA_BUS_FMT_Y8_1X8:
> +               return 8;
> +
> +       /* LVDS */
> +       case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> +               return 6;
> +
> +       /* LVDS */

When applying your patches, I got a warning that both of your "/* LVDS
*/" comments have spaces before the tab.

I'm also not sure what the "sections" are supposed to mean. Are you
saying that the bus formats are only for the given transport type?
...so we can't use MEDIA_BUS_FMT_RGB666_1X18 for eDP?

-Doug

Re: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt

[Doug Anderson]

Hi,

On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> There is a number of bridge drivers that supports a single media bus
> format for DSI. Add a helper to avoid duplicating the code.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  7 +++++
>  2 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a7a05e1e26bb..94f313dc196f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,
>         return input_fmts;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt);
> +
> +/**
> + * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format

Is the description right? It's called "input" format but it defines an
output format?


> + * @bridge: bridge control structure
> + * @bridge_state: new bridge state
> + * @crtc_state: new CRTC state
> + * @conn_state: new connector state
> + * @output_fmt: tested output bus format
> + * @num_input_fmts: will contain the size of the returned array

Maybe indicate that it's always 1 in the comments?

> + *
> + * This helper is an implementation of the
> + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports
> + * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
> + *
> + * RETURNS

kernel-doc can't parse this return syntax and warns:

warning: No description found for return value of
'drm_atomic_helper_bridge_dsi_input_bus_fmt'


> + * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
> + * or NULL if the allocation failed
> + */
> +u32 *
> +drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
> +                                          struct drm_bridge_state *bridge_state,
> +                                          struct drm_crtc_state *crtc_state,
> +                                          struct drm_connector_state *conn_state,
> +                                          u32 output_fmt,
> +                                          unsigned int *num_input_fmts)
> +{
> +       u32 *input_fmts;
> +
> +       *num_input_fmts = 0;
> +
> +       input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);

I probably wouldn't have bothered with `kcalloc` for something that's
always just one value and you're setting it. Why not just

input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);

...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you
said it didn't compile?

Also: if it was common for others to want to provide fixed formats, I
wonder about adding a helper function that did most of the work here?
Dunno what it would be named since it's already a bit a of handful,
but I'd expect to call it like:

static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 };
return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats)

Then my_neat_helper() could do kmemdup() on the array passed and fill
in "num_output_formats" to be either the array size of 0 (if the
kmemdup failed).


> +       if (!input_fmts)
> +               return NULL;
> +
> +       /* This is the DSI-end bus format */
> +       input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;

I'm not an expert, but I'm curious. Can't DSI run in other formats?
...or maybe I'm misunderstanding what this is for. I guess I'm not
sure how it relates to:

enum mipi_dsi_pixel_format {
  MIPI_DSI_FMT_RGB888,
  MIPI_DSI_FMT_RGB666,
  MIPI_DSI_FMT_RGB666_PACKED,
  MIPI_DSI_FMT_RGB565,
};


-Doug

Re: [PATCH v1 4/9] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

[Doug Anderson]

Hi,

On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Move away from the deprecated enable/diable operations in
> drm_bridge_funcs and enable atomic use.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ba136a188be7..d681ab68205c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -796,7 +796,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>         return MODE_OK;
>  }
>
> -static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
> +                                       struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1055,7 +1056,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>         return ret;
>  }
>
> -static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> +                                      struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>         const char *last_err_str = "No supported DP rate";
> @@ -1124,7 +1126,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>                            VSTREAM_ENABLE);
>  }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +                                          struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1137,7 +1140,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>         usleep_range(100, 110);
>  }
>
> -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +                                            struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1158,10 +1162,11 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .attach = ti_sn_bridge_attach,
>         .detach = ti_sn_bridge_detach,
>         .mode_valid = ti_sn_bridge_mode_valid,
> -       .pre_enable = ti_sn_bridge_pre_enable,
> -       .enable = ti_sn_bridge_enable,
> -       .disable = ti_sn_bridge_disable,
> -       .post_disable = ti_sn_bridge_post_disable,
> +       .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> +       .atomic_enable = ti_sn_atomic_bridge_enable,
> +       .atomic_disable = ti_sn_atomic_bridge_disable,
> +       .atomic_post_disable = ti_sn_bridge_post_disable,

Compiler doesn't like the fact that you are inconsistent about whether
it's "atomic_bridge" or "bridge_atomic". Probably should settle on
"bridge_atomic"? ...and the "post_disable" needs "atomic" in the
name...

> +       DRM_BRIDGE_STATE_OPS,

Wow, is it really that simple? I guess it seems to work OK...

Since I don't actually know tons about atomic and whether this is
enough, consider my Reviewed-by tag to be pretty weak. That being
said, this _seems_ right to me?

So once it compiles then I'm fine w/ my (weak) Reviewed-by and my Tested-by.

-Doug

Re: [PATCH v1 5/9] drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state

[Doug Anderson]

Hi,

On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> To prepare for DRM_BRIDGE_ATTACH_NO_CONNECTOR support,
> fix so the bpc is found using the output format.
>
> This avoids the use of the connector stored in the private data.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index d681ab68205c..dc6ec40bc1ef 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -33,6 +33,7 @@
>  #include <drm/drm_panel.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/media-bus-format.h>
>
>  #define SN_DEVICE_REV_REG                      0x08
>  #define SN_DPPLL_SRC_REG                       0x0A
> @@ -823,9 +824,11 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
>         regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
>  }
>
> -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> +static unsigned int ti_sn_bridge_get_bpp(struct drm_bridge_state *bridge_state)
>  {
> -       if (pdata->connector.display_info.bpc <= 6)
> +       int bpc = media_bus_format_to_bpc(bridge_state->output_bus_cfg.format);
> +
> +       if (bpc <= 6)
>                 return 18;
>         else
>                 return 24;

Unfortunately this doesn't work as hoped. :(
`bridge_state->output_bus_cfg.format` is 0 in my testing...

...and then media_bus_format_to_bpc() returns an error, which is
negative and <= 6. That's not super ideal...

I guess this gets down to what the output bus format is _supposed_ to
be for eDP. Based on my understanding of eDP there isn't really a
concept of a fixed "bus format" that the panel ought to work at. There
is a concept of the number of bits per component (6, 8, 10, 12) that a
panel can run at but then after that I believe the format is standard,
or at least it's something that's dynamic / negotiated. Also note that
the format on the bus is more related to the current mode we're
running the panel in than some inherent property of the panel. For
instance, a 10 bpc panel can likely have data provided in 8 bpc and 6
bpc. I've also seen 6 bpc panels that can accept 8 bpc data and can
dither it. In any case, this type of stuff is really all dynamic for
eDP. The old value we were looking at was really being interpreted as
the "max" bpc that the panel could run in and we'd choose to run the
panel in 8 bpc if the panel supported it and 6 bpc otherwise (this
bridge chip only supports 6bpc or 8bpc).

So I guess the question is: how do we move forward here?

Do we need to somehow figure out how to get "output_bus_cfg.format"
filled in? Any suggestions for how to do that? Do we just implement
atomic_get_output_bus_fmts() and then pick one of
MEDIA_BUS_FMT_RGB666_1X18 or MEDIA_BUS_FMT_RBG888_1X24 based on the
bpc in the connector state? ...or do we just list both of them and
something magically will pick the right one? Hrm, I tried that and it
didn't magically work, but I didn't debug further...

One thing I realized is that, at least in theory, we might not know
whether we want to run in 6 bpc or 8 bpc until we've done link
training. It's at least somewhat plausible that there could be edge
cases where we'd want to fall back to the low bpc if link training
failed at the higher bpc. The driver doesn't support that right now,
but ideally the design wouldn't preclude it. At the moment link
training happens in enable. Maybe that's a problem to worry about
another day, though?

-Doug

Re: [PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Doug Anderson]

Hi,

On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Slightly awkward to fish out the display_info when we aren't creating
> own connector.  But I don't see an obvious better way.
>
> v3:
>  - Rebased and dropped the ti_sn_bridge_get_bpp() patch
>    as this was solved in a different way (Sam)
>
> v2:
>  - Remove error return with NO_CONNECTOR flag (Rob)
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)

This is fine by me assuming we can fix the previous patches.

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

NOTE: to me, this isn't something to do _instead_ of my patch [1] but
_in addition_ to it. ${SUBJECT} patch transitions us to a more modern
approach of having the connector created elsewhere but doesn't remove
the old fallback code. Might as well clean the fallback code up unless
you think it's going to simply be deleted right away or something?

[1] https://lore.kernel.org/r/20220204161245.v2.1.I3ab26b7f197cc56c874246a43e57913e9c2c1028@changeid

Re: [PATCH v1 1/9] drm/bridge: add DRM_BRIDGE_STATE_OPS macro

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Feb 06, 2022 at 04:43:57PM +0100, Sam Ravnborg wrote:
> The DRM_BRIDGE_STATE_OPS can be used as shortcut for bridge drivers that
> do not subclass drm_bridge_state to assign the default operations for
> reset, duplicate and destroy of the state.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  include/drm/drm_bridge.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 061d87313fac..fc00304be643 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -935,4 +935,16 @@ static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
>  }
>  #endif
>  
> +/**
> + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs

I'd name the macro DRM_BRIDGE_STATE_DEFAULT_OPS.

> + *
> + * Bridge driver that do not subclass &drm_bridge_state can use the helpers
> + * for reset, duplicate, and destroy. This macro provides a shortcut for
> + * setting the helpers in the &drm_bridge_funcs structure.
> + */
> +#define DRM_BRIDGE_STATE_OPS \
> +	.atomic_reset = drm_atomic_helper_bridge_reset,				\
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,	\
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state

I'm not a big fan of such macros for a small number of operations, as I
find that it obfuscates the code a bit, but that could change once I get
used to the new macro :-)

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

> +
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/9] drm: add drm specific media-bus-format header file

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Feb 06, 2022 at 04:43:58PM +0100, Sam Ravnborg wrote:
> For now the header file includes a single method to retreive the bpc

s/retreive/retrieve/

> from the bus format.
> The supported MEDIA_BUS_* codes are the ones used for the current panels
> in DRM. The list can be extended as there are a need for more formats.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 include/drm/media-bus-format.h
> 
> diff --git a/include/drm/media-bus-format.h b/include/drm/media-bus-format.h
> new file mode 100644
> index 000000000000..d4d18f19a70f
> --- /dev/null
> +++ b/include/drm/media-bus-format.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022 Sam Ravnborg
> + */
> +
> +#ifndef __LINUX_DRM_MEDIA_BUS_FORMAT
> +#define __LINUX_DRM_MEDIA_BUS_FORMAT
> +
> +#include <linux/bug.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/types.h>
> +
> +/**
> + * media_bus_format_to_bpc - The bits per color channel for the bus_format
> + *
> + * Based on the supplied bus_format return the maximum number of bits
> + * per color channel.
> + *
> + * RETURNS
> + * The number of bits per color channel, or -EINVAL if the bus_format
> + * is unknown.
> + */
> +static inline int media_bus_format_to_bpc(u32 bus_format)
> +{
> +	switch (bus_format) {
> +	/* DPI */
> +	case MEDIA_BUS_FMT_RGB565_1X16:
> +	case MEDIA_BUS_FMT_RGB666_1X18:
> +		return 6;
> +
> +	/* DPI */
> +	case MEDIA_BUS_FMT_RGB888_1X24:
> +	case MEDIA_BUS_FMT_RGB888_3X8:
> +	case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
> +	case MEDIA_BUS_FMT_Y8_1X8:
> +		return 8;
> +
> +     	/* LVDS */

Wrong indentation.

> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> +		return 6;
> +
> +     	/* LVDS */
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> +		return 8;
> +
> +	default:
> +		WARN(1, "Unknown MEDIA_BUS format %d\n", bus_format);
> +		return -EINVAL;
> +	}
> +}

This seems a bit big for an inline function.

If we add more helper functions, it will result in lots of switch...case
statements. Could we use the same approach as in drm_fourcc.h with a
format info structure ?

> +
> +#endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt

[Laurent Pinchart]

Hello,

On Mon, Feb 07, 2022 at 02:32:45PM -0800, Doug Anderson wrote:
> On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > There is a number of bridge drivers that supports a single media bus
> > format for DSI. Add a helper to avoid duplicating the code.
> >
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
> >  include/drm/drm_atomic_helper.h     |  7 +++++
> >  2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index a7a05e1e26bb..94f313dc196f 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,
> >         return input_fmts;
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt);
> > +
> > +/**
> > + * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format
> 
> Is the description right? It's called "input" format but it defines an
> output format?
> 
> 
> > + * @bridge: bridge control structure
> > + * @bridge_state: new bridge state
> > + * @crtc_state: new CRTC state
> > + * @conn_state: new connector state
> > + * @output_fmt: tested output bus format
> > + * @num_input_fmts: will contain the size of the returned array
> 
> Maybe indicate that it's always 1 in the comments?
> 
> > + *
> > + * This helper is an implementation of the
> > + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports
> > + * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
> > + *
> > + * RETURNS
> 
> kernel-doc can't parse this return syntax and warns:
> 
> warning: No description found for return value of
> 'drm_atomic_helper_bridge_dsi_input_bus_fmt'
> 
> 
> > + * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
> > + * or NULL if the allocation failed
> > + */
> > +u32 *
> > +drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
> > +                                          struct drm_bridge_state *bridge_state,
> > +                                          struct drm_crtc_state *crtc_state,
> > +                                          struct drm_connector_state *conn_state,
> > +                                          u32 output_fmt,
> > +                                          unsigned int *num_input_fmts)
> > +{
> > +       u32 *input_fmts;
> > +
> > +       *num_input_fmts = 0;
> > +
> > +       input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);
> 
> I probably wouldn't have bothered with `kcalloc` for something that's
> always just one value and you're setting it. Why not just
> 
> input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
> 
> ...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you
> said it didn't compile?
> 
> Also: if it was common for others to want to provide fixed formats, I
> wonder about adding a helper function that did most of the work here?
> Dunno what it would be named since it's already a bit a of handful,
> but I'd expect to call it like:
> 
> static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 };
> return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats)
> 
> Then my_neat_helper() could do kmemdup() on the array passed and fill
> in "num_output_formats" to be either the array size of 0 (if the
> kmemdup failed).

I quite like that approach. We could even have a wrapper macro that adds
the ARRAY_SIZE() argument automatically.

> > +       if (!input_fmts)
> > +               return NULL;
> > +
> > +       /* This is the DSI-end bus format */
> > +       input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> 
> I'm not an expert, but I'm curious. Can't DSI run in other formats?
> ...or maybe I'm misunderstanding what this is for. I guess I'm not
> sure how it relates to:
> 
> enum mipi_dsi_pixel_format {
>   MIPI_DSI_FMT_RGB888,
>   MIPI_DSI_FMT_RGB666,
>   MIPI_DSI_FMT_RGB666_PACKED,
>   MIPI_DSI_FMT_RGB565,
> };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 4/9] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Feb 06, 2022 at 04:44:00PM +0100, Sam Ravnborg wrote:
> Move away from the deprecated enable/diable operations in

s/diable/disable/

> drm_bridge_funcs and enable atomic use.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ba136a188be7..d681ab68205c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -796,7 +796,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
>  
> -static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
> +					struct drm_bridge_state *old_bridge_state)
>  {
>  	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>  
> @@ -1055,7 +1056,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>  	return ret;
>  }
>  
> -static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> +				       struct drm_bridge_state *old_bridge_state)
>  {
>  	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>  	const char *last_err_str = "No supported DP rate";
> @@ -1124,7 +1126,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>  			   VSTREAM_ENABLE);
>  }
>  
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +					   struct drm_bridge_state *old_bridge_state)
>  {
>  	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>  
> @@ -1137,7 +1140,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>  	usleep_range(100, 110);
>  }
>  
> -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +					     struct drm_bridge_state *old_bridge_state)
>  {
>  	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>  
> @@ -1158,10 +1162,11 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>  	.attach = ti_sn_bridge_attach,
>  	.detach = ti_sn_bridge_detach,
>  	.mode_valid = ti_sn_bridge_mode_valid,
> -	.pre_enable = ti_sn_bridge_pre_enable,
> -	.enable = ti_sn_bridge_enable,
> -	.disable = ti_sn_bridge_disable,
> -	.post_disable = ti_sn_bridge_post_disable,
> +	.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> +	.atomic_enable = ti_sn_atomic_bridge_enable,
> +	.atomic_disable = ti_sn_atomic_bridge_disable,
> +	.atomic_post_disable = ti_sn_bridge_post_disable,
> +	DRM_BRIDGE_STATE_OPS,

With the compilation fix,

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

>  };
>  
>  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 9/9] drm/bridge: Drop unused drm_bridge_chain functions

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Feb 06, 2022 at 04:44:05PM +0100, Sam Ravnborg wrote:
> The drm_bridge_chain_{pre_enable,enable,disable,post_disable} has no
> users left and we have atomic variants that should be used.
> Drop them so they do not gain new users.
> 
> Adjust a few comments to avoid references to the dropped functions.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Reviewed-by: Maxime Ripard <maxime@cerno.tech>

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

> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_bridge.c | 110 -----------------------------------
>  include/drm/drm_bridge.h     |  28 ---------
>  2 files changed, 138 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index c96847fc0ebc..7a57d6816105 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -485,61 +485,6 @@ drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
>  
> -/**
> - * drm_bridge_chain_disable - disables all bridges in the encoder chain
> - * @bridge: bridge control structure
> - *
> - * Calls &drm_bridge_funcs.disable op for all the bridges in the encoder
> - * chain, starting from the last bridge to the first. These are called before
> - * calling the encoder's prepare op.
> - *
> - * Note: the bridge passed should be the one closest to the encoder
> - */
> -void drm_bridge_chain_disable(struct drm_bridge *bridge)
> -{
> -	struct drm_encoder *encoder;
> -	struct drm_bridge *iter;
> -
> -	if (!bridge)
> -		return;
> -
> -	encoder = bridge->encoder;
> -	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> -		if (iter->funcs->disable)
> -			iter->funcs->disable(iter);
> -
> -		if (iter == bridge)
> -			break;
> -	}
> -}
> -EXPORT_SYMBOL(drm_bridge_chain_disable);
> -
> -/**
> - * drm_bridge_chain_post_disable - cleans up after disabling all bridges in the
> - *				   encoder chain
> - * @bridge: bridge control structure
> - *
> - * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
> - * encoder chain, starting from the first bridge to the last. These are called
> - * after completing the encoder's prepare op.
> - *
> - * Note: the bridge passed should be the one closest to the encoder
> - */
> -void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
> -{
> -	struct drm_encoder *encoder;
> -
> -	if (!bridge)
> -		return;
> -
> -	encoder = bridge->encoder;
> -	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> -		if (bridge->funcs->post_disable)
> -			bridge->funcs->post_disable(bridge);
> -	}
> -}
> -EXPORT_SYMBOL(drm_bridge_chain_post_disable);
> -
>  /**
>   * drm_bridge_chain_mode_set - set proposed mode for all bridges in the
>   *			       encoder chain
> @@ -569,61 +514,6 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>  
> -/**
> - * drm_bridge_chain_pre_enable - prepares for enabling all bridges in the
> - *				 encoder chain
> - * @bridge: bridge control structure
> - *
> - * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
> - * chain, starting from the last bridge to the first. These are called
> - * before calling the encoder's commit op.
> - *
> - * Note: the bridge passed should be the one closest to the encoder
> - */
> -void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> -{
> -	struct drm_encoder *encoder;
> -	struct drm_bridge *iter;
> -
> -	if (!bridge)
> -		return;
> -
> -	encoder = bridge->encoder;
> -	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> -		if (iter->funcs->pre_enable)
> -			iter->funcs->pre_enable(iter);
> -
> -		if (iter == bridge)
> -			break;
> -	}
> -}
> -EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
> -
> -/**
> - * drm_bridge_chain_enable - enables all bridges in the encoder chain
> - * @bridge: bridge control structure
> - *
> - * Calls &drm_bridge_funcs.enable op for all the bridges in the encoder
> - * chain, starting from the first bridge to the last. These are called
> - * after completing the encoder's commit op.
> - *
> - * Note that the bridge passed should be the one closest to the encoder
> - */
> -void drm_bridge_chain_enable(struct drm_bridge *bridge)
> -{
> -	struct drm_encoder *encoder;
> -
> -	if (!bridge)
> -		return;
> -
> -	encoder = bridge->encoder;
> -	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> -		if (bridge->funcs->enable)
> -			bridge->funcs->enable(bridge);
> -	}
> -}
> -EXPORT_SYMBOL(drm_bridge_chain_enable);
> -
>  /**
>   * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
>   * @bridge: bridge control structure
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index fc00304be643..ed2e4a8fe510 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -297,12 +297,6 @@ struct drm_bridge_funcs {
>  	 * not enable the display link feeding the next bridge in the chain (if
>  	 * there is one) when this callback is called.
>  	 *
> -	 * Note that this function will only be invoked in the context of an
> -	 * atomic commit. It will not be invoked from
> -	 * &drm_bridge_chain_pre_enable. It would be prudent to also provide an
> -	 * implementation of @pre_enable if you are expecting driver calls into
> -	 * &drm_bridge_chain_pre_enable.
> -	 *
>  	 * The @atomic_pre_enable callback is optional.
>  	 */
>  	void (*atomic_pre_enable)(struct drm_bridge *bridge,
> @@ -323,11 +317,6 @@ struct drm_bridge_funcs {
>  	 * callback must enable the display link feeding the next bridge in the
>  	 * chain if there is one.
>  	 *
> -	 * Note that this function will only be invoked in the context of an
> -	 * atomic commit. It will not be invoked from &drm_bridge_chain_enable.
> -	 * It would be prudent to also provide an implementation of @enable if
> -	 * you are expecting driver calls into &drm_bridge_chain_enable.
> -	 *
>  	 * The @atomic_enable callback is optional.
>  	 */
>  	void (*atomic_enable)(struct drm_bridge *bridge,
> @@ -345,12 +334,6 @@ struct drm_bridge_funcs {
>  	 * The bridge can assume that the display pipe (i.e. clocks and timing
>  	 * signals) feeding it is still running when this callback is called.
>  	 *
> -	 * Note that this function will only be invoked in the context of an
> -	 * atomic commit. It will not be invoked from
> -	 * &drm_bridge_chain_disable. It would be prudent to also provide an
> -	 * implementation of @disable if you are expecting driver calls into
> -	 * &drm_bridge_chain_disable.
> -	 *
>  	 * The @atomic_disable callback is optional.
>  	 */
>  	void (*atomic_disable)(struct drm_bridge *bridge,
> @@ -370,13 +353,6 @@ struct drm_bridge_funcs {
>  	 * signals) feeding it is no longer running when this callback is
>  	 * called.
>  	 *
> -	 * Note that this function will only be invoked in the context of an
> -	 * atomic commit. It will not be invoked from
> -	 * &drm_bridge_chain_post_disable.
> -	 * It would be prudent to also provide an implementation of
> -	 * @post_disable if you are expecting driver calls into
> -	 * &drm_bridge_chain_post_disable.
> -	 *
>  	 * The @atomic_post_disable callback is optional.
>  	 */
>  	void (*atomic_post_disable)(struct drm_bridge *bridge,
> @@ -868,13 +844,9 @@ enum drm_mode_status
>  drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
>  			    const struct drm_display_info *info,
>  			    const struct drm_display_mode *mode);
> -void drm_bridge_chain_disable(struct drm_bridge *bridge);
> -void drm_bridge_chain_post_disable(struct drm_bridge *bridge);
>  void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
>  			       const struct drm_display_mode *mode,
>  			       const struct drm_display_mode *adjusted_mode);
> -void drm_bridge_chain_pre_enable(struct drm_bridge *bridge);
> -void drm_bridge_chain_enable(struct drm_bridge *bridge);
>  
>  int drm_atomic_bridge_chain_check(struct drm_bridge *bridge,
>  				  struct drm_crtc_state *crtc_state,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Laurent Pinchart]

Hello,

On Mon, Feb 07, 2022 at 02:34:34PM -0800, Doug Anderson wrote:
> On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Slightly awkward to fish out the display_info when we aren't creating
> > own connector.  But I don't see an obvious better way.
> >
> > v3:
> >  - Rebased and dropped the ti_sn_bridge_get_bpp() patch
> >    as this was solved in a different way (Sam)
> >
> > v2:
> >  - Remove error return with NO_CONNECTOR flag (Rob)
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Rob Clark <robdclark@chromium.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Robert Foss <robert.foss@linaro.org>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> This is fine by me assuming we can fix the previous patches.
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Likewise,

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

> NOTE: to me, this isn't something to do _instead_ of my patch [1] but
> _in addition_ to it. ${SUBJECT} patch transitions us to a more modern
> approach of having the connector created elsewhere but doesn't remove
> the old fallback code. Might as well clean the fallback code up unless
> you think it's going to simply be deleted right away or something?

I don't really mind either way, but I of course would favour removal of
connector support as soon as practical :-)

> [1] https://lore.kernel.org/r/20220204161245.v2.1.I3ab26b7f197cc56c874246a43e57913e9c2c1028@changeid

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt

[Sam Ravnborg]

On Tue, Feb 08, 2022 at 02:44:25AM +0200, Laurent Pinchart wrote:
> Hello,
> 
> On Mon, Feb 07, 2022 at 02:32:45PM -0800, Doug Anderson wrote:
> > On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> > >
> > > There is a number of bridge drivers that supports a single media bus
> > > format for DSI. Add a helper to avoid duplicating the code.
> > >
> > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
> > >  include/drm/drm_atomic_helper.h     |  7 +++++
> > >  2 files changed, 48 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index a7a05e1e26bb..94f313dc196f 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,
> > >         return input_fmts;
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt);
> > > +
> > > +/**
> > > + * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format
> > 
> > Is the description right? It's called "input" format but it defines an
> > output format?
> > 
> > 
> > > + * @bridge: bridge control structure
> > > + * @bridge_state: new bridge state
> > > + * @crtc_state: new CRTC state
> > > + * @conn_state: new connector state
> > > + * @output_fmt: tested output bus format
> > > + * @num_input_fmts: will contain the size of the returned array
> > 
> > Maybe indicate that it's always 1 in the comments?
> > 
> > > + *
> > > + * This helper is an implementation of the
> > > + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports
> > > + * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
> > > + *
> > > + * RETURNS
> > 
> > kernel-doc can't parse this return syntax and warns:
> > 
> > warning: No description found for return value of
> > 'drm_atomic_helper_bridge_dsi_input_bus_fmt'
> > 
> > 
> > > + * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
> > > + * or NULL if the allocation failed
> > > + */
> > > +u32 *
> > > +drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
> > > +                                          struct drm_bridge_state *bridge_state,
> > > +                                          struct drm_crtc_state *crtc_state,
> > > +                                          struct drm_connector_state *conn_state,
> > > +                                          u32 output_fmt,
> > > +                                          unsigned int *num_input_fmts)
> > > +{
> > > +       u32 *input_fmts;
> > > +
> > > +       *num_input_fmts = 0;
> > > +
> > > +       input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);
> > 
> > I probably wouldn't have bothered with `kcalloc` for something that's
> > always just one value and you're setting it. Why not just
> > 
> > input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
> > 
> > ...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you
> > said it didn't compile?
> > 
> > Also: if it was common for others to want to provide fixed formats, I
> > wonder about adding a helper function that did most of the work here?
> > Dunno what it would be named since it's already a bit a of handful,
> > but I'd expect to call it like:
> > 
> > static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 };
> > return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats)
> > 
> > Then my_neat_helper() could do kmemdup() on the array passed and fill
> > in "num_output_formats" to be either the array size of 0 (if the
> > kmemdup failed).
> 
> I quite like that approach. We could even have a wrapper macro that adds
> the ARRAY_SIZE() argument automatically.

Agree, will try to give that a spin.
And will then also process all the nice feedback from Douglas.

	Sam

Re: [PATCH v1 5/9] drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state

[Sam Ravnborg]

On Mon, Feb 07, 2022 at 02:34:10PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > To prepare for DRM_BRIDGE_ATTACH_NO_CONNECTOR support,
> > fix so the bpc is found using the output format.
> >
> > This avoids the use of the connector stored in the private data.
> >
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Robert Foss <robert.foss@linaro.org>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index d681ab68205c..dc6ec40bc1ef 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -33,6 +33,7 @@
> >  #include <drm/drm_panel.h>
> >  #include <drm/drm_print.h>
> >  #include <drm/drm_probe_helper.h>
> > +#include <drm/media-bus-format.h>
> >
> >  #define SN_DEVICE_REV_REG                      0x08
> >  #define SN_DPPLL_SRC_REG                       0x0A
> > @@ -823,9 +824,11 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> >         regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> >  }
> >
> > -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> > +static unsigned int ti_sn_bridge_get_bpp(struct drm_bridge_state *bridge_state)
> >  {
> > -       if (pdata->connector.display_info.bpc <= 6)
> > +       int bpc = media_bus_format_to_bpc(bridge_state->output_bus_cfg.format);
> > +
> > +       if (bpc <= 6)
> >                 return 18;
> >         else
> >                 return 24;
> 
> Unfortunately this doesn't work as hoped. :(
> `bridge_state->output_bus_cfg.format` is 0 in my testing...
> 
> ...and then media_bus_format_to_bpc() returns an error, which is
> negative and <= 6. That's not super ideal...
> 
> I guess this gets down to what the output bus format is _supposed_ to
> be for eDP. Based on my understanding of eDP there isn't really a
> concept of a fixed "bus format" that the panel ought to work at. There
> is a concept of the number of bits per component (6, 8, 10, 12) that a
> panel can run at but then after that I believe the format is standard,
> or at least it's something that's dynamic / negotiated. Also note that
> the format on the bus is more related to the current mode we're
> running the panel in than some inherent property of the panel. For
> instance, a 10 bpc panel can likely have data provided in 8 bpc and 6
> bpc. I've also seen 6 bpc panels that can accept 8 bpc data and can
> dither it. In any case, this type of stuff is really all dynamic for
> eDP. The old value we were looking at was really being interpreted as
> the "max" bpc that the panel could run in and we'd choose to run the
> panel in 8 bpc if the panel supported it and 6 bpc otherwise (this
> bridge chip only supports 6bpc or 8bpc).
> 
> So I guess the question is: how do we move forward here?

I skipped a patch to find the connector - will try to give that a spin
again.
Thanks for the testing and the feedback!

	Sam


> 
> Do we need to somehow figure out how to get "output_bus_cfg.format"
> filled in? Any suggestions for how to do that? Do we just implement
> atomic_get_output_bus_fmts() and then pick one of
> MEDIA_BUS_FMT_RGB666_1X18 or MEDIA_BUS_FMT_RBG888_1X24 based on the
> bpc in the connector state? ...or do we just list both of them and
> something magically will pick the right one? Hrm, I tried that and it
> didn't magically work, but I didn't debug further...
> 
> One thing I realized is that, at least in theory, we might not know
> whether we want to run in 6 bpc or 8 bpc until we've done link
> training. It's at least somewhat plausible that there could be edge
> cases where we'd want to fall back to the low bpc if link training
> failed at the higher bpc. The driver doesn't support that right now,
> but ideally the design wouldn't preclude it. At the moment link
> training happens in enable. Maybe that's a problem to worry about
> another day, though?
> 
> -Doug

Re: [PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Sam Ravnborg]

Hi Douglas,

On Mon, Feb 07, 2022 at 02:34:34PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Slightly awkward to fish out the display_info when we aren't creating
> > own connector.  But I don't see an obvious better way.
> >
> > v3:
> >  - Rebased and dropped the ti_sn_bridge_get_bpp() patch
> >    as this was solved in a different way (Sam)
> >
> > v2:
> >  - Remove error return with NO_CONNECTOR flag (Rob)
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Rob Clark <robdclark@chromium.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Robert Foss <robert.foss@linaro.org>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> This is fine by me assuming we can fix the previous patches.
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> NOTE: to me, this isn't something to do _instead_ of my patch [1] but
> _in addition_ to it. ${SUBJECT} patch transitions us to a more modern
> approach of having the connector created elsewhere but doesn't remove
> the old fallback code. Might as well clean the fallback code up unless
> you think it's going to simply be deleted right away or something?

Until we know all users have NO_CONNECTOR support we need the connector
creation code.
Please just go ahead with your patches as you already got r-b from
Laurent. For the debugfs patch I will look at it soonish unless someone
beats me.

	Sam


> 
> [1] https://lore.kernel.org/r/20220204161245.v2.1.I3ab26b7f197cc56c874246a43e57913e9c2c1028@changeid

Re: [PATCH v1 1/9] drm/bridge: add DRM_BRIDGE_STATE_OPS macro

[Sam Ravnborg]

Hi Laurent,

On Tue, Feb 08, 2022 at 02:30:42AM +0200, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Sun, Feb 06, 2022 at 04:43:57PM +0100, Sam Ravnborg wrote:
> > The DRM_BRIDGE_STATE_OPS can be used as shortcut for bridge drivers that
> > do not subclass drm_bridge_state to assign the default operations for
> > reset, duplicate and destroy of the state.
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Robert Foss <robert.foss@linaro.org>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> >  include/drm/drm_bridge.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 061d87313fac..fc00304be643 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -935,4 +935,16 @@ static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> >  }
> >  #endif
> >  
> > +/**
> > + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs
> 
> I'd name the macro DRM_BRIDGE_STATE_DEFAULT_OPS.
OK, done.

> 
> > + *
> > + * Bridge driver that do not subclass &drm_bridge_state can use the helpers
> > + * for reset, duplicate, and destroy. This macro provides a shortcut for
> > + * setting the helpers in the &drm_bridge_funcs structure.
> > + */
> > +#define DRM_BRIDGE_STATE_OPS \
> > +	.atomic_reset = drm_atomic_helper_bridge_reset,				\
> > +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,	\
> > +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state
> 
> I'm not a big fan of such macros for a small number of operations, as I
> find that it obfuscates the code a bit, but that could change once I get
> used to the new macro :-)
The use of the macro is a nice signal that all the relevant default
operations are specified - no need to look up if all are included.

I have on my TODO to update all relevant bridge drivers to use it.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  #endif
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Kieran Bingham]

Hi Sam,

Quoting Sam Ravnborg (2022-02-06 15:44:02)
> From: Rob Clark <robdclark@chromium.org>
> 
> Slightly awkward to fish out the display_info when we aren't creating
> own connector.  But I don't see an obvious better way.
> 
> v3:
>  - Rebased and dropped the ti_sn_bridge_get_bpp() patch
>    as this was solved in a different way (Sam)
> 
> v2:
>  - Remove error return with NO_CONNECTOR flag (Rob)
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index dc6ec40bc1ef..a9041dfd2ae5 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -746,11 +746,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>         int ret;
>  
> -       if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -               DRM_ERROR("Fix bridge driver to make connector optional!");
> -               return -EINVAL;
> -       }
> -
>         pdata->aux.drm_dev = bridge->dev;
>         ret = drm_dp_aux_register(&pdata->aux);
>         if (ret < 0) {
> @@ -758,12 +753,14 @@ 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;
> +       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> +               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;
> +               /* We never want the next bridge to *also* create a connector: */
> +               flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> +       }
>  
>         /* Attach the next bridge */
>         ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
> @@ -774,7 +771,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>         return 0;
>  
>  err_dsi_host:
> -       drm_connector_cleanup(&pdata->connector);
> +       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))

I think this is unreachable / always false due to the
   flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR.


I've solved this locally by doing:

-	/* Attach the next bridge */
+	/*
+	 * Attach the next bridge We never want the next bridge to *also* create
+	 * a connector:
+	 */
 	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
-				&pdata->bridge, flags);
+				&pdata->bridge,
+				flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (ret < 0)
 		goto err_initted_aux;

+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+		return 0;
+
 	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;


Which also fixes the support for
  flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR

--
Regards

Kieran

> +               drm_connector_cleanup(&pdata->connector);
>  err_conn_init:
>         drm_dp_aux_unregister(&pdata->aux);
>         return ret;
> -- 
> 2.32.0
>

Re: [PATCH v1 0/9] drm/bridge: ps8640 and ti-sn65dsi86 updates

[Kieran Bingham]

Hi Sam,

Quoting Sam Ravnborg (2022-02-06 19:09:11)
> > 
> > The code builds - but needs testing.
> 
> Hrmff, no it does not build. The fixes was by accident not included.
> Will wait a bit for feedback before posting a v2.
> 
>         Sam

Do you have any plan to send a v2 on this series?

I have built up a series to extend the ti-sn65dsi86 which is now based
on this. (which means I'll have an implied Tested-by: tag for these as
well).

--
Kieran

Re: [PATCH v1 0/9] drm/bridge: ps8640 and ti-sn65dsi86 updates

[Sam Ravnborg]

Hi Kieran,

On Wed, Jun 22, 2022 at 11:07:26AM +0100, Kieran Bingham wrote:
> Hi Sam,
> 
> Quoting Sam Ravnborg (2022-02-06 19:09:11)
> > > 
> > > The code builds - but needs testing.
> > 
> > Hrmff, no it does not build. The fixes was by accident not included.
> > Will wait a bit for feedback before posting a v2.
> > 
> >         Sam
> 
> Do you have any plan to send a v2 on this series?
> 
> I have built up a series to extend the ti-sn65dsi86 which is now based
> on this. (which means I'll have an implied Tested-by: tag for these as
> well).

That is too good not to do something about it. I will give it a spin
this weekend - I do not have time until then.

	Sam