[PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
and is completely different from the LCDIFv3 found in i.MX23 in that it
has a completely scrambled register layout compared to all previous LCDIF
variants. The new LCDIFv3 also supports 36bit address space. However,
except for the complete bit reshuffling, this is still LCDIF and it still
works like one.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
index 900a56cae80e6..9831ab53a053d 100644
--- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
+++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
@@ -28,6 +28,7 @@ properties:
               - fsl,imx7d-lcdif
               - fsl,imx8mm-lcdif
               - fsl,imx8mn-lcdif
+              - fsl,imx8mp-lcdif
               - fsl,imx8mq-lcdif
           - const: fsl,imx6sx-lcdif
 
-- 
2.34.1

[PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
and is completely different from the LCDIFv3 found in i.MX23 in that it
has a completely scrambled register layout compared to all previous LCDIF
variants. The new LCDIFv3 also supports 36bit address space. However,
except for the complete bit reshuffling, this is still LCDIF and it still
works like one.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
index 900a56cae80e6..9831ab53a053d 100644
--- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
+++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
@@ -28,6 +28,7 @@ properties:
               - fsl,imx7d-lcdif
               - fsl,imx8mm-lcdif
               - fsl,imx8mn-lcdif
+              - fsl,imx8mp-lcdif
               - fsl,imx8mq-lcdif
           - const: fsl,imx6sx-lcdif
 
-- 
2.34.1

[PATCH 2/9] drm: mxsfb: Simplify LCDIF clock handling

[Marek Vasut]

The current clock handling in the LCDIF driver is a convoluted mess.
Implement runtime PM ops which turn the clock ON and OFF and let the
pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable
and .atomic_disable callbacks turn the clock ON and OFF at the right
time.

This requires slight reordering in mxsfb_crtc_atomic_enable() and
mxsfb_crtc_atomic_disable(), since all the register accesses must
happen only with clock enabled and clock frequency configuration
must happen with clock disabled.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 100 +++++++++++++++++-------------
 drivers/gpu/drm/mxsfb/mxsfb_kms.c |  27 +++-----
 2 files changed, 66 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 375f26d4a4172..bb15e32d8a014 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -72,18 +72,6 @@ static const struct mxsfb_devdata mxsfb_devdata[] = {
 	},
 };
 
-void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb)
-{
-	if (mxsfb->clk_axi)
-		clk_prepare_enable(mxsfb->clk_axi);
-}
-
-void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb)
-{
-	if (mxsfb->clk_axi)
-		clk_disable_unprepare(mxsfb->clk_axi);
-}
-
 static struct drm_framebuffer *
 mxsfb_fb_create(struct drm_device *dev, struct drm_file *file_priv,
 		const struct drm_mode_fb_cmd2 *mode_cmd)
@@ -172,13 +160,9 @@ static void mxsfb_irq_disable(struct drm_device *drm)
 {
 	struct mxsfb_drm_private *mxsfb = drm->dev_private;
 
-	mxsfb_enable_axi_clk(mxsfb);
-
 	/* Disable and clear VBLANK IRQ */
 	writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR);
 	writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-
-	mxsfb_disable_axi_clk(mxsfb);
 }
 
 static int mxsfb_irq_install(struct drm_device *dev, int irq)
@@ -224,33 +208,33 @@ static int mxsfb_load(struct drm_device *drm,
 	if (IS_ERR(mxsfb->clk))
 		return PTR_ERR(mxsfb->clk);
 
-	mxsfb->clk_axi = devm_clk_get(drm->dev, "axi");
+	mxsfb->clk_axi = devm_clk_get_optional(drm->dev, "axi");
 	if (IS_ERR(mxsfb->clk_axi))
-		mxsfb->clk_axi = NULL;
+		return PTR_ERR(mxsfb->clk_axi);
 
-	mxsfb->clk_disp_axi = devm_clk_get(drm->dev, "disp_axi");
+	mxsfb->clk_disp_axi = devm_clk_get_optional(drm->dev, "disp_axi");
 	if (IS_ERR(mxsfb->clk_disp_axi))
-		mxsfb->clk_disp_axi = NULL;
+		return PTR_ERR(mxsfb->clk_disp_axi);
+
+	platform_set_drvdata(pdev, drm);
 
 	ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
 	if (ret)
 		return ret;
 
-	pm_runtime_enable(drm->dev);
-
 	/* Modeset init */
 	drm_mode_config_init(drm);
 
 	ret = mxsfb_kms_init(mxsfb);
 	if (ret < 0) {
 		dev_err(drm->dev, "Failed to initialize KMS pipeline\n");
-		goto err_vblank;
+		return ret;
 	}
 
 	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
 	if (ret < 0) {
 		dev_err(drm->dev, "Failed to initialise vblank\n");
-		goto err_vblank;
+		return ret;
 	}
 
 	/* Start with vertical blanking interrupt reporting disabled. */
@@ -260,7 +244,7 @@ static int mxsfb_load(struct drm_device *drm,
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
 			dev_err(drm->dev, "Cannot connect bridge: %d\n", ret);
-		goto err_vblank;
+		return ret;
 	}
 
 	drm->mode_config.min_width	= MXSFB_MIN_XRES;
@@ -274,44 +258,37 @@ static int mxsfb_load(struct drm_device *drm,
 
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
-		goto err_vblank;
+		return ret;
 	mxsfb->irq = ret;
 
-	pm_runtime_get_sync(drm->dev);
 	ret = mxsfb_irq_install(drm, mxsfb->irq);
-	pm_runtime_put_sync(drm->dev);
-
 	if (ret < 0) {
 		dev_err(drm->dev, "Failed to install IRQ handler\n");
-		goto err_vblank;
+		return ret;
 	}
 
 	drm_kms_helper_poll_init(drm);
 
-	platform_set_drvdata(pdev, drm);
-
 	drm_helper_hpd_irq_event(drm);
 
-	return 0;
-
-err_vblank:
-	pm_runtime_disable(drm->dev);
+	pm_runtime_enable(drm->dev);
 
-	return ret;
+	return 0;
 }
 
 static void mxsfb_unload(struct drm_device *drm)
 {
+	pm_runtime_get_sync(drm->dev);
+
 	drm_kms_helper_poll_fini(drm);
 	drm_mode_config_cleanup(drm);
 
-	pm_runtime_get_sync(drm->dev);
 	mxsfb_irq_uninstall(drm);
+
 	pm_runtime_put_sync(drm->dev);
+	pm_runtime_disable(drm->dev);
 
 	drm->dev_private = NULL;
-
-	pm_runtime_disable(drm->dev);
 }
 
 DEFINE_DRM_GEM_CMA_FOPS(fops);
@@ -388,23 +365,60 @@ static void mxsfb_shutdown(struct platform_device *pdev)
 	drm_atomic_helper_shutdown(drm);
 }
 
-#ifdef CONFIG_PM_SLEEP
+static int mxsfb_rpm_suspend(struct device *dev)
+{
+	struct drm_device *drm = dev_get_drvdata(dev);
+	struct mxsfb_drm_private *mxsfb = drm->dev_private;
+
+	/* These clock supply the DISPLAY CLOCK Domain */
+	clk_disable_unprepare(mxsfb->clk);
+	/* These clock supply the System Bus, AXI, Write Path, LFIFO */
+	clk_disable_unprepare(mxsfb->clk_disp_axi);
+	/* These clock supply the Control Bus, APB, APBH Ctrl Registers */
+	clk_disable_unprepare(mxsfb->clk_axi);
+
+	return 0;
+}
+
+static int mxsfb_rpm_resume(struct device *dev)
+{
+	struct drm_device *drm = dev_get_drvdata(dev);
+	struct mxsfb_drm_private *mxsfb = drm->dev_private;
+
+	/* These clock supply the Control Bus, APB, APBH Ctrl Registers */
+	clk_prepare_enable(mxsfb->clk_axi);
+	/* These clock supply the System Bus, AXI, Write Path, LFIFO */
+	clk_prepare_enable(mxsfb->clk_disp_axi);
+	/* These clock supply the DISPLAY CLOCK Domain */
+	clk_prepare_enable(mxsfb->clk);
+
+	return 0;
+}
+
 static int mxsfb_suspend(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
+	int ret;
 
-	return drm_mode_config_helper_suspend(drm);
+	ret = drm_mode_config_helper_suspend(drm);
+	if (ret)
+		return ret;
+
+	return mxsfb_rpm_suspend(dev);
 }
 
 static int mxsfb_resume(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
 
+	mxsfb_rpm_resume(dev);
+
 	return drm_mode_config_helper_resume(drm);
 }
-#endif
 
 static const struct dev_pm_ops mxsfb_pm_ops = {
+	.runtime_suspend = mxsfb_rpm_suspend,
+	.runtime_resume = mxsfb_rpm_resume,
 	SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume)
 };
 
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 4cfb6c0016799..657b6afbbf1f9 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -100,10 +100,6 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
 {
 	u32 reg;
 
-	if (mxsfb->clk_disp_axi)
-		clk_prepare_enable(mxsfb->clk_disp_axi);
-	clk_prepare_enable(mxsfb->clk);
-
 	/* Increase number of outstanding requests on all supported IPs */
 	if (mxsfb->devdata->has_ctrl2) {
 		reg = readl(mxsfb->base + LCDC_V4_CTRL2);
@@ -168,10 +164,6 @@ static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb)
 	reg = readl(mxsfb->base + LCDC_VDCTRL4);
 	reg &= ~VDCTRL4_SYNC_SIGNALS_ON;
 	writel(reg, mxsfb->base + LCDC_VDCTRL4);
-
-	clk_disable_unprepare(mxsfb->clk);
-	if (mxsfb->clk_disp_axi)
-		clk_disable_unprepare(mxsfb->clk_disp_axi);
 }
 
 /*
@@ -250,8 +242,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 
 	mxsfb_set_formats(mxsfb, bus_format);
 
-	clk_set_rate(mxsfb->clk, m->crtc_clock * 1000);
-
 	if (mxsfb->bridge && mxsfb->bridge->timings)
 		bus_flags = mxsfb->bridge->timings->input_bus_flags;
 
@@ -346,16 +336,12 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 				     struct drm_atomic_state *state)
 {
 	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev);
+	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
 	struct drm_bridge_state *bridge_state;
 	struct drm_device *drm = mxsfb->drm;
 	u32 bus_format = 0;
 	dma_addr_t paddr;
 
-	pm_runtime_get_sync(drm->dev);
-	mxsfb_enable_axi_clk(mxsfb);
-
-	drm_crtc_vblank_on(crtc);
-
 	/* If there is a bridge attached to the LCDIF, use its bus format */
 	if (mxsfb->bridge) {
 		bridge_state =
@@ -382,6 +368,10 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	if (!bus_format)
 		bus_format = MEDIA_BUS_FMT_RGB888_1X24;
 
+	clk_set_rate(mxsfb->clk, m->crtc_clock * 1000);
+
+	pm_runtime_get_sync(drm->dev);
+
 	mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
 
 	/* Write cur_buf as well to avoid an initial corrupt frame */
@@ -392,6 +382,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	}
 
 	mxsfb_enable_controller(mxsfb);
+
+	drm_crtc_vblank_on(crtc);
 }
 
 static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
@@ -401,6 +393,8 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
 	struct drm_device *drm = mxsfb->drm;
 	struct drm_pending_vblank_event *event;
 
+	drm_crtc_vblank_off(crtc);
+
 	mxsfb_disable_controller(mxsfb);
 
 	spin_lock_irq(&drm->event_lock);
@@ -411,9 +405,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
 	}
 	spin_unlock_irq(&drm->event_lock);
 
-	drm_crtc_vblank_off(crtc);
-
-	mxsfb_disable_axi_clk(mxsfb);
 	pm_runtime_put_sync(drm->dev);
 }
 
-- 
2.34.1

[PATCH 3/9] drm: mxsfb: Simplify LCDIF IRQ handling

[Marek Vasut]

The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation
from the LCDIF block already and this is called in mxsfb_load() before
request_irq(), so explicitly disabling IRQ using custom function like
mxsfb_irq_disable() is not needed, remove it. The request_irq() call
would return -ENOTCONN if IRQ is IRQ_NOTCONNECTED already, so remove
the unnecessary check. Finally, remove both mxsfb_irq_install() and
mxsfb_irq_uninstall() as well, since they are no longer useful.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 38 +++++++------------------------
 1 file changed, 8 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index bb15e32d8a014..11298df50917c 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -156,33 +156,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void mxsfb_irq_disable(struct drm_device *drm)
-{
-	struct mxsfb_drm_private *mxsfb = drm->dev_private;
-
-	/* Disable and clear VBLANK IRQ */
-	writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-	writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-}
-
-static int mxsfb_irq_install(struct drm_device *dev, int irq)
-{
-	if (irq == IRQ_NOTCONNECTED)
-		return -ENOTCONN;
-
-	mxsfb_irq_disable(dev);
-
-	return request_irq(irq, mxsfb_irq_handler, 0,  dev->driver->name, dev);
-}
-
-static void mxsfb_irq_uninstall(struct drm_device *dev)
-{
-	struct mxsfb_drm_private *mxsfb = dev->dev_private;
-
-	mxsfb_irq_disable(dev);
-	free_irq(mxsfb->irq, dev);
-}
-
 static int mxsfb_load(struct drm_device *drm,
 		      const struct mxsfb_devdata *devdata)
 {
@@ -261,7 +234,8 @@ static int mxsfb_load(struct drm_device *drm,
 		return ret;
 	mxsfb->irq = ret;
 
-	ret = mxsfb_irq_install(drm, mxsfb->irq);
+	ret = request_irq(mxsfb->irq, mxsfb_irq_handler, 0,
+			  drm->driver->name, drm);
 	if (ret < 0) {
 		dev_err(drm->dev, "Failed to install IRQ handler\n");
 		return ret;
@@ -278,16 +252,20 @@ static int mxsfb_load(struct drm_device *drm,
 
 static void mxsfb_unload(struct drm_device *drm)
 {
+	struct mxsfb_drm_private *mxsfb = drm->dev_private;
+
 	pm_runtime_get_sync(drm->dev);
 
+	drm_crtc_vblank_off(&mxsfb->crtc);
+
 	drm_kms_helper_poll_fini(drm);
 	drm_mode_config_cleanup(drm);
 
-	mxsfb_irq_uninstall(drm);
-
 	pm_runtime_put_sync(drm->dev);
 	pm_runtime_disable(drm->dev);
 
+	free_irq(mxsfb->irq, drm->dev);
+
 	drm->dev_private = NULL;
 }
 
-- 
2.34.1

[PATCH 4/9] drm: mxsfb: Wrap FIFO reset and comments into mxsfb_reset_block()

[Marek Vasut]

Wrap FIFO reset and comments into mxsfb_reset_block(), this is a clean up.
No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 36 +++++++++++++++++--------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 657b6afbbf1f9..015b289d93a3c 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -183,6 +183,12 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
 {
 	int ret;
 
+	/*
+	 * It seems, you can't re-program the controller if it is still
+	 * running. This may lead to shifted pictures (FIFO issue?), so
+	 * first stop the controller and drain its FIFOs.
+	 */
+
 	ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_SFTRST);
 	if (ret)
 		return ret;
@@ -193,7 +199,20 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
 	if (ret)
 		return ret;
 
-	return clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE);
+	ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE);
+	if (ret)
+		return ret;
+
+	/* Clear the FIFOs */
+	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
+	readl(mxsfb->base + LCDC_CTRL1);
+	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
+	readl(mxsfb->base + LCDC_CTRL1);
+
+	if (mxsfb->devdata->has_overlay)
+		writel(0, mxsfb->base + LCDC_AS_CTRL);
+
+	return 0;
 }
 
 static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
@@ -220,26 +239,11 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
 	int err;
 
-	/*
-	 * It seems, you can't re-program the controller if it is still
-	 * running. This may lead to shifted pictures (FIFO issue?), so
-	 * first stop the controller and drain its FIFOs.
-	 */
-
 	/* Mandatory eLCDIF reset as per the Reference Manual */
 	err = mxsfb_reset_block(mxsfb);
 	if (err)
 		return;
 
-	/* Clear the FIFOs */
-	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
-	readl(mxsfb->base + LCDC_CTRL1);
-	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-	readl(mxsfb->base + LCDC_CTRL1);
-
-	if (mxsfb->devdata->has_overlay)
-		writel(0, mxsfb->base + LCDC_AS_CTRL);
-
 	mxsfb_set_formats(mxsfb, bus_format);
 
 	if (mxsfb->bridge && mxsfb->bridge->timings)
-- 
2.34.1

[PATCH 5/9] drm: mxsfb: Move mxsfb_get_fb_paddr() away from register IO functions

[Marek Vasut]

Move mxsfb_get_fb_paddr() out of the way, away from register IO functions.
This is a clean up. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 015b289d93a3c..7b0abd0472aae 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -43,6 +43,21 @@ static u32 set_hsync_pulse_width(struct mxsfb_drm_private *mxsfb, u32 val)
 		mxsfb->devdata->hs_wdth_shift;
 }
 
+static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
+{
+	struct drm_framebuffer *fb = plane->state->fb;
+	struct drm_gem_cma_object *gem;
+
+	if (!fb)
+		return 0;
+
+	gem = drm_fb_cma_get_gem_obj(fb, 0);
+	if (!gem)
+		return 0;
+
+	return gem->paddr;
+}
+
 /*
  * Setup the MXSFB registers for decoding the pixels out of the framebuffer and
  * outputting them on the bus.
@@ -215,21 +230,6 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
 	return 0;
 }
 
-static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
-{
-	struct drm_framebuffer *fb = plane->state->fb;
-	struct drm_gem_cma_object *gem;
-
-	if (!fb)
-		return 0;
-
-	gem = drm_fb_cma_get_gem_obj(fb, 0);
-	if (!gem)
-		return 0;
-
-	return gem->paddr;
-}
-
 static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 				     const u32 bus_format)
 {
-- 
2.34.1

[PATCH 6/9] drm: mxsfb: Factor out mxsfb_set_mode()

[Marek Vasut]

Pull mode registers programming from mxsfb_enable_controller() into
dedicated function mxsfb_set_mode(). This is a clean up. No functional
change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 96 +++++++++++++++++--------------
 1 file changed, 52 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 7b0abd0472aae..14f5cc590a51b 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -111,6 +111,57 @@ static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb,
 	writel(ctrl, mxsfb->base + LCDC_CTRL);
 }
 
+static void mxsfb_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags)
+{
+	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
+	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
+
+	writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) |
+	       TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay),
+	       mxsfb->base + mxsfb->devdata->transfer_count);
+
+	vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start;
+
+	vdctrl0 = VDCTRL0_ENABLE_PRESENT |	/* Always in DOTCLOCK mode */
+		  VDCTRL0_VSYNC_PERIOD_UNIT |
+		  VDCTRL0_VSYNC_PULSE_WIDTH_UNIT |
+		  VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len);
+	if (m->flags & DRM_MODE_FLAG_PHSYNC)
+		vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;
+	if (m->flags & DRM_MODE_FLAG_PVSYNC)
+		vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;
+	/* Make sure Data Enable is high active by default */
+	if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
+		vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;
+	/*
+	 * DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric,
+	 * controllers VDCTRL0_DOTCLK is display centric.
+	 * Drive on positive edge       -> display samples on falling edge
+	 * DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING
+	 */
+	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
+		vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;
+
+	writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
+
+	/* Frame length in lines. */
+	writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1);
+
+	/* Line length in units of clocks or pixels. */
+	hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start;
+	writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) |
+	       VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal),
+	       mxsfb->base + LCDC_VDCTRL2);
+
+	writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) |
+	       SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),
+	       mxsfb->base + LCDC_VDCTRL3);
+
+	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
+	       mxsfb->base + LCDC_VDCTRL4);
+
+}
+
 static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
 {
 	u32 reg;
@@ -236,7 +287,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 	struct drm_device *drm = mxsfb->crtc.dev;
 	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
 	u32 bus_flags = mxsfb->connector->display_info.bus_flags;
-	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
 	int err;
 
 	/* Mandatory eLCDIF reset as per the Reference Manual */
@@ -256,49 +306,7 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 			     bus_flags);
 	DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
 
-	writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) |
-	       TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay),
-	       mxsfb->base + mxsfb->devdata->transfer_count);
-
-	vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start;
-
-	vdctrl0 = VDCTRL0_ENABLE_PRESENT |	/* Always in DOTCLOCK mode */
-		  VDCTRL0_VSYNC_PERIOD_UNIT |
-		  VDCTRL0_VSYNC_PULSE_WIDTH_UNIT |
-		  VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len);
-	if (m->flags & DRM_MODE_FLAG_PHSYNC)
-		vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;
-	if (m->flags & DRM_MODE_FLAG_PVSYNC)
-		vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;
-	/* Make sure Data Enable is high active by default */
-	if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
-		vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;
-	/*
-	 * DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric,
-	 * controllers VDCTRL0_DOTCLK is display centric.
-	 * Drive on positive edge       -> display samples on falling edge
-	 * DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING
-	 */
-	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
-		vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;
-
-	writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
-
-	/* Frame length in lines. */
-	writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1);
-
-	/* Line length in units of clocks or pixels. */
-	hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start;
-	writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) |
-	       VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal),
-	       mxsfb->base + LCDC_VDCTRL2);
-
-	writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) |
-	       SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),
-	       mxsfb->base + LCDC_VDCTRL3);
-
-	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
-	       mxsfb->base + LCDC_VDCTRL4);
+	mxsfb_set_mode(mxsfb, bus_flags);
 }
 
 static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc,
-- 
2.34.1

[PATCH 7/9] drm: mxsfb: Reorder mxsfb_crtc_mode_set_nofb()

[Marek Vasut]

Reorder mxsfb_crtc_mode_set_nofb() such that all functions which perform
register IO are called from one single location in this function. This is
a clean up. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 14f5cc590a51b..497603964add8 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -289,13 +289,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 	u32 bus_flags = mxsfb->connector->display_info.bus_flags;
 	int err;
 
-	/* Mandatory eLCDIF reset as per the Reference Manual */
-	err = mxsfb_reset_block(mxsfb);
-	if (err)
-		return;
-
-	mxsfb_set_formats(mxsfb, bus_format);
-
 	if (mxsfb->bridge && mxsfb->bridge->timings)
 		bus_flags = mxsfb->bridge->timings->input_bus_flags;
 
@@ -306,6 +299,13 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 			     bus_flags);
 	DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
 
+	/* Mandatory eLCDIF reset as per the Reference Manual */
+	err = mxsfb_reset_block(mxsfb);
+	if (err)
+		return;
+
+	mxsfb_set_formats(mxsfb, bus_format);
+
 	mxsfb_set_mode(mxsfb, bus_flags);
 }
 
-- 
2.34.1

[PATCH 8/9] drm: mxsfb: Factor out mxsfb_update_buffer()

[Marek Vasut]

Pull functionality responsible for programming framebuffer address into
the controller into dedicated function mxsfb_update_buffer(). This is a
clean up. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 497603964add8..4baa3db1f3d10 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -58,6 +58,22 @@ static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
 	return gem->paddr;
 }
 
