[PATCH 0/2] drm: lcdif: Add i.MX93 LCDIF support

[PATCH 0/2] drm: lcdif: Add i.MX93 LCDIF support

[Liu Ying]

Hi,

This patch set aims to add i.MX93 LCDIF display controller support
in the existing LCDIF DRM driver.  The LCDIF embedded in i.MX93 SoC
is essentially the same to those embedded in i.MX8mp SoC.  Through
internal bridges, i.MX93 LCDIF may drive a MIPI DSI display or a LVDS
display or a parallel display.

Patch 1/2 adds device tree binding support for i.MX93 LCDIF in the
existing fsl,lcdif.yaml.

Patch 2/2 adds i.MX93 LCDIF support in the existing LCDIF DRM driver.

Liu Ying (2):
  dt-bindings: lcdif: Add i.MX93 LCDIF support
  drm: lcdif: Add i.MX93 LCDIF support

 .../bindings/display/fsl,lcdif.yaml           |   7 +-
 drivers/gpu/drm/mxsfb/lcdif_drv.c             |  73 ++++++-
 drivers/gpu/drm/mxsfb/lcdif_drv.h             |   6 +-
 drivers/gpu/drm/mxsfb/lcdif_kms.c             | 206 ++++++++++++------
 4 files changed, 214 insertions(+), 78 deletions(-)

-- 
2.37.1

[PATCH 1/2] dt-bindings: lcdif: Add i.MX93 LCDIF support

[Liu Ying]

There is one LCDIF embedded in i.MX93 SoC to connect with
MIPI DSI controller through LCDIF cross line pattern(controlled
by mediamix blk-ctrl) or connect with LVDS display bridge(LDB)
directly or connect with a parallel display through parallel
display format(also controlled by mediamix blk-ctrl).  i.MX93
LCDIF IP is essentially the same to i.MX8MP LCDIF IP.  Add device
tree binding for i.MX93 LCDIF.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
index 75b4efd70ba8..fc11ab5fc465 100644
--- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
+++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
@@ -21,6 +21,7 @@ properties:
           - fsl,imx28-lcdif
           - fsl,imx6sx-lcdif
           - fsl,imx8mp-lcdif
+          - fsl,imx93-lcdif
       - items:
           - enum:
               - fsl,imx6sl-lcdif
@@ -88,7 +89,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: fsl,imx8mp-lcdif
+            enum:
+              - fsl,imx8mp-lcdif
+              - fsl,imx93-lcdif
     then:
       properties:
         clocks:
@@ -107,6 +110,7 @@ allOf:
               enum:
                 - fsl,imx6sx-lcdif
                 - fsl,imx8mp-lcdif
+                - fsl,imx93-lcdif
     then:
       properties:
         clocks:
@@ -123,6 +127,7 @@ allOf:
               - fsl,imx8mm-lcdif
               - fsl,imx8mn-lcdif
               - fsl,imx8mp-lcdif
+              - fsl,imx93-lcdif
     then:
       required:
         - power-domains
-- 
2.37.1

Re: [PATCH 1/2] dt-bindings: lcdif: Add i.MX93 LCDIF support

[Krzysztof Kozlowski]

On 23/01/2023 08:23, Liu Ying wrote:
> There is one LCDIF embedded in i.MX93 SoC to connect with
> MIPI DSI controller through LCDIF cross line pattern(controlled
> by mediamix blk-ctrl) or connect with LVDS display bridge(LDB)
> directly or connect with a parallel display through parallel
> display format(also controlled by mediamix blk-ctrl).  i.MX93
> LCDIF IP is essentially the same to i.MX8MP LCDIF IP.  Add device
> tree binding for i.MX93 LCDIF.
> 

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: lcdif: Add i.MX93 LCDIF support

[Marek Vasut]

On 1/23/23 08:23, Liu Ying wrote:
> There is one LCDIF embedded in i.MX93 SoC to connect with
> MIPI DSI controller through LCDIF cross line pattern(controlled
> by mediamix blk-ctrl) or connect with LVDS display bridge(LDB)
> directly or connect with a parallel display through parallel
> display format(also controlled by mediamix blk-ctrl).  i.MX93
> LCDIF IP is essentially the same to i.MX8MP LCDIF IP.  Add device
> tree binding for i.MX93 LCDIF.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>

Reviewed-by: Marek Vasut <marex@denx.de>

[PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Liu Ying]

The LCDIF embedded in i.MX93 SoC is essentially the same to those
in i.MX8mp SoC.  However, i.MX93 LCDIF may connect with MIPI DSI
controller through LCDIF cross line pattern(controlled by mediamix
blk-ctrl) or connect with LVDS display bridge(LDB) directly or a
parallel display(also through mediamix blk-ctrl), so add multiple
encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the
LCDIF DRM driver and find a bridge to attach the relevant encoder's
chain when needed.  While at it, derive lcdif_crtc_state structure
from drm_crtc_state structure to introduce bus_format and bus_flags
states so that the next downstream bridges may use consistent bus
format and bus flags.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/mxsfb/lcdif_drv.c |  73 +++++++++--
 drivers/gpu/drm/mxsfb/lcdif_drv.h |   6 +-
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 206 ++++++++++++++++++++----------
 3 files changed, 208 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c
