Skip to contentSkip to conversation

drm/imx/dcss: Add interconnect support

PATCH In review Guido Günther 2 participants 3 messages Dec 1, 2020

Subsystems (4)
DRM drivers DRM misc Arm i.MX and Layerscape NXP i.MX 8MQ dcss driver
lore
Share & tools
lore

Guido Günther

author
#0 lore raw

cover drm/imx/dcss: Add interconnect support

This allows us to raise DRAM bandiwth to a high enough value for a stable picture on i.mx8mq. We pick a bandwidth that should be sufficient for 4k@60Hz.

This was tested on a Librem 5 with the (not yet) mainline mhdp DP controller. Without that initial boot works fine but e.g. fb unblank results in a cyan screen.

Modelled like mdp5_kms.

Guido Günther (1):
drm/imx/dcss: Add interconnect support

 drivers/gpu/drm/imx/dcss/dcss-dev.c | 47 +++++++++++++++++++++++++++--
 drivers/gpu/drm/imx/dcss/dcss-dev.h |  3 ++
 2 files changed, 48 insertions(+), 2 deletions(-)
Signature · 6 lines
2.29.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#1 lore raw

PATCH 1/1 drm/imx/dcss: Add interconnect support

This allows us to raise DRAM bandiwth to a high enough value for a stable picture on i.mx8mq. We pick a bandwidth that should be sufficient for 4k@60Hz.

Modelled like mdp5_kms.

Signed-off-by Guido Günther <[email protected]>
---
 drivers/gpu/drm/imx/dcss/dcss-dev.c | 47 +++++++++++++++++++++++++++--
 drivers/gpu/drm/imx/dcss/dcss-dev.h |  3 ++
 2 files changed, 48 insertions(+), 2 deletions(-)
drivers/gpu/drm/imx/dcss/dcss-dev.c +45 -2
index c849533ca83e..e336f03448d6 100644
@@ -15,6 +15,9 @@
 #include "dcss-dev.h"
 #include "dcss-kms.h"
 
+/* sufficient for 4K at 60 Hz */
+#define DCSS_BW_MAX GBps_to_icc(2)
+
 static void dcss_clocks_enable(struct dcss_dev *dcss)
 {
 	clk_prepare_enable(dcss->axi_clk);

@@ -162,6 +165,31 @@ static void dcss_clks_release(struct dcss_dev *dcss)
 	devm_clk_put(dcss->dev, dcss->apb_clk);
 }
 
+static int dcss_init_icc(struct dcss_dev *dcss)
+{
+	int ret;
+	struct icc_path *icc_path;
+
+	/* Optional interconnect request */
+	icc_path = of_icc_get(dcss->dev, NULL);
+	if (IS_ERR(icc_path)) {
+		ret = PTR_ERR(icc_path);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+		/* no interconnect support is not necessarily a fatal
+		 * condition, the platform may simply not have an
+		 * interconnect driver yet.  But warn about it in case
+		 * bootloader didn't setup bus clocks high enough for
+		 * scanout.
+		 */
+		dev_warn(dcss->dev, "No interconnect support may cause display underflows!\n");
+		return 0;
+	}
+	dcss->icc_path = icc_path;
+	dcss->icc_peak_bw = DCSS_BW_MAX;
+	return 0;
+}
+
 struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output)
 {
 	struct platform_device *pdev = to_platform_device(dev);

@@ -190,10 +218,14 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output)
 	dcss->devtype = devtype;
 	dcss->hdmi_output = hdmi_output;
 
+	ret = dcss_init_icc(dcss);
+	if (ret < 0)
+		goto err;
+
 	ret = dcss_clks_init(dcss);
 	if (ret) {
 		dev_err(dev, "clocks initialization failed\n");
-		goto err;
+		goto icc_err;
 	}
 
 	dcss->of_port = of_graph_get_port_by_id(dev->of_node, 0);

@@ -223,7 +255,8 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output)
 
 clks_err:
 	dcss_clks_release(dcss);
-
+icc_err:
+	icc_put(dcss->icc_path);
 err:
 	kfree(dcss);
 

@@ -243,6 +276,8 @@ void dcss_dev_destroy(struct dcss_dev *dcss)
 
 	dcss_clks_release(dcss);
 
+	icc_put(dcss->icc_path);
+
 	kfree(dcss);
 }
 

@@ -267,6 +302,8 @@ int dcss_dev_suspend(struct device *dev)
 
 	dcss_clocks_disable(dcss);
 