+static void
+mxsfb_update_buffer(struct mxsfb_drm_private *mxsfb, struct drm_plane *plane,
+		    bool both)
+{
+	dma_addr_t paddr;
+
+	paddr = mxsfb_get_fb_paddr(plane);
+	if (!paddr)
+		return;
+
+	if (both)
+		writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);
+
+	writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
+}
+
 /*
  * Setup the MXSFB registers for decoding the pixels out of the framebuffer and
  * outputting them on the bus.
@@ -352,7 +368,6 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	struct drm_bridge_state *bridge_state;
 	struct drm_device *drm = mxsfb->drm;
 	u32 bus_format = 0;
-	dma_addr_t paddr;
 
 	/* If there is a bridge attached to the LCDIF, use its bus format */
 	if (mxsfb->bridge) {
@@ -387,11 +402,7 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
 
 	/* Write cur_buf as well to avoid an initial corrupt frame */
-	paddr = mxsfb_get_fb_paddr(crtc->primary);
-	if (paddr) {
-		writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);
-		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
-	}
+	mxsfb_update_buffer(mxsfb, crtc->primary, true);
 
 	mxsfb_enable_controller(mxsfb);
 
@@ -491,11 +502,8 @@ static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane,
 					      struct drm_atomic_state *state)
 {
 	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
-	dma_addr_t paddr;
 
-	paddr = mxsfb_get_fb_paddr(plane);
-	if (paddr)
-		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
+	mxsfb_update_buffer(mxsfb, plane, false);
 }
 
 static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane,
-- 
2.34.1

[PATCH 9/9] drm: mxsfb: Add support for i.MX8MP LCDIF variant

[Marek Vasut]

Add support for i.MX8MP LCDIF variant. This is called LCDIFv3 and is
completely different from the LCDIFv3 found in i.MX23 in that it has
a completely scrambled register layout compared to all previous LCDIF
variants. The new LCDIFv3 also supports 36bit address space.

However, except for the complete bit reshuffling, this is still LCDIF
and it still works like one, the boilerplate code is also the same,
hence it is part of this driver. This is probably still a bit better
than a separate driver with a lot of duplicated code.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c  |  34 ++++-
 drivers/gpu/drm/mxsfb/mxsfb_drv.h  |   1 +
 drivers/gpu/drm/mxsfb/mxsfb_kms.c  | 219 +++++++++++++++++++++++++++--
 drivers/gpu/drm/mxsfb/mxsfb_regs.h | 136 ++++++++++++++++++
 4 files changed, 374 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 11298df50917c..7fd7fd1496f7d 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -40,6 +40,8 @@ enum mxsfb_devtype {
 	 * i.MX family number as the version.
 	 */
 	MXSFB_V6,
+	/* Starting at i.MX8MP the register layout is scrambled. */
+	MXSFB_V8,
 };
 
 static const struct mxsfb_devdata mxsfb_devdata[] = {
@@ -51,6 +53,7 @@ static const struct mxsfb_devdata mxsfb_devdata[] = {
 		.hs_wdth_shift	= 24,
 		.has_overlay	= false,
 		.has_ctrl2	= false,
+		.has_regsv8	= false,
 	},
 	[MXSFB_V4] = {
 		.transfer_count	= LCDC_V4_TRANSFER_COUNT,
@@ -60,6 +63,7 @@ static const struct mxsfb_devdata mxsfb_devdata[] = {
 		.hs_wdth_shift	= 18,
 		.has_overlay	= false,
 		.has_ctrl2	= true,
+		.has_regsv8	= false,
 	},
 	[MXSFB_V6] = {
 		.transfer_count	= LCDC_V4_TRANSFER_COUNT,
@@ -69,6 +73,13 @@ static const struct mxsfb_devdata mxsfb_devdata[] = {
 		.hs_wdth_shift	= 18,
 		.has_overlay	= true,
 		.has_ctrl2	= true,
+		.has_regsv8	= false,
+	},
+	[MXSFB_V8] = {
+		/* Old register layout details do not apply here. */
+		.has_overlay	= false,
+		.has_ctrl2	= false,
+		.has_regsv8	= true,
 	},
 };
 
@@ -156,6 +167,22 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t mxsfb_v8_irq_handler(int irq, void *data)
+{
+	struct drm_device *drm = data;
+	struct mxsfb_drm_private *mxsfb = drm->dev_private;
+	u32 reg;
+
+	reg = readl(mxsfb->base + LCDC_V8_INT_STATUS_D0);
+
+	if (reg & INT_STATUS_D0_VS_BLANK)
+		drm_crtc_handle_vblank(&mxsfb->crtc);
+
+	writel(INT_STATUS_D0_VS_BLANK, mxsfb->base + LCDC_V8_INT_STATUS_D0);
+
+	return IRQ_HANDLED;
+}
+
 static int mxsfb_load(struct drm_device *drm,
 		      const struct mxsfb_devdata *devdata)
 {
@@ -191,7 +218,8 @@ static int mxsfb_load(struct drm_device *drm,
 
 	platform_set_drvdata(pdev, drm);
 
-	ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
+	ret = dma_set_mask_and_coherent(drm->dev,
+				DMA_BIT_MASK(devdata->has_regsv8 ? 36 : 32));
 	if (ret)
 		return ret;
 
@@ -234,7 +262,8 @@ static int mxsfb_load(struct drm_device *drm,
 		return ret;
 	mxsfb->irq = ret;
 
-	ret = request_irq(mxsfb->irq, mxsfb_irq_handler, 0,
+	ret = request_irq(mxsfb->irq, devdata->has_regsv8 ?
+			  mxsfb_v8_irq_handler : mxsfb_irq_handler, 0,
 			  drm->driver->name, drm);
 	if (ret < 0) {
 		dev_err(drm->dev, "Failed to install IRQ handler\n");
@@ -286,6 +315,7 @@ static const struct of_device_id mxsfb_dt_ids[] = {
 	{ .compatible = "fsl,imx23-lcdif", .data = &mxsfb_devdata[MXSFB_V3], },
 	{ .compatible = "fsl,imx28-lcdif", .data = &mxsfb_devdata[MXSFB_V4], },
 	{ .compatible = "fsl,imx6sx-lcdif", .data = &mxsfb_devdata[MXSFB_V6], },
+	{ .compatible = "fsl,imx8mp-lcdif", .data = &mxsfb_devdata[MXSFB_V8], },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mxsfb_dt_ids);
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
index ddb5b0417a82c..74c5e6013ca43 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
@@ -23,6 +23,7 @@ struct mxsfb_devdata {
 	unsigned int	hs_wdth_shift;
 	bool		has_overlay;
 	bool		has_ctrl2;
+	bool		has_regsv8;
 };
 
 struct mxsfb_drm_private {
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 4baa3db1f3d10..52df73982ccc1 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -74,6 +74,26 @@ mxsfb_update_buffer(struct mxsfb_drm_private *mxsfb, struct drm_plane *plane,
 	writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
 }
 
+static void
+mxsfb_v8_update_buffer(struct mxsfb_drm_private *mxsfb, struct drm_plane *plane)
+{
+	dma_addr_t paddr;
+	u32 reg;
+
+	paddr = mxsfb_get_fb_paddr(plane);
+	if (!paddr)
+		return;
+
+	writel(lower_32_bits(paddr),
+	       mxsfb->base + LCDC_V8_CTRLDESCL_LOW0_4);
+	writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
+	       mxsfb->base + LCDC_V8_CTRLDESCL_HIGH0_4);
+
+	reg = readl(mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
+	writel(reg, mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+}
+
 /*
  * Setup the MXSFB registers for decoding the pixels out of the framebuffer and
  * outputting them on the bus.
@@ -127,6 +147,57 @@ static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb,
 	writel(ctrl, mxsfb->base + LCDC_CTRL);
 }
 
+static void mxsfb_v8_set_formats(struct mxsfb_drm_private *mxsfb,
+				 const u32 bus_format)
+{
+	struct drm_device *drm = mxsfb->drm;
+	const u32 format = mxsfb->crtc.primary->state->fb->format->format;
+
+	switch (bus_format) {
+	case MEDIA_BUS_FMT_RGB565_1X16:
+		writel(DISP_PARA_LINE_PATTERN_RGB565,
+		       mxsfb->base + LCDC_V8_DISP_PARA);
+		break;
+	case MEDIA_BUS_FMT_RGB888_1X24:
+		writel(DISP_PARA_LINE_PATTERN_RGB888,
+		       mxsfb->base + LCDC_V8_DISP_PARA);
+		break;
+	default:
+		dev_err(drm->dev, "Unknown media bus format 0x%x\n", bus_format);
+		break;
+	}
+
+	switch (format) {
+	case DRM_FORMAT_RGB565:
+		writel(CTRLDESCL0_5_BPP_16_RGB565,
+		       mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+		break;
+	case DRM_FORMAT_RGB888:
+		writel(CTRLDESCL0_5_BPP_24_RGB888,
+		       mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+		break;
+	case DRM_FORMAT_XRGB1555:
+		writel(CTRLDESCL0_5_BPP_16_ARGB1555,
+		       mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+		break;
+	case DRM_FORMAT_XRGB4444:
+		writel(CTRLDESCL0_5_BPP_16_ARGB4444,
+		       mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+		break;
+	case DRM_FORMAT_XBGR8888:
+		writel(CTRLDESCL0_5_BPP_32_ABGR8888,
+		       mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+		break;
+	case DRM_FORMAT_XRGB8888:
+		writel(CTRLDESCL0_5_BPP_32_ARGB8888,
+		       mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+		break;
+	default:
+		dev_err(drm->dev, "Unknown pixel format 0x%x\n", format);
+		break;
+	}
+}
+
 static void mxsfb_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags)
 {
 	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
@@ -178,6 +249,47 @@ static void mxsfb_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags)
 
 }
 
+static void mxsfb_v8_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags)
+{
+	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
+	u32 ctrl;
+
+	if (m->flags & DRM_MODE_FLAG_PHSYNC)
+		ctrl |= CTRL_INV_HS;
+	if (m->flags & DRM_MODE_FLAG_PVSYNC)
+		ctrl |= CTRL_INV_VS;
+	/* Make sure Data Enable is high active by default */
+	if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
+		ctrl |= CTRL_INV_DE;
+	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
+		ctrl |= CTRL_INV_PXCK;
+
+	writel(ctrl, mxsfb->base + LCDC_CTRL);
+
+	writel(DISP_SIZE_DELTA_Y(m->crtc_vdisplay) |
+	       DISP_SIZE_DELTA_X(m->crtc_hdisplay),
+	       mxsfb->base + LCDC_V8_DISP_SIZE);
+
+	writel(HSYN_PARA_BP_H(m->htotal - m->hsync_end) |
+	       HSYN_PARA_FP_H(m->hsync_start - m->hdisplay),
+	       mxsfb->base + LCDC_V8_HSYN_PARA);
+
+	writel(VSYN_PARA_BP_V(m->vtotal - m->vsync_end) |
+	       VSYN_PARA_FP_V(m->vsync_start - m->vdisplay),
+	       mxsfb->base + LCDC_V8_VSYN_PARA);
+
+	writel(VSYN_HSYN_WIDTH_PW_V(m->vsync_end - m->vsync_start) |
+	       VSYN_HSYN_WIDTH_PW_H(m->hsync_end - m->hsync_start),
+	       mxsfb->base + LCDC_V8_VSYN_HSYN_WIDTH);
+
+	writel(CTRLDESCL0_1_HEIGHT(m->crtc_vdisplay) |
+	       CTRLDESCL0_1_WIDTH(m->crtc_hdisplay),
+	       mxsfb->base + LCDC_V8_CTRLDESCL0_1);
+
+	writel(CTRLDESCL0_3_PITCH(mxsfb->crtc.primary->state->fb->pitches[0]),
+	       mxsfb->base + LCDC_V8_CTRLDESCL0_3);
+}
+
 static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
 {
 	u32 reg;
@@ -230,6 +342,19 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
 	writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_SET);
 }
 
+static void mxsfb_v8_enable_controller(struct mxsfb_drm_private *mxsfb)
+{
+	u32 reg;
+
+	reg = readl(mxsfb->base + LCDC_V8_DISP_PARA);
+	reg |= DISP_PARA_DISP_ON;
+	writel(reg, mxsfb->base + LCDC_V8_DISP_PARA);
+
+	reg = readl(mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+	reg |= CTRLDESCL0_5_EN;
+	writel(reg, mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+}
+
 static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb)
 {
 	u32 reg;
@@ -248,6 +373,19 @@ static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb)
 	writel(reg, mxsfb->base + LCDC_VDCTRL4);
 }
 
+static void mxsfb_v8_disable_controller(struct mxsfb_drm_private *mxsfb)
+{
+	u32 reg;
+
+	reg = readl(mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+	reg &= ~CTRLDESCL0_5_EN;
+	writel(reg, mxsfb->base + LCDC_V8_CTRLDESCL0_5);
+
+	reg = readl(mxsfb->base + LCDC_V8_DISP_PARA);
+	reg &= ~DISP_PARA_DISP_ON;
+	writel(reg, mxsfb->base + LCDC_V8_DISP_PARA);
+}
+
 /*
  * Clear the bit and poll it cleared.  This is usually called with
  * a reset address and mask being either SFTRST(bit 31) or CLKGATE
@@ -297,6 +435,26 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
 	return 0;
 }
 
+static int mxsfb_v8_reset_block(struct mxsfb_drm_private *mxsfb)
+{
+	u32 reg;
+	int ret;
+
+	writel(CTRL_SW_RESET, mxsfb->base + LCDC_CTRL + REG_SET);
+
+	ret = readl_poll_timeout(mxsfb->base + LCDC_CTRL, reg,
+				 (reg & CTRL_SW_RESET), 0,
+				 RESET_TIMEOUT);
+	if (ret)
+		return ret;
+
+	writel(CTRL_SW_RESET, mxsfb->base + LCDC_CTRL + REG_CLR);
+
+	return readl_poll_timeout(mxsfb->base + LCDC_CTRL, reg,
+				  !(reg & CTRL_SW_RESET), 0,
+				  RESET_TIMEOUT);
+}
+
 static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 				     const u32 bus_format)
 {
@@ -315,14 +473,25 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 			     bus_flags);
 	DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
 
-	/* Mandatory eLCDIF reset as per the Reference Manual */
-	err = mxsfb_reset_block(mxsfb);
-	if (err)
-		return;
+	if (mxsfb->devdata->has_regsv8) {
+		/* Mandatory eLCDIF reset as per the Reference Manual */
+		err = mxsfb_v8_reset_block(mxsfb);
+		if (err)
+			return;
+
+		mxsfb_v8_set_formats(mxsfb, bus_format);
 
-	mxsfb_set_formats(mxsfb, bus_format);
+		mxsfb_v8_set_mode(mxsfb, bus_flags);
+	} else {
+		/* Mandatory eLCDIF reset as per the Reference Manual */
+		err = mxsfb_reset_block(mxsfb);
+		if (err)
+			return;
 
-	mxsfb_set_mode(mxsfb, bus_flags);
+		mxsfb_set_formats(mxsfb, bus_format);
+
+		mxsfb_set_mode(mxsfb, bus_flags);
+	}
 }
 
 static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc,
@@ -402,9 +571,15 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
 
 	/* Write cur_buf as well to avoid an initial corrupt frame */
-	mxsfb_update_buffer(mxsfb, crtc->primary, true);
+	if (mxsfb->devdata->has_regsv8) {
+		mxsfb_v8_update_buffer(mxsfb, crtc->primary);
 
-	mxsfb_enable_controller(mxsfb);
+		mxsfb_v8_enable_controller(mxsfb);
+	} else {
+		mxsfb_update_buffer(mxsfb, crtc->primary, true);
+
+		mxsfb_enable_controller(mxsfb);
+	}
 
 	drm_crtc_vblank_on(crtc);
 }
@@ -418,7 +593,10 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
 
 	drm_crtc_vblank_off(crtc);
 
-	mxsfb_disable_controller(mxsfb);
+	if (mxsfb->devdata->has_regsv8)
+		mxsfb_v8_disable_controller(mxsfb);
+	else
+		mxsfb_disable_controller(mxsfb);
 
 	spin_lock_irq(&drm->event_lock);
 	event = crtc->state->event;
@@ -436,8 +614,13 @@ static int mxsfb_crtc_enable_vblank(struct drm_crtc *crtc)
 	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev);
 
 	/* Clear and enable VBLANK IRQ */
-	writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-	writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_SET);
+	if (mxsfb->devdata->has_regsv8) {
+		writel(INT_STATUS_D0_VS_BLANK, mxsfb->base + LCDC_V8_INT_STATUS_D0);
+		writel(INT_ENABLE_D0_VS_BLANK_EN, mxsfb->base + LCDC_V8_INT_ENABLE_D0);
+	} else {
+		writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
+		writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_SET);
+	}
 
 	return 0;
 }
@@ -447,8 +630,13 @@ static void mxsfb_crtc_disable_vblank(struct drm_crtc *crtc)
 	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev);
 
 	/* Disable and clear VBLANK IRQ */
-	writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-	writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
+	if (mxsfb->devdata->has_regsv8) {
+		writel(0, mxsfb->base + LCDC_V8_INT_ENABLE_D0);
+		writel(INT_STATUS_D0_VS_BLANK, mxsfb->base + LCDC_V8_INT_STATUS_D0);
+	} else {
+		writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR);
+		writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
+	}
 }
 
 static const struct drm_crtc_helper_funcs mxsfb_crtc_helper_funcs = {
@@ -503,7 +691,10 @@ static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane,
 {
 	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
 
-	mxsfb_update_buffer(mxsfb, plane, false);
+	if (mxsfb->devdata->has_regsv8)
+		mxsfb_v8_update_buffer(mxsfb, plane);
+	else
+		mxsfb_update_buffer(mxsfb, plane, false);
 }
 
 static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
index 694fea13e893e..43e08088a8a03 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h
+++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
@@ -34,6 +34,30 @@
 #define LCDC_AS_CLRKEYLOW		0x240
 #define LCDC_AS_CLRKEYHIGH		0x250
 
+/* V8 register set */
+#define LCDC_V8_DISP_PARA		0x10
+#define LCDC_V8_DISP_SIZE		0x14
+#define LCDC_V8_HSYN_PARA		0x18
+#define LCDC_V8_VSYN_PARA		0x1c
+#define LCDC_V8_VSYN_HSYN_WIDTH		0x20
+#define LCDC_V8_INT_STATUS_D0		0x24
+#define LCDC_V8_INT_ENABLE_D0		0x28
+#define LCDC_V8_INT_STATUS_D1		0x30
+#define LCDC_V8_INT_ENABLE_D1		0x34
+#define LCDC_V8_CTRLDESCL0_1		0x200
+#define LCDC_V8_CTRLDESCL0_3		0x208
+#define LCDC_V8_CTRLDESCL_LOW0_4	0x20c
+#define LCDC_V8_CTRLDESCL_HIGH0_4	0x210
+#define LCDC_V8_CTRLDESCL0_5		0x214
+#define LCDC_V8_CSC0_CTRL		0x21c
+#define LCDC_V8_CSC0_COEF0		0x220
+#define LCDC_V8_CSC0_COEF1		0x224
+#define LCDC_V8_CSC0_COEF2		0x228
+#define LCDC_V8_CSC0_COEF3		0x22c
+#define LCDC_V8_CSC0_COEF4		0x230
+#define LCDC_V8_CSC0_COEF5		0x234
+#define LCDC_V8_PANIC0_THRES		0x238
+
 #define CTRL_SFTRST			BIT(31)
 #define CTRL_CLKGATE			BIT(30)
 #define CTRL_BYPASS_COUNT		BIT(19)
@@ -121,6 +145,118 @@
 #define AS_CTRL_ALPHA_CTRL_EMBEDDED	(0 << 1)
 #define AS_CTRL_AS_ENABLE		BIT(0)
 
+/* V8 register set */
+#define CTRL_SW_RESET			BIT(31)
+#define CTRL_FETCH_START_OPTION_FPV	0
+#define CTRL_FETCH_START_OPTION_PWV	BIT(8)
+#define CTRL_FETCH_START_OPTION_BPV	BIT(9)
+#define CTRL_FETCH_START_OPTION_RESV	GENMASK(9, 8)
+#define CTRL_FETCH_START_OPTION_MASK	GENMASK(9, 8)
+#define CTRL_NEG				BIT(4)
+#define CTRL_INV_PXCK			BIT(3)
+#define CTRL_INV_DE			BIT(2)
+#define CTRL_INV_VS			BIT(1)
+#define CTRL_INV_HS			BIT(0)
+
+#define DISP_PARA_DISP_ON		BIT(31)
+#define DISP_PARA_SWAP_EN		BIT(30)
+#define DISP_PARA_LINE_PATTERN_RGB565	GENMASK(28, 26)
+#define DISP_PARA_LINE_PATTERN_RGB888	0
+#define DISP_PARA_LINE_PATTERN_MASK	GENMASK(29, 26)
+#define DISP_PARA_DISP_MODE_MASK	GENMASK(25, 24)
+#define DISP_PARA_BGND_R_MASK		GENMASK(23, 16)
+#define DISP_PARA_BGND_G_MASK		GENMASK(15, 8)
+#define DISP_PARA_BGND_B_MASK		GENMASK(7, 0)
+
+#define DISP_SIZE_DELTA_Y(n)		(((n) & 0xffff) << 16)
+#define DISP_SIZE_DELTA_Y_MASK		GENMASK(31, 16)
+#define DISP_SIZE_DELTA_X(n)		((n) & 0xffff)
+#define DISP_SIZE_DELTA_X_MASK		GENMASK(15, 0)
+
+#define HSYN_PARA_BP_H(n)		(((n) & 0xffff) << 16)
+#define HSYN_PARA_BP_H_MASK		GENMASK(31, 16)
+#define HSYN_PARA_FP_H(n)		((n) & 0xffff)
+#define HSYN_PARA_FP_H_MASK		GENMASK(15, 0)
+
+#define VSYN_PARA_BP_V(n)		(((n) & 0xffff) << 16)
+#define VSYN_PARA_BP_V_MASK		GENMASK(31, 16)
+#define VSYN_PARA_FP_V(n)		((n) & 0xffff)
+#define VSYN_PARA_FP_V_MASK		GENMASK(15, 0)
+
+#define VSYN_HSYN_WIDTH_PW_V(n)		(((n) & 0xffff) << 16)
+#define VSYN_HSYN_WIDTH_PW_V_MASK	GENMASK(31, 16)
+#define VSYN_HSYN_WIDTH_PW_H(n)		((n) & 0xffff)
+#define VSYN_HSYN_WIDTH_PW_H_MASK	GENMASK(15, 0)
+
+#define INT_STATUS_D0_FIFO_EMPTY	BIT(24)
+#define INT_STATUS_D0_DMA_DONE		BIT(16)
+#define INT_STATUS_D0_DMA_ERR		BIT(8)
+#define INT_STATUS_D0_VS_BLANK		BIT(2)
+#define INT_STATUS_D0_UNDERRUN		BIT(1)
+#define INT_STATUS_D0_VSYNC		BIT(0)
+
+#define INT_ENABLE_D0_FIFO_EMPTY_EN	BIT(24)
+#define INT_ENABLE_D0_DMA_DONE_EN	BIT(16)
+#define INT_ENABLE_D0_DMA_ERR_EN	BIT(8)
+#define INT_ENABLE_D0_VS_BLANK_EN	BIT(2)
+#define INT_ENABLE_D0_UNDERRUN_EN	BIT(1)
+#define INT_ENABLE_D0_VSYNC_EN		BIT(0)
+
+#define INT_STATUS_D1_PLANE_PANIC	BIT(0)
+
+#define INT_ENABLE_D1_PLANE_PANIC_EN	BIT(0)
+
+#define CTRLDESCL0_1_HEIGHT(n)		(((n) & 0xffff) << 16)
+#define CTRLDESCL0_1_HEIGHT_MASK		GENMASK(31, 16)
+#define CTRLDESCL0_1_WIDTH(n)		((n) & 0xffff)
+#define CTRLDESCL0_1_WIDTH_MASK		GENMASK(15, 0)
+
+#define CTRLDESCL0_3_PITCH(n)		((n) & 0xffff)
+#define CTRLDESCL0_3_PITCH_MASK		GENMASK(15, 0)
+
+#define CTRLDESCL_HIGH0_4_ADDR_HIGH(n)	((n) & 0xf)
+#define CTRLDESCL_HIGH0_4_ADDR_HIGH_MASK	GENMASK(3, 0)
+
+#define CTRLDESCL0_5_EN			BIT(31)
+#define CTRLDESCL0_5_SHADOW_LOAD_EN	BIT(30)
+#define CTRLDESCL0_5_BPP_16_RGB565	BIT(26)
+#define CTRLDESCL0_5_BPP_16_ARGB1555	(BIT(26) | BIT(24))
+#define CTRLDESCL0_5_BPP_16_ARGB4444	(BIT(26) | BIT(25))
+#define CTRLDESCL0_5_BPP_YCbCr422	(BIT(26) | BIT(25) | BIT(24))
+#define CTRLDESCL0_5_BPP_24_RGB888	BIT(27)
+#define CTRLDESCL0_5_BPP_32_ARGB8888	(BIT(27) | BIT(24))
+#define CTRLDESCL0_5_BPP_32_ABGR8888	(BIT(27) | BIT(25))
+#define CTRLDESCL0_5_BPP_MASK		GENMASK(27, 24)
+#define CTRLDESCL0_5_YUV_FORMAT_Y2VY1U	0
+#define CTRLDESCL0_5_YUV_FORMAT_Y2UY1V	BIT(14)
+#define CTRLDESCL0_5_YUV_FORMAT_VY2UY1	BIT(15)
+#define CTRLDESCL0_5_YUV_FORMAT_UY2VY1	(BIT(15) | BIT(14))
+#define CTRLDESCL0_5_YUV_FORMAT_MASK	GENMASK(15, 14)
+
+#define CSC0_CTRL_CSC_MODE_MASK		GENMASK(2, 1)
+#define CSC0_CTRL_BYPASS		BIT(0)
+
+#define CSC0_COEF0_A2_MASK		GENMASK(26, 16)
+#define CSC0_COEF0_A1_MASK		GENMASK(10, 0)
+
+#define CSC0_COEF1_B1_MASK		GENMASK(26, 16)
+#define CSC0_COEF1_A3_MASK		GENMASK(10, 0)
+
+#define CSC0_COEF2_B3_MASK		GENMASK(26, 16)
+#define CSC0_COEF2_B2_MASK		GENMASK(10, 0)
+
+#define CSC0_COEF3_C2_MASK		GENMASK(26, 16)
+#define CSC0_COEF3_C1_MASK		GENMASK(10, 0)
+
+#define CSC0_COEF4_D1_MASK		GENMASK(24, 16)
+#define CSC0_COEF4_C3_MASK		GENMASK(10, 0)
+
+#define CSC0_COEF5_D3_MASK		GENMASK(24, 16)
+#define CSC0_COEF5_D2_MASK		GENMASK(8, 0)
+
+#define PANIC0_THRES_LOW_MASK		GENMASK(24, 16)
+#define PANIC0_THRES_HIGH_MASK		GENMASK(8, 0)
+
 #define MXSFB_MIN_XRES			120
 #define MXSFB_MIN_YRES			120
 #define MXSFB_MAX_XRES			0xffff
-- 
2.34.1

Re: [PATCH 9/9] drm: mxsfb: Add support for i.MX8MP LCDIF variant

[kernel test robot]

Hi Marek,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-exynos/exynos-drm-next next-20220225]
[cannot apply to drm/drm-next drm-tip/drm-tip tegra-drm/drm/tegra/for-next v5.17-rc6]
[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/Marek-Vasut/dt-bindings-mxsfb-Add-compatible-for-i-MX8MP/20220228-084809
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-a002-20220228 (https://download.01.org/0day-ci/archive/20220228/202202281124.RFKJe01p-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
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/d6832d6fb879aabce18d9b451ed1ead1da38c333
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Marek-Vasut/dt-bindings-mxsfb-Add-compatible-for-i-MX8MP/20220228-084809
        git checkout d6832d6fb879aabce18d9b451ed1ead1da38c333
        # 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=i386 SHELL=/bin/bash drivers/gpu/drm/mxsfb/

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

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mxsfb/mxsfb_kms.c:258:3: warning: variable 'ctrl' is uninitialized when used here [-Wuninitialized]
                   ctrl |= CTRL_INV_HS;
                   ^~~~
   drivers/gpu/drm/mxsfb/mxsfb_kms.c:255:10: note: initialize the variable 'ctrl' to silence this warning
           u32 ctrl;
                   ^
                    = 0
   1 warning generated.


vim +/ctrl +258 drivers/gpu/drm/mxsfb/mxsfb_kms.c

   251	
   252	static void mxsfb_v8_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags)
   253	{
   254		struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
   255		u32 ctrl;
   256	
   257		if (m->flags & DRM_MODE_FLAG_PHSYNC)
 > 258			ctrl |= CTRL_INV_HS;
   259		if (m->flags & DRM_MODE_FLAG_PVSYNC)
   260			ctrl |= CTRL_INV_VS;
   261		/* Make sure Data Enable is high active by default */
   262		if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
   263			ctrl |= CTRL_INV_DE;
   264		if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
   265			ctrl |= CTRL_INV_PXCK;
   266	
   267		writel(ctrl, mxsfb->base + LCDC_CTRL);
   268	
   269		writel(DISP_SIZE_DELTA_Y(m->crtc_vdisplay) |
   270		       DISP_SIZE_DELTA_X(m->crtc_hdisplay),
   271		       mxsfb->base + LCDC_V8_DISP_SIZE);
   272	
   273		writel(HSYN_PARA_BP_H(m->htotal - m->hsync_end) |
   274		       HSYN_PARA_FP_H(m->hsync_start - m->hdisplay),
   275		       mxsfb->base + LCDC_V8_HSYN_PARA);
   276	
   277		writel(VSYN_PARA_BP_V(m->vtotal - m->vsync_end) |
   278		       VSYN_PARA_FP_V(m->vsync_start - m->vdisplay),
   279		       mxsfb->base + LCDC_V8_VSYN_PARA);
   280	
   281		writel(VSYN_HSYN_WIDTH_PW_V(m->vsync_end - m->vsync_start) |
   282		       VSYN_HSYN_WIDTH_PW_H(m->hsync_end - m->hsync_start),
   283		       mxsfb->base + LCDC_V8_VSYN_HSYN_WIDTH);
   284	
   285		writel(CTRLDESCL0_1_HEIGHT(m->crtc_vdisplay) |
   286		       CTRLDESCL0_1_WIDTH(m->crtc_hdisplay),
   287		       mxsfb->base + LCDC_V8_CTRLDESCL0_1);
   288	
   289		writel(CTRLDESCL0_3_PITCH(mxsfb->crtc.primary->state->fb->pitches[0]),
   290		       mxsfb->base + LCDC_V8_CTRLDESCL0_3);
   291	}
   292	

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

Re: [PATCH 9/9] drm: mxsfb: Add support for i.MX8MP LCDIF variant

[kernel test robot]

Hi Marek,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-exynos/exynos-drm-next next-20220225]
[cannot apply to drm/drm-next drm-tip/drm-tip tegra-drm/drm/tegra/for-next v5.17-rc6]
[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/Marek-Vasut/dt-bindings-mxsfb-Add-compatible-for-i-MX8MP/20220228-084809
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-a002-20220228 (https://download.01.org/0day-ci/archive/20220228/202202281124.RFKJe01p-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
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/d6832d6fb879aabce18d9b451ed1ead1da38c333
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Marek-Vasut/dt-bindings-mxsfb-Add-compatible-for-i-MX8MP/20220228-084809
        git checkout d6832d6fb879aabce18d9b451ed1ead1da38c333
        # 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=i386 SHELL=/bin/bash drivers/gpu/drm/mxsfb/

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

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mxsfb/mxsfb_kms.c:258:3: warning: variable 'ctrl' is uninitialized when used here [-Wuninitialized]
                   ctrl |= CTRL_INV_HS;
                   ^~~~
   drivers/gpu/drm/mxsfb/mxsfb_kms.c:255:10: note: initialize the variable 'ctrl' to silence this warning
           u32 ctrl;
                   ^
                    = 0
   1 warning generated.


vim +/ctrl +258 drivers/gpu/drm/mxsfb/mxsfb_kms.c

   251	
   252	static void mxsfb_v8_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags)
   253	{
   254		struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
   255		u32 ctrl;
   256	
   257		if (m->flags & DRM_MODE_FLAG_PHSYNC)
 > 258			ctrl |= CTRL_INV_HS;
   259		if (m->flags & DRM_MODE_FLAG_PVSYNC)
   260			ctrl |= CTRL_INV_VS;
   261		/* Make sure Data Enable is high active by default */
   262		if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
   263			ctrl |= CTRL_INV_DE;
   264		if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
   265			ctrl |= CTRL_INV_PXCK;
   266	
   267		writel(ctrl, mxsfb->base + LCDC_CTRL);
   268	
   269		writel(DISP_SIZE_DELTA_Y(m->crtc_vdisplay) |
   270		       DISP_SIZE_DELTA_X(m->crtc_hdisplay),
   271		       mxsfb->base + LCDC_V8_DISP_SIZE);
   272	
   273		writel(HSYN_PARA_BP_H(m->htotal - m->hsync_end) |
   274		       HSYN_PARA_FP_H(m->hsync_start - m->hdisplay),
   275		       mxsfb->base + LCDC_V8_HSYN_PARA);
   276	
   277		writel(VSYN_PARA_BP_V(m->vtotal - m->vsync_end) |
   278		       VSYN_PARA_FP_V(m->vsync_start - m->vdisplay),
   279		       mxsfb->base + LCDC_V8_VSYN_PARA);
   280	
   281		writel(VSYN_HSYN_WIDTH_PW_V(m->vsync_end - m->vsync_start) |
   282		       VSYN_HSYN_WIDTH_PW_H(m->hsync_end - m->hsync_start),
   283		       mxsfb->base + LCDC_V8_VSYN_HSYN_WIDTH);
   284	
   285		writel(CTRLDESCL0_1_HEIGHT(m->crtc_vdisplay) |
   286		       CTRLDESCL0_1_WIDTH(m->crtc_hdisplay),
   287		       mxsfb->base + LCDC_V8_CTRLDESCL0_1);
   288	
   289		writel(CTRLDESCL0_3_PITCH(mxsfb->crtc.primary->state->fb->pitches[0]),
   290		       mxsfb->base + LCDC_V8_CTRLDESCL0_3);
   291	}
   292	

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

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

Hi Marek,

On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
> Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> and is completely different from the LCDIFv3 found in i.MX23 in that it

In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.

> has a completely scrambled register layout compared to all previous LCDIF

It looks like no single register of i.MX8MP LCDIFv3 overlaps with
registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
totally different from the LCDIF block diagram, according to the SoC
reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
and vertical size of graphic, position of graphic on the panel, address
of graphic in memory and color formats or color palettes, which is not
supported by LCDIF and impacts display driver control mechanism
considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
interface, while LCDIFv3 only supports parallel output as a counterpart
of the DOTCLK interface.

Generally speaking, LCDIFv3 is just a new display IP which happens to
have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
are display controllers for scanning out frames onto display devices, I
don't think they are in one family.

So, LCDIFv3 deserves a new separate dt-binding, IMO.

> variants. The new LCDIFv3 also supports 36bit address space. However,
> except for the complete bit reshuffling, this is still LCDIF and it still
> works like one.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Robby Cai <robby.cai@nxp.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> index 900a56cae80e6..9831ab53a053d 100644
> --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> @@ -28,6 +28,7 @@ properties:
>                - fsl,imx7d-lcdif
>                - fsl,imx8mm-lcdif
>                - fsl,imx8mn-lcdif
> +              - fsl,imx8mp-lcdif

Even if LCDIFv3 is covered by this dt-binding(which is obviously not
the case), 'fsl,imx8mp-lcdif' should be after 'fsl,imx6x-lcdif' as an
enum, otherwise LCDIFv3 is compatible to LCDIF.

Regards,
Liu Ying

>                - fsl,imx8mq-lcdif
>            - const: fsl,imx6sx-lcdif
>  

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

Hi Marek,

On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
> Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> and is completely different from the LCDIFv3 found in i.MX23 in that it

In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.

> has a completely scrambled register layout compared to all previous LCDIF

It looks like no single register of i.MX8MP LCDIFv3 overlaps with
registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
totally different from the LCDIF block diagram, according to the SoC
reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
and vertical size of graphic, position of graphic on the panel, address
of graphic in memory and color formats or color palettes, which is not
supported by LCDIF and impacts display driver control mechanism
considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
interface, while LCDIFv3 only supports parallel output as a counterpart
of the DOTCLK interface.

Generally speaking, LCDIFv3 is just a new display IP which happens to
have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
are display controllers for scanning out frames onto display devices, I
don't think they are in one family.

So, LCDIFv3 deserves a new separate dt-binding, IMO.

> variants. The new LCDIFv3 also supports 36bit address space. However,
> except for the complete bit reshuffling, this is still LCDIF and it still
> works like one.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Robby Cai <robby.cai@nxp.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> index 900a56cae80e6..9831ab53a053d 100644
> --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> @@ -28,6 +28,7 @@ properties:
>                - fsl,imx7d-lcdif
>                - fsl,imx8mm-lcdif
>                - fsl,imx8mn-lcdif
> +              - fsl,imx8mp-lcdif

Even if LCDIFv3 is covered by this dt-binding(which is obviously not
the case), 'fsl,imx8mp-lcdif' should be after 'fsl,imx6x-lcdif' as an
enum, otherwise LCDIFv3 is compatible to LCDIF.

Regards,
Liu Ying

>                - fsl,imx8mq-lcdif
>            - const: fsl,imx6sx-lcdif
>  

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 2/28/22 07:37, Liu Ying wrote:
> Hi Marek,

Hi,

> On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
>> Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
>> and is completely different from the LCDIFv3 found in i.MX23 in that it
> 
> In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.

See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF 
V4 .

>> has a completely scrambled register layout compared to all previous LCDIF
> 
> It looks like no single register of i.MX8MP LCDIFv3 overlaps with
> registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
> totally different from the LCDIF block diagram, according to the SoC
> reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
> and vertical size of graphic, position of graphic on the panel, address
> of graphic in memory and color formats or color palettes, which is not
> supported by LCDIF and impacts display driver control mechanism
> considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
> interface, while LCDIFv3 only supports parallel output as a counterpart
> of the DOTCLK interface.
> 
> Generally speaking, LCDIFv3 is just a new display IP which happens to
> have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
> are display controllers for scanning out frames onto display devices, I
> don't think they are in one family.
> 
> So, LCDIFv3 deserves a new separate dt-binding, IMO.

It seems to me a lot of those bits just map to their previous 
equivalents in older LCDIF, others were dropped, so this is some sort of 
new LCDIF mutation, is it not ?

I am aware NXP has a separate driver in its downstream, but I'm not 
convinced the duplication of boilerplate code by introducing a separate 
driver for what looks like another LCDIF variant is the right approach.

>> variants. The new LCDIFv3 also supports 36bit address space. However,
>> except for the complete bit reshuffling, this is still LCDIF and it still
>> works like one.

[...]

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 2/28/22 07:37, Liu Ying wrote:
> Hi Marek,

Hi,

> On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
>> Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
>> and is completely different from the LCDIFv3 found in i.MX23 in that it
> 
> In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.

See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF 
V4 .

>> has a completely scrambled register layout compared to all previous LCDIF
> 
> It looks like no single register of i.MX8MP LCDIFv3 overlaps with
> registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
> totally different from the LCDIF block diagram, according to the SoC
> reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
> and vertical size of graphic, position of graphic on the panel, address
> of graphic in memory and color formats or color palettes, which is not
> supported by LCDIF and impacts display driver control mechanism
> considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
> interface, while LCDIFv3 only supports parallel output as a counterpart
> of the DOTCLK interface.
> 
> Generally speaking, LCDIFv3 is just a new display IP which happens to
> have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
> are display controllers for scanning out frames onto display devices, I
> don't think they are in one family.
> 
> So, LCDIFv3 deserves a new separate dt-binding, IMO.

It seems to me a lot of those bits just map to their previous 
equivalents in older LCDIF, others were dropped, so this is some sort of 
new LCDIF mutation, is it not ?

I am aware NXP has a separate driver in its downstream, but I'm not 
convinced the duplication of boilerplate code by introducing a separate 
driver for what looks like another LCDIF variant is the right approach.

>> variants. The new LCDIFv3 also supports 36bit address space. However,
>> except for the complete bit reshuffling, this is still LCDIF and it still
>> works like one.

[...]

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Mon, 2022-02-28 at 07:57 +0100, Marek Vasut wrote:
> On 2/28/22 07:37, Liu Ying wrote:
> > Hi Marek,
> 
> Hi,
> 
> > On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
> > > Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> > > and is completely different from the LCDIFv3 found in i.MX23 in that it
> > 
> > In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.
> 
> See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF 
> V4 .

Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display
controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'.  There
is not even a register in i.MX8MP display controller to decribe the
version.

> 
> > > has a completely scrambled register layout compared to all previous LCDIF
> > 
> > It looks like no single register of i.MX8MP LCDIFv3 overlaps with
> > registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
> > totally different from the LCDIF block diagram, according to the SoC
> > reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
> > and vertical size of graphic, position of graphic on the panel, address
> > of graphic in memory and color formats or color palettes, which is not
> > supported by LCDIF and impacts display driver control mechanism
> > considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
> > interface, while LCDIFv3 only supports parallel output as a counterpart
> > of the DOTCLK interface.
> > 
> > Generally speaking, LCDIFv3 is just a new display IP which happens to
> > have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
> > are display controllers for scanning out frames onto display devices, I
> > don't think they are in one family.
> > 
> > So, LCDIFv3 deserves a new separate dt-binding, IMO.
> 
> It seems to me a lot of those bits just map to their previous 
> equivalents in older LCDIF, others were dropped, so this is some sort of 
> new LCDIF mutation, is it not ?

I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names
of registers and the names of register bits . 

> 
> I am aware NXP has a separate driver in its downstream, but I'm not 
> convinced the duplication of boilerplate code by introducing a separate 
> driver for what looks like another LCDIF variant is the right approach.

Hmmm, given the two IPs, I think there should be separate drivers.
 With one single driver, there would be too many 'if/else' checks to
separate the logics for the IPs, just like Patch 9/9 does.  The
boilerplate code to do things like registering a drm device is
acceptable, IMO.

Aside from that, with separate drivers, we don't have to test too many
SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'. 

> 
> > > variants. The new LCDIFv3 also supports 36bit address space. However,
> > > except for the complete bit reshuffling, this is still LCDIF and it still
> > > works like one.
> 
> [...]

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Mon, 2022-02-28 at 07:57 +0100, Marek Vasut wrote:
> On 2/28/22 07:37, Liu Ying wrote:
> > Hi Marek,
> 
> Hi,
> 
> > On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
> > > Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> > > and is completely different from the LCDIFv3 found in i.MX23 in that it
> > 
> > In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.
> 
> See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF 
> V4 .

Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display
controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'.  There
is not even a register in i.MX8MP display controller to decribe the
version.

> 
> > > has a completely scrambled register layout compared to all previous LCDIF
> > 
> > It looks like no single register of i.MX8MP LCDIFv3 overlaps with
> > registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
> > totally different from the LCDIF block diagram, according to the SoC
> > reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
> > and vertical size of graphic, position of graphic on the panel, address
> > of graphic in memory and color formats or color palettes, which is not
> > supported by LCDIF and impacts display driver control mechanism
> > considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
> > interface, while LCDIFv3 only supports parallel output as a counterpart
> > of the DOTCLK interface.
> > 
> > Generally speaking, LCDIFv3 is just a new display IP which happens to
> > have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
> > are display controllers for scanning out frames onto display devices, I
> > don't think they are in one family.
> > 
> > So, LCDIFv3 deserves a new separate dt-binding, IMO.
> 
> It seems to me a lot of those bits just map to their previous 
> equivalents in older LCDIF, others were dropped, so this is some sort of 
> new LCDIF mutation, is it not ?

I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names
of registers and the names of register bits . 

> 
> I am aware NXP has a separate driver in its downstream, but I'm not 
> convinced the duplication of boilerplate code by introducing a separate 
> driver for what looks like another LCDIF variant is the right approach.

Hmmm, given the two IPs, I think there should be separate drivers.
 With one single driver, there would be too many 'if/else' checks to
separate the logics for the IPs, just like Patch 9/9 does.  The
boilerplate code to do things like registering a drm device is
acceptable, IMO.

Aside from that, with separate drivers, we don't have to test too many
SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'. 

> 
> > > variants. The new LCDIFv3 also supports 36bit address space. However,
> > > except for the complete bit reshuffling, this is still LCDIF and it still
> > > works like one.
> 
> [...]

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 2/28/22 09:18, Liu Ying wrote:

Hi,

>>> On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
>>>> Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
>>>> and is completely different from the LCDIFv3 found in i.MX23 in that it
>>>
>>> In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.
>>
>> See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF
>> V4 .
> 
> Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display
> controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'.  There
> is not even a register in i.MX8MP display controller to decribe the
> version.

We also don't have a version register on MX6SX and we call it LCDIF V6 
in the driver. The naming scheme is confusing.

>>>> has a completely scrambled register layout compared to all previous LCDIF
>>>
>>> It looks like no single register of i.MX8MP LCDIFv3 overlaps with
>>> registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
>>> totally different from the LCDIF block diagram, according to the SoC
>>> reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
>>> and vertical size of graphic, position of graphic on the panel, address
>>> of graphic in memory and color formats or color palettes, which is not
>>> supported by LCDIF and impacts display driver control mechanism
>>> considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
>>> interface, while LCDIFv3 only supports parallel output as a counterpart
>>> of the DOTCLK interface.
>>>
>>> Generally speaking, LCDIFv3 is just a new display IP which happens to
>>> have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
>>> are display controllers for scanning out frames onto display devices, I
>>> don't think they are in one family.
>>>
>>> So, LCDIFv3 deserves a new separate dt-binding, IMO.
>>
>> It seems to me a lot of those bits just map to their previous
>> equivalents in older LCDIF, others were dropped, so this is some sort of
>> new LCDIF mutation, is it not ?
> 
> I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names
> of registers and the names of register bits .
> 
>>
>> I am aware NXP has a separate driver in its downstream, but I'm not
>> convinced the duplication of boilerplate code by introducing a separate
>> driver for what looks like another LCDIF variant is the right approach.
> 
> Hmmm, given the two IPs, I think there should be separate drivers.
>   With one single driver, there would be too many 'if/else' checks to
> separate the logics for the IPs, just like Patch 9/9 does.  The
> boilerplate code to do things like registering a drm device is
> acceptable, IMO.
> 
> Aside from that, with separate drivers, we don't have to test too many
> SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'.

But then, with two drivers, you also might miss fixes which get applied 
to one driver and not the other, eventually the two drivers will diverge 
and that's not good.

I might wait for opinion from the others whether this should be one or 
two drivers.

btw is there any plan to have LCDIFv4 or this LCDIFv3 in some other SoC 
than iMX8MP ?

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 2/28/22 09:18, Liu Ying wrote:

Hi,

>>> On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
>>>> Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
>>>> and is completely different from the LCDIFv3 found in i.MX23 in that it
>>>
>>> In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.
>>
>> See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF
>> V4 .
> 
> Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display
> controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'.  There
> is not even a register in i.MX8MP display controller to decribe the
> version.

We also don't have a version register on MX6SX and we call it LCDIF V6 
in the driver. The naming scheme is confusing.

>>>> has a completely scrambled register layout compared to all previous LCDIF
>>>
>>> It looks like no single register of i.MX8MP LCDIFv3 overlaps with
>>> registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
>>> totally different from the LCDIF block diagram, according to the SoC
>>> reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
>>> and vertical size of graphic, position of graphic on the panel, address
>>> of graphic in memory and color formats or color palettes, which is not
>>> supported by LCDIF and impacts display driver control mechanism
>>> considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
>>> interface, while LCDIFv3 only supports parallel output as a counterpart
>>> of the DOTCLK interface.
>>>
>>> Generally speaking, LCDIFv3 is just a new display IP which happens to
>>> have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
>>> are display controllers for scanning out frames onto display devices, I
>>> don't think they are in one family.
>>>
>>> So, LCDIFv3 deserves a new separate dt-binding, IMO.
>>
>> It seems to me a lot of those bits just map to their previous
>> equivalents in older LCDIF, others were dropped, so this is some sort of
>> new LCDIF mutation, is it not ?
> 
> I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names
> of registers and the names of register bits .
> 
>>
>> I am aware NXP has a separate driver in its downstream, but I'm not
>> convinced the duplication of boilerplate code by introducing a separate
>> driver for what looks like another LCDIF variant is the right approach.
> 
> Hmmm, given the two IPs, I think there should be separate drivers.
>   With one single driver, there would be too many 'if/else' checks to
> separate the logics for the IPs, just like Patch 9/9 does.  The
> boilerplate code to do things like registering a drm device is
> acceptable, IMO.
> 
> Aside from that, with separate drivers, we don't have to test too many
> SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'.

But then, with two drivers, you also might miss fixes which get applied 
to one driver and not the other, eventually the two drivers will diverge 
and that's not good.

I might wait for opinion from the others whether this should be one or 
two drivers.

btw is there any plan to have LCDIFv4 or this LCDIFv3 in some other SoC 
than iMX8MP ?

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Mon, 2022-02-28 at 16:34 +0100, Marek Vasut wrote:
> On 2/28/22 09:18, Liu Ying wrote:
> 
> Hi,

Hi,

> 
> > > > On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
> > > > > Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> > > > > and is completely different from the LCDIFv3 found in i.MX23 in that it
> > > > 
> > > > In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.
> > > 
> > > See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF
> > > V4 .
> > 
> > Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display
> > controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'.  There
> > is not even a register in i.MX8MP display controller to decribe the
> > version.
> 
> We also don't have a version register on MX6SX and we call it LCDIF V6 
> in the driver. The naming scheme is confusing.

It looks ok for the current mxsfb drm driver to use its own version
tracking mechanism to distinguish kinda small difference across LCDIF
variants.  However, LCDIFv3 in i.MX8mp is a totally different IP, which
does not apply to the tracking mechanism.

> 
> > > > > has a completely scrambled register layout compared to all previous LCDIF
> > > > 
> > > > It looks like no single register of i.MX8MP LCDIFv3 overlaps with
> > > > registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
> > > > totally different from the LCDIF block diagram, according to the SoC
> > > > reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
> > > > and vertical size of graphic, position of graphic on the panel, address
> > > > of graphic in memory and color formats or color palettes, which is not
> > > > supported by LCDIF and impacts display driver control mechanism
> > > > considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
> > > > interface, while LCDIFv3 only supports parallel output as a counterpart
> > > > of the DOTCLK interface.
> > > > 
> > > > Generally speaking, LCDIFv3 is just a new display IP which happens to
> > > > have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
> > > > are display controllers for scanning out frames onto display devices, I
> > > > don't think they are in one family.
> > > > 
> > > > So, LCDIFv3 deserves a new separate dt-binding, IMO.
> > > 
> > > It seems to me a lot of those bits just map to their previous
> > > equivalents in older LCDIF, others were dropped, so this is some sort of
> > > new LCDIF mutation, is it not ?
> > 
> > I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names
> > of registers and the names of register bits .
> > 
> > > I am aware NXP has a separate driver in its downstream, but I'm not
> > > convinced the duplication of boilerplate code by introducing a separate
> > > driver for what looks like another LCDIF variant is the right approach.
> > 
> > Hmmm, given the two IPs, I think there should be separate drivers.
> >   With one single driver, there would be too many 'if/else' checks to
> > separate the logics for the IPs, just like Patch 9/9 does.  The
> > boilerplate code to do things like registering a drm device is
> > acceptable, IMO.
> > 
> > Aside from that, with separate drivers, we don't have to test too many
> > SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'.
> 
> But then, with two drivers, you also might miss fixes which get applied 
> to one driver and not the other, eventually the two drivers will diverge 
> and that's not good.

Given the two totally different IPs, I don't see bugs of IP control
logics should be fixed for both drivers. Naturally, the two would
diverge due to different HWs. Looking at Patch 9/9, it basically
squashes code to control LCDIFv3 into the mxsfb drm driver with
'if/else' checks(barely no common control code), which is hard to
maintain and not able to achieve good scalability for both 'LCDIFv3'
and 'LCDIF'.

> 
> I might wait for opinion from the others whether this should be one or 
> two drivers.
> 
> btw is there any plan to have LCDIFv4 or this LCDIFv3 in some other SoC 
> than iMX8MP ?

I don't think this kind of information significantly impacts the
decision on two drivers or one.  And, I cannot share that until the
company unveils.

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Mon, 2022-02-28 at 16:34 +0100, Marek Vasut wrote:
> On 2/28/22 09:18, Liu Ying wrote:
> 
> Hi,

Hi,

> 
> > > > On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
> > > > > Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> > > > > and is completely different from the LCDIFv3 found in i.MX23 in that it
> > > > 
> > > > In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.
> > > 
> > > See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF
> > > V4 .
> > 
> > Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display
> > controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'.  There
> > is not even a register in i.MX8MP display controller to decribe the
> > version.
> 
> We also don't have a version register on MX6SX and we call it LCDIF V6 
> in the driver. The naming scheme is confusing.

It looks ok for the current mxsfb drm driver to use its own version
tracking mechanism to distinguish kinda small difference across LCDIF
variants.  However, LCDIFv3 in i.MX8mp is a totally different IP, which
does not apply to the tracking mechanism.

> 
> > > > > has a completely scrambled register layout compared to all previous LCDIF
> > > > 
> > > > It looks like no single register of i.MX8MP LCDIFv3 overlaps with
> > > > registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
> > > > totally different from the LCDIF block diagram, according to the SoC
> > > > reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
> > > > and vertical size of graphic, position of graphic on the panel, address
> > > > of graphic in memory and color formats or color palettes, which is not
> > > > supported by LCDIF and impacts display driver control mechanism
> > > > considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
> > > > interface, while LCDIFv3 only supports parallel output as a counterpart
> > > > of the DOTCLK interface.
> > > > 
> > > > Generally speaking, LCDIFv3 is just a new display IP which happens to
> > > > have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
> > > > are display controllers for scanning out frames onto display devices, I
> > > > don't think they are in one family.
> > > > 
> > > > So, LCDIFv3 deserves a new separate dt-binding, IMO.
> > > 
> > > It seems to me a lot of those bits just map to their previous
> > > equivalents in older LCDIF, others were dropped, so this is some sort of
> > > new LCDIF mutation, is it not ?
> > 
> > I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names
> > of registers and the names of register bits .
> > 
> > > I am aware NXP has a separate driver in its downstream, but I'm not
> > > convinced the duplication of boilerplate code by introducing a separate
> > > driver for what looks like another LCDIF variant is the right approach.
> > 
> > Hmmm, given the two IPs, I think there should be separate drivers.
> >   With one single driver, there would be too many 'if/else' checks to
> > separate the logics for the IPs, just like Patch 9/9 does.  The
> > boilerplate code to do things like registering a drm device is
> > acceptable, IMO.
> > 
> > Aside from that, with separate drivers, we don't have to test too many
> > SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'.
> 
> But then, with two drivers, you also might miss fixes which get applied 
> to one driver and not the other, eventually the two drivers will diverge 
> and that's not good.

Given the two totally different IPs, I don't see bugs of IP control
logics should be fixed for both drivers. Naturally, the two would
diverge due to different HWs. Looking at Patch 9/9, it basically
squashes code to control LCDIFv3 into the mxsfb drm driver with
'if/else' checks(barely no common control code), which is hard to
maintain and not able to achieve good scalability for both 'LCDIFv3'
and 'LCDIF'.

> 
> I might wait for opinion from the others whether this should be one or 
> two drivers.
> 
> btw is there any plan to have LCDIFv4 or this LCDIFv3 in some other SoC 
> than iMX8MP ?

I don't think this kind of information significantly impacts the
decision on two drivers or one.  And, I cannot share that until the
company unveils.

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Hi Marek, hi Liu,

Am Dienstag, dem 01.03.2022 um 10:44 +0800 schrieb Liu Ying:
> On Mon, 2022-02-28 at 16:34 +0100, Marek Vasut wrote:
> > On 2/28/22 09:18, Liu Ying wrote:
> > 
> > Hi,
> 
> Hi,
> 
> > 
> > > > > On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
> > > > > > Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> > > > > > and is completely different from the LCDIFv3 found in i.MX23 in that it
> > > > > 
> > > > > In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.
> > > > 
> > > > See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF
> > > > V4 .
> > > 
> > > Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display
> > > controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'.  There
> > > is not even a register in i.MX8MP display controller to decribe the
> > > version.
> > 
> > We also don't have a version register on MX6SX and we call it LCDIF V6 
> > in the driver. The naming scheme is confusing.
> 
> It looks ok for the current mxsfb drm driver to use its own version
> tracking mechanism to distinguish kinda small difference across LCDIF
> variants.  However, LCDIFv3 in i.MX8mp is a totally different IP, which
> does not apply to the tracking mechanism.
> 
> > 
> > > > > > has a completely scrambled register layout compared to all previous LCDIF
> > > > > 
> > > > > It looks like no single register of i.MX8MP LCDIFv3 overlaps with
> > > > > registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
> > > > > totally different from the LCDIF block diagram, according to the SoC
> > > > > reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
> > > > > and vertical size of graphic, position of graphic on the panel, address
> > > > > of graphic in memory and color formats or color palettes, which is not
> > > > > supported by LCDIF and impacts display driver control mechanism
> > > > > considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
> > > > > interface, while LCDIFv3 only supports parallel output as a counterpart
> > > > > of the DOTCLK interface.
> > > > > 
> > > > > Generally speaking, LCDIFv3 is just a new display IP which happens to
> > > > > have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
> > > > > are display controllers for scanning out frames onto display devices, I
> > > > > don't think they are in one family.
> > > > > 
> > > > > So, LCDIFv3 deserves a new separate dt-binding, IMO.
> > > > 
> > > > It seems to me a lot of those bits just map to their previous
> > > > equivalents in older LCDIF, others were dropped, so this is some sort of
> > > > new LCDIF mutation, is it not ?
> > > 
> > > I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names
> > > of registers and the names of register bits .
> > > 
> > > > I am aware NXP has a separate driver in its downstream, but I'm not
> > > > convinced the duplication of boilerplate code by introducing a separate
> > > > driver for what looks like another LCDIF variant is the right approach.
> > > 
> > > Hmmm, given the two IPs, I think there should be separate drivers.
> > >   With one single driver, there would be too many 'if/else' checks to
> > > separate the logics for the IPs, just like Patch 9/9 does.  The
> > > boilerplate code to do things like registering a drm device is
> > > acceptable, IMO.
> > > 
> > > Aside from that, with separate drivers, we don't have to test too many
> > > SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'.
> > 
> > But then, with two drivers, you also might miss fixes which get applied 
> > to one driver and not the other, eventually the two drivers will diverge 
> > and that's not good.
> 
> Given the two totally different IPs, I don't see bugs of IP control
> logics should be fixed for both drivers. Naturally, the two would
> diverge due to different HWs. Looking at Patch 9/9, it basically
> squashes code to control LCDIFv3 into the mxsfb drm driver with
> 'if/else' checks(barely no common control code), which is hard to
> maintain and not able to achieve good scalability for both 'LCDIFv3'
> and 'LCDIF'.

I tend to agree with Liu here. Writing a DRM driver isn't that much
boilerplate anymore with all the helpers we have available in the
framework today.

The IP is so different from the currently supported LCDIF controllers
that I think trying to support this one in the existing driver actually
increases the chances to break something when modifying the driver in
the future. Not everyone is able to test all LCDIF versions. My vote is
on having a separate driver for the i.MX8MP LCDIF.

Regards,
Lucas

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/1/22 11:04, Lucas Stach wrote:

Hi,

[...]

>> Given the two totally different IPs, I don't see bugs of IP control
>> logics should be fixed for both drivers. Naturally, the two would
>> diverge due to different HWs. Looking at Patch 9/9, it basically
>> squashes code to control LCDIFv3 into the mxsfb drm driver with
>> 'if/else' checks(barely no common control code), which is hard to
>> maintain and not able to achieve good scalability for both 'LCDIFv3'
>> and 'LCDIF'.
> 
> I tend to agree with Liu here. Writing a DRM driver isn't that much
> boilerplate anymore with all the helpers we have available in the
> framework today.

I did write a separate driver for this IP before I spent time merging 
them into single driver, that's when I realized a single driver is much 
better and discarded the separate driver idea.

> The IP is so different from the currently supported LCDIF controllers
> that I think trying to support this one in the existing driver actually
> increases the chances to break something when modifying the driver in
> the future. Not everyone is able to test all LCDIF versions. My vote is
> on having a separate driver for the i.MX8MP LCDIF.

If you look at both controllers, it is clear it is still the LCDIF 
behind, even the CSC that is bolted on would suggest that.

I am also not happy when I look at the amount of duplication a separate 
driver would create, it will be some 50% of the code that would be just 
duplicated.

[...]

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> On 3/1/22 11:04, Lucas Stach wrote:
> 
> Hi,
> 
> [...]
> 
> > > Given the two totally different IPs, I don't see bugs of IP control
> > > logics should be fixed for both drivers. Naturally, the two would
> > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > 'if/else' checks(barely no common control code), which is hard to
> > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > and 'LCDIF'.
> > 
> > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > boilerplate anymore with all the helpers we have available in the
> > framework today.
> 
> I did write a separate driver for this IP before I spent time merging 
> them into single driver, that's when I realized a single driver is much 
> better and discarded the separate driver idea.
> 
> > The IP is so different from the currently supported LCDIF controllers
> > that I think trying to support this one in the existing driver actually
> > increases the chances to break something when modifying the driver in
> > the future. Not everyone is able to test all LCDIF versions. My vote is
> > on having a separate driver for the i.MX8MP LCDIF.
> 
> If you look at both controllers, it is clear it is still the LCDIF 
> behind, even the CSC that is bolted on would suggest that.

Yes, but from a driver PoV what you care about is not really the
hardware blocks used to implement something, but the programming model,
i.e. the register interface exposed to software.

> 
> I am also not happy when I look at the amount of duplication a separate 
> driver would create, it will be some 50% of the code that would be just 
> duplicated.
> 
Yea, the duplicated code is still significant, as the HW itself is so
simple. However, if you find yourself in the situation where basically
every actual register access in the driver ends up being in a "if (some
HW rev) ... " clause, i still think it would be better to have a
separate driver, as the programming interface is just different.