index cc2ceb301b96..4d41f6b6eb14 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
@@ -9,13 +9,16 @@
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_fbdev_generic.h>
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
@@ -38,21 +41,70 @@ static const struct drm_mode_config_helper_funcs lcdif_mode_config_helpers = {
 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
 };
 
+static const struct drm_encoder_funcs lcdif_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
 {
-	struct drm_device *drm = lcdif->drm;
+	struct device *dev = lcdif->drm->dev;
+	struct device_node *ep;
 	struct drm_bridge *bridge;
 	int ret;
 
-	bridge = devm_drm_of_get_bridge(drm->dev, drm->dev->of_node, 0, 0);
-	if (IS_ERR(bridge))
-		return PTR_ERR(bridge);
-
-	ret = drm_bridge_attach(&lcdif->encoder, bridge, NULL, 0);
-	if (ret)
-		return dev_err_probe(drm->dev, ret, "Failed to attach bridge\n");
-
-	lcdif->bridge = bridge;
+	for_each_endpoint_of_node(dev->of_node, ep) {
+		struct device_node *remote;
+		struct of_endpoint of_ep;
+		struct drm_encoder *encoder;
+
+		remote = of_graph_get_remote_port_parent(ep);
+		if (!remote || !of_device_is_available(remote)) {
+			of_node_put(remote);
+			continue;
+		}
+		of_node_put(remote);
+
+		ret = of_graph_parse_endpoint(ep, &of_ep);
+		if (ret < 0) {
+			dev_err(dev, "Failed to parse endpoint %pOF\n", ep);
+			of_node_put(ep);
+			return ret;
+		}
+
+		if (of_ep.id >= MAX_DISPLAYS) {
+			dev_warn(dev, "invalid endpoint id %u\n", of_ep.id);
+			continue;
+		}
+
+		bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, of_ep.id);
+		if (IS_ERR(bridge)) {
+			of_node_put(ep);
+			return dev_err_probe(dev, PTR_ERR(bridge),
+					     "Failed to get bridge for endpoint%u\n",
+					     of_ep.id);
+		}
+
+		encoder = &lcdif->encoders[of_ep.id];
+		encoder->possible_crtcs = drm_crtc_mask(&lcdif->crtc);
+		ret = drm_encoder_init(lcdif->drm, encoder, &lcdif_encoder_funcs,
+				       DRM_MODE_ENCODER_NONE, NULL);
+		if (ret) {
+			dev_err(dev, "Failed to initialize encoder for endpoint%u: %d\n",
+				of_ep.id, ret);
+			of_node_put(ep);
+			return ret;
+		}
+
+		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
+		if (ret) {
+			of_node_put(ep);
+			return dev_err_probe(dev, ret,
+					     "Failed to attach bridge for endpoint%u\n",
+					     of_ep.id);
+		}
+
+		lcdif->bridges[of_ep.id] = bridge;
+	}
 
 	return 0;
 }
@@ -199,6 +251,7 @@ static const struct drm_driver lcdif_driver = {
 
 static const struct of_device_id lcdif_dt_ids[] = {
 	{ .compatible = "fsl,imx8mp-lcdif" },
+	{ .compatible = "fsl,imx93-lcdif" },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, lcdif_dt_ids);
diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.h b/drivers/gpu/drm/mxsfb/lcdif_drv.h
index 6cdba6e20c02..64459c3904e8 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_drv.h
+++ b/drivers/gpu/drm/mxsfb/lcdif_drv.h
@@ -14,6 +14,8 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_plane.h>
 
+#define MAX_DISPLAYS	3
+
 struct clk;
 
 struct lcdif_drm_private {
@@ -30,8 +32,8 @@ struct lcdif_drm_private {
 		/* i.MXRT does support overlay planes, add them here. */
 	} planes;
 	struct drm_crtc			crtc;
-	struct drm_encoder		encoder;
-	struct drm_bridge		*bridge;
+	struct drm_encoder		encoders[MAX_DISPLAYS];
+	struct drm_bridge		*bridges[MAX_DISPLAYS];
 };
 
 static inline struct lcdif_drm_private *
diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index 262bc43b1079..ba36447ed900 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -17,6 +17,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_color_mgmt.h>
+#include <drm/drm_connector.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_fb_dma_helper.h>
@@ -30,6 +31,18 @@
 #include "lcdif_drv.h"
 #include "lcdif_regs.h"
 
+struct lcdif_crtc_state {
+	struct drm_crtc_state	base;
+	u32			bus_format;
+	u32			bus_flags;
+};
+
+static inline struct lcdif_crtc_state *
+to_lcdif_crtc_state(struct drm_crtc_state *s)
+{
+	return container_of(s, struct lcdif_crtc_state, base);
+}
+
 /* -----------------------------------------------------------------------------
  * CRTC
  */
@@ -385,48 +398,109 @@ static void lcdif_reset_block(struct lcdif_drm_private *lcdif)
 	readl(lcdif->base + LCDC_V8_CTRL);
 }
 
-static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
-				     struct drm_plane_state *plane_state,
-				     struct drm_bridge_state *bridge_state,
-				     const u32 bus_format)
+static void lcdif_crtc_mode_set_nofb(struct drm_crtc_state *crtc_state,
+				     struct drm_plane_state *plane_state)
 {
-	struct drm_device *drm = lcdif->crtc.dev;
-	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
-	u32 bus_flags = 0;
-
-	if (lcdif->bridge && lcdif->bridge->timings)
-		bus_flags = lcdif->bridge->timings->input_bus_flags;
-	else if (bridge_state)
-		bus_flags = bridge_state->input_bus_cfg.flags;
+	struct lcdif_crtc_state *lcdif_crtc_state = to_lcdif_crtc_state(crtc_state);
+	struct drm_device *drm = crtc_state->crtc->dev;
+	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm);
+	struct drm_display_mode *m = &crtc_state->adjusted_mode;
 
 	DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: %dkHz)\n",
 			     m->crtc_clock,
 			     (int)(clk_get_rate(lcdif->clk) / 1000));
 	DRM_DEV_DEBUG_DRIVER(drm->dev, "Bridge bus_flags: 0x%08X\n",
-			     bus_flags);
+			     lcdif_crtc_state->bus_flags);
 	DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
 
 	/* Mandatory eLCDIF reset as per the Reference Manual */
 	lcdif_reset_block(lcdif);
 
-	lcdif_set_formats(lcdif, plane_state, bus_format);
+	lcdif_set_formats(lcdif, plane_state, lcdif_crtc_state->bus_format);
 
-	lcdif_set_mode(lcdif, bus_flags);
+	lcdif_set_mode(lcdif, lcdif_crtc_state->bus_flags);
 }
 
 static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
 				   struct drm_atomic_state *state)
 {
+	struct drm_device *drm = crtc->dev;
 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
 									  crtc);
+	struct lcdif_crtc_state *lcdif_crtc_state = to_lcdif_crtc_state(crtc_state);
 	bool has_primary = crtc_state->plane_mask &
 			   drm_plane_mask(crtc->primary);
+	struct drm_connector_state *connector_state;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder;
+	struct drm_bridge_state *bridge_state;
+	struct drm_bridge *bridge;
+	u32 bus_format, bus_flags;
+	bool format_set = false, flags_set = false;
+	int ret, i;
 
 	/* The primary plane has to be enabled when the CRTC is active. */
 	if (crtc_state->active && !has_primary)
 		return -EINVAL;
 
-	return drm_atomic_add_affected_planes(state, crtc);
+	ret = drm_atomic_add_affected_planes(state, crtc);
+	if (ret)
+		return ret;
+
+	/* Try to find consistent bus format and flags across first bridges. */
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
+		if (!connector_state->crtc)
+			continue;
+
+		encoder = connector_state->best_encoder;
+
+		bridge = drm_bridge_chain_get_first_bridge(encoder);
+		if (!bridge)
+			continue;
+
+		/*
+		 * Use bridge's input bus format if possible, otherwise use
+		 * MEDIA_BUS_FMT_FIXED.
+		 */
+		bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
+		if (!bridge_state)
+			bus_format = MEDIA_BUS_FMT_FIXED;
+		else
+			bus_format = bridge_state->input_bus_cfg.format;
+
+		if (bus_format == MEDIA_BUS_FMT_FIXED) {
+			dev_warn(drm->dev,
+				 "[ENCODER:%d:%s]'s bridge does not provide bus format, assuming MEDIA_BUS_FMT_RGB888_1X24.\n"
+				 "Please fix bridge driver by handling atomic_get_input_bus_fmts.\n",
+				 encoder->base.id, encoder->name);
+			bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+		}
+
+		if (!format_set) {
+			lcdif_crtc_state->bus_format = bus_format;
+			format_set = true;
+		} else if (lcdif_crtc_state->bus_format != bus_format) {
+			DRM_DEV_DEBUG_DRIVER(drm->dev, "inconsistent bus format\n");
+			return -EINVAL;
+		}
+
+		if (bridge->timings)
+			bus_flags = bridge->timings->input_bus_flags;
+		else if (bridge_state)
+			bus_flags = bridge_state->input_bus_cfg.flags;
+		else
+			bus_flags = 0;
+
+		if (!flags_set) {
+			lcdif_crtc_state->bus_flags = bus_flags;
+			flags_set = true;
+		} else if (lcdif_crtc_state->bus_flags != bus_flags) {
+			DRM_DEV_DEBUG_DRIVER(drm->dev, "inconsistent bus flags\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
 }
 
 static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
@@ -458,44 +532,21 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
 				     struct drm_atomic_state *state)
 {
 	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
-	struct drm_plane_state *new_pstate = drm_atomic_get_new_plane_state(state,
-									    crtc->primary);
+	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
+										 crtc->primary);
 	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
-	struct drm_bridge_state *bridge_state = NULL;
 	struct drm_device *drm = lcdif->drm;
-	u32 bus_format = 0;
 	dma_addr_t paddr;
 
-	/* If there is a bridge attached to the LCDIF, use its bus format */
-	if (lcdif->bridge) {
-		bridge_state =
-			drm_atomic_get_new_bridge_state(state,
-							lcdif->bridge);
-		if (!bridge_state)
-			bus_format = MEDIA_BUS_FMT_FIXED;
-		else
-			bus_format = bridge_state->input_bus_cfg.format;
-
-		if (bus_format == MEDIA_BUS_FMT_FIXED) {
-			dev_warn_once(drm->dev,
-				      "Bridge does not provide bus format, assuming MEDIA_BUS_FMT_RGB888_1X24.\n"
-				      "Please fix bridge driver by handling atomic_get_input_bus_fmts.\n");
-			bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-		}
-	}
-
-	/* If all else fails, default to RGB888_1X24 */
-	if (!bus_format)
-		bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-
 	clk_set_rate(lcdif->clk, m->crtc_clock * 1000);
 
 	pm_runtime_get_sync(drm->dev);
 
-	lcdif_crtc_mode_set_nofb(lcdif, new_pstate, bridge_state, bus_format);
+	lcdif_crtc_mode_set_nofb(new_crtc_state, new_plane_state);
 
 	/* Write cur_buf as well to avoid an initial corrupt frame */
-	paddr = drm_fb_dma_get_gem_addr(new_pstate->fb, new_pstate, 0);
+	paddr = drm_fb_dma_get_gem_addr(new_plane_state->fb, new_plane_state, 0);
 	if (paddr) {
 		writel(lower_32_bits(paddr),
 		       lcdif->base + LCDC_V8_CTRLDESCL_LOW0_4);
@@ -529,6 +580,46 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc *crtc,
 	pm_runtime_put_sync(drm->dev);
 }
 
+static void lcdif_crtc_reset(struct drm_crtc *crtc)
+{
+	struct lcdif_crtc_state *state;
+
+	if (crtc->state)
+		__drm_atomic_helper_crtc_destroy_state(crtc->state);
+
+	kfree(to_lcdif_crtc_state(crtc->state));
+	crtc->state = NULL;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (state)
+		__drm_atomic_helper_crtc_reset(crtc, &state->base);
+}
+
+static struct drm_crtc_state *
+lcdif_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
+{
+	struct lcdif_crtc_state *old = to_lcdif_crtc_state(crtc->state);
+	struct lcdif_crtc_state *new;
+
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return NULL;
+
+	__drm_atomic_helper_crtc_duplicate_state(crtc, &new->base);
+
+	new->bus_format = old->bus_format;
+	new->bus_flags = old->bus_flags;
+
+	return &new->base;
+}
+
+static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc,
+					    struct drm_crtc_state *state)
+{
+	__drm_atomic_helper_crtc_destroy_state(state);
+	kfree(to_lcdif_crtc_state(state));
+}
+
 static int lcdif_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
@@ -557,24 +648,16 @@ static const struct drm_crtc_helper_funcs lcdif_crtc_helper_funcs = {
 };
 
 static const struct drm_crtc_funcs lcdif_crtc_funcs = {
-	.reset = drm_atomic_helper_crtc_reset,
+	.reset = lcdif_crtc_reset,
 	.destroy = drm_crtc_cleanup,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
-	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.atomic_duplicate_state = lcdif_crtc_atomic_duplicate_state,
+	.atomic_destroy_state = lcdif_crtc_atomic_destroy_state,
 	.enable_vblank = lcdif_crtc_enable_vblank,
 	.disable_vblank = lcdif_crtc_disable_vblank,
 };
 
-/* -----------------------------------------------------------------------------
- * Encoder
- */
-
-static const struct drm_encoder_funcs lcdif_encoder_funcs = {
-	.destroy = drm_encoder_cleanup,
-};
-
 /* -----------------------------------------------------------------------------
  * Planes
  */
@@ -667,7 +750,6 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
 					BIT(DRM_COLOR_YCBCR_BT2020);
 	const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
 				     BIT(DRM_COLOR_YCBCR_FULL_RANGE);
-	struct drm_encoder *encoder = &lcdif->encoder;
 	struct drm_crtc *crtc = &lcdif->crtc;
 	int ret;
 
@@ -691,13 +773,7 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
 		return ret;
 
 	drm_crtc_helper_add(crtc, &lcdif_crtc_helper_funcs);