+	icc_set_bw(dcss->icc_path, 0, 0);
+
 	return 0;
 }
 

@@ -281,6 +318,8 @@ int dcss_dev_resume(struct device *dev)
 		return 0;
 	}
 
+	icc_set_bw(dcss->icc_path, 0, dcss->icc_peak_bw);
+
 	dcss_clocks_enable(dcss);
 
 	dcss_blkctl_cfg(dcss->blkctl);

@@ -307,6 +346,8 @@ int dcss_dev_runtime_suspend(struct device *dev)
 
 	dcss_clocks_disable(dcss);
 
+	icc_set_bw(dcss->icc_path, 0, 0);
+
 	return 0;
 }
 

@@ -314,6 +355,8 @@ int dcss_dev_runtime_resume(struct device *dev)
 {
 	struct dcss_dev *dcss = dcss_drv_dev_to_dcss(dev);
 
+	icc_set_bw(dcss->icc_path, 0, dcss->icc_peak_bw);
+
 	dcss_clocks_enable(dcss);
 
 	dcss_blkctl_cfg(dcss->blkctl);
drivers/gpu/drm/imx/dcss/dcss-dev.h +3 -0
index c642ae17837f..1b35a6f0d0d4 100644
@@ -8,6 +8,7 @@
 
 #include <drm/drm_fourcc.h>
 #include <linux/io.h>
+#include <linux/interconnect.h>
 #include <video/videomode.h>
 
 #define SET			0x04

@@ -85,6 +86,8 @@ struct dcss_dev {
 	struct clk *pll_phy_ref_clk;
 
 	bool hdmi_output;
+	struct icc_path *icc_path;
+	u32 icc_peak_bw;
 
 	void (*disable_callback)(void *data);
 	struct completion disable_completion;
Signature · 6 lines
2.29.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
reviewer
#2 lore raw

Am Dienstag, den 01.12.2020, 12:34 +0100 schrieb Guido Günther:

Quoted 20 lines

This allows us to raise DRAM bandiwth to a high enough value for a stable picture on i.mx8mq. We pick a bandwidth that should be sufficient for 4k@60Hz.

Modelled like mdp5_kms.

Signed-off-by Guido Günther <[email protected]>

---
 drivers/gpu/drm/imx/dcss/dcss-dev.c | 47 +++++++++++++++++++++++++++-- drivers/gpu/drm/imx/dcss/dcss-dev.h | 3 ++

 2 files changed, 48 insertions(+), 2 deletions(-)
drivers/gpu/drm/imx/dcss/dcss-dev.c +0 -0
index c849533ca83e..e336f03448d6 100644
@@ -15,6 +15,9 @@
  #include "dcss-dev.h"
  #include "dcss-kms.h"

drivers/gpu/drm/imx/dcss/dcss-dev.c line 20

Quoted 3 lines
drivers/gpu/drm/imx/dcss/dcss-dev.c +3 -0
+/* sufficient for 4K at 60 Hz */
+#define DCSS_BW_MAX GBps_to_icc(2)
+

Same comment as for the similar change in mxsfb: this should not be a static value, but should be scaled proportional to the current mode and plane settings. With a single static value you keep the clocks way too high for most use-cases with smaller displays, but fail to account for the additional bandwidth requirement when more than one plane is active.

Regards,
Lucas

Quoted 191 lines

 static void dcss_clocks_enable(struct dcss_dev *dcss) {
  clk_prepare_enable(dcss->axi_clk);

drivers/gpu/drm/imx/dcss/dcss-dev.c +0 -0
@@ -162,6 +165,31 @@ static void dcss_clks_release(struct dcss_dev *dcss)
  	devm_clk_put(dcss->dev, dcss->apb_clk);
  }

drivers/gpu/drm/imx/dcss/dcss-dev.c +25 -0
+static int dcss_init_icc(struct dcss_dev *dcss)
+{
+	int ret;
+	struct icc_path *icc_path;
+
+	/* Optional interconnect request */
+	icc_path = of_icc_get(dcss->dev, NULL);
+	if (IS_ERR(icc_path)) {
+		ret = PTR_ERR(icc_path);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+		/* no interconnect support is not necessarily a fatal
+		 * condition, the platform may simply not have an
+		 * interconnect driver yet.  But warn about it in case
+		 * bootloader didn't setup bus clocks high enough for
+		 * scanout.
+		 */
+		dev_warn(dcss->dev, "No interconnect support may cause display underflows!\n");
+		return 0;
+	}
+	dcss->icc_path = icc_path;
+	dcss->icc_peak_bw = DCSS_BW_MAX;
+	return 0;
+}
+

 struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output) {
  struct platform_device *pdev = to_platform_device(dev);

drivers/gpu/drm/imx/dcss/dcss-dev.c +0 -0
@@ -190,10 +218,14 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output)
  	dcss->devtype = devtype;
  	dcss->hdmi_output = hdmi_output;

drivers/gpu/drm/imx/dcss/dcss-dev.c +4 -0
+	ret = dcss_init_icc(dcss);
+	if (ret < 0)
+		goto err;
+

  ret = dcss_clks_init(dcss);
  if (ret) {
  dev_err(dev, "clocks initialization failed\n");

drivers/gpu/drm/imx/dcss/dcss-dev.c +1 -1
-		goto err;
+		goto icc_err;

  }

  dcss->of_port = of_graph_get_port_by_id(dev->of_node, 0);

drivers/gpu/drm/imx/dcss/dcss-dev.c +0 -0
@@ -223,7 +255,8 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output)

 clks_err:
  dcss_clks_release(dcss);