Regards,
Lucas

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Adam Ford]

On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > On 3/1/22 11:04, Lucas Stach wrote:
> >
> > Hi,
> >
> > [...]
> >
> > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > logics should be fixed for both drivers. Naturally, the two would
> > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > 'if/else' checks(barely no common control code), which is hard to
> > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > and 'LCDIF'.
> > >
> > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > boilerplate anymore with all the helpers we have available in the
> > > framework today.
> >
> > I did write a separate driver for this IP before I spent time merging
> > them into single driver, that's when I realized a single driver is much
> > better and discarded the separate driver idea.
> >
> > > The IP is so different from the currently supported LCDIF controllers
> > > that I think trying to support this one in the existing driver actually
> > > increases the chances to break something when modifying the driver in
> > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > on having a separate driver for the i.MX8MP LCDIF.
> >
> > If you look at both controllers, it is clear it is still the LCDIF
> > behind, even the CSC that is bolted on would suggest that.
>
> Yes, but from a driver PoV what you care about is not really the
> hardware blocks used to implement something, but the programming model,
> i.e. the register interface exposed to software.
>
> >
> > I am also not happy when I look at the amount of duplication a separate
> > driver would create, it will be some 50% of the code that would be just
> > duplicated.
> >
> Yea, the duplicated code is still significant, as the HW itself is so
> simple. However, if you find yourself in the situation where basically
> every actual register access in the driver ends up being in a "if (some
> HW rev) ... " clause, i still think it would be better to have a
> separate driver, as the programming interface is just different.

I tend to agree with Marek on this one.  We have an instance where the
blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
but different enough where each SoC has it's own set of tables and
some checks.   Lucas created the framework, and others adapted it for
various SoC's.  If there really is nearly 50% common code for the
LCDIF, why not either leave the driver as one or split the common code
into its own driver like lcdif-common and then have smaller drivers
that handle their specific variations.

adam
>
> Regards,
> Lucas
>
>

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Adam Ford]

On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > On 3/1/22 11:04, Lucas Stach wrote:
> >
> > Hi,
> >
> > [...]
> >
> > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > logics should be fixed for both drivers. Naturally, the two would
> > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > 'if/else' checks(barely no common control code), which is hard to
> > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > and 'LCDIF'.
> > >
> > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > boilerplate anymore with all the helpers we have available in the
> > > framework today.
> >
> > I did write a separate driver for this IP before I spent time merging
> > them into single driver, that's when I realized a single driver is much
> > better and discarded the separate driver idea.
> >
> > > The IP is so different from the currently supported LCDIF controllers
> > > that I think trying to support this one in the existing driver actually
> > > increases the chances to break something when modifying the driver in
> > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > on having a separate driver for the i.MX8MP LCDIF.
> >
> > If you look at both controllers, it is clear it is still the LCDIF
> > behind, even the CSC that is bolted on would suggest that.
>
> Yes, but from a driver PoV what you care about is not really the
> hardware blocks used to implement something, but the programming model,
> i.e. the register interface exposed to software.
>
> >
> > I am also not happy when I look at the amount of duplication a separate
> > driver would create, it will be some 50% of the code that would be just
> > duplicated.
> >
> Yea, the duplicated code is still significant, as the HW itself is so
> simple. However, if you find yourself in the situation where basically
> every actual register access in the driver ends up being in a "if (some
> HW rev) ... " clause, i still think it would be better to have a
> separate driver, as the programming interface is just different.

I tend to agree with Marek on this one.  We have an instance where the
blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
but different enough where each SoC has it's own set of tables and
some checks.   Lucas created the framework, and others adapted it for
various SoC's.  If there really is nearly 50% common code for the
LCDIF, why not either leave the driver as one or split the common code
into its own driver like lcdif-common and then have smaller drivers
that handle their specific variations.

adam
>
> Regards,
> Lucas
>
>

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > On 3/1/22 11:04, Lucas Stach wrote:
> > > 
> > > Hi,
> > > 
> > > [...]
> > > 
> > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > and 'LCDIF'.
> > > > 
> > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > boilerplate anymore with all the helpers we have available in the
> > > > framework today.
> > > 
> > > I did write a separate driver for this IP before I spent time merging
> > > them into single driver, that's when I realized a single driver is much
> > > better and discarded the separate driver idea.
> > > 
> > > > The IP is so different from the currently supported LCDIF controllers
> > > > that I think trying to support this one in the existing driver actually
> > > > increases the chances to break something when modifying the driver in
> > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > on having a separate driver for the i.MX8MP LCDIF.
> > > 
> > > If you look at both controllers, it is clear it is still the LCDIF
> > > behind, even the CSC that is bolted on would suggest that.
> > 
> > Yes, but from a driver PoV what you care about is not really the
> > hardware blocks used to implement something, but the programming model,
> > i.e. the register interface exposed to software.
> > 
> > > 
> > > I am also not happy when I look at the amount of duplication a separate
> > > driver would create, it will be some 50% of the code that would be just
> > > duplicated.
> > > 
> > Yea, the duplicated code is still significant, as the HW itself is so
> > simple. However, if you find yourself in the situation where basically
> > every actual register access in the driver ends up being in a "if (some
> > HW rev) ... " clause, i still think it would be better to have a
> > separate driver, as the programming interface is just different.
> 
> I tend to agree with Marek on this one.  We have an instance where the
> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> but different enough where each SoC has it's own set of tables and
> some checks.   Lucas created the framework, and others adapted it for
> various SoC's.  If there really is nearly 50% common code for the
> LCDIF, why not either leave the driver as one or split the common code
> into its own driver like lcdif-common and then have smaller drivers
> that handle their specific variations.

I don't know exactly how the standalone driver looks like, but I guess
the overlap is not really in any real HW specific parts, but the common
DRM boilerplate, so there isn't much point in creating a common lcdif
driver.

As you brought up the blk-ctrl as an example: I'm all for supporting
slightly different hardware in the same driver, as long as the HW
interface is close enough. But then I also opted for a separate 8MP
blk-ctrl driver for those blk-ctrls that differ significantly from the
others, as I think it would make the common driver unmaintainable
trying to support all the different variants in one driver.

Regards,
Lucas

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > On 3/1/22 11:04, Lucas Stach wrote:
> > > 
> > > Hi,
> > > 
> > > [...]
> > > 
> > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > and 'LCDIF'.
> > > > 
> > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > boilerplate anymore with all the helpers we have available in the
> > > > framework today.
> > > 
> > > I did write a separate driver for this IP before I spent time merging
> > > them into single driver, that's when I realized a single driver is much
> > > better and discarded the separate driver idea.
> > > 
> > > > The IP is so different from the currently supported LCDIF controllers
> > > > that I think trying to support this one in the existing driver actually
> > > > increases the chances to break something when modifying the driver in
> > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > on having a separate driver for the i.MX8MP LCDIF.
> > > 
> > > If you look at both controllers, it is clear it is still the LCDIF
> > > behind, even the CSC that is bolted on would suggest that.
> > 
> > Yes, but from a driver PoV what you care about is not really the
> > hardware blocks used to implement something, but the programming model,
> > i.e. the register interface exposed to software.
> > 
> > > 
> > > I am also not happy when I look at the amount of duplication a separate
> > > driver would create, it will be some 50% of the code that would be just
> > > duplicated.
> > > 
> > Yea, the duplicated code is still significant, as the HW itself is so
> > simple. However, if you find yourself in the situation where basically
> > every actual register access in the driver ends up being in a "if (some
> > HW rev) ... " clause, i still think it would be better to have a
> > separate driver, as the programming interface is just different.
> 
> I tend to agree with Marek on this one.  We have an instance where the
> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> but different enough where each SoC has it's own set of tables and
> some checks.   Lucas created the framework, and others adapted it for
> various SoC's.  If there really is nearly 50% common code for the
> LCDIF, why not either leave the driver as one or split the common code
> into its own driver like lcdif-common and then have smaller drivers
> that handle their specific variations.

I don't know exactly how the standalone driver looks like, but I guess
the overlap is not really in any real HW specific parts, but the common
DRM boilerplate, so there isn't much point in creating a common lcdif
driver.

As you brought up the blk-ctrl as an example: I'm all for supporting
slightly different hardware in the same driver, as long as the HW
interface is close enough. But then I also opted for a separate 8MP
blk-ctrl driver for those blk-ctrls that differ significantly from the
others, as I think it would make the common driver unmaintainable
trying to support all the different variants in one driver.

Regards,
Lucas

RE: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Robby Cai]

>-----Original Message-----
>From: Lucas Stach <l.stach@pengutronix.de>
>Sent: 2022年3月1日 21:19
>To: Adam Ford <aford173@gmail.com>
>Cc: Marek Vasut <marex@denx.de>; Ying Liu (OSS) <victor.liu@oss.nxp.com>;
>dri-devel <dri-devel@lists.freedesktop.org>; devicetree
><devicetree@vger.kernel.org>; Peng Fan <peng.fan@nxp.com>; Alexander Stein
><alexander.stein@ew.tq-group.com>; Rob Herring <robh+dt@kernel.org>;
>Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Sam Ravnborg
><sam@ravnborg.org>; Robby Cai <robby.cai@nxp.com>
>Subject: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for
>i.MX8MP
>
>Caution: EXT Email
>
>Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
>> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de>
>wrote:
>> >
>> > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
>> > > On 3/1/22 11:04, Lucas Stach wrote:
>> > >
>> > > Hi,
>> > >
>> > > [...]
>> > >
>> > > > > Given the two totally different IPs, I don't see bugs of IP
>> > > > > control logics should be fixed for both drivers. Naturally,
>> > > > > the two would diverge due to different HWs. Looking at Patch
>> > > > > 9/9, it basically squashes code to control LCDIFv3 into the
>> > > > > mxsfb drm driver with 'if/else' checks(barely no common
>> > > > > control code), which is hard to maintain and not able to achieve good
>scalability for both 'LCDIFv3'
>> > > > > and 'LCDIF'.
>> > > >
>> > > > I tend to agree with Liu here. Writing a DRM driver isn't that
>> > > > much boilerplate anymore with all the helpers we have available
>> > > > in the framework today.
>> > >
>> > > I did write a separate driver for this IP before I spent time
>> > > merging them into single driver, that's when I realized a single
>> > > driver is much better and discarded the separate driver idea.
>> > >
>> > > > The IP is so different from the currently supported LCDIF
>> > > > controllers that I think trying to support this one in the
>> > > > existing driver actually increases the chances to break
>> > > > something when modifying the driver in the future. Not everyone
>> > > > is able to test all LCDIF versions. My vote is on having a separate driver
>for the i.MX8MP LCDIF.
>> > >
>> > > If you look at both controllers, it is clear it is still the LCDIF
>> > > behind, even the CSC that is bolted on would suggest that.
>> >
>> > Yes, but from a driver PoV what you care about is not really the
>> > hardware blocks used to implement something, but the programming
>> > model, i.e. the register interface exposed to software.
>> >
>> > >
>> > > I am also not happy when I look at the amount of duplication a
>> > > separate driver would create, it will be some 50% of the code that
>> > > would be just duplicated.
>> > >
>> > Yea, the duplicated code is still significant, as the HW itself is
>> > so simple. However, if you find yourself in the situation where
>> > basically every actual register access in the driver ends up being
>> > in a "if (some HW rev) ... " clause, i still think it would be
>> > better to have a separate driver, as the programming interface is just
>different.
>>
>> I tend to agree with Marek on this one.  We have an instance where the
>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>> but different enough where each SoC has it's own set of tables and
>> some checks.   Lucas created the framework, and others adapted it for
>> various SoC's.  If there really is nearly 50% common code for the
>> LCDIF, why not either leave the driver as one or split the common code
>> into its own driver like lcdif-common and then have smaller drivers
>> that handle their specific variations.
>
>I don't know exactly how the standalone driver looks like, but I guess the
>overlap is not really in any real HW specific parts, but the common DRM
>boilerplate, so there isn't much point in creating a common lcdif driver.
>
>As you brought up the blk-ctrl as an example: I'm all for supporting slightly
>different hardware in the same driver, as long as the HW interface is close
>enough. But then I also opted for a separate 8MP blk-ctrl driver for those
>blk-ctrls that differ significantly from the others, as I think it would make the
>common driver unmaintainable trying to support all the different variants in
>one driver.
>
>Regards,
>Lucas

LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series, although it's also called 'LCDIF'.
We prefer not mix these two series of IPs in one driver for ease of maintenance and extension.

Regards,
Robby   

RE: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Robby Cai]

>-----Original Message-----
>From: Lucas Stach <l.stach@pengutronix.de>
>Sent: 2022年3月1日 21:19
>To: Adam Ford <aford173@gmail.com>
>Cc: Marek Vasut <marex@denx.de>; Ying Liu (OSS) <victor.liu@oss.nxp.com>;
>dri-devel <dri-devel@lists.freedesktop.org>; devicetree
><devicetree@vger.kernel.org>; Peng Fan <peng.fan@nxp.com>; Alexander Stein
><alexander.stein@ew.tq-group.com>; Rob Herring <robh+dt@kernel.org>;
>Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Sam Ravnborg
><sam@ravnborg.org>; Robby Cai <robby.cai@nxp.com>
>Subject: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for
>i.MX8MP
>
>Caution: EXT Email
>
>Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
>> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de>
>wrote:
>> >
>> > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
>> > > On 3/1/22 11:04, Lucas Stach wrote:
>> > >
>> > > Hi,
>> > >
>> > > [...]
>> > >
>> > > > > Given the two totally different IPs, I don't see bugs of IP
>> > > > > control logics should be fixed for both drivers. Naturally,
>> > > > > the two would diverge due to different HWs. Looking at Patch
>> > > > > 9/9, it basically squashes code to control LCDIFv3 into the
>> > > > > mxsfb drm driver with 'if/else' checks(barely no common
>> > > > > control code), which is hard to maintain and not able to achieve good
>scalability for both 'LCDIFv3'
>> > > > > and 'LCDIF'.
>> > > >
>> > > > I tend to agree with Liu here. Writing a DRM driver isn't that
>> > > > much boilerplate anymore with all the helpers we have available
>> > > > in the framework today.
>> > >
>> > > I did write a separate driver for this IP before I spent time
>> > > merging them into single driver, that's when I realized a single
>> > > driver is much better and discarded the separate driver idea.
>> > >
>> > > > The IP is so different from the currently supported LCDIF
>> > > > controllers that I think trying to support this one in the
>> > > > existing driver actually increases the chances to break
>> > > > something when modifying the driver in the future. Not everyone
>> > > > is able to test all LCDIF versions. My vote is on having a separate driver
>for the i.MX8MP LCDIF.
>> > >
>> > > If you look at both controllers, it is clear it is still the LCDIF
>> > > behind, even the CSC that is bolted on would suggest that.
>> >
>> > Yes, but from a driver PoV what you care about is not really the
>> > hardware blocks used to implement something, but the programming
>> > model, i.e. the register interface exposed to software.
>> >
>> > >
>> > > I am also not happy when I look at the amount of duplication a
>> > > separate driver would create, it will be some 50% of the code that
>> > > would be just duplicated.
>> > >
>> > Yea, the duplicated code is still significant, as the HW itself is
>> > so simple. However, if you find yourself in the situation where
>> > basically every actual register access in the driver ends up being
>> > in a "if (some HW rev) ... " clause, i still think it would be
>> > better to have a separate driver, as the programming interface is just
>different.
>>
>> I tend to agree with Marek on this one.  We have an instance where the
>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>> but different enough where each SoC has it's own set of tables and
>> some checks.   Lucas created the framework, and others adapted it for
>> various SoC's.  If there really is nearly 50% common code for the
>> LCDIF, why not either leave the driver as one or split the common code
>> into its own driver like lcdif-common and then have smaller drivers
>> that handle their specific variations.
>
>I don't know exactly how the standalone driver looks like, but I guess the
>overlap is not really in any real HW specific parts, but the common DRM
>boilerplate, so there isn't much point in creating a common lcdif driver.
>
>As you brought up the blk-ctrl as an example: I'm all for supporting slightly
>different hardware in the same driver, as long as the HW interface is close
>enough. But then I also opted for a separate 8MP blk-ctrl driver for those
>blk-ctrls that differ significantly from the others, as I think it would make the
>common driver unmaintainable trying to support all the different variants in
>one driver.
>
>Regards,
>Lucas

LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series, although it's also called 'LCDIF'.
We prefer not mix these two series of IPs in one driver for ease of maintenance and extension.

Regards,
Robby   

Re: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/1/22 14:37, Robby Cai wrote:

Hi,

[...]

>>> I tend to agree with Marek on this one.  We have an instance where the
>>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>>> but different enough where each SoC has it's own set of tables and
>>> some checks.   Lucas created the framework, and others adapted it for
>>> various SoC's.  If there really is nearly 50% common code for the
>>> LCDIF, why not either leave the driver as one or split the common code
>>> into its own driver like lcdif-common and then have smaller drivers
>>> that handle their specific variations.
>>
>> I don't know exactly how the standalone driver looks like, but I guess the
>> overlap is not really in any real HW specific parts, but the common DRM
>> boilerplate, so there isn't much point in creating a common lcdif driver.
>>
>> As you brought up the blk-ctrl as an example: I'm all for supporting slightly
>> different hardware in the same driver, as long as the HW interface is close
>> enough. But then I also opted for a separate 8MP blk-ctrl driver for those
>> blk-ctrls that differ significantly from the others, as I think it would make the
>> common driver unmaintainable trying to support all the different variants in
>> one driver.
>>
>> Regards,
>> Lucas
> 
> LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series, although it's also called 'LCDIF'.
> We prefer not mix these two series of IPs in one driver for ease of maintenance and extension.

Where does the MX8MP LCDIF come from then, SGTL maybe ?

Re: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/1/22 14:37, Robby Cai wrote:

Hi,

[...]

>>> I tend to agree with Marek on this one.  We have an instance where the
>>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>>> but different enough where each SoC has it's own set of tables and
>>> some checks.   Lucas created the framework, and others adapted it for
>>> various SoC's.  If there really is nearly 50% common code for the
>>> LCDIF, why not either leave the driver as one or split the common code
>>> into its own driver like lcdif-common and then have smaller drivers
>>> that handle their specific variations.
>>
>> I don't know exactly how the standalone driver looks like, but I guess the
>> overlap is not really in any real HW specific parts, but the common DRM
>> boilerplate, so there isn't much point in creating a common lcdif driver.
>>
>> As you brought up the blk-ctrl as an example: I'm all for supporting slightly
>> different hardware in the same driver, as long as the HW interface is close
>> enough. But then I also opted for a separate 8MP blk-ctrl driver for those
>> blk-ctrls that differ significantly from the others, as I think it would make the
>> common driver unmaintainable trying to support all the different variants in
>> one driver.
>>
>> Regards,
>> Lucas
> 
> LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series, although it's also called 'LCDIF'.
> We prefer not mix these two series of IPs in one driver for ease of maintenance and extension.

Where does the MX8MP LCDIF come from then, SGTL maybe ?

RE: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Robby Cai]

>-----Original Message-----
>From: Marek Vasut <marex@denx.de>
>Sent: 2022年3月2日 10:50
>To: Robby Cai <robby.cai@nxp.com>; Lucas Stach <l.stach@pengutronix.de>;
>Adam Ford <aford173@gmail.com>
>Cc: Ying Liu (OSS) <victor.liu@oss.nxp.com>; dri-devel
><dri-devel@lists.freedesktop.org>; devicetree <devicetree@vger.kernel.org>;
>Peng Fan <peng.fan@nxp.com>; Alexander Stein
><alexander.stein@ew.tq-group.com>; Rob Herring <robh+dt@kernel.org>;
>Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Sam Ravnborg
><sam@ravnborg.org>
>Subject: Re: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for
>i.MX8MP
>
>Caution: EXT Email
>
>On 3/1/22 14:37, Robby Cai wrote:
>
>Hi,
>
>[...]
>
>>>> I tend to agree with Marek on this one.  We have an instance where
>>>> the blk-ctrl and the GPC driver between 8m, mini, nano, plus are
>>>> close, but different enough where each SoC has it's own set of tables and
>>>> some checks.   Lucas created the framework, and others adapted it for
>>>> various SoC's.  If there really is nearly 50% common code for the
>>>> LCDIF, why not either leave the driver as one or split the common
>>>> code into its own driver like lcdif-common and then have smaller
>>>> drivers that handle their specific variations.
>>>
>>> I don't know exactly how the standalone driver looks like, but I
>>> guess the overlap is not really in any real HW specific parts, but
>>> the common DRM boilerplate, so there isn't much point in creating a
>common lcdif driver.
>>>
>>> As you brought up the blk-ctrl as an example: I'm all for supporting
>>> slightly different hardware in the same driver, as long as the HW
>>> interface is close enough. But then I also opted for a separate 8MP
>>> blk-ctrl driver for those blk-ctrls that differ significantly from
>>> the others, as I think it would make the common driver unmaintainable
>>> trying to support all the different variants in one driver.
>>>
>>> Regards,
>>> Lucas
>>
>> LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series,
>although it's also called 'LCDIF'.
>> We prefer not mix these two series of IPs in one driver for ease of
>maintenance and extension.
>
>Where does the MX8MP LCDIF come from then, SGTL maybe ?

AFAIK, it's RT1170. You may have a check on RM [1]. Interestingly, this SoC has both eLCDIF and LCDIFv2, two IPs we are talking about.

[1] https://www.nxp.com/webapp/Download?colCode=IMXRT1170RM

Regards,
Robby

RE: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Robby Cai]

>-----Original Message-----
>From: Marek Vasut <marex@denx.de>
>Sent: 2022年3月2日 10:50
>To: Robby Cai <robby.cai@nxp.com>; Lucas Stach <l.stach@pengutronix.de>;
>Adam Ford <aford173@gmail.com>
>Cc: Ying Liu (OSS) <victor.liu@oss.nxp.com>; dri-devel
><dri-devel@lists.freedesktop.org>; devicetree <devicetree@vger.kernel.org>;
>Peng Fan <peng.fan@nxp.com>; Alexander Stein
><alexander.stein@ew.tq-group.com>; Rob Herring <robh+dt@kernel.org>;
>Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Sam Ravnborg
><sam@ravnborg.org>
>Subject: Re: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for
>i.MX8MP
>
>Caution: EXT Email
>
>On 3/1/22 14:37, Robby Cai wrote:
>
>Hi,
>
>[...]
>
>>>> I tend to agree with Marek on this one.  We have an instance where
>>>> the blk-ctrl and the GPC driver between 8m, mini, nano, plus are
>>>> close, but different enough where each SoC has it's own set of tables and
>>>> some checks.   Lucas created the framework, and others adapted it for
>>>> various SoC's.  If there really is nearly 50% common code for the
>>>> LCDIF, why not either leave the driver as one or split the common
>>>> code into its own driver like lcdif-common and then have smaller
>>>> drivers that handle their specific variations.
>>>
>>> I don't know exactly how the standalone driver looks like, but I
>>> guess the overlap is not really in any real HW specific parts, but
>>> the common DRM boilerplate, so there isn't much point in creating a
>common lcdif driver.
>>>
>>> As you brought up the blk-ctrl as an example: I'm all for supporting
>>> slightly different hardware in the same driver, as long as the HW
>>> interface is close enough. But then I also opted for a separate 8MP
>>> blk-ctrl driver for those blk-ctrls that differ significantly from
>>> the others, as I think it would make the common driver unmaintainable
>>> trying to support all the different variants in one driver.
>>>
>>> Regards,
>>> Lucas
>>
>> LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series,
>although it's also called 'LCDIF'.
>> We prefer not mix these two series of IPs in one driver for ease of
>maintenance and extension.
>
>Where does the MX8MP LCDIF come from then, SGTL maybe ?

AFAIK, it's RT1170. You may have a check on RM [1]. Interestingly, this SoC has both eLCDIF and LCDIFv2, two IPs we are talking about.

[1] https://www.nxp.com/webapp/Download?colCode=IMXRT1170RM

Regards,
Robby

Re: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/2/22 14:14, Robby Cai wrote:

Hi

[...]

>>> LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series,
>> although it's also called 'LCDIF'.
>>> We prefer not mix these two series of IPs in one driver for ease of
>> maintenance and extension.
>>
>> Where does the MX8MP LCDIF come from then, SGTL maybe ?
> 
> AFAIK, it's RT1170. You may have a check on RM [1]. Interestingly, this SoC has both eLCDIF and LCDIFv2, two IPs we are talking about.
> 
> [1] https://www.nxp.com/webapp/Download?colCode=IMXRT1170RM

That's interesting, I wasn't aware of this one. So that's MX8MP LCDIF + 
overlays. Did the LCDIF v2 in iMXRT1070 have any predecessor too ?

Re: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/2/22 14:14, Robby Cai wrote:

Hi

[...]

>>> LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series,
>> although it's also called 'LCDIF'.
>>> We prefer not mix these two series of IPs in one driver for ease of
>> maintenance and extension.
>>
>> Where does the MX8MP LCDIF come from then, SGTL maybe ?
> 
> AFAIK, it's RT1170. You may have a check on RM [1]. Interestingly, this SoC has both eLCDIF and LCDIFv2, two IPs we are talking about.
> 
> [1] https://www.nxp.com/webapp/Download?colCode=IMXRT1170RM

That's interesting, I wasn't aware of this one. So that's MX8MP LCDIF + 
overlays. Did the LCDIF v2 in iMXRT1070 have any predecessor too ?

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/1/22 14:18, Lucas Stach wrote:
> Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
>> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>>>
>>> Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
>>>> On 3/1/22 11:04, Lucas Stach wrote:
>>>>
>>>> Hi,
>>>>
>>>> [...]
>>>>
>>>>>> Given the two totally different IPs, I don't see bugs of IP control
>>>>>> logics should be fixed for both drivers. Naturally, the two would
>>>>>> diverge due to different HWs. Looking at Patch 9/9, it basically
>>>>>> squashes code to control LCDIFv3 into the mxsfb drm driver with
>>>>>> 'if/else' checks(barely no common control code), which is hard to
>>>>>> maintain and not able to achieve good scalability for both 'LCDIFv3'
>>>>>> and 'LCDIF'.
>>>>>
>>>>> I tend to agree with Liu here. Writing a DRM driver isn't that much
>>>>> boilerplate anymore with all the helpers we have available in the
>>>>> framework today.
>>>>
>>>> I did write a separate driver for this IP before I spent time merging
>>>> them into single driver, that's when I realized a single driver is much
>>>> better and discarded the separate driver idea.
>>>>
>>>>> The IP is so different from the currently supported LCDIF controllers
>>>>> that I think trying to support this one in the existing driver actually
>>>>> increases the chances to break something when modifying the driver in
>>>>> the future. Not everyone is able to test all LCDIF versions. My vote is
>>>>> on having a separate driver for the i.MX8MP LCDIF.
>>>>
>>>> If you look at both controllers, it is clear it is still the LCDIF
>>>> behind, even the CSC that is bolted on would suggest that.
>>>
>>> Yes, but from a driver PoV what you care about is not really the
>>> hardware blocks used to implement something, but the programming model,
>>> i.e. the register interface exposed to software.
>>>
>>>>
>>>> I am also not happy when I look at the amount of duplication a separate
>>>> driver would create, it will be some 50% of the code that would be just
>>>> duplicated.
>>>>
>>> Yea, the duplicated code is still significant, as the HW itself is so
>>> simple. However, if you find yourself in the situation where basically
>>> every actual register access in the driver ends up being in a "if (some
>>> HW rev) ... " clause, i still think it would be better to have a
>>> separate driver, as the programming interface is just different.
>>
>> I tend to agree with Marek on this one.  We have an instance where the
>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>> but different enough where each SoC has it's own set of tables and
>> some checks.   Lucas created the framework, and others adapted it for
>> various SoC's.  If there really is nearly 50% common code for the
>> LCDIF, why not either leave the driver as one or split the common code
>> into its own driver like lcdif-common and then have smaller drivers
>> that handle their specific variations.
> 
> I don't know exactly how the standalone driver looks like, but I guess
> the overlap is not really in any real HW specific parts, but the common
> DRM boilerplate, so there isn't much point in creating a common lcdif
> driver.

The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
that, there is some 400 LoC which are specific to old LCDIF and this 
patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
shared boilerplate that would be duplicated .

> As you brought up the blk-ctrl as an example: I'm all for supporting
> slightly different hardware in the same driver, as long as the HW
> interface is close enough. But then I also opted for a separate 8MP
> blk-ctrl driver for those blk-ctrls that differ significantly from the
> others, as I think it would make the common driver unmaintainable
> trying to support all the different variants in one driver.

But then you also need to maintain two sets of boilerplate, they 
diverge, and that is not good.

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/1/22 14:18, Lucas Stach wrote:
> Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
>> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>>>
>>> Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
>>>> On 3/1/22 11:04, Lucas Stach wrote:
>>>>
>>>> Hi,
>>>>
>>>> [...]
>>>>
>>>>>> Given the two totally different IPs, I don't see bugs of IP control
>>>>>> logics should be fixed for both drivers. Naturally, the two would
>>>>>> diverge due to different HWs. Looking at Patch 9/9, it basically
>>>>>> squashes code to control LCDIFv3 into the mxsfb drm driver with
>>>>>> 'if/else' checks(barely no common control code), which is hard to
>>>>>> maintain and not able to achieve good scalability for both 'LCDIFv3'
>>>>>> and 'LCDIF'.
>>>>>
>>>>> I tend to agree with Liu here. Writing a DRM driver isn't that much
>>>>> boilerplate anymore with all the helpers we have available in the
>>>>> framework today.
>>>>
>>>> I did write a separate driver for this IP before I spent time merging
>>>> them into single driver, that's when I realized a single driver is much
>>>> better and discarded the separate driver idea.
>>>>
>>>>> The IP is so different from the currently supported LCDIF controllers
>>>>> that I think trying to support this one in the existing driver actually
>>>>> increases the chances to break something when modifying the driver in
>>>>> the future. Not everyone is able to test all LCDIF versions. My vote is
>>>>> on having a separate driver for the i.MX8MP LCDIF.
>>>>
>>>> If you look at both controllers, it is clear it is still the LCDIF
>>>> behind, even the CSC that is bolted on would suggest that.
>>>
>>> Yes, but from a driver PoV what you care about is not really the
>>> hardware blocks used to implement something, but the programming model,
>>> i.e. the register interface exposed to software.
>>>
>>>>
>>>> I am also not happy when I look at the amount of duplication a separate
>>>> driver would create, it will be some 50% of the code that would be just
>>>> duplicated.
>>>>
>>> Yea, the duplicated code is still significant, as the HW itself is so
>>> simple. However, if you find yourself in the situation where basically
>>> every actual register access in the driver ends up being in a "if (some
>>> HW rev) ... " clause, i still think it would be better to have a
>>> separate driver, as the programming interface is just different.
>>
>> I tend to agree with Marek on this one.  We have an instance where the
>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>> but different enough where each SoC has it's own set of tables and
>> some checks.   Lucas created the framework, and others adapted it for
>> various SoC's.  If there really is nearly 50% common code for the
>> LCDIF, why not either leave the driver as one or split the common code
>> into its own driver like lcdif-common and then have smaller drivers
>> that handle their specific variations.
> 
> I don't know exactly how the standalone driver looks like, but I guess
> the overlap is not really in any real HW specific parts, but the common
> DRM boilerplate, so there isn't much point in creating a common lcdif
> driver.

The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
that, there is some 400 LoC which are specific to old LCDIF and this 
patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
shared boilerplate that would be duplicated .

> As you brought up the blk-ctrl as an example: I'm all for supporting
> slightly different hardware in the same driver, as long as the HW
> interface is close enough. But then I also opted for a separate 8MP
> blk-ctrl driver for those blk-ctrls that differ significantly from the
> others, as I think it would make the common driver unmaintainable
> trying to support all the different variants in one driver.

But then you also need to maintain two sets of boilerplate, they 
diverge, and that is not good.

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> On 3/1/22 14:18, Lucas Stach wrote:
> > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > 
> > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > and 'LCDIF'.
> > > > > > 
> > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > framework today.
> > > > > 
> > > > > I did write a separate driver for this IP before I spent time merging
> > > > > them into single driver, that's when I realized a single driver is much
> > > > > better and discarded the separate driver idea.
> > > > > 
> > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > that I think trying to support this one in the existing driver actually
> > > > > > increases the chances to break something when modifying the driver in
> > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > 
> > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > behind, even the CSC that is bolted on would suggest that.
> > > > 
> > > > Yes, but from a driver PoV what you care about is not really the
> > > > hardware blocks used to implement something, but the programming model,
> > > > i.e. the register interface exposed to software.
> > > > 
> > > > > 
> > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > driver would create, it will be some 50% of the code that would be just
> > > > > duplicated.
> > > > > 
> > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > simple. However, if you find yourself in the situation where basically
> > > > every actual register access in the driver ends up being in a "if (some
> > > > HW rev) ... " clause, i still think it would be better to have a
> > > > separate driver, as the programming interface is just different.
> > > 
> > > I tend to agree with Marek on this one.  We have an instance where the
> > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > but different enough where each SoC has it's own set of tables and
> > > some checks.   Lucas created the framework, and others adapted it for
> > > various SoC's.  If there really is nearly 50% common code for the
> > > LCDIF, why not either leave the driver as one or split the common code
> > > into its own driver like lcdif-common and then have smaller drivers
> > > that handle their specific variations.
> > 
> > I don't know exactly how the standalone driver looks like, but I guess
> > the overlap is not really in any real HW specific parts, but the common
> > DRM boilerplate, so there isn't much point in creating a common lcdif
> > driver.
> 
> The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> that, there is some 400 LoC which are specific to old LCDIF and this 
> patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> shared boilerplate that would be duplicated .

That is probably ignoring the fact that the 8MP LCDIF does not support
any overlays, so it could use the drm_simple_display_pipe
infrastructure to reduce the needed boilerplate.
> 
> > As you brought up the blk-ctrl as an example: I'm all for supporting
> > slightly different hardware in the same driver, as long as the HW
> > interface is close enough. But then I also opted for a separate 8MP
> > blk-ctrl driver for those blk-ctrls that differ significantly from the
> > others, as I think it would make the common driver unmaintainable
> > trying to support all the different variants in one driver.
> 
> But then you also need to maintain two sets of boilerplate, they 
> diverge, and that is not good.

I don't think that there is much chance for bugs going unfixed due to
divergence in the boilerplate, especially if you use the simple pipe
framework to handle most of that stuff for you, which gives you a lot
of code sharing with other simple DRM drivers.

Regards,
Lucas

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> On 3/1/22 14:18, Lucas Stach wrote:
> > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > 
> > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > and 'LCDIF'.
> > > > > > 
> > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > framework today.
> > > > > 
> > > > > I did write a separate driver for this IP before I spent time merging
> > > > > them into single driver, that's when I realized a single driver is much
> > > > > better and discarded the separate driver idea.
> > > > > 
> > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > that I think trying to support this one in the existing driver actually
> > > > > > increases the chances to break something when modifying the driver in
> > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > 
> > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > behind, even the CSC that is bolted on would suggest that.
> > > > 
> > > > Yes, but from a driver PoV what you care about is not really the
> > > > hardware blocks used to implement something, but the programming model,
> > > > i.e. the register interface exposed to software.
> > > > 
> > > > > 
> > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > driver would create, it will be some 50% of the code that would be just
> > > > > duplicated.
> > > > > 
> > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > simple. However, if you find yourself in the situation where basically
> > > > every actual register access in the driver ends up being in a "if (some
> > > > HW rev) ... " clause, i still think it would be better to have a
> > > > separate driver, as the programming interface is just different.
> > > 
> > > I tend to agree with Marek on this one.  We have an instance where the
> > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > but different enough where each SoC has it's own set of tables and
> > > some checks.   Lucas created the framework, and others adapted it for
> > > various SoC's.  If there really is nearly 50% common code for the
> > > LCDIF, why not either leave the driver as one or split the common code
> > > into its own driver like lcdif-common and then have smaller drivers
> > > that handle their specific variations.
> > 
> > I don't know exactly how the standalone driver looks like, but I guess
> > the overlap is not really in any real HW specific parts, but the common
> > DRM boilerplate, so there isn't much point in creating a common lcdif
> > driver.
> 
> The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> that, there is some 400 LoC which are specific to old LCDIF and this 
> patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> shared boilerplate that would be duplicated .

That is probably ignoring the fact that the 8MP LCDIF does not support
any overlays, so it could use the drm_simple_display_pipe
infrastructure to reduce the needed boilerplate.
> 
> > As you brought up the blk-ctrl as an example: I'm all for supporting
> > slightly different hardware in the same driver, as long as the HW
> > interface is close enough. But then I also opted for a separate 8MP
> > blk-ctrl driver for those blk-ctrls that differ significantly from the
> > others, as I think it would make the common driver unmaintainable
> > trying to support all the different variants in one driver.
> 
> But then you also need to maintain two sets of boilerplate, they 
> diverge, and that is not good.

I don't think that there is much chance for bugs going unfixed due to
divergence in the boilerplate, especially if you use the simple pipe
framework to handle most of that stuff for you, which gives you a lot
of code sharing with other simple DRM drivers.

Regards,
Lucas

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > On 3/1/22 14:18, Lucas Stach wrote:
> > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > and 'LCDIF'.
> > > > > > > 
> > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > framework today.
> > > > > > 
> > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > better and discarded the separate driver idea.
> > > > > > 
> > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > 
> > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > 
> > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > hardware blocks used to implement something, but the programming model,
> > > > > i.e. the register interface exposed to software.
> > > > > 
> > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > duplicated.
> > > > > > 
> > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > simple. However, if you find yourself in the situation where basically
> > > > > every actual register access in the driver ends up being in a "if (some
> > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > separate driver, as the programming interface is just different.
> > > > 
> > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > but different enough where each SoC has it's own set of tables and
> > > > some checks.   Lucas created the framework, and others adapted it for
> > > > various SoC's.  If there really is nearly 50% common code for the
> > > > LCDIF, why not either leave the driver as one or split the common code
> > > > into its own driver like lcdif-common and then have smaller drivers
> > > > that handle their specific variations.
> > > 
> > > I don't know exactly how the standalone driver looks like, but I guess
> > > the overlap is not really in any real HW specific parts, but the common
> > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > driver.
> > 
> > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > that, there is some 400 LoC which are specific to old LCDIF and this 
> > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > shared boilerplate that would be duplicated .
> 
> That is probably ignoring the fact that the 8MP LCDIF does not support
> any overlays, so it could use the drm_simple_display_pipe
> infrastructure to reduce the needed boilerplate.

The drm_simple_display_pipe infrastructure is probably too simple for
i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
outputs respectively. To use that infrastructure means there would be
three dri cards in all. However, the three LCDIF instances can be
wrapped by the one drm device, which is not the boilerplate code in the
current mxsfb driver may handle.

Regards,
Liu Ying

> > > As you brought up the blk-ctrl as an example: I'm all for supporting
> > > slightly different hardware in the same driver, as long as the HW
> > > interface is close enough. But then I also opted for a separate 8MP
> > > blk-ctrl driver for those blk-ctrls that differ significantly from the
> > > others, as I think it would make the common driver unmaintainable
> > > trying to support all the different variants in one driver.
> > 
> > But then you also need to maintain two sets of boilerplate, they 
> > diverge, and that is not good.
> 
> I don't think that there is much chance for bugs going unfixed due to
> divergence in the boilerplate, especially if you use the simple pipe
> framework to handle most of that stuff for you, which gives you a lot
> of code sharing with other simple DRM drivers.
> 
> Regards,
> Lucas
> 
> 

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > On 3/1/22 14:18, Lucas Stach wrote:
> > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > and 'LCDIF'.
> > > > > > > 
> > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > framework today.
> > > > > > 
> > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > better and discarded the separate driver idea.
> > > > > > 
> > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > 
> > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > 
> > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > hardware blocks used to implement something, but the programming model,
> > > > > i.e. the register interface exposed to software.
> > > > > 
> > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > duplicated.
> > > > > > 
> > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > simple. However, if you find yourself in the situation where basically
> > > > > every actual register access in the driver ends up being in a "if (some
> > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > separate driver, as the programming interface is just different.
> > > > 
> > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > but different enough where each SoC has it's own set of tables and
> > > > some checks.   Lucas created the framework, and others adapted it for
> > > > various SoC's.  If there really is nearly 50% common code for the
> > > > LCDIF, why not either leave the driver as one or split the common code
> > > > into its own driver like lcdif-common and then have smaller drivers
> > > > that handle their specific variations.
> > > 
> > > I don't know exactly how the standalone driver looks like, but I guess
> > > the overlap is not really in any real HW specific parts, but the common
> > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > driver.
> > 
> > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > that, there is some 400 LoC which are specific to old LCDIF and this 
> > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > shared boilerplate that would be duplicated .
> 
> That is probably ignoring the fact that the 8MP LCDIF does not support
> any overlays, so it could use the drm_simple_display_pipe
> infrastructure to reduce the needed boilerplate.

The drm_simple_display_pipe infrastructure is probably too simple for
i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
outputs respectively. To use that infrastructure means there would be
three dri cards in all. However, the three LCDIF instances can be
wrapped by the one drm device, which is not the boilerplate code in the
current mxsfb driver may handle.

Regards,
Liu Ying

> > > As you brought up the blk-ctrl as an example: I'm all for supporting
> > > slightly different hardware in the same driver, as long as the HW
> > > interface is close enough. But then I also opted for a separate 8MP
> > > blk-ctrl driver for those blk-ctrls that differ significantly from the
> > > others, as I think it would make the common driver unmaintainable
> > > trying to support all the different variants in one driver.
> > 
> > But then you also need to maintain two sets of boilerplate, they 
> > diverge, and that is not good.
> 
> I don't think that there is much chance for bugs going unfixed due to
> divergence in the boilerplate, especially if you use the simple pipe
> framework to handle most of that stuff for you, which gives you a lot
> of code sharing with other simple DRM drivers.
> 
> Regards,
> Lucas
> 
> 

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
> On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> > Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > > On 3/1/22 14:18, Lucas Stach wrote:
> > > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > > and 'LCDIF'.
> > > > > > > > 
> > > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > > framework today.
> > > > > > > 
> > > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > > better and discarded the separate driver idea.
> > > > > > > 
> > > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > > 
> > > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > > 
> > > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > > hardware blocks used to implement something, but the programming model,
> > > > > > i.e. the register interface exposed to software.
> > > > > > 
> > > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > > duplicated.
> > > > > > > 
> > > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > > simple. However, if you find yourself in the situation where basically
> > > > > > every actual register access in the driver ends up being in a "if (some
> > > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > > separate driver, as the programming interface is just different.
> > > > > 
> > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > but different enough where each SoC has it's own set of tables and
> > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > that handle their specific variations.
> > > > 
> > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > the overlap is not really in any real HW specific parts, but the common
> > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > driver.
> > > 
> > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > > that, there is some 400 LoC which are specific to old LCDIF and this 
> > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > > shared boilerplate that would be duplicated .
> > 
> > That is probably ignoring the fact that the 8MP LCDIF does not support
> > any overlays, so it could use the drm_simple_display_pipe
> > infrastructure to reduce the needed boilerplate.
> 
> The drm_simple_display_pipe infrastructure is probably too simple for
> i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
> embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
> outputs respectively. To use that infrastructure means there would be
> three dri cards in all. However, the three LCDIF instances can be
> wrapped by the one drm device, which is not the boilerplate code in the
> current mxsfb driver may handle.

While that may make things a little simpler for userspace, I'm not sure
if this is the right thing to do. It complicates the driver a lot,
especially if you want to get things like independent power management,
etc. right. It also creates a fake view for userspace, where is looks
like there might be some shared resources between the different display
paths, while in reality they are fully independent.

While we do something similar on the GPU side and collect all GPU cores
under a single DRM device, I'm not fully convinced that this was a good
decision. It now comes back to bite us when the SoC topologies get a
little more interesting and e.g. devices are behind different IOMMU
streams.

Regards,
Lucas

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
> On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> > Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > > On 3/1/22 14:18, Lucas Stach wrote:
> > > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > > and 'LCDIF'.
> > > > > > > > 
> > > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > > framework today.
> > > > > > > 
> > > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > > better and discarded the separate driver idea.
> > > > > > > 
> > > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > > 
> > > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > > 
> > > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > > hardware blocks used to implement something, but the programming model,
> > > > > > i.e. the register interface exposed to software.
> > > > > > 
> > > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > > duplicated.
> > > > > > > 
> > > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > > simple. However, if you find yourself in the situation where basically
> > > > > > every actual register access in the driver ends up being in a "if (some
> > > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > > separate driver, as the programming interface is just different.
> > > > > 
> > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > but different enough where each SoC has it's own set of tables and
> > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > that handle their specific variations.
> > > > 
> > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > the overlap is not really in any real HW specific parts, but the common
> > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > driver.
> > > 
> > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > > that, there is some 400 LoC which are specific to old LCDIF and this 
> > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > > shared boilerplate that would be duplicated .
> > 
> > That is probably ignoring the fact that the 8MP LCDIF does not support
> > any overlays, so it could use the drm_simple_display_pipe
> > infrastructure to reduce the needed boilerplate.
> 
> The drm_simple_display_pipe infrastructure is probably too simple for
> i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
> embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
> outputs respectively. To use that infrastructure means there would be
> three dri cards in all. However, the three LCDIF instances can be
> wrapped by the one drm device, which is not the boilerplate code in the
> current mxsfb driver may handle.

While that may make things a little simpler for userspace, I'm not sure
if this is the right thing to do. It complicates the driver a lot,
especially if you want to get things like independent power management,
etc. right. It also creates a fake view for userspace, where is looks
like there might be some shared resources between the different display
paths, while in reality they are fully independent.

While we do something similar on the GPU side and collect all GPU cores
under a single DRM device, I'm not fully convinced that this was a good
decision. It now comes back to bite us when the SoC topologies get a
little more interesting and e.g. devices are behind different IOMMU
streams.

Regards,
Lucas

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Wed, 2022-03-02 at 12:57 +0100, Lucas Stach wrote:
> Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
> > On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> > > Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > > > On 3/1/22 14:18, Lucas Stach wrote:
> > > > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > [...]
> > > > > > > > 
> > > > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > > > and 'LCDIF'.
> > > > > > > > > 
> > > > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > > > framework today.
> > > > > > > > 
> > > > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > > > better and discarded the separate driver idea.
> > > > > > > > 
> > > > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > > > 
> > > > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > > > 
> > > > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > > > hardware blocks used to implement something, but the programming model,
> > > > > > > i.e. the register interface exposed to software.
> > > > > > > 
> > > > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > > > duplicated.
> > > > > > > > 
> > > > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > > > simple. However, if you find yourself in the situation where basically
> > > > > > > every actual register access in the driver ends up being in a "if (some
> > > > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > > > separate driver, as the programming interface is just different.
> > > > > > 
> > > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > > but different enough where each SoC has it's own set of tables and
> > > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > > that handle their specific variations.
> > > > > 
> > > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > > the overlap is not really in any real HW specific parts, but the common
> > > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > > driver.
> > > > 
> > > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > > > that, there is some 400 LoC which are specific to old LCDIF and this 
> > > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > > > shared boilerplate that would be duplicated .
> > > 
> > > That is probably ignoring the fact that the 8MP LCDIF does not support
> > > any overlays, so it could use the drm_simple_display_pipe
> > > infrastructure to reduce the needed boilerplate.
> > 
> > The drm_simple_display_pipe infrastructure is probably too simple for
> > i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
> > embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
> > outputs respectively. To use that infrastructure means there would be
> > three dri cards in all. However, the three LCDIF instances can be
> > wrapped by the one drm device, which is not the boilerplate code in the
> > current mxsfb driver may handle.
> 
> While that may make things a little simpler for userspace, I'm not sure
> if this is the right thing to do. It complicates the driver a lot,
> especially if you want to get things like independent power management,
> etc. right. It also creates a fake view for userspace, where is looks
> like there might be some shared resources between the different display
> paths, while in reality they are fully independent.

Trade-off will be made between one drm device and three. My first
impression of using the drm_simple_display_pipe infrastructure is that
it's too simple and less flexible/scalable, because SoC designer will
be likely to add muxes between CRTCs and encoders/bridges or overlay
plane(s) in next generations of SoCs(SW developers don't seem have good
reasons to suggest not to do that).  Another concern is that whether
the userspace may use the three drm devices well or not. 

A few more points:
1) With one drm device, userspace may use drm lease APIs to control
those independant pipes with drm masters(not sure about the userspace
maturity).
2) Code to gather all LCDIFs as one drm device has chance to be created
as helpers once there are similar use cases in other drivers(maybe,
there is/are already).
3) Power management doesn't seem to be a problem, since each LCDIF has
it's own struct device which can be used to do runtime PM at some
drm_crtc_helper_funcs callbacks.
4) Regarding the fake view of shared resources, atomic check can handle
that, so it doesn't seem to be a big problem, either.

> 
> While we do something similar on the GPU side and collect all GPU cores
> under a single DRM device, I'm not fully convinced that this was a good
> decision. It now comes back to bite us when the SoC topologies get a
> little more interesting and e.g. devices are behind different IOMMU
> streams.

Right, SoC topologies may change, like the aforementioned muxes.
Generally speaking, I think one drm device is more flexible and
scalable than three.

Regards,
Liu Ying

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Wed, 2022-03-02 at 12:57 +0100, Lucas Stach wrote:
> Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
> > On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> > > Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > > > On 3/1/22 14:18, Lucas Stach wrote:
> > > > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > [...]
> > > > > > > > 
> > > > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > > > and 'LCDIF'.
> > > > > > > > > 
> > > > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > > > framework today.
> > > > > > > > 
> > > > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > > > better and discarded the separate driver idea.
> > > > > > > > 
> > > > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > > > 
> > > > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > > > 
> > > > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > > > hardware blocks used to implement something, but the programming model,
> > > > > > > i.e. the register interface exposed to software.
> > > > > > > 
> > > > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > > > duplicated.
> > > > > > > > 
> > > > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > > > simple. However, if you find yourself in the situation where basically
> > > > > > > every actual register access in the driver ends up being in a "if (some
> > > > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > > > separate driver, as the programming interface is just different.
> > > > > > 
> > > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > > but different enough where each SoC has it's own set of tables and
> > > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > > that handle their specific variations.
> > > > > 
> > > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > > the overlap is not really in any real HW specific parts, but the common
> > > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > > driver.
> > > > 
> > > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > > > that, there is some 400 LoC which are specific to old LCDIF and this 
> > > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > > > shared boilerplate that would be duplicated .
> > > 
> > > That is probably ignoring the fact that the 8MP LCDIF does not support
> > > any overlays, so it could use the drm_simple_display_pipe
> > > infrastructure to reduce the needed boilerplate.
> > 
> > The drm_simple_display_pipe infrastructure is probably too simple for
> > i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
> > embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
> > outputs respectively. To use that infrastructure means there would be
> > three dri cards in all. However, the three LCDIF instances can be
> > wrapped by the one drm device, which is not the boilerplate code in the
> > current mxsfb driver may handle.
> 
> While that may make things a little simpler for userspace, I'm not sure
> if this is the right thing to do. It complicates the driver a lot,
> especially if you want to get things like independent power management,
> etc. right. It also creates a fake view for userspace, where is looks
> like there might be some shared resources between the different display
> paths, while in reality they are fully independent.

Trade-off will be made between one drm device and three. My first
impression of using the drm_simple_display_pipe infrastructure is that
it's too simple and less flexible/scalable, because SoC designer will
be likely to add muxes between CRTCs and encoders/bridges or overlay
plane(s) in next generations of SoCs(SW developers don't seem have good
reasons to suggest not to do that).  Another concern is that whether
the userspace may use the three drm devices well or not. 

A few more points:
1) With one drm device, userspace may use drm lease APIs to control
those independant pipes with drm masters(not sure about the userspace
maturity).
2) Code to gather all LCDIFs as one drm device has chance to be created
as helpers once there are similar use cases in other drivers(maybe,
there is/are already).
3) Power management doesn't seem to be a problem, since each LCDIF has
it's own struct device which can be used to do runtime PM at some
drm_crtc_helper_funcs callbacks.
4) Regarding the fake view of shared resources, atomic check can handle
that, so it doesn't seem to be a big problem, either.

> 
> While we do something similar on the GPU side and collect all GPU cores
> under a single DRM device, I'm not fully convinced that this was a good
> decision. It now comes back to bite us when the SoC topologies get a
> little more interesting and e.g. devices are behind different IOMMU
> streams.

Right, SoC topologies may change, like the aforementioned muxes.
Generally speaking, I think one drm device is more flexible and
scalable than three.

Regards,
Liu Ying

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/3/22 03:54, Liu Ying wrote:
> On Wed, 2022-03-02 at 12:57 +0100, Lucas Stach wrote:
>> Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
>>> On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
>>>> Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
>>>>> On 3/1/22 14:18, Lucas Stach wrote:
>>>>>> Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
>>>>>>> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>>>>>>>> Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
>>>>>>>>> On 3/1/22 11:04, Lucas Stach wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>>> Given the two totally different IPs, I don't see bugs of IP control
>>>>>>>>>>> logics should be fixed for both drivers. Naturally, the two would
>>>>>>>>>>> diverge due to different HWs. Looking at Patch 9/9, it basically
>>>>>>>>>>> squashes code to control LCDIFv3 into the mxsfb drm driver with
>>>>>>>>>>> 'if/else' checks(barely no common control code), which is hard to
>>>>>>>>>>> maintain and not able to achieve good scalability for both 'LCDIFv3'
>>>>>>>>>>> and 'LCDIF'.
>>>>>>>>>>
>>>>>>>>>> I tend to agree with Liu here. Writing a DRM driver isn't that much
>>>>>>>>>> boilerplate anymore with all the helpers we have available in the
>>>>>>>>>> framework today.
>>>>>>>>>
>>>>>>>>> I did write a separate driver for this IP before I spent time merging
>>>>>>>>> them into single driver, that's when I realized a single driver is much
>>>>>>>>> better and discarded the separate driver idea.
>>>>>>>>>
>>>>>>>>>> The IP is so different from the currently supported LCDIF controllers
>>>>>>>>>> that I think trying to support this one in the existing driver actually
>>>>>>>>>> increases the chances to break something when modifying the driver in
>>>>>>>>>> the future. Not everyone is able to test all LCDIF versions. My vote is
>>>>>>>>>> on having a separate driver for the i.MX8MP LCDIF.
>>>>>>>>>
>>>>>>>>> If you look at both controllers, it is clear it is still the LCDIF
>>>>>>>>> behind, even the CSC that is bolted on would suggest that.
>>>>>>>>
>>>>>>>> Yes, but from a driver PoV what you care about is not really the
>>>>>>>> hardware blocks used to implement something, but the programming model,
>>>>>>>> i.e. the register interface exposed to software.
>>>>>>>>
>>>>>>>>> I am also not happy when I look at the amount of duplication a separate
>>>>>>>>> driver would create, it will be some 50% of the code that would be just
>>>>>>>>> duplicated.
>>>>>>>>>
>>>>>>>> Yea, the duplicated code is still significant, as the HW itself is so
>>>>>>>> simple. However, if you find yourself in the situation where basically
>>>>>>>> every actual register access in the driver ends up being in a "if (some
>>>>>>>> HW rev) ... " clause, i still think it would be better to have a
>>>>>>>> separate driver, as the programming interface is just different.
>>>>>>>
>>>>>>> I tend to agree with Marek on this one.  We have an instance where the
>>>>>>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>>>>>>> but different enough where each SoC has it's own set of tables and
>>>>>>> some checks.   Lucas created the framework, and others adapted it for
>>>>>>> various SoC's.  If there really is nearly 50% common code for the
>>>>>>> LCDIF, why not either leave the driver as one or split the common code
>>>>>>> into its own driver like lcdif-common and then have smaller drivers
>>>>>>> that handle their specific variations.
>>>>>>
>>>>>> I don't know exactly how the standalone driver looks like, but I guess
>>>>>> the overlap is not really in any real HW specific parts, but the common
>>>>>> DRM boilerplate, so there isn't much point in creating a common lcdif
>>>>>> driver.
>>>>>
>>>>> The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
>>>>> that, there is some 400 LoC which are specific to old LCDIF and this
>>>>> patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
>>>>> shared boilerplate that would be duplicated .
>>>>
>>>> That is probably ignoring the fact that the 8MP LCDIF does not support
>>>> any overlays, so it could use the drm_simple_display_pipe
>>>> infrastructure to reduce the needed boilerplate.
>>>
>>> The drm_simple_display_pipe infrastructure is probably too simple for
>>> i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
>>> embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
>>> outputs respectively. To use that infrastructure means there would be
>>> three dri cards in all. However, the three LCDIF instances can be
>>> wrapped by the one drm device, which is not the boilerplate code in the
>>> current mxsfb driver may handle.
>>
>> While that may make things a little simpler for userspace, I'm not sure
>> if this is the right thing to do. It complicates the driver a lot,
>> especially if you want to get things like independent power management,
>> etc. right. It also creates a fake view for userspace, where is looks
>> like there might be some shared resources between the different display
>> paths, while in reality they are fully independent.
> 
> Trade-off will be made between one drm device and three. My first
> impression of using the drm_simple_display_pipe infrastructure is that
> it's too simple and less flexible/scalable, because SoC designer will
> be likely to add muxes between CRTCs and encoders/bridges or overlay
> plane(s) in next generations of SoCs(SW developers don't seem have good
> reasons to suggest not to do that).  Another concern is that whether
> the userspace may use the three drm devices well or not.
> 
> A few more points:
> 1) With one drm device, userspace may use drm lease APIs to control
> those independant pipes with drm masters(not sure about the userspace
> maturity).
> 2) Code to gather all LCDIFs as one drm device has chance to be created
> as helpers once there are similar use cases in other drivers(maybe,
> there is/are already).
> 3) Power management doesn't seem to be a problem, since each LCDIF has
> it's own struct device which can be used to do runtime PM at some
> drm_crtc_helper_funcs callbacks.
> 4) Regarding the fake view of shared resources, atomic check can handle
> that, so it doesn't seem to be a big problem, either.
> 
>>
>> While we do something similar on the GPU side and collect all GPU cores
>> under a single DRM device, I'm not fully convinced that this was a good
>> decision. It now comes back to bite us when the SoC topologies get a
>> little more interesting and e.g. devices are behind different IOMMU
>> streams.
> 
> Right, SoC topologies may change, like the aforementioned muxes.
> Generally speaking, I think one drm device is more flexible and
> scalable than three.

I agree with Lucas on one driver instance - one IP instance. Each IP 
instance is separate, so it should have separate driver instance bound 
to it.

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/3/22 03:54, Liu Ying wrote:
> On Wed, 2022-03-02 at 12:57 +0100, Lucas Stach wrote:
>> Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
>>> On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
>>>> Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
>>>>> On 3/1/22 14:18, Lucas Stach wrote:
>>>>>> Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
>>>>>>> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>>>>>>>> Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
>>>>>>>>> On 3/1/22 11:04, Lucas Stach wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>>> Given the two totally different IPs, I don't see bugs of IP control
>>>>>>>>>>> logics should be fixed for both drivers. Naturally, the two would
>>>>>>>>>>> diverge due to different HWs. Looking at Patch 9/9, it basically
>>>>>>>>>>> squashes code to control LCDIFv3 into the mxsfb drm driver with
>>>>>>>>>>> 'if/else' checks(barely no common control code), which is hard to
>>>>>>>>>>> maintain and not able to achieve good scalability for both 'LCDIFv3'
>>>>>>>>>>> and 'LCDIF'.
>>>>>>>>>>
>>>>>>>>>> I tend to agree with Liu here. Writing a DRM driver isn't that much
>>>>>>>>>> boilerplate anymore with all the helpers we have available in the
>>>>>>>>>> framework today.
>>>>>>>>>
>>>>>>>>> I did write a separate driver for this IP before I spent time merging
>>>>>>>>> them into single driver, that's when I realized a single driver is much
>>>>>>>>> better and discarded the separate driver idea.
>>>>>>>>>
>>>>>>>>>> The IP is so different from the currently supported LCDIF controllers
>>>>>>>>>> that I think trying to support this one in the existing driver actually
>>>>>>>>>> increases the chances to break something when modifying the driver in
>>>>>>>>>> the future. Not everyone is able to test all LCDIF versions. My vote is
>>>>>>>>>> on having a separate driver for the i.MX8MP LCDIF.
>>>>>>>>>
>>>>>>>>> If you look at both controllers, it is clear it is still the LCDIF
>>>>>>>>> behind, even the CSC that is bolted on would suggest that.
>>>>>>>>
>>>>>>>> Yes, but from a driver PoV what you care about is not really the
>>>>>>>> hardware blocks used to implement something, but the programming model,
>>>>>>>> i.e. the register interface exposed to software.
>>>>>>>>
>>>>>>>>> I am also not happy when I look at the amount of duplication a separate
>>>>>>>>> driver would create, it will be some 50% of the code that would be just
>>>>>>>>> duplicated.
>>>>>>>>>
>>>>>>>> Yea, the duplicated code is still significant, as the HW itself is so
>>>>>>>> simple. However, if you find yourself in the situation where basically
>>>>>>>> every actual register access in the driver ends up being in a "if (some
>>>>>>>> HW rev) ... " clause, i still think it would be better to have a
>>>>>>>> separate driver, as the programming interface is just different.
>>>>>>>
>>>>>>> I tend to agree with Marek on this one.  We have an instance where the
>>>>>>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>>>>>>> but different enough where each SoC has it's own set of tables and
>>>>>>> some checks.   Lucas created the framework, and others adapted it for
>>>>>>> various SoC's.  If there really is nearly 50% common code for the
>>>>>>> LCDIF, why not either leave the driver as one or split the common code
>>>>>>> into its own driver like lcdif-common and then have smaller drivers
>>>>>>> that handle their specific variations.
>>>>>>
>>>>>> I don't know exactly how the standalone driver looks like, but I guess
>>>>>> the overlap is not really in any real HW specific parts, but the common
>>>>>> DRM boilerplate, so there isn't much point in creating a common lcdif
>>>>>> driver.
>>>>>
>>>>> The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
>>>>> that, there is some 400 LoC which are specific to old LCDIF and this
>>>>> patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
>>>>> shared boilerplate that would be duplicated .
>>>>
>>>> That is probably ignoring the fact that the 8MP LCDIF does not support
>>>> any overlays, so it could use the drm_simple_display_pipe
>>>> infrastructure to reduce the needed boilerplate.
>>>
>>> The drm_simple_display_pipe infrastructure is probably too simple for
>>> i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
>>> embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
>>> outputs respectively. To use that infrastructure means there would be
>>> three dri cards in all. However, the three LCDIF instances can be
>>> wrapped by the one drm device, which is not the boilerplate code in the
>>> current mxsfb driver may handle.
>>
>> While that may make things a little simpler for userspace, I'm not sure
>> if this is the right thing to do. It complicates the driver a lot,
>> especially if you want to get things like independent power management,
>> etc. right. It also creates a fake view for userspace, where is looks
>> like there might be some shared resources between the different display
>> paths, while in reality they are fully independent.
> 
> Trade-off will be made between one drm device and three. My first
> impression of using the drm_simple_display_pipe infrastructure is that
> it's too simple and less flexible/scalable, because SoC designer will
> be likely to add muxes between CRTCs and encoders/bridges or overlay
> plane(s) in next generations of SoCs(SW developers don't seem have good
> reasons to suggest not to do that).  Another concern is that whether
> the userspace may use the three drm devices well or not.
> 
> A few more points:
> 1) With one drm device, userspace may use drm lease APIs to control
> those independant pipes with drm masters(not sure about the userspace
> maturity).
> 2) Code to gather all LCDIFs as one drm device has chance to be created
> as helpers once there are similar use cases in other drivers(maybe,
> there is/are already).
> 3) Power management doesn't seem to be a problem, since each LCDIF has
> it's own struct device which can be used to do runtime PM at some
> drm_crtc_helper_funcs callbacks.
> 4) Regarding the fake view of shared resources, atomic check can handle
> that, so it doesn't seem to be a big problem, either.
> 
>>
>> While we do something similar on the GPU side and collect all GPU cores
>> under a single DRM device, I'm not fully convinced that this was a good
>> decision. It now comes back to bite us when the SoC topologies get a
>> little more interesting and e.g. devices are behind different IOMMU
>> streams.
> 
> Right, SoC topologies may change, like the aforementioned muxes.
> Generally speaking, I think one drm device is more flexible and
> scalable than three.

I agree with Lucas on one driver instance - one IP instance. Each IP 
instance is separate, so it should have separate driver instance bound 
to it.

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Donnerstag, dem 03.03.2022 um 10:54 +0800 schrieb Liu Ying:
> On Wed, 2022-03-02 at 12:57 +0100, Lucas Stach wrote:
> > Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
> > > On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> > > > Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > > > > On 3/1/22 14:18, Lucas Stach wrote:
> > > > > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > > > > 
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > [...]
> > > > > > > > > 
> > > > > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > > > > and 'LCDIF'.
> > > > > > > > > > 
> > > > > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > > > > framework today.
> > > > > > > > > 
> > > > > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > > > > better and discarded the separate driver idea.
> > > > > > > > > 
> > > > > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > > > > 
> > > > > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > > > > 
> > > > > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > > > > hardware blocks used to implement something, but the programming model,
> > > > > > > > i.e. the register interface exposed to software.
> > > > > > > > 
> > > > > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > > > > duplicated.
> > > > > > > > > 
> > > > > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > > > > simple. However, if you find yourself in the situation where basically
> > > > > > > > every actual register access in the driver ends up being in a "if (some
> > > > > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > > > > separate driver, as the programming interface is just different.
> > > > > > > 
> > > > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > > > but different enough where each SoC has it's own set of tables and
> > > > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > > > that handle their specific variations.
> > > > > > 
> > > > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > > > the overlap is not really in any real HW specific parts, but the common
> > > > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > > > driver.
> > > > > 
> > > > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > > > > that, there is some 400 LoC which are specific to old LCDIF and this 
> > > > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > > > > shared boilerplate that would be duplicated .
> > > > 
> > > > That is probably ignoring the fact that the 8MP LCDIF does not support
> > > > any overlays, so it could use the drm_simple_display_pipe
> > > > infrastructure to reduce the needed boilerplate.
> > > 
> > > The drm_simple_display_pipe infrastructure is probably too simple for
> > > i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
> > > embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
> > > outputs respectively. To use that infrastructure means there would be
> > > three dri cards in all. However, the three LCDIF instances can be
> > > wrapped by the one drm device, which is not the boilerplate code in the
> > > current mxsfb driver may handle.
> > 
> > While that may make things a little simpler for userspace, I'm not sure
> > if this is the right thing to do. It complicates the driver a lot,
> > especially if you want to get things like independent power management,
> > etc. right. It also creates a fake view for userspace, where is looks
> > like there might be some shared resources between the different display
> > paths, while in reality they are fully independent.
> 
> Trade-off will be made between one drm device and three. My first
> impression of using the drm_simple_display_pipe infrastructure is that
> it's too simple and less flexible/scalable, because SoC designer will
> be likely to add muxes between CRTCs and encoders/bridges or overlay
> plane(s) in next generations of SoCs(SW developers don't seem have good
> reasons to suggest not to do that).  Another concern is that whether
> the userspace may use the three drm devices well or not. 
> 
> A few more points:
> 1) With one drm device, userspace may use drm lease APIs to control
> those independant pipes with drm masters(not sure about the userspace
> maturity).

I'm not sure why you are so keen on using DRM leases in your
downstream. Actually this argument is a argument in _favor_ of
independent DRM devices: you don't need to deal with leases when every
userspace component can just exclusively use a DRM device.

> 2) Code to gather all LCDIFs as one drm device has chance to be created
> as helpers once there are similar use cases in other drivers(maybe,
> there is/are already).

We already gather the GPU cores in etnaviv and as I said this decision
proves to add complications in the long run. For example prime import
with the DRM helpers is currently bound to the DRM device, so if your
actual HW devices backing the DRM device have differing DMA constraints
things get really messy.

> 3) Power management doesn't seem to be a problem, since each LCDIF has
> it's own struct device which can be used to do runtime PM at some
> drm_crtc_helper_funcs callbacks.

It's not a big problem, but it adds complexity that wouldn't be there
if you have a simple 1 IP instance <-> 1 DRM device mapping.

> 4) Regarding the fake view of shared resources, atomic check can handle
> that, so it doesn't seem to be a big problem, either.

Sure, there isn't even anything to handle, as the pipes are truly
independent.

> 
> > 
> > While we do something similar on the GPU side and collect all GPU cores
> > under a single DRM device, I'm not fully convinced that this was a good
> > decision. It now comes back to bite us when the SoC topologies get a
> > little more interesting and e.g. devices are behind different IOMMU
> > streams.
> 
> Right, SoC topologies may change, like the aforementioned muxes.
> Generally speaking, I think one drm device is more flexible and
> scalable than three.
> 
> Regards,
> Liu Ying
> 
> 

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Donnerstag, dem 03.03.2022 um 10:54 +0800 schrieb Liu Ying:
> On Wed, 2022-03-02 at 12:57 +0100, Lucas Stach wrote:
> > Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
> > > On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> > > > Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > > > > On 3/1/22 14:18, Lucas Stach wrote:
> > > > > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > > > > 
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > [...]
> > > > > > > > > 
> > > > > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > > > > and 'LCDIF'.
> > > > > > > > > > 
> > > > > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > > > > framework today.
> > > > > > > > > 
> > > > > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > > > > better and discarded the separate driver idea.
> > > > > > > > > 
> > > > > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > > > > 
> > > > > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > > > > 
> > > > > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > > > > hardware blocks used to implement something, but the programming model,
> > > > > > > > i.e. the register interface exposed to software.
> > > > > > > > 
> > > > > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > > > > duplicated.
> > > > > > > > > 
> > > > > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > > > > simple. However, if you find yourself in the situation where basically
> > > > > > > > every actual register access in the driver ends up being in a "if (some
> > > > > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > > > > separate driver, as the programming interface is just different.
> > > > > > > 
> > > > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > > > but different enough where each SoC has it's own set of tables and
> > > > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > > > that handle their specific variations.
> > > > > > 
> > > > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > > > the overlap is not really in any real HW specific parts, but the common
> > > > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > > > driver.
> > > > > 
> > > > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > > > > that, there is some 400 LoC which are specific to old LCDIF and this 
> > > > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > > > > shared boilerplate that would be duplicated .
> > > > 
> > > > That is probably ignoring the fact that the 8MP LCDIF does not support
> > > > any overlays, so it could use the drm_simple_display_pipe
> > > > infrastructure to reduce the needed boilerplate.
> > > 
> > > The drm_simple_display_pipe infrastructure is probably too simple for
> > > i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
> > > embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
> > > outputs respectively. To use that infrastructure means there would be
> > > three dri cards in all. However, the three LCDIF instances can be
> > > wrapped by the one drm device, which is not the boilerplate code in the
> > > current mxsfb driver may handle.
> > 
> > While that may make things a little simpler for userspace, I'm not sure
> > if this is the right thing to do. It complicates the driver a lot,
> > especially if you want to get things like independent power management,
> > etc. right. It also creates a fake view for userspace, where is looks
> > like there might be some shared resources between the different display
> > paths, while in reality they are fully independent.
> 
> Trade-off will be made between one drm device and three. My first
> impression of using the drm_simple_display_pipe infrastructure is that
> it's too simple and less flexible/scalable, because SoC designer will
> be likely to add muxes between CRTCs and encoders/bridges or overlay
> plane(s) in next generations of SoCs(SW developers don't seem have good
> reasons to suggest not to do that).  Another concern is that whether
> the userspace may use the three drm devices well or not. 
> 
> A few more points:
> 1) With one drm device, userspace may use drm lease APIs to control
> those independant pipes with drm masters(not sure about the userspace
> maturity).

I'm not sure why you are so keen on using DRM leases in your
downstream. Actually this argument is a argument in _favor_ of
independent DRM devices: you don't need to deal with leases when every
userspace component can just exclusively use a DRM device.

> 2) Code to gather all LCDIFs as one drm device has chance to be created
> as helpers once there are similar use cases in other drivers(maybe,
> there is/are already).

We already gather the GPU cores in etnaviv and as I said this decision
proves to add complications in the long run. For example prime import
with the DRM helpers is currently bound to the DRM device, so if your
actual HW devices backing the DRM device have differing DMA constraints
things get really messy.

> 3) Power management doesn't seem to be a problem, since each LCDIF has
> it's own struct device which can be used to do runtime PM at some
> drm_crtc_helper_funcs callbacks.

It's not a big problem, but it adds complexity that wouldn't be there
if you have a simple 1 IP instance <-> 1 DRM device mapping.

> 4) Regarding the fake view of shared resources, atomic check can handle
> that, so it doesn't seem to be a big problem, either.

Sure, there isn't even anything to handle, as the pipes are truly
independent.

> 
> > 
> > While we do something similar on the GPU side and collect all GPU cores
> > under a single DRM device, I'm not fully convinced that this was a good
> > decision. It now comes back to bite us when the SoC topologies get a
> > little more interesting and e.g. devices are behind different IOMMU
> > streams.
> 
> Right, SoC topologies may change, like the aforementioned muxes.
> Generally speaking, I think one drm device is more flexible and
> scalable than three.
> 
> Regards,
> Liu Ying
> 
> 

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Thu, 2022-03-03 at 09:19 +0100, Lucas Stach wrote:
> Am Donnerstag, dem 03.03.2022 um 10:54 +0800 schrieb Liu Ying:
> > On Wed, 2022-03-02 at 12:57 +0100, Lucas Stach wrote:
> > > Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
> > > > On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> > > > > Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > > > > > On 3/1/22 14:18, Lucas Stach wrote:
> > > > > > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > > > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > > > > > 
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > [...]
> > > > > > > > > > 
> > > > > > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > > > > > and 'LCDIF'.
> > > > > > > > > > > 
> > > > > > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > > > > > framework today.
> > > > > > > > > > 
> > > > > > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > > > > > better and discarded the separate driver idea.
> > > > > > > > > > 
> > > > > > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > > > > > 
> > > > > > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > > > > > 
> > > > > > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > > > > > hardware blocks used to implement something, but the programming model,
> > > > > > > > > i.e. the register interface exposed to software.
> > > > > > > > > 
> > > > > > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > > > > > duplicated.
> > > > > > > > > > 
> > > > > > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > > > > > simple. However, if you find yourself in the situation where basically
> > > > > > > > > every actual register access in the driver ends up being in a "if (some
> > > > > > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > > > > > separate driver, as the programming interface is just different.
> > > > > > > > 
> > > > > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > > > > but different enough where each SoC has it's own set of tables and
> > > > > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > > > > that handle their specific variations.
> > > > > > > 
> > > > > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > > > > the overlap is not really in any real HW specific parts, but the common
> > > > > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > > > > driver.
> > > > > > 
> > > > > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > > > > > that, there is some 400 LoC which are specific to old LCDIF and this 
> > > > > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > > > > > shared boilerplate that would be duplicated .
> > > > > 
> > > > > That is probably ignoring the fact that the 8MP LCDIF does not support
> > > > > any overlays, so it could use the drm_simple_display_pipe
> > > > > infrastructure to reduce the needed boilerplate.
> > > > 
> > > > The drm_simple_display_pipe infrastructure is probably too simple for
> > > > i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
> > > > embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
> > > > outputs respectively. To use that infrastructure means there would be
> > > > three dri cards in all. However, the three LCDIF instances can be
> > > > wrapped by the one drm device, which is not the boilerplate code in the
> > > > current mxsfb driver may handle.
> > > 
> > > While that may make things a little simpler for userspace, I'm not sure
> > > if this is the right thing to do. It complicates the driver a lot,
> > > especially if you want to get things like independent power management,
> > > etc. right. It also creates a fake view for userspace, where is looks
> > > like there might be some shared resources between the different display
> > > paths, while in reality they are fully independent.
> > 
> > Trade-off will be made between one drm device and three. My first
> > impression of using the drm_simple_display_pipe infrastructure is that
> > it's too simple and less flexible/scalable, because SoC designer will
> > be likely to add muxes between CRTCs and encoders/bridges or overlay
> > plane(s) in next generations of SoCs(SW developers don't seem have good
> > reasons to suggest not to do that).  Another concern is that whether
> > the userspace may use the three drm devices well or not. 
> > 
> > A few more points:
> > 1) With one drm device, userspace may use drm lease APIs to control
> > those independant pipes with drm masters(not sure about the userspace
> > maturity).
> 
> I'm not sure why you are so keen on using DRM leases in your
> downstream. Actually this argument is a argument in _favor_ of
> independent DRM devices: you don't need to deal with leases when every
> userspace component can just exclusively use a DRM device.

Userspace may choose to use drm lease APIs to talk to kernel. It's a
feature kinda nice to have. Imagine that an user has already written an
application which uses the APIs. 

> 
> > 2) Code to gather all LCDIFs as one drm device has chance to be created
> > as helpers once there are similar use cases in other drivers(maybe,
> > there is/are already).
> 
> We already gather the GPU cores in etnaviv and as I said this decision
> proves to add complications in the long run. For example prime import
> with the DRM helpers is currently bound to the DRM device, so if your
> actual HW devices backing the DRM device have differing DMA constraints
> things get really messy.

LCDIFs in one SoC are very likely symmetric from the embodying system
PoV. So, maybe, that's not a big problem?

> 
> > 3) Power management doesn't seem to be a problem, since each LCDIF has
> > it's own struct device which can be used to do runtime PM at some
> > drm_crtc_helper_funcs callbacks.
> 
> It's not a big problem, but it adds complexity that wouldn't be there
> if you have a simple 1 IP instance <-> 1 DRM device mapping.

Just a little bit more complexity, I think. I tend to take that to
achieve better flexibilty and scalabilty(to support potential muxes).
It's a trade-off as I mentioned - I tend to choose one drm device, but
_no_ strong opinion on three.  Once the muxes become real, it looks
like we have to use the 'one drm device' solution.

> 
> > 4) Regarding the fake view of shared resources, atomic check can handle
> > that, so it doesn't seem to be a big problem, either.
> 
> Sure, there isn't even anything to handle, as the pipes are truly
> independent.
> 

Yes, but it's still a trade-off.

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Liu Ying]

On Thu, 2022-03-03 at 09:19 +0100, Lucas Stach wrote:
> Am Donnerstag, dem 03.03.2022 um 10:54 +0800 schrieb Liu Ying:
> > On Wed, 2022-03-02 at 12:57 +0100, Lucas Stach wrote:
> > > Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
> > > > On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
> > > > > Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
> > > > > > On 3/1/22 14:18, Lucas Stach wrote:
> > > > > > > Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
> > > > > > > > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
> > > > > > > > > > On 3/1/22 11:04, Lucas Stach wrote:
> > > > > > > > > > 
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > [...]
> > > > > > > > > > 
> > > > > > > > > > > > Given the two totally different IPs, I don't see bugs of IP control
> > > > > > > > > > > > logics should be fixed for both drivers. Naturally, the two would
> > > > > > > > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically
> > > > > > > > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with
> > > > > > > > > > > > 'if/else' checks(barely no common control code), which is hard to
> > > > > > > > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3'
> > > > > > > > > > > > and 'LCDIF'.
> > > > > > > > > > > 
> > > > > > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much
> > > > > > > > > > > boilerplate anymore with all the helpers we have available in the
> > > > > > > > > > > framework today.
> > > > > > > > > > 
> > > > > > > > > > I did write a separate driver for this IP before I spent time merging
> > > > > > > > > > them into single driver, that's when I realized a single driver is much
> > > > > > > > > > better and discarded the separate driver idea.
> > > > > > > > > > 
> > > > > > > > > > > The IP is so different from the currently supported LCDIF controllers
> > > > > > > > > > > that I think trying to support this one in the existing driver actually
> > > > > > > > > > > increases the chances to break something when modifying the driver in
> > > > > > > > > > > the future. Not everyone is able to test all LCDIF versions. My vote is
> > > > > > > > > > > on having a separate driver for the i.MX8MP LCDIF.
> > > > > > > > > > 
> > > > > > > > > > If you look at both controllers, it is clear it is still the LCDIF
> > > > > > > > > > behind, even the CSC that is bolted on would suggest that.
> > > > > > > > > 
> > > > > > > > > Yes, but from a driver PoV what you care about is not really the
> > > > > > > > > hardware blocks used to implement something, but the programming model,
> > > > > > > > > i.e. the register interface exposed to software.
> > > > > > > > > 
> > > > > > > > > > I am also not happy when I look at the amount of duplication a separate
> > > > > > > > > > driver would create, it will be some 50% of the code that would be just
> > > > > > > > > > duplicated.
> > > > > > > > > > 
> > > > > > > > > Yea, the duplicated code is still significant, as the HW itself is so
> > > > > > > > > simple. However, if you find yourself in the situation where basically
> > > > > > > > > every actual register access in the driver ends up being in a "if (some
> > > > > > > > > HW rev) ... " clause, i still think it would be better to have a
> > > > > > > > > separate driver, as the programming interface is just different.
> > > > > > > > 
> > > > > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > > > > but different enough where each SoC has it's own set of tables and
> > > > > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > > > > that handle their specific variations.
> > > > > > > 
> > > > > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > > > > the overlap is not really in any real HW specific parts, but the common
> > > > > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > > > > driver.
> > > > > > 
> > > > > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of 
> > > > > > that, there is some 400 LoC which are specific to old LCDIF and this 
> > > > > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of 
> > > > > > shared boilerplate that would be duplicated .
> > > > > 
> > > > > That is probably ignoring the fact that the 8MP LCDIF does not support
> > > > > any overlays, so it could use the drm_simple_display_pipe
> > > > > infrastructure to reduce the needed boilerplate.
> > > > 
> > > > The drm_simple_display_pipe infrastructure is probably too simple for
> > > > i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
> > > > embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
> > > > outputs respectively. To use that infrastructure means there would be
> > > > three dri cards in all. However, the three LCDIF instances can be
> > > > wrapped by the one drm device, which is not the boilerplate code in the
> > > > current mxsfb driver may handle.
> > > 
> > > While that may make things a little simpler for userspace, I'm not sure
> > > if this is the right thing to do. It complicates the driver a lot,
> > > especially if you want to get things like independent power management,
> > > etc. right. It also creates a fake view for userspace, where is looks
> > > like there might be some shared resources between the different display
> > > paths, while in reality they are fully independent.
> > 
> > Trade-off will be made between one drm device and three. My first
> > impression of using the drm_simple_display_pipe infrastructure is that
> > it's too simple and less flexible/scalable, because SoC designer will
> > be likely to add muxes between CRTCs and encoders/bridges or overlay
> > plane(s) in next generations of SoCs(SW developers don't seem have good
> > reasons to suggest not to do that).  Another concern is that whether
> > the userspace may use the three drm devices well or not. 
> > 
> > A few more points:
> > 1) With one drm device, userspace may use drm lease APIs to control
> > those independant pipes with drm masters(not sure about the userspace
> > maturity).
> 
> I'm not sure why you are so keen on using DRM leases in your
> downstream. Actually this argument is a argument in _favor_ of
> independent DRM devices: you don't need to deal with leases when every
> userspace component can just exclusively use a DRM device.

Userspace may choose to use drm lease APIs to talk to kernel. It's a
feature kinda nice to have. Imagine that an user has already written an
application which uses the APIs. 

> 
> > 2) Code to gather all LCDIFs as one drm device has chance to be created
> > as helpers once there are similar use cases in other drivers(maybe,
> > there is/are already).
> 
> We already gather the GPU cores in etnaviv and as I said this decision
> proves to add complications in the long run. For example prime import
> with the DRM helpers is currently bound to the DRM device, so if your
> actual HW devices backing the DRM device have differing DMA constraints
> things get really messy.

LCDIFs in one SoC are very likely symmetric from the embodying system
PoV. So, maybe, that's not a big problem?

> 
> > 3) Power management doesn't seem to be a problem, since each LCDIF has
> > it's own struct device which can be used to do runtime PM at some
> > drm_crtc_helper_funcs callbacks.
> 
> It's not a big problem, but it adds complexity that wouldn't be there
> if you have a simple 1 IP instance <-> 1 DRM device mapping.

Just a little bit more complexity, I think. I tend to take that to
achieve better flexibilty and scalabilty(to support potential muxes).
It's a trade-off as I mentioned - I tend to choose one drm device, but
_no_ strong opinion on three.  Once the muxes become real, it looks
like we have to use the 'one drm device' solution.

> 
> > 4) Regarding the fake view of shared resources, atomic check can handle
> > that, so it doesn't seem to be a big problem, either.
> 
> Sure, there isn't even anything to handle, as the pipes are truly
> independent.
> 

Yes, but it's still a trade-off.

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/2/22 10:23, Lucas Stach wrote:

[...]

>>>> I tend to agree with Marek on this one.  We have an instance where the
>>>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>>>> but different enough where each SoC has it's own set of tables and
>>>> some checks.   Lucas created the framework, and others adapted it for
>>>> various SoC's.  If there really is nearly 50% common code for the
>>>> LCDIF, why not either leave the driver as one or split the common code
>>>> into its own driver like lcdif-common and then have smaller drivers
>>>> that handle their specific variations.
>>>
>>> I don't know exactly how the standalone driver looks like, but I guess
>>> the overlap is not really in any real HW specific parts, but the common
>>> DRM boilerplate, so there isn't much point in creating a common lcdif
>>> driver.
>>
>> The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
>> that, there is some 400 LoC which are specific to old LCDIF and this
>> patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
>> shared boilerplate that would be duplicated .
> 
> That is probably ignoring the fact that the 8MP LCDIF does not support
> any overlays, so it could use the drm_simple_display_pipe
> infrastructure to reduce the needed boilerplate.

It seems the IMXRT1070 LCDIF v2 (heh ...) does support overlays, so no, 
the mxsfb and hypothetical lcdif drivers would look really very similar.

>>> As you brought up the blk-ctrl as an example: I'm all for supporting
>>> slightly different hardware in the same driver, as long as the HW
>>> interface is close enough. But then I also opted for a separate 8MP
>>> blk-ctrl driver for those blk-ctrls that differ significantly from the
>>> others, as I think it would make the common driver unmaintainable
>>> trying to support all the different variants in one driver.
>>
>> But then you also need to maintain two sets of boilerplate, they
>> diverge, and that is not good.
> 
> I don't think that there is much chance for bugs going unfixed due to
> divergence in the boilerplate, especially if you use the simple pipe
> framework to handle most of that stuff for you, which gives you a lot
> of code sharing with other simple DRM drivers.

But I can not use the simple pipe because overlays, see imxrt1070 .

[...]

We can always split the drivers later if this becomes unmaintainable 
too, no ?

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/2/22 10:23, Lucas Stach wrote:

[...]

>>>> I tend to agree with Marek on this one.  We have an instance where the
>>>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>>>> but different enough where each SoC has it's own set of tables and
>>>> some checks.   Lucas created the framework, and others adapted it for
>>>> various SoC's.  If there really is nearly 50% common code for the
>>>> LCDIF, why not either leave the driver as one or split the common code
>>>> into its own driver like lcdif-common and then have smaller drivers
>>>> that handle their specific variations.
>>>
>>> I don't know exactly how the standalone driver looks like, but I guess
>>> the overlap is not really in any real HW specific parts, but the common
>>> DRM boilerplate, so there isn't much point in creating a common lcdif
>>> driver.
>>
>> The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
>> that, there is some 400 LoC which are specific to old LCDIF and this
>> patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
>> shared boilerplate that would be duplicated .
> 
> That is probably ignoring the fact that the 8MP LCDIF does not support
> any overlays, so it could use the drm_simple_display_pipe
> infrastructure to reduce the needed boilerplate.

It seems the IMXRT1070 LCDIF v2 (heh ...) does support overlays, so no, 
the mxsfb and hypothetical lcdif drivers would look really very similar.

>>> As you brought up the blk-ctrl as an example: I'm all for supporting
>>> slightly different hardware in the same driver, as long as the HW
>>> interface is close enough. But then I also opted for a separate 8MP
>>> blk-ctrl driver for those blk-ctrls that differ significantly from the
>>> others, as I think it would make the common driver unmaintainable
>>> trying to support all the different variants in one driver.
>>
>> But then you also need to maintain two sets of boilerplate, they
>> diverge, and that is not good.
> 
> I don't think that there is much chance for bugs going unfixed due to
> divergence in the boilerplate, especially if you use the simple pipe
> framework to handle most of that stuff for you, which gives you a lot
> of code sharing with other simple DRM drivers.

But I can not use the simple pipe because overlays, see imxrt1070 .

[...]

We can always split the drivers later if this becomes unmaintainable 
too, no ?

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Donnerstag, dem 03.03.2022 um 04:14 +0100 schrieb Marek Vasut:
> On 3/2/22 10:23, Lucas Stach wrote:
> 
> [...]
> 
> > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > but different enough where each SoC has it's own set of tables and
> > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > that handle their specific variations.
> > > > 
> > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > the overlap is not really in any real HW specific parts, but the common
> > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > driver.
> > > 
> > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
> > > that, there is some 400 LoC which are specific to old LCDIF and this
> > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
> > > shared boilerplate that would be duplicated .
> > 
> > That is probably ignoring the fact that the 8MP LCDIF does not support
> > any overlays, so it could use the drm_simple_display_pipe
> > infrastructure to reduce the needed boilerplate.
> 
> It seems the IMXRT1070 LCDIF v2 (heh ...) does support overlays, so no, 
> the mxsfb and hypothetical lcdif drivers would look really very similar.
> 
> > > > As you brought up the blk-ctrl as an example: I'm all for supporting
> > > > slightly different hardware in the same driver, as long as the HW
> > > > interface is close enough. But then I also opted for a separate 8MP
> > > > blk-ctrl driver for those blk-ctrls that differ significantly from the
> > > > others, as I think it would make the common driver unmaintainable
> > > > trying to support all the different variants in one driver.
> > > 
> > > But then you also need to maintain two sets of boilerplate, they
> > > diverge, and that is not good.
> > 
> > I don't think that there is much chance for bugs going unfixed due to
> > divergence in the boilerplate, especially if you use the simple pipe
> > framework to handle most of that stuff for you, which gives you a lot
> > of code sharing with other simple DRM drivers.
> 
> But I can not use the simple pipe because overlays, see imxrt1070 .
> 
> [...]
> 
> We can always split the drivers later if this becomes unmaintainable 
> too, no ?

Not if you want to keep the same userspace running. As userspace has
some ties to the DRM driver name, e.g. for finding the right GBM
implementation, splitting the driver later on would be a UABI break.

Regards,
Lucas

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

Am Donnerstag, dem 03.03.2022 um 04:14 +0100 schrieb Marek Vasut:
> On 3/2/22 10:23, Lucas Stach wrote:
> 
> [...]
> 
> > > > > I tend to agree with Marek on this one.  We have an instance where the
> > > > > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
> > > > > but different enough where each SoC has it's own set of tables and
> > > > > some checks.   Lucas created the framework, and others adapted it for
> > > > > various SoC's.  If there really is nearly 50% common code for the
> > > > > LCDIF, why not either leave the driver as one or split the common code
> > > > > into its own driver like lcdif-common and then have smaller drivers
> > > > > that handle their specific variations.
> > > > 
> > > > I don't know exactly how the standalone driver looks like, but I guess
> > > > the overlap is not really in any real HW specific parts, but the common
> > > > DRM boilerplate, so there isn't much point in creating a common lcdif
> > > > driver.
> > > 
> > > The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
> > > that, there is some 400 LoC which are specific to old LCDIF and this
> > > patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
> > > shared boilerplate that would be duplicated .
> > 
> > That is probably ignoring the fact that the 8MP LCDIF does not support
> > any overlays, so it could use the drm_simple_display_pipe
> > infrastructure to reduce the needed boilerplate.
> 
> It seems the IMXRT1070 LCDIF v2 (heh ...) does support overlays, so no, 
> the mxsfb and hypothetical lcdif drivers would look really very similar.
> 
> > > > As you brought up the blk-ctrl as an example: I'm all for supporting
> > > > slightly different hardware in the same driver, as long as the HW
> > > > interface is close enough. But then I also opted for a separate 8MP
> > > > blk-ctrl driver for those blk-ctrls that differ significantly from the
> > > > others, as I think it would make the common driver unmaintainable
> > > > trying to support all the different variants in one driver.
> > > 
> > > But then you also need to maintain two sets of boilerplate, they
> > > diverge, and that is not good.
> > 
> > I don't think that there is much chance for bugs going unfixed due to
> > divergence in the boilerplate, especially if you use the simple pipe
> > framework to handle most of that stuff for you, which gives you a lot
> > of code sharing with other simple DRM drivers.
> 
> But I can not use the simple pipe because overlays, see imxrt1070 .
> 
> [...]
> 
> We can always split the drivers later if this becomes unmaintainable 
> too, no ?

Not if you want to keep the same userspace running. As userspace has
some ties to the DRM driver name, e.g. for finding the right GBM
implementation, splitting the driver later on would be a UABI break.

Regards,
Lucas

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/3/22 09:21, Lucas Stach wrote:
> Am Donnerstag, dem 03.03.2022 um 04:14 +0100 schrieb Marek Vasut:
>> On 3/2/22 10:23, Lucas Stach wrote:
>>
>> [...]
>>
>>>>>> I tend to agree with Marek on this one.  We have an instance where the
>>>>>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>>>>>> but different enough where each SoC has it's own set of tables and
>>>>>> some checks.   Lucas created the framework, and others adapted it for
>>>>>> various SoC's.  If there really is nearly 50% common code for the
>>>>>> LCDIF, why not either leave the driver as one or split the common code
>>>>>> into its own driver like lcdif-common and then have smaller drivers
>>>>>> that handle their specific variations.
>>>>>
>>>>> I don't know exactly how the standalone driver looks like, but I guess
>>>>> the overlap is not really in any real HW specific parts, but the common
>>>>> DRM boilerplate, so there isn't much point in creating a common lcdif
>>>>> driver.
>>>>
>>>> The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
>>>> that, there is some 400 LoC which are specific to old LCDIF and this
>>>> patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
>>>> shared boilerplate that would be duplicated .
>>>
>>> That is probably ignoring the fact that the 8MP LCDIF does not support
>>> any overlays, so it could use the drm_simple_display_pipe
>>> infrastructure to reduce the needed boilerplate.
>>
>> It seems the IMXRT1070 LCDIF v2 (heh ...) does support overlays, so no,
>> the mxsfb and hypothetical lcdif drivers would look really very similar.
>>
>>>>> As you brought up the blk-ctrl as an example: I'm all for supporting
>>>>> slightly different hardware in the same driver, as long as the HW
>>>>> interface is close enough. But then I also opted for a separate 8MP
>>>>> blk-ctrl driver for those blk-ctrls that differ significantly from the
>>>>> others, as I think it would make the common driver unmaintainable
>>>>> trying to support all the different variants in one driver.
>>>>
>>>> But then you also need to maintain two sets of boilerplate, they
>>>> diverge, and that is not good.
>>>
>>> I don't think that there is much chance for bugs going unfixed due to
>>> divergence in the boilerplate, especially if you use the simple pipe
>>> framework to handle most of that stuff for you, which gives you a lot
>>> of code sharing with other simple DRM drivers.
>>
>> But I can not use the simple pipe because overlays, see imxrt1070 .
>>
>> [...]
>>
>> We can always split the drivers later if this becomes unmaintainable
>> too, no ?
> 
> Not if you want to keep the same userspace running. As userspace has
> some ties to the DRM driver name, e.g. for finding the right GBM
> implementation, splitting the driver later on would be a UABI break.

Hum, so what other options do we have left ? Duplicate 60% of the driver 
right away ?

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/3/22 09:21, Lucas Stach wrote:
> Am Donnerstag, dem 03.03.2022 um 04:14 +0100 schrieb Marek Vasut:
>> On 3/2/22 10:23, Lucas Stach wrote:
>>
>> [...]
>>
>>>>>> I tend to agree with Marek on this one.  We have an instance where the
>>>>>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>>>>>> but different enough where each SoC has it's own set of tables and
>>>>>> some checks.   Lucas created the framework, and others adapted it for
>>>>>> various SoC's.  If there really is nearly 50% common code for the
>>>>>> LCDIF, why not either leave the driver as one or split the common code
>>>>>> into its own driver like lcdif-common and then have smaller drivers
>>>>>> that handle their specific variations.
>>>>>
>>>>> I don't know exactly how the standalone driver looks like, but I guess
>>>>> the overlap is not really in any real HW specific parts, but the common
>>>>> DRM boilerplate, so there isn't much point in creating a common lcdif
>>>>> driver.
>>>>
>>>> The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
>>>> that, there is some 400 LoC which are specific to old LCDIF and this
>>>> patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
>>>> shared boilerplate that would be duplicated .
>>>
>>> That is probably ignoring the fact that the 8MP LCDIF does not support
>>> any overlays, so it could use the drm_simple_display_pipe
>>> infrastructure to reduce the needed boilerplate.
>>
>> It seems the IMXRT1070 LCDIF v2 (heh ...) does support overlays, so no,
>> the mxsfb and hypothetical lcdif drivers would look really very similar.
>>
>>>>> As you brought up the blk-ctrl as an example: I'm all for supporting
>>>>> slightly different hardware in the same driver, as long as the HW
>>>>> interface is close enough. But then I also opted for a separate 8MP
>>>>> blk-ctrl driver for those blk-ctrls that differ significantly from the
>>>>> others, as I think it would make the common driver unmaintainable
>>>>> trying to support all the different variants in one driver.
>>>>
>>>> But then you also need to maintain two sets of boilerplate, they
>>>> diverge, and that is not good.
>>>
>>> I don't think that there is much chance for bugs going unfixed due to
>>> divergence in the boilerplate, especially if you use the simple pipe
>>> framework to handle most of that stuff for you, which gives you a lot
>>> of code sharing with other simple DRM drivers.
>>
>> But I can not use the simple pipe because overlays, see imxrt1070 .
>>
>> [...]
>>
>> We can always split the drivers later if this becomes unmaintainable
>> too, no ?
> 
> Not if you want to keep the same userspace running. As userspace has
> some ties to the DRM driver name, e.g. for finding the right GBM
> implementation, splitting the driver later on would be a UABI break.

Hum, so what other options do we have left ? Duplicate 60% of the driver 
right away ?

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Laurent Pinchart]

Hi Marek,

Thank you for the patch.

On Mon, Feb 28, 2022 at 01:45:57AM +0100, Marek Vasut wrote:
> Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> and is completely different from the LCDIFv3 found in i.MX23 in that it
> has a completely scrambled register layout compared to all previous LCDIF
> variants. The new LCDIFv3 also supports 36bit address space. However,
> except for the complete bit reshuffling, this is still LCDIF and it still
> works like one.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Robby Cai <robby.cai@nxp.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> index 900a56cae80e6..9831ab53a053d 100644
> --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> @@ -28,6 +28,7 @@ properties:
>                - fsl,imx7d-lcdif
>                - fsl,imx8mm-lcdif
>                - fsl,imx8mn-lcdif
> +              - fsl,imx8mp-lcdif

As the hardware isn't backward-compatible with any other version, I
think the new compatible string should go in the previous enum block,
not in this one. We don't want the imx6sx fallback.

>                - fsl,imx8mq-lcdif
>            - const: fsl,imx6sx-lcdif
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Laurent Pinchart]

Hi Marek,

Thank you for the patch.

On Mon, Feb 28, 2022 at 01:45:57AM +0100, Marek Vasut wrote:
> Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> and is completely different from the LCDIFv3 found in i.MX23 in that it
> has a completely scrambled register layout compared to all previous LCDIF
> variants. The new LCDIFv3 also supports 36bit address space. However,
> except for the complete bit reshuffling, this is still LCDIF and it still
> works like one.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Robby Cai <robby.cai@nxp.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> index 900a56cae80e6..9831ab53a053d 100644
> --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> @@ -28,6 +28,7 @@ properties:
>                - fsl,imx7d-lcdif
>                - fsl,imx8mm-lcdif
>                - fsl,imx8mn-lcdif
> +              - fsl,imx8mp-lcdif

As the hardware isn't backward-compatible with any other version, I
think the new compatible string should go in the previous enum block,
not in this one. We don't want the imx6sx fallback.

>                - fsl,imx8mq-lcdif
>            - const: fsl,imx6sx-lcdif
>  

-- 
Regards,

Laurent Pinchart