-	ret = drm_crtc_init_with_planes(lcdif->drm, crtc,
-					&lcdif->planes.primary, NULL,
-					&lcdif_crtc_funcs, NULL);
-	if (ret)
-		return ret;
-
-	encoder->possible_crtcs = drm_crtc_mask(crtc);
-	return drm_encoder_init(lcdif->drm, encoder, &lcdif_encoder_funcs,
-				DRM_MODE_ENCODER_NONE, NULL);
+	return drm_crtc_init_with_planes(lcdif->drm, crtc,
+					 &lcdif->planes.primary, NULL,
+					 &lcdif_crtc_funcs, NULL);
 }
-- 
2.37.1

Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Lothar Waßmann]

Hi,

On Mon, 23 Jan 2023 15:23:58 +0800 Liu Ying wrote:
> The LCDIF embedded in i.MX93 SoC is essentially the same to those
> in i.MX8mp SoC.  However, i.MX93 LCDIF may connect with MIPI DSI
> controller through LCDIF cross line pattern(controlled by mediamix
> blk-ctrl) or connect with LVDS display bridge(LDB) directly or a
> parallel display(also through mediamix blk-ctrl), so add multiple
> encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the
> LCDIF DRM driver and find a bridge to attach the relevant encoder's
> chain when needed.  While at it, derive lcdif_crtc_state structure
> from drm_crtc_state structure to introduce bus_format and bus_flags
> states so that the next downstream bridges may use consistent bus
> format and bus flags.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  drivers/gpu/drm/mxsfb/lcdif_drv.c |  73 +++++++++--
>  drivers/gpu/drm/mxsfb/lcdif_drv.h |   6 +-
>  drivers/gpu/drm/mxsfb/lcdif_kms.c | 206 ++++++++++++++++++++----------
>  3 files changed, 208 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> index cc2ceb301b96..4d41f6b6eb14 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -9,13 +9,16 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_drv.h>
> +#include <drm/drm_encoder.h>
>  #include <drm/drm_fbdev_generic.h>
>  #include <drm/drm_gem_dma_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> @@ -38,21 +41,70 @@ static const struct drm_mode_config_helper_funcs lcdif_mode_config_helpers = {
>  	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>  };
>  
> +static const struct drm_encoder_funcs lcdif_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
>  static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
>  {
> -	struct drm_device *drm = lcdif->drm;
> +	struct device *dev = lcdif->drm->dev;
> +	struct device_node *ep;
>  	struct drm_bridge *bridge;
>  	int ret;
>  
> -	bridge = devm_drm_of_get_bridge(drm->dev, drm->dev->of_node, 0, 0);
> -	if (IS_ERR(bridge))
> -		return PTR_ERR(bridge);
> -
> -	ret = drm_bridge_attach(&lcdif->encoder, bridge, NULL, 0);
> -	if (ret)
> -		return dev_err_probe(drm->dev, ret, "Failed to attach bridge\n");
> -
> -	lcdif->bridge = bridge;
> +	for_each_endpoint_of_node(dev->of_node, ep) {
> +		struct device_node *remote;
> +		struct of_endpoint of_ep;
> +		struct drm_encoder *encoder;
> +
> +		remote = of_graph_get_remote_port_parent(ep);
> +		if (!remote || !of_device_is_available(remote)) {
'!remote ||' is redundant, since of_device_is_available already checks
for a NULL pointer.

[...]

> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index 262bc43b1079..ba36447ed900 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
[...]
> @@ -529,6 +580,46 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc *crtc,
>  	pm_runtime_put_sync(drm->dev);
>  }
>  
> +static void lcdif_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct lcdif_crtc_state *state;
> +
> +	if (crtc->state)
> +		__drm_atomic_helper_crtc_destroy_state(crtc->state);
> +
> +	kfree(to_lcdif_crtc_state(crtc->state));
>
If crtc-state can be NULL at this point, this will only work as long as
'base' is the first member of the lcdif_crtc_state struct (which
currently is the case, but there is no guarantee that this will always
be this way), otherwise the if clause above is not needed.



Lothar Waßmann

Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Liu Ying]

On Mon, 2023-01-23 at 09:13 +0100, Lothar Waßmann wrote:
> Hi,

Hi,

> 
> On Mon, 23 Jan 2023 15:23:58 +0800 Liu Ying wrote:
> > The LCDIF embedded in i.MX93 SoC is essentially the same to those
> > in i.MX8mp SoC.  However, i.MX93 LCDIF may connect with MIPI DSI
> > controller through LCDIF cross line pattern(controlled by mediamix
> > blk-ctrl) or connect with LVDS display bridge(LDB) directly or a
> > parallel display(also through mediamix blk-ctrl), so add multiple
> > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the
> > LCDIF DRM driver and find a bridge to attach the relevant encoder's
> > chain when needed.  While at it, derive lcdif_crtc_state structure
> > from drm_crtc_state structure to introduce bus_format and bus_flags
> > states so that the next downstream bridges may use consistent bus
> > format and bus flags.
> > 
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> >  drivers/gpu/drm/mxsfb/lcdif_drv.c |  73 +++++++++--
> >  drivers/gpu/drm/mxsfb/lcdif_drv.h |   6 +-
> >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 206 ++++++++++++++++++++----
> > ------
> >  3 files changed, 208 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > index cc2ceb301b96..4d41f6b6eb14 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > @@ -9,13 +9,16 @@
> >  #include <linux/dma-mapping.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_graph.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> >  #include <drm/drm_drv.h>
> > +#include <drm/drm_encoder.h>
> >  #include <drm/drm_fbdev_generic.h>
> >  #include <drm/drm_gem_dma_helper.h>
> >  #include <drm/drm_gem_framebuffer_helper.h>
> > @@ -38,21 +41,70 @@ static const struct
> > drm_mode_config_helper_funcs lcdif_mode_config_helpers = {
> >  	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> >  };
> >  
> > +static const struct drm_encoder_funcs lcdif_encoder_funcs = {
> > +	.destroy = drm_encoder_cleanup,
> > +};
> > +
> >  static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
> >  {
> > -	struct drm_device *drm = lcdif->drm;
> > +	struct device *dev = lcdif->drm->dev;
> > +	struct device_node *ep;
> >  	struct drm_bridge *bridge;
> >  	int ret;
> >  
> > -	bridge = devm_drm_of_get_bridge(drm->dev, drm->dev->of_node, 0,
> > 0);
> > -	if (IS_ERR(bridge))
> > -		return PTR_ERR(bridge);
> > -
> > -	ret = drm_bridge_attach(&lcdif->encoder, bridge, NULL, 0);
> > -	if (ret)
> > -		return dev_err_probe(drm->dev, ret, "Failed to attach
> > bridge\n");
> > -
> > -	lcdif->bridge = bridge;
> > +	for_each_endpoint_of_node(dev->of_node, ep) {
> > +		struct device_node *remote;
> > +		struct of_endpoint of_ep;
> > +		struct drm_encoder *encoder;
> > +
> > +		remote = of_graph_get_remote_port_parent(ep);
> > +		if (!remote || !of_device_is_available(remote)) {
> 
> '!remote ||' is redundant, since of_device_is_available already
> checks
> for a NULL pointer.

of_device_is_available() does check for a NULL pointer when it calls
__of_device_is_available() (after it takes devtree_lock raw spinlock
and then releases devtree_lock).  So, if remote is NULL, '!remote ||'
exits the if clause a bit earlier without calling
of_device_is_available() to take/release devtree_lock, which means a
little bit better performance.  But, people may say that the
performance gain is trivial. drm_of_component_probe() in drm_of.c also 
checks '!remote' before checking '!of_device_is_available(remote)'. Do
you still think that both drm_of_component_probe() and
lcdif_attach_bridge() should drop '!remote ||'?

> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index 262bc43b1079..ba36447ed900 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> 
> [...]
> > @@ -529,6 +580,46 @@ static void lcdif_crtc_atomic_disable(struct
> > drm_crtc *crtc,
> >  	pm_runtime_put_sync(drm->dev);
> >  }
> >  
> > +static void lcdif_crtc_reset(struct drm_crtc *crtc)
> > +{
> > +	struct lcdif_crtc_state *state;
> > +
> > +	if (crtc->state)
> > +		__drm_atomic_helper_crtc_destroy_state(crtc->state);
> > +
> > +	kfree(to_lcdif_crtc_state(crtc->state));
> > 
> 
> If crtc-state can be NULL at this point, this will only work as long
> as
> 'base' is the first member of the lcdif_crtc_state struct (which
> currently is the case, but there is no guarantee that this will
> always
> be this way), otherwise the if clause above is not needed.

crtc->state can be NULL when e.g. at driver load time, like kerneldoc
for drm_atomic_helper_crtc_reset() mentions.  I may add a comment for
the 'base' member to note that it should be always the first member of
the lcdif_crtc_state structure.  Do you think that will be ok?

Thanks,
Liu Ying

> 
> 
> 
> Lothar Waßmann

Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Marek Vasut]

On 1/23/23 08:23, Liu Ying wrote:
> The LCDIF embedded in i.MX93 SoC is essentially the same to those
> in i.MX8mp SoC.  However, i.MX93 LCDIF may connect with MIPI DSI
> controller through LCDIF cross line pattern(controlled by mediamix
> blk-ctrl) or connect with LVDS display bridge(LDB) directly or a
> parallel display(also through mediamix blk-ctrl), so add multiple
> encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the
> LCDIF DRM driver and find a bridge to attach the relevant encoder's
> chain when needed.  While at it, derive lcdif_crtc_state structure
> from drm_crtc_state structure to introduce bus_format and bus_flags
> states so that the next downstream bridges may use consistent bus
> format and bus flags.

Would it be possible to split this patch into preparatory clean up and 
i.MX93 addition ? It seems like the patch is doing two things according 
to the commit message.

Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Liu Ying]

On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote:
> On 1/23/23 08:23, Liu Ying wrote:
> > The LCDIF embedded in i.MX93 SoC is essentially the same to those
> > in i.MX8mp SoC.  However, i.MX93 LCDIF may connect with MIPI DSI
> > controller through LCDIF cross line pattern(controlled by mediamix
> > blk-ctrl) or connect with LVDS display bridge(LDB) directly or a
> > parallel display(also through mediamix blk-ctrl), so add multiple
> > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the
> > LCDIF DRM driver and find a bridge to attach the relevant encoder's
> > chain when needed.  While at it, derive lcdif_crtc_state structure
> > from drm_crtc_state structure to introduce bus_format and bus_flags
> > states so that the next downstream bridges may use consistent bus
> > format and bus flags.
> 
> Would it be possible to split this patch into preparatory clean up
> and 
> i.MX93 addition ? It seems like the patch is doing two things
> according 
> to the commit message.

IMHO, all the patch does is for i.MX93 addition, not for clean up. 
Note that the single LCDIF embedded in i.MX93 SoC may connect with MIPI
DSI/LVDS/parallel related bridges to drive triple displays
_simultaneously_ in theory, while the three LCDIF instances embedded in
i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays respectively(one
LCDIF maps to one display).  The multiple encoders addition and the new
checks for consistent bus format and bus flags are only for i.MX93
LCDIF, not for i.MX8mp LCDIF.  Also, I think the multiple encoders
addition and the new checks should be done together - if the new checks
come first, then the new checks do not make sense(no multiple displays
driven by LCDIF); if the new checks come later, then it would be a bug
to allow inconsistent bus format and bus flags across the next
downstream bridges when only adding multiple encoders support(also, I
don't know which encoder's bridge should determine the LCDIF output bus
format and bus flags, since the three encoders come together with the
three next bridges).

Regards,
Liu Ying

Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Alexander Stein]

Hi,

Am Dienstag, 24. Januar 2023, 08:59:39 CET schrieb Liu Ying:
> On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote:
> > On 1/23/23 08:23, Liu Ying wrote:
> > > The LCDIF embedded in i.MX93 SoC is essentially the same to those
> > > in i.MX8mp SoC.  However, i.MX93 LCDIF may connect with MIPI DSI
> > > controller through LCDIF cross line pattern(controlled by mediamix
> > > blk-ctrl) or connect with LVDS display bridge(LDB) directly or a
> > > parallel display(also through mediamix blk-ctrl), so add multiple
> > > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the
> > > LCDIF DRM driver and find a bridge to attach the relevant encoder's
> > > chain when needed.  While at it, derive lcdif_crtc_state structure
> > > from drm_crtc_state structure to introduce bus_format and bus_flags
> > > states so that the next downstream bridges may use consistent bus
> > > format and bus flags.
> > 
> > Would it be possible to split this patch into preparatory clean up
> > and
> > i.MX93 addition ? It seems like the patch is doing two things
> > according
> > to the commit message.
> 
> IMHO, all the patch does is for i.MX93 addition, not for clean up.
> Note that the single LCDIF embedded in i.MX93 SoC may connect with MIPI
> DSI/LVDS/parallel related bridges to drive triple displays
> _simultaneously_ in theory, while the three LCDIF instances embedded in
> i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays respectively(one
> LCDIF maps to one display).  The multiple encoders addition and the new
> checks for consistent bus format and bus flags are only for i.MX93
> LCDIF, not for i.MX8mp LCDIF.  Also, I think the multiple encoders
> addition and the new checks should be done together - if the new checks
> come first, then the new checks do not make sense(no multiple displays
> driven by LCDIF); 

You are right on this one, but on the other hand there are lot of preparing 
patches already. Even if it is useless by itself, having the bus format & flag 
checks in a separate patch, it is easier to review, IMHO.

> if the new checks come later, then it would be a bug
> to allow inconsistent bus format and bus flags across the next
> downstream bridges when only adding multiple encoders support(also, I
> don't know which encoder's bridge should determine the LCDIF output bus
> format and bus flags, since the three encoders come together with the
> three next bridges).

Agreed, this order is a no-go.

Best regards,
Alexander

> Regards,
> Liu Ying

Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Liu Ying]

On Tue, 2023-01-24 at 12:15 +0100, Alexander Stein wrote:
> Hi,

Hi,

> 
> Am Dienstag, 24. Januar 2023, 08:59:39 CET schrieb Liu Ying:
> > On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote:
> > > On 1/23/23 08:23, Liu Ying wrote:
> > > > The LCDIF embedded in i.MX93 SoC is essentially the same to
> > > > those
> > > > in i.MX8mp SoC.  However, i.MX93 LCDIF may connect with MIPI
> > > > DSI
> > > > controller through LCDIF cross line pattern(controlled by
> > > > mediamix
> > > > blk-ctrl) or connect with LVDS display bridge(LDB) directly or
> > > > a
> > > > parallel display(also through mediamix blk-ctrl), so add
> > > > multiple
> > > > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in
> > > > the
> > > > LCDIF DRM driver and find a bridge to attach the relevant
> > > > encoder's
> > > > chain when needed.  While at it, derive lcdif_crtc_state
> > > > structure
> > > > from drm_crtc_state structure to introduce bus_format and
> > > > bus_flags
> > > > states so that the next downstream bridges may use consistent
> > > > bus
> > > > format and bus flags.
> > > 
> > > Would it be possible to split this patch into preparatory clean
> > > up
> > > and
> > > i.MX93 addition ? It seems like the patch is doing two things
> > > according
> > > to the commit message.
> > 
> > IMHO, all the patch does is for i.MX93 addition, not for clean up.
> > Note that the single LCDIF embedded in i.MX93 SoC may connect with
> > MIPI
> > DSI/LVDS/parallel related bridges to drive triple displays
> > _simultaneously_ in theory, while the three LCDIF instances
> > embedded in
> > i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays
> > respectively(one
> > LCDIF maps to one display).  The multiple encoders addition and the
> > new
> > checks for consistent bus format and bus flags are only for i.MX93
> > LCDIF, not for i.MX8mp LCDIF.  Also, I think the multiple encoders
> > addition and the new checks should be done together - if the new
> > checks
> > come first, then the new checks do not make sense(no multiple
> > displays
> > driven by LCDIF); 
> 
> You are right on this one, but on the other hand there are lot of
> preparing 
> patches already. Even if it is useless by itself, having the bus
> format & flag 
> checks in a separate patch, it is easier to review, IMHO.

TBH, this way of separating patch looks too artificial. It's odd to
check consistent bus format and bus flags for multiple bridges while
there is only one encoder.

What I can do is to make a separate patch to introduce the
lcdif_crtc_state structure(with bus_format and bus_flags members) and
determine the format and flags in ->atomic_check() instead of
->atomic_enable().  And then, add i.MX93 LCDIF support with an another
patch which adds multiple encoders+bridges and new checks for
consistent bus format and bus flags.  Sounds good?

Regards,
Liu Ying  

> 
> > if the new checks come later, then it would be a bug
> > to allow inconsistent bus format and bus flags across the next
> > downstream bridges when only adding multiple encoders support(also,
> > I
> > don't know which encoder's bridge should determine the LCDIF output
> > bus
> > format and bus flags, since the three encoders come together with
> > the
> > three next bridges).
> 
> Agreed, this order is a no-go.
> 
> Best regards,
> Alexander
> 
> > Regards,
> > Liu Ying
> 
> 
> 
> 

Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Marek Vasut]

On 1/24/23 12:15, Alexander Stein wrote:
> Hi,

Hi,

> Am Dienstag, 24. Januar 2023, 08:59:39 CET schrieb Liu Ying:
>> On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote:
>>> On 1/23/23 08:23, Liu Ying wrote:
>>>> The LCDIF embedded in i.MX93 SoC is essentially the same to those
>>>> in i.MX8mp SoC.  However, i.MX93 LCDIF may connect with MIPI DSI
>>>> controller through LCDIF cross line pattern(controlled by mediamix
>>>> blk-ctrl) or connect with LVDS display bridge(LDB) directly or a
>>>> parallel display(also through mediamix blk-ctrl), so add multiple
>>>> encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the
>>>> LCDIF DRM driver and find a bridge to attach the relevant encoder's
>>>> chain when needed.  While at it, derive lcdif_crtc_state structure
>>>> from drm_crtc_state structure to introduce bus_format and bus_flags
>>>> states so that the next downstream bridges may use consistent bus
>>>> format and bus flags.
>>>
>>> Would it be possible to split this patch into preparatory clean up
>>> and
>>> i.MX93 addition ? It seems like the patch is doing two things
>>> according
>>> to the commit message.
>>
>> IMHO, all the patch does is for i.MX93 addition, not for clean up.
>> Note that the single LCDIF embedded in i.MX93 SoC may connect with MIPI
>> DSI/LVDS/parallel related bridges to drive triple displays
>> _simultaneously_ in theory, while the three LCDIF instances embedded in
>> i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays respectively(one
>> LCDIF maps to one display).  The multiple encoders addition and the new
>> checks for consistent bus format and bus flags are only for i.MX93
>> LCDIF, not for i.MX8mp LCDIF.  Also, I think the multiple encoders
>> addition and the new checks should be done together - if the new checks
>> come first, then the new checks do not make sense(no multiple displays
>> driven by LCDIF);
> 
> You are right on this one, but on the other hand there are lot of preparing
> patches already. Even if it is useless by itself, having the bus format & flag
> checks in a separate patch, it is easier to review, IMHO.

I agree on the ease of review.

[...]