drivers/gpu/drm/imx/dcss/dcss-dev.c +2 -1
-
+icc_err:
+	icc_put(dcss->icc_path);

 err:
  kfree(dcss);

drivers/gpu/drm/imx/dcss/dcss-dev.c +0 -0
@@ -243,6 +276,8 @@ void dcss_dev_destroy(struct dcss_dev *dcss)

  dcss_clks_release(dcss);

drivers/gpu/drm/imx/dcss/dcss-dev.c +2 -0
+	icc_put(dcss->icc_path);
+

  kfree(dcss);
 }

drivers/gpu/drm/imx/dcss/dcss-dev.c +0 -0
@@ -267,6 +302,8 @@ int dcss_dev_suspend(struct device *dev)

  dcss_clocks_disable(dcss);

drivers/gpu/drm/imx/dcss/dcss-dev.c +2 -0
+	icc_set_bw(dcss->icc_path, 0, 0);
+

  return 0;
 }

drivers/gpu/drm/imx/dcss/dcss-dev.c +0 -0
@@ -281,6 +318,8 @@ int dcss_dev_resume(struct device *dev)
  		return 0;
  	}

drivers/gpu/drm/imx/dcss/dcss-dev.c +2 -0
+	icc_set_bw(dcss->icc_path, 0, dcss->icc_peak_bw);
+

  dcss_clocks_enable(dcss);

  dcss_blkctl_cfg(dcss->blkctl);

drivers/gpu/drm/imx/dcss/dcss-dev.c +0 -0
@@ -307,6 +346,8 @@ int dcss_dev_runtime_suspend(struct device *dev)

  dcss_clocks_disable(dcss);

drivers/gpu/drm/imx/dcss/dcss-dev.c +2 -0
+	icc_set_bw(dcss->icc_path, 0, 0);
+

  return 0;
 }

drivers/gpu/drm/imx/dcss/dcss-dev.c +0 -0
@@ -314,6 +355,8 @@ int dcss_dev_runtime_resume(struct device *dev)
  {
  	struct dcss_dev *dcss = dcss_drv_dev_to_dcss(dev);

drivers/gpu/drm/imx/dcss/dcss-dev.c +2 -0
+	icc_set_bw(dcss->icc_path, 0, dcss->icc_peak_bw);
+

  dcss_clocks_enable(dcss);

  dcss_blkctl_cfg(dcss->blkctl);

drivers/gpu/drm/imx/dcss/dcss-dev.h +0 -0
index c642ae17837f..1b35a6f0d0d4 100644
@@ -8,6 +8,7 @@

 #include <drm/drm_fourcc.h>
 #include <linux/io.h>
+#include <linux/interconnect.h>
 #include <video/videomode.h>

 #define SET 0x04

drivers/gpu/drm/imx/dcss/dcss-dev.h +0 -0
@@ -85,6 +86,8 @@ struct dcss_dev {
  	struct clk *pll_phy_ref_clk;

  bool hdmi_output;

drivers/gpu/drm/imx/dcss/dcss-dev.h +2 -0
+	struct icc_path *icc_path;
+	u32 icc_peak_bw;

Quoted 2 lines

  void (*disable_callback)(void *data);
  struct completion disable_completion;

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel