[PATCH v2 0/2] drm: rcar-du: Rework clock configuration

[PATCH v2 0/2] drm: rcar-du: Rework clock configuration

[Jacopo Mondi]

Hello,
  second round for improved input dot clock selection procedure for R-Car DU
peripheral.

Please refer to the v1 cover letter for the background:
https://lkml.org/lkml/2018/7/30/702

compared to v1 I have squashed the small patch I initially separated from
Laurent's one, and have closed a comment on alignment from Kieran there.

The second patch has been greatly simplified, not taking into account
non-rounded clock rates provided by the external clock source. In this way the
only rates to compared are the CPG generated one and the one provided by the
external clock source.

Tested on M3-W Salvator-X board with VGA output in 1920x1080 mode.

With this patch applied:
 rcar-du feb00000.display: mode clock 148500000 ext rate 148500000
 rcar-du feb00000.display: rcar_du_crtc_set_display_timing: ESCR 0x0000000

The requested pixel clock is opportunely provided by the external clock source.

Without this patch:
 rcar-du feb00000.display: mode clock 148500000 extrate 108000000 rate 133333328 ESCR 0x00100002

The requested pixel clock is approximated by the CPG generated clock to 133,3
MHz, which for some monitor is not enough to correctly display any output.

Thanks
  j

Jacopo Mondi (1):
  drm: rcar-du: Improve non-DPLL clock selection

Laurent Pinchart (1):
  drm: rcar-du: Rework clock configuration based on hardware limits

v1 -> v2:
- Squash v1' [1/3] and [2/3]
- Simplify [3/3] only taking into account rounded rates provided by the external
  clock source.

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 143 +++++++++++++++++++--------------
 1 file changed, 83 insertions(+), 60 deletions(-)

--
2.7.4

[PATCH v2 0/2] drm: rcar-du: Rework clock configuration

[Jacopo Mondi]

Hello,
  second round for improved input dot clock selection procedure for R-Car DU
peripheral.

Please refer to the v1 cover letter for the background:
https://lkml.org/lkml/2018/7/30/702

compared to v1 I have squashed the small patch I initially separated from
Laurent's one, and have closed a comment on alignment from Kieran there.

The second patch has been greatly simplified, not taking into account
non-rounded clock rates provided by the external clock source. In this way the
only rates to compared are the CPG generated one and the one provided by the
external clock source.

Tested on M3-W Salvator-X board with VGA output in 1920x1080 mode.

With this patch applied:
 rcar-du feb00000.display: mode clock 148500000 ext rate 148500000
 rcar-du feb00000.display: rcar_du_crtc_set_display_timing: ESCR 0x0000000

The requested pixel clock is opportunely provided by the external clock source.

Without this patch:
 rcar-du feb00000.display: mode clock 148500000 extrate 108000000 rate 133333328 ESCR 0x00100002

The requested pixel clock is approximated by the CPG generated clock to 133,3
MHz, which for some monitor is not enough to correctly display any output.

Thanks
  j

Jacopo Mondi (1):
  drm: rcar-du: Improve non-DPLL clock selection

Laurent Pinchart (1):
  drm: rcar-du: Rework clock configuration based on hardware limits

v1 -> v2:
- Squash v1' [1/3] and [2/3]
- Simplify [3/3] only taking into account rounded rates provided by the external
  clock source.

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 143 +++++++++++++++++++--------------
 1 file changed, 83 insertions(+), 60 deletions(-)

--
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm: rcar-du: Rework clock configuration based on hardware limits

[Jacopo Mondi]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

The DU channels that have a display PLL (DPLL) can only use external
clock sources, and don't have an internal clock divider (with the
exception of H3 ES1.x where the post-divider is present and needs to be
used as a workaround for a DPLL silicon issue).

Rework the clock configuration to take this into account, avoiding
selection of non-existing clock sources or usage of a missing
post-divider.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 134 ++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index b52b3e8..077e681 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -208,78 +208,90 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
 	struct rcar_du_device *rcdu = rcrtc->group->dev;
 	unsigned long mode_clock = mode->clock * 1000;
-	unsigned long clk;
-	u32 value;
+	u32 dsmr;
 	u32 escr;
-	u32 div;
 
-	/*
-	 * Compute the clock divisor and select the internal or external dot
-	 * clock based on the requested frequency.
-	 */
-	clk = clk_get_rate(rcrtc->clock);
-	div = DIV_ROUND_CLOSEST(clk, mode_clock);
-	div = clamp(div, 1U, 64U) - 1;
-	escr = div | ESCR_DCLKSEL_CLKS;
-
-	if (rcrtc->extclock) {
+	if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
+		unsigned long target = mode_clock;
 		struct dpll_info dpll = { 0 };
 		unsigned long extclk;
-		unsigned long extrate;
-		unsigned long rate;
-		u32 extdiv;
+		u32 dpllcr;
+		u32 div = 0;
 
-		extclk = clk_get_rate(rcrtc->extclock);
-		if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
-			unsigned long target = mode_clock;
+		/*
+		 * DU channels that have a display PLL can't use the internal
+		 * system clock, and have no internal clock divider.
+		 */
 
-			/*
-			 * The H3 ES1.x exhibits dot clock duty cycle stability
-			 * issues. We can work around them by configuring the
-			 * DPLL to twice the desired frequency, coupled with a
-			 * /2 post-divider. This isn't needed on other SoCs and
-			 * breaks HDMI output on M3-W for a currently unknown
-			 * reason, so restrict the workaround to H3 ES1.x.
-			 */
-			if (soc_device_match(rcar_du_r8a7795_es1))
-				target *= 2;
+		if (WARN_ON(!rcrtc->extclock))
+			return;
 
-			rcar_du_dpll_divider(rcrtc, &dpll, extclk, target);
-			extclk = dpll.output;
+		/*
+		 * The H3 ES1.x exhibits dot clock duty cycle stability issues.
+		 * We can work around them by configuring the DPLL to twice the
+		 * desired frequency, coupled with a /2 post-divider. Restrict
+		 * the workaround to H3 ES1.x as ES2.0 and all other SoCs have
+		 * no post-divider when a display PLL is present (as shown by
+		 * the workaround breaking HDMI output on M3-W during testing).
+		 */
+		if (soc_device_match(rcar_du_r8a7795_es1)) {
+			target *= 2;
+			div = 1;
 		}
 
-		extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
-		extdiv = clamp(extdiv, 1U, 64U) - 1;
+		extclk = clk_get_rate(rcrtc->extclock);
+		rcar_du_dpll_divider(rcrtc, &dpll, extclk, target);
 
-		rate = clk / (div + 1);
-		extrate = extclk / (extdiv + 1);
+		dpllcr = DPLLCR_CODE | DPLLCR_CLKE
+		       | DPLLCR_FDPLL(dpll.fdpll)
+		       | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
+		       | DPLLCR_STBY;
 
-		if (abs((long)extrate - (long)mode_clock) <
-		    abs((long)rate - (long)mode_clock)) {
+		if (rcrtc->index == 1)
+			dpllcr |= DPLLCR_PLCS1
+			       |  DPLLCR_INCS_DOTCLKIN1;
+		else
+			dpllcr |= DPLLCR_PLCS0
+			       |  DPLLCR_INCS_DOTCLKIN0;
 
-			if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
-				u32 dpllcr = DPLLCR_CODE | DPLLCR_CLKE
-					   | DPLLCR_FDPLL(dpll.fdpll)
-					   | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
-					   | DPLLCR_STBY;
+		rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr);
 
-				if (rcrtc->index == 1)
-					dpllcr |= DPLLCR_PLCS1
-					       |  DPLLCR_INCS_DOTCLKIN1;
-				else
-					dpllcr |= DPLLCR_PLCS0
-					       |  DPLLCR_INCS_DOTCLKIN0;
+		escr = ESCR_DCLKSEL_DCLKIN | div;
+	} else {
+		unsigned long clk;
+		u32 div;
 
-				rcar_du_group_write(rcrtc->group, DPLLCR,
-						    dpllcr);
-			}
+		/*
+		 * Compute the clock divisor and select the internal or external
+		 * dot clock based on the requested frequency.
+		 */
+		clk = clk_get_rate(rcrtc->clock);
+		div = DIV_ROUND_CLOSEST(clk, mode_clock);
+		div = clamp(div, 1U, 64U) - 1;
 
-			escr = ESCR_DCLKSEL_DCLKIN | extdiv;
-		}
+		escr = ESCR_DCLKSEL_CLKS | div;
 
-		dev_dbg(rcrtc->group->dev->dev,
-			"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
-			mode_clock, extrate, rate, escr);
+		if (rcrtc->extclock) {
+			unsigned long extclk;
+			unsigned long extrate;
+			unsigned long rate;
+			u32 extdiv;
+
+			extclk = clk_get_rate(rcrtc->extclock);
+			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
+			extdiv = clamp(extdiv, 1U, 64U) - 1;
+
+			extrate = extclk / (extdiv + 1);
+			rate = clk / (div + 1);
+
+			if (abs((long)extrate - (long)mode_clock) <
+			    abs((long)rate - (long)mode_clock))
+				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
+
+			dev_dbg(rcrtc->group->dev->dev,
+				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
+				mode_clock, extrate, rate, escr);
+		}
 	}
 
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
@@ -287,11 +299,11 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
 
 	/* Signal polarities */
-	value = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0)
-	      | ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? DSMR_HSL : 0)
-	      | ((mode->flags & DRM_MODE_FLAG_INTERLACE) ? DSMR_ODEV : 0)
-	      | DSMR_DIPM_DISP | DSMR_CSPM;
-	rcar_du_crtc_write(rcrtc, DSMR, value);
+	dsmr = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0)
+	     | ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? DSMR_HSL : 0)
+	     | ((mode->flags & DRM_MODE_FLAG_INTERLACE) ? DSMR_ODEV : 0)
+	     | DSMR_DIPM_DISP | DSMR_CSPM;
+	rcar_du_crtc_write(rcrtc, DSMR, dsmr);
 
 	/* Display timings */
 	rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start - 19);
-- 
2.7.4

[PATCH v2 1/2] drm: rcar-du: Rework clock configuration based on hardware limits

[Jacopo Mondi]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

The DU channels that have a display PLL (DPLL) can only use external
clock sources, and don't have an internal clock divider (with the
exception of H3 ES1.x where the post-divider is present and needs to be
used as a workaround for a DPLL silicon issue).

Rework the clock configuration to take this into account, avoiding
selection of non-existing clock sources or usage of a missing
post-divider.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 134 ++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index b52b3e8..077e681 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -208,78 +208,90 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
 	struct rcar_du_device *rcdu = rcrtc->group->dev;
 	unsigned long mode_clock = mode->clock * 1000;
-	unsigned long clk;
-	u32 value;
+	u32 dsmr;
 	u32 escr;
-	u32 div;
 
-	/*
-	 * Compute the clock divisor and select the internal or external dot
-	 * clock based on the requested frequency.
-	 */
-	clk = clk_get_rate(rcrtc->clock);
-	div = DIV_ROUND_CLOSEST(clk, mode_clock);
-	div = clamp(div, 1U, 64U) - 1;
-	escr = div | ESCR_DCLKSEL_CLKS;
-
-	if (rcrtc->extclock) {
+	if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
+		unsigned long target = mode_clock;
 		struct dpll_info dpll = { 0 };
 		unsigned long extclk;
-		unsigned long extrate;
-		unsigned long rate;
-		u32 extdiv;
+		u32 dpllcr;
+		u32 div = 0;
 
-		extclk = clk_get_rate(rcrtc->extclock);
-		if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
-			unsigned long target = mode_clock;
+		/*
+		 * DU channels that have a display PLL can't use the internal
+		 * system clock, and have no internal clock divider.
+		 */
 
-			/*
-			 * The H3 ES1.x exhibits dot clock duty cycle stability
-			 * issues. We can work around them by configuring the
-			 * DPLL to twice the desired frequency, coupled with a
-			 * /2 post-divider. This isn't needed on other SoCs and
-			 * breaks HDMI output on M3-W for a currently unknown
-			 * reason, so restrict the workaround to H3 ES1.x.
-			 */
-			if (soc_device_match(rcar_du_r8a7795_es1))
-				target *= 2;
+		if (WARN_ON(!rcrtc->extclock))
+			return;
 
-			rcar_du_dpll_divider(rcrtc, &dpll, extclk, target);
-			extclk = dpll.output;
+		/*
+		 * The H3 ES1.x exhibits dot clock duty cycle stability issues.
+		 * We can work around them by configuring the DPLL to twice the
+		 * desired frequency, coupled with a /2 post-divider. Restrict
+		 * the workaround to H3 ES1.x as ES2.0 and all other SoCs have
+		 * no post-divider when a display PLL is present (as shown by
+		 * the workaround breaking HDMI output on M3-W during testing).
+		 */
+		if (soc_device_match(rcar_du_r8a7795_es1)) {
+			target *= 2;
+			div = 1;
 		}
 
-		extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
-		extdiv = clamp(extdiv, 1U, 64U) - 1;
+		extclk = clk_get_rate(rcrtc->extclock);
+		rcar_du_dpll_divider(rcrtc, &dpll, extclk, target);
 
-		rate = clk / (div + 1);
-		extrate = extclk / (extdiv + 1);
+		dpllcr = DPLLCR_CODE | DPLLCR_CLKE
+		       | DPLLCR_FDPLL(dpll.fdpll)
+		       | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
+		       | DPLLCR_STBY;
 
-		if (abs((long)extrate - (long)mode_clock) <
-		    abs((long)rate - (long)mode_clock)) {
+		if (rcrtc->index == 1)
+			dpllcr |= DPLLCR_PLCS1
+			       |  DPLLCR_INCS_DOTCLKIN1;
+		else
+			dpllcr |= DPLLCR_PLCS0
+			       |  DPLLCR_INCS_DOTCLKIN0;
 
-			if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
-				u32 dpllcr = DPLLCR_CODE | DPLLCR_CLKE
-					   | DPLLCR_FDPLL(dpll.fdpll)
-					   | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m)
-					   | DPLLCR_STBY;
+		rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr);
 
-				if (rcrtc->index == 1)
-					dpllcr |= DPLLCR_PLCS1
-					       |  DPLLCR_INCS_DOTCLKIN1;
-				else
-					dpllcr |= DPLLCR_PLCS0
-					       |  DPLLCR_INCS_DOTCLKIN0;
+		escr = ESCR_DCLKSEL_DCLKIN | div;
+	} else {
+		unsigned long clk;
+		u32 div;
 
-				rcar_du_group_write(rcrtc->group, DPLLCR,
-						    dpllcr);
-			}
+		/*
+		 * Compute the clock divisor and select the internal or external
+		 * dot clock based on the requested frequency.
+		 */
+		clk = clk_get_rate(rcrtc->clock);
+		div = DIV_ROUND_CLOSEST(clk, mode_clock);
+		div = clamp(div, 1U, 64U) - 1;
 
-			escr = ESCR_DCLKSEL_DCLKIN | extdiv;
-		}
+		escr = ESCR_DCLKSEL_CLKS | div;
 
-		dev_dbg(rcrtc->group->dev->dev,
-			"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
-			mode_clock, extrate, rate, escr);
+		if (rcrtc->extclock) {
+			unsigned long extclk;
+			unsigned long extrate;
+			unsigned long rate;
+			u32 extdiv;
+
+			extclk = clk_get_rate(rcrtc->extclock);
+			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
+			extdiv = clamp(extdiv, 1U, 64U) - 1;
+
+			extrate = extclk / (extdiv + 1);
+			rate = clk / (div + 1);
+
+			if (abs((long)extrate - (long)mode_clock) <
+			    abs((long)rate - (long)mode_clock))
+				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
+
+			dev_dbg(rcrtc->group->dev->dev,
+				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
+				mode_clock, extrate, rate, escr);
+		}
 	}
 
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
@@ -287,11 +299,11 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
 
 	/* Signal polarities */
-	value = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0)
-	      | ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? DSMR_HSL : 0)
-	      | ((mode->flags & DRM_MODE_FLAG_INTERLACE) ? DSMR_ODEV : 0)
-	      | DSMR_DIPM_DISP | DSMR_CSPM;
-	rcar_du_crtc_write(rcrtc, DSMR, value);
+	dsmr = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0)
+	     | ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? DSMR_HSL : 0)
+	     | ((mode->flags & DRM_MODE_FLAG_INTERLACE) ? DSMR_ODEV : 0)
+	     | DSMR_DIPM_DISP | DSMR_CSPM;
+	rcar_du_crtc_write(rcrtc, DSMR, dsmr);
 
 	/* Display timings */
 	rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start - 19);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm: rcar-du: Improve non-DPLL clock selection

[Jacopo Mondi]

From: Jacopo Mondi <jacopo@jmondi.org>

DU channels not equipped with a DPLL use an internal (aka SoC provided) or
external clock source combined with an internal divider to generate the
desired output dot clock frequency.

The current clock selection procedure does not fully exploit the ability
of external clock sources to generate the exact dot clock frequency by
themselves, but relies instead on tuning the internal DU clock divider
only, resulting in a less precise clock generation process.

When possible, and desirable, ask the external clock source for the exact
output dot clock frequency, and test the returned frequency against the
internally generated one to better approximate the desired output dot clock.

This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
where the DU's input dotclock.in is generated by the versaclock VC5 clock
source, which is capable of generating the exact rate the DU needs as pixel
clock output.

This patch fixes higher resolution modes wich requires an high pixel clock
output currently not working on non-HDMI DU channel (as VGA 1920x1080@60Hz).

Fixes: 1b30dbde8 "drm: rcar-du: Add support for external pixel clock"
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 53 ++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 077e681..98ae697 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -258,42 +258,53 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)

 		escr = ESCR_DCLKSEL_DCLKIN | div;
 	} else {
-		unsigned long clk;
+		unsigned long dotclkin_rate;
+		struct clk *dotclkin_clk;
+		unsigned long cpg_dist;
 		u32 div;

 		/*
 		 * Compute the clock divisor and select the internal or external
 		 * dot clock based on the requested frequency.
 		 */
-		clk = clk_get_rate(rcrtc->clock);
-		div = DIV_ROUND_CLOSEST(clk, mode_clock);
-		div = clamp(div, 1U, 64U) - 1;
-
+		dotclkin_clk = rcrtc->clock;
+		dotclkin_rate = clk_get_rate(rcrtc->clock);
+		div = clamp(DIV_ROUND_CLOSEST(dotclkin_rate, mode_clock),
+			    1UL, 64UL) - 1;
+		cpg_dist = abs(dotclkin_rate / (div + 1) - mode_clock);
 		escr = ESCR_DCLKSEL_CLKS | div;

 		if (rcrtc->extclock) {
-			unsigned long extclk;
-			unsigned long extrate;
-			unsigned long rate;
-			u32 extdiv;
-
-			extclk = clk_get_rate(rcrtc->extclock);
-			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
-			extdiv = clamp(extdiv, 1U, 64U) - 1;
+			unsigned long ext_rate;
+			unsigned long ext_dist;

-			extrate = extclk / (extdiv + 1);
-			rate = clk / (div + 1);
+			/*
+			 * If an external clock source is present ask it
+			 * for the exact dot clock rate, and test the
+			 * returned value against the cpg provided one.
+			 */
+			ext_rate = clk_round_rate(rcrtc->extclock,
+						  mode_clock);

-			if (abs((long)extrate - (long)mode_clock) <
-			    abs((long)rate - (long)mode_clock))
-				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
+			div = clamp(DIV_ROUND_CLOSEST(ext_rate, mode_clock),
+				    1UL, 64UL) - 1;
+			ext_dist = abs(ext_rate / (div + 1) - mode_clock);

-			dev_dbg(rcrtc->group->dev->dev,
-				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
-				mode_clock, extrate, rate, escr);
+			if (ext_dist < cpg_dist) {
+				dotclkin_clk = rcrtc->extclock;
+				dotclkin_rate = ext_rate;
+				escr = ESCR_DCLKSEL_DCLKIN | div;
+			}
 		}
+
+		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
+			mode_clock, dotclkin_clk == rcrtc->clock ? "cpg" : "ext",
+			dotclkin_rate);
+		clk_set_rate(dotclkin_clk, dotclkin_rate);
 	}

+	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
+
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
 			    escr);
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
--
2.7.4

[PATCH v2 2/2] drm: rcar-du: Improve non-DPLL clock selection

[Jacopo Mondi]

From: Jacopo Mondi <jacopo@jmondi.org>

DU channels not equipped with a DPLL use an internal (aka SoC provided) or
external clock source combined with an internal divider to generate the
desired output dot clock frequency.

The current clock selection procedure does not fully exploit the ability
of external clock sources to generate the exact dot clock frequency by
themselves, but relies instead on tuning the internal DU clock divider
only, resulting in a less precise clock generation process.

When possible, and desirable, ask the external clock source for the exact
output dot clock frequency, and test the returned frequency against the
internally generated one to better approximate the desired output dot clock.

This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
where the DU's input dotclock.in is generated by the versaclock VC5 clock
source, which is capable of generating the exact rate the DU needs as pixel
clock output.

This patch fixes higher resolution modes wich requires an high pixel clock
output currently not working on non-HDMI DU channel (as VGA 1920x1080@60Hz).

Fixes: 1b30dbde8 "drm: rcar-du: Add support for external pixel clock"
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 53 ++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 077e681..98ae697 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -258,42 +258,53 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)

 		escr = ESCR_DCLKSEL_DCLKIN | div;
 	} else {
-		unsigned long clk;
+		unsigned long dotclkin_rate;
+		struct clk *dotclkin_clk;
+		unsigned long cpg_dist;
 		u32 div;

 		/*
 		 * Compute the clock divisor and select the internal or external
 		 * dot clock based on the requested frequency.
 		 */
-		clk = clk_get_rate(rcrtc->clock);
-		div = DIV_ROUND_CLOSEST(clk, mode_clock);
-		div = clamp(div, 1U, 64U) - 1;
-
+		dotclkin_clk = rcrtc->clock;
+		dotclkin_rate = clk_get_rate(rcrtc->clock);
+		div = clamp(DIV_ROUND_CLOSEST(dotclkin_rate, mode_clock),
+			    1UL, 64UL) - 1;
+		cpg_dist = abs(dotclkin_rate / (div + 1) - mode_clock);
 		escr = ESCR_DCLKSEL_CLKS | div;

 		if (rcrtc->extclock) {
-			unsigned long extclk;
-			unsigned long extrate;
-			unsigned long rate;
-			u32 extdiv;
-
-			extclk = clk_get_rate(rcrtc->extclock);
-			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
-			extdiv = clamp(extdiv, 1U, 64U) - 1;
+			unsigned long ext_rate;
+			unsigned long ext_dist;

-			extrate = extclk / (extdiv + 1);
-			rate = clk / (div + 1);
+			/*
+			 * If an external clock source is present ask it
+			 * for the exact dot clock rate, and test the
+			 * returned value against the cpg provided one.
+			 */
+			ext_rate = clk_round_rate(rcrtc->extclock,
+						  mode_clock);

-			if (abs((long)extrate - (long)mode_clock) <
-			    abs((long)rate - (long)mode_clock))
-				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
+			div = clamp(DIV_ROUND_CLOSEST(ext_rate, mode_clock),
+				    1UL, 64UL) - 1;
+			ext_dist = abs(ext_rate / (div + 1) - mode_clock);

-			dev_dbg(rcrtc->group->dev->dev,
-				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
-				mode_clock, extrate, rate, escr);
+			if (ext_dist < cpg_dist) {
+				dotclkin_clk = rcrtc->extclock;
+				dotclkin_rate = ext_rate;
+				escr = ESCR_DCLKSEL_DCLKIN | div;
+			}
 		}
+
+		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
+			mode_clock, dotclkin_clk == rcrtc->clock ? "cpg" : "ext",
+			dotclkin_rate);
+		clk_set_rate(dotclkin_clk, dotclkin_rate);
 	}

+	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
+
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
 			    escr);
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
--
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: rcar-du: Improve non-DPLL clock selection

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Monday, 20 August 2018 18:26:17 EEST Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> DU channels not equipped with a DPLL use an internal (aka SoC provided) or

I'd say "SoC internal (provided by the CPG)"

> external clock source combined with an internal divider to generate the

and here "a DU internal divider" to avoid confusion with an SoC internal 
divider outside of the DU.

> desired output dot clock frequency.
> 
> The current clock selection procedure does not fully exploit the ability
> of external clock sources to generate the exact dot clock frequency by
> themselves, but relies instead on tuning the internal DU clock divider
> only, resulting in a less precise clock generation process.
> 
> When possible, and desirable, ask the external clock source for the exact
> output dot clock frequency, and test the returned frequency against the
> internally generated one to better approximate the desired output dot clock.

To make this a big more generic, I' say "and select the clock source that 
produces the frequency closest to the desired output dot clock".

> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> where the DU's input dotclock.in is generated by the versaclock VC5 clock
> source, which is capable of generating the exact rate the DU needs as pixel
> clock output.
> 
> This patch fixes higher resolution modes wich requires an high pixel clock

s/wich/which/

> output currently not working on non-HDMI DU channel (as VGA 1920x1080@60Hz).

Maybe "(such as 1920x1080@60Hz on the VGA output)" ?
> 
> Fixes: 1b30dbde8 "drm: rcar-du: Add support for external pixel clock"

Please use the output of git show --pretty=fixes to generate the fixes line. 
Your SHA1 needs more digits, and the subject should be enclosed with 
parentheses.

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 53 +++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 077e681..98ae697 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -258,42 +258,53 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc)
> 
>  		escr = ESCR_DCLKSEL_DCLKIN | div;
>  	} else {
> -		unsigned long clk;
> +		unsigned long dotclkin_rate;
> +		struct clk *dotclkin_clk;
> +		unsigned long cpg_dist;
>  		u32 div;
> 
>  		/*
>  		 * Compute the clock divisor and select the internal or external
>  		 * dot clock based on the requested frequency.
>  		 */
> -		clk = clk_get_rate(rcrtc->clock);
> -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> -		div = clamp(div, 1U, 64U) - 1;
> -
> +		dotclkin_clk = rcrtc->clock;
> +		dotclkin_rate = clk_get_rate(rcrtc->clock);
> +		div = clamp(DIV_ROUND_CLOSEST(dotclkin_rate, mode_clock),
> +			    1UL, 64UL) - 1;
> +		cpg_dist = abs(dotclkin_rate / (div + 1) - mode_clock);
>  		escr = ESCR_DCLKSEL_CLKS | div;
> 
>  		if (rcrtc->extclock) {
> -			unsigned long extclk;
> -			unsigned long extrate;
> -			unsigned long rate;
> -			u32 extdiv;
> -
> -			extclk = clk_get_rate(rcrtc->extclock);
> -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> +			unsigned long ext_rate;
> +			unsigned long ext_dist;
> 
> -			extrate = extclk / (extdiv + 1);
> -			rate = clk / (div + 1);
> +			/*
> +			 * If an external clock source is present ask it
> +			 * for the exact dot clock rate, and test the
> +			 * returned value against the cpg provided one.
> +			 */
> +			ext_rate = clk_round_rate(rcrtc->extclock,
> +						  mode_clock);
> 
> -			if (abs((long)extrate - (long)mode_clock) <
> -			    abs((long)rate - (long)mode_clock))
> -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> +			div = clamp(DIV_ROUND_CLOSEST(ext_rate, mode_clock),
> +				    1UL, 64UL) - 1;
> +			ext_dist = abs(ext_rate / (div + 1) - mode_clock);
> 
> -			dev_dbg(rcrtc->group->dev->dev,
> -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> -				mode_clock, extrate, rate, escr);
> +			if (ext_dist < cpg_dist) {
> +				dotclkin_clk = rcrtc->extclock;
> +				dotclkin_rate = ext_rate;
> +				escr = ESCR_DCLKSEL_DCLKIN | div;
> +			}
>  		}

I think we can do something much simpler here by factoring some code out to a 
function. I'll send a proposal in reply to this e-mail.

> +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> +			mode_clock, dotclkin_clk == rcrtc->clock ? "cpg" : "ext",
> +			dotclkin_rate);
> +		clk_set_rate(dotclkin_clk, dotclkin_rate);
>  	}
> 
> +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> +
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
>  			    escr);
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] drm: rcar-du: Improve non-DPLL clock selection

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Monday, 20 August 2018 18:26:17 EEST Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> DU channels not equipped with a DPLL use an internal (aka SoC provided) or

I'd say "SoC internal (provided by the CPG)"

> external clock source combined with an internal divider to generate the

and here "a DU internal divider" to avoid confusion with an SoC internal 
divider outside of the DU.

> desired output dot clock frequency.
> 
> The current clock selection procedure does not fully exploit the ability
> of external clock sources to generate the exact dot clock frequency by
> themselves, but relies instead on tuning the internal DU clock divider
> only, resulting in a less precise clock generation process.
> 
> When possible, and desirable, ask the external clock source for the exact
> output dot clock frequency, and test the returned frequency against the
> internally generated one to better approximate the desired output dot clock.

To make this a big more generic, I' say "and select the clock source that 
produces the frequency closest to the desired output dot clock".

> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> where the DU's input dotclock.in is generated by the versaclock VC5 clock
> source, which is capable of generating the exact rate the DU needs as pixel
> clock output.
> 
> This patch fixes higher resolution modes wich requires an high pixel clock

s/wich/which/

> output currently not working on non-HDMI DU channel (as VGA 1920x1080@60Hz).

Maybe "(such as 1920x1080@60Hz on the VGA output)" ?
> 
> Fixes: 1b30dbde8 "drm: rcar-du: Add support for external pixel clock"

Please use the output of git show --pretty=fixes to generate the fixes line. 
Your SHA1 needs more digits, and the subject should be enclosed with 
parentheses.

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 53 +++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 077e681..98ae697 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -258,42 +258,53 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc)
> 
>  		escr = ESCR_DCLKSEL_DCLKIN | div;
>  	} else {
> -		unsigned long clk;
> +		unsigned long dotclkin_rate;
> +		struct clk *dotclkin_clk;
> +		unsigned long cpg_dist;
>  		u32 div;
> 
>  		/*
>  		 * Compute the clock divisor and select the internal or external
>  		 * dot clock based on the requested frequency.
>  		 */
> -		clk = clk_get_rate(rcrtc->clock);
> -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> -		div = clamp(div, 1U, 64U) - 1;
> -
> +		dotclkin_clk = rcrtc->clock;
> +		dotclkin_rate = clk_get_rate(rcrtc->clock);
> +		div = clamp(DIV_ROUND_CLOSEST(dotclkin_rate, mode_clock),
> +			    1UL, 64UL) - 1;
> +		cpg_dist = abs(dotclkin_rate / (div + 1) - mode_clock);
>  		escr = ESCR_DCLKSEL_CLKS | div;
> 
>  		if (rcrtc->extclock) {
> -			unsigned long extclk;
> -			unsigned long extrate;
> -			unsigned long rate;
> -			u32 extdiv;
> -
> -			extclk = clk_get_rate(rcrtc->extclock);
> -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> +			unsigned long ext_rate;
> +			unsigned long ext_dist;
> 
> -			extrate = extclk / (extdiv + 1);
> -			rate = clk / (div + 1);
> +			/*
> +			 * If an external clock source is present ask it
> +			 * for the exact dot clock rate, and test the
> +			 * returned value against the cpg provided one.
> +			 */
> +			ext_rate = clk_round_rate(rcrtc->extclock,
> +						  mode_clock);
> 
> -			if (abs((long)extrate - (long)mode_clock) <
> -			    abs((long)rate - (long)mode_clock))
> -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> +			div = clamp(DIV_ROUND_CLOSEST(ext_rate, mode_clock),
> +				    1UL, 64UL) - 1;
> +			ext_dist = abs(ext_rate / (div + 1) - mode_clock);
> 
> -			dev_dbg(rcrtc->group->dev->dev,
> -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> -				mode_clock, extrate, rate, escr);
> +			if (ext_dist < cpg_dist) {
> +				dotclkin_clk = rcrtc->extclock;
> +				dotclkin_rate = ext_rate;
> +				escr = ESCR_DCLKSEL_DCLKIN | div;
> +			}
>  		}

I think we can do something much simpler here by factoring some code out to a 
function. I'll send a proposal in reply to this e-mail.

> +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> +			mode_clock, dotclkin_clk == rcrtc->clock ? "cpg" : "ext",
> +			dotclkin_rate);
> +		clk_set_rate(dotclkin_clk, dotclkin_rate);
>  	}
> 
> +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> +
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
>  			    escr);
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: rcar-du: Improve non-DPLL clock selection

[Laurent Pinchart]

From: Jacopo Mondi <jacopo@jmondi.org>

DU channels not equipped with a DPLL use an SoC internal (provided by
the CPG) or external clock source combined with a DU internal divider to
generate the desired output dot clock frequency.

The current clock selection procedure does not fully exploit the ability
of external clock sources to generate the exact dot clock frequency by
themselves, but relies instead on tuning the internal DU clock divider
only, resulting in a less precise clock generation process.

When possible, and desirable, ask the external clock source for the
exact output dot clock frequency, and select the clock source that
produces the frequency closest to the desired output dot clock.

This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
where the DU's input dotclock.in is generated by the versaclock VC5
clock source, which is capable of generating the exact rate the DU needs
as pixel clock output.

This patch fixes higher resolution modes which requires an high pixel
clock output currently not working on non-HDMI DU channel (such as
1920x1080@60Hz on the VGA output).

Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
[Factor out code to a helper function]
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index f8068170905a..2c9405458bbf 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
 		 best_diff);
 }
 
+struct du_clk_params {
+	struct clk *clk;
+	unsigned long rate;
+	unsigned long diff;
+	u32 escr;
+};
+
+static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk *clk,
+				 unsigned long target, u32 escr,
+				 struct du_clk_params *params)
+{
+	unsigned long rate;
+	unsigned long diff;
+	u32 div;
+
+	/*
+	 * If the target rate has already been achieved perfectly we can't do
+	 * better.
+	 */
+	if (params->diff == 0)
+		return;
+
+	/*
+	 * Compute the input clock rate and internal divisor values to obtain
+	 * the clock rate closest to the target frequency.
+	 */
+	rate = clk_round_rate(clk, target);
+	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
+	diff = abs(rate / (div + 1) - target);
+
+	/*
+	 * If the resulting frequency is better than any previously obtained,
+	 * store the parameters.
+	 */
+	if (diff < params->diff) {
+		params->clk = clk;
+		params->rate = rate;
+		params->diff = diff;
+		params->escr = escr | div;
+	}
+}
+
 static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
 	{ .soc_id = "r8a7795", .revision = "ES1.*" },
 	{ /* sentinel */ }
@@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 
 		escr = ESCR_DCLKSEL_DCLKIN | div;
 	} else {
-		unsigned long clk;
-		u32 div;
-
-		/*
-		 * Compute the clock divisor and select the internal or external
-		 * dot clock based on the requested frequency.
-		 */
-		clk = clk_get_rate(rcrtc->clock);
-		div = DIV_ROUND_CLOSEST(clk, mode_clock);
-		div = clamp(div, 1U, 64U) - 1;
-
-		escr = ESCR_DCLKSEL_CLKS | div;
-
-		if (rcrtc->extclock) {
-			unsigned long extclk;
-			unsigned long extrate;
-			unsigned long rate;
-			u32 extdiv;
+		struct du_clk_params params = { .diff = (unsigned long)-1 };
 
-			extclk = clk_get_rate(rcrtc->extclock);
-			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
-			extdiv = clamp(extdiv, 1U, 64U) - 1;
+		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
+				     ESCR_DCLKSEL_CLKS, &params);
+		if (rcrtc->extclock)
+			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
+					     ESCR_DCLKSEL_DCLKIN, &params);
 
-			extrate = extclk / (extdiv + 1);
-			rate = clk / (div + 1);
+		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
+			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
+			params.rate);
 
-			if (abs((long)extrate - (long)mode_clock) <
-			    abs((long)rate - (long)mode_clock))
-				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
-
-			dev_dbg(rcrtc->group->dev->dev,
-				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
-				mode_clock, extrate, rate, escr);
-		}
+		clk_set_rate(params.clk, params.rate);
+		escr = params.escr;
 	}
 
+	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
+
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
 			    escr);
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
-- 
Regards,

Laurent Pinchart

[PATCH] drm: rcar-du: Improve non-DPLL clock selection

[Laurent Pinchart]

From: Jacopo Mondi <jacopo@jmondi.org>

DU channels not equipped with a DPLL use an SoC internal (provided by
the CPG) or external clock source combined with a DU internal divider to
generate the desired output dot clock frequency.

The current clock selection procedure does not fully exploit the ability
of external clock sources to generate the exact dot clock frequency by
themselves, but relies instead on tuning the internal DU clock divider
only, resulting in a less precise clock generation process.

When possible, and desirable, ask the external clock source for the
exact output dot clock frequency, and select the clock source that
produces the frequency closest to the desired output dot clock.

This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
where the DU's input dotclock.in is generated by the versaclock VC5
clock source, which is capable of generating the exact rate the DU needs
as pixel clock output.

This patch fixes higher resolution modes which requires an high pixel
clock output currently not working on non-HDMI DU channel (such as
1920x1080@60Hz on the VGA output).

Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
[Factor out code to a helper function]
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index f8068170905a..2c9405458bbf 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
 		 best_diff);
 }
 
+struct du_clk_params {
+	struct clk *clk;
+	unsigned long rate;
+	unsigned long diff;
+	u32 escr;
+};
+
+static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk *clk,
+				 unsigned long target, u32 escr,
+				 struct du_clk_params *params)
+{
+	unsigned long rate;
+	unsigned long diff;
+	u32 div;
+
+	/*
+	 * If the target rate has already been achieved perfectly we can't do
+	 * better.
+	 */
+	if (params->diff == 0)
+		return;
+
+	/*
+	 * Compute the input clock rate and internal divisor values to obtain
+	 * the clock rate closest to the target frequency.
+	 */
+	rate = clk_round_rate(clk, target);
+	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
+	diff = abs(rate / (div + 1) - target);
+
+	/*
+	 * If the resulting frequency is better than any previously obtained,
+	 * store the parameters.
+	 */
+	if (diff < params->diff) {
+		params->clk = clk;
+		params->rate = rate;
+		params->diff = diff;
+		params->escr = escr | div;
+	}
+}
+
 static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
 	{ .soc_id = "r8a7795", .revision = "ES1.*" },
 	{ /* sentinel */ }
@@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 
 		escr = ESCR_DCLKSEL_DCLKIN | div;
 	} else {
-		unsigned long clk;
-		u32 div;
-
-		/*
-		 * Compute the clock divisor and select the internal or external
-		 * dot clock based on the requested frequency.
-		 */
-		clk = clk_get_rate(rcrtc->clock);
-		div = DIV_ROUND_CLOSEST(clk, mode_clock);
-		div = clamp(div, 1U, 64U) - 1;
-
-		escr = ESCR_DCLKSEL_CLKS | div;
-
-		if (rcrtc->extclock) {
-			unsigned long extclk;
-			unsigned long extrate;
-			unsigned long rate;
-			u32 extdiv;
+		struct du_clk_params params = { .diff = (unsigned long)-1 };
 
-			extclk = clk_get_rate(rcrtc->extclock);
-			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
-			extdiv = clamp(extdiv, 1U, 64U) - 1;
+		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
+				     ESCR_DCLKSEL_CLKS, &params);
+		if (rcrtc->extclock)
+			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
+					     ESCR_DCLKSEL_DCLKIN, &params);
 
-			extrate = extclk / (extdiv + 1);
-			rate = clk / (div + 1);
+		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
+			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
+			params.rate);
 
-			if (abs((long)extrate - (long)mode_clock) <
-			    abs((long)rate - (long)mode_clock))
-				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
-
-			dev_dbg(rcrtc->group->dev->dev,
-				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
-				mode_clock, extrate, rate, escr);
-		}
+		clk_set_rate(params.clk, params.rate);
+		escr = params.escr;
 	}
 
+	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
+
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
 			    escr);
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[Laurent Pinchart]

Hi Jacopo,

On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> DU channels not equipped with a DPLL use an SoC internal (provided by
> the CPG) or external clock source combined with a DU internal divider to
> generate the desired output dot clock frequency.
> 
> The current clock selection procedure does not fully exploit the ability
> of external clock sources to generate the exact dot clock frequency by
> themselves, but relies instead on tuning the internal DU clock divider
> only, resulting in a less precise clock generation process.
> 
> When possible, and desirable, ask the external clock source for the
> exact output dot clock frequency, and select the clock source that
> produces the frequency closest to the desired output dot clock.
> 
> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> where the DU's input dotclock.in is generated by the versaclock VC5
> clock source, which is capable of generating the exact rate the DU needs
> as pixel clock output.
> 
> This patch fixes higher resolution modes which requires an high pixel
> clock output currently not working on non-HDMI DU channel (such as
> 1920x1080@60Hz on the VGA output).

Just for the record, with this patch the following modes (as printed by 
modetest) on the VGA output now produce correct result with my monitor:

  1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync, nvsync
  1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync, pvsync
  1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync

The second mode used to not display at all, with a message telling that 
timings were out of range, and the other two modes used to produce a displayed 
image partly shifted or scaled out of the screen boundaries.

The following modes still produce an image partly out of the screen 
boundaries.

  1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
  1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
  1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
  1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
  1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync

And this one is reported to be out of range.

  1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync, pvsync

There is thus still room for improvement (some of the issues are possibly due 
to my monitor though), but there's also an improvement, and no noticeable 
regression.

> Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> [Factor out code to a helper function]
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index f8068170905a..2c9405458bbf
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> *rcrtc, best_diff);
>  }
> 
> +struct du_clk_params {
> +	struct clk *clk;
> +	unsigned long rate;
> +	unsigned long diff;
> +	u32 escr;
> +};
> +
> +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> *clk, +				 unsigned long target, u32 escr,
> +				 struct du_clk_params *params)
> +{
> +	unsigned long rate;
> +	unsigned long diff;
> +	u32 div;
> +
> +	/*
> +	 * If the target rate has already been achieved perfectly we can't do
> +	 * better.
> +	 */
> +	if (params->diff == 0)
> +		return;
> +
> +	/*
> +	 * Compute the input clock rate and internal divisor values to obtain
> +	 * the clock rate closest to the target frequency.
> +	 */
> +	rate = clk_round_rate(clk, target);
> +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> +	diff = abs(rate / (div + 1) - target);
> +
> +	/*
> +	 * If the resulting frequency is better than any previously obtained,
> +	 * store the parameters.
> +	 */
> +	if (diff < params->diff) {
> +		params->clk = clk;
> +		params->rate = rate;
> +		params->diff = diff;
> +		params->escr = escr | div;
> +	}
> +}
> +
>  static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
>  	{ .soc_id = "r8a7795", .revision = "ES1.*" },
>  	{ /* sentinel */ }
> @@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc)
> 
>  		escr = ESCR_DCLKSEL_DCLKIN | div;
>  	} else {
> -		unsigned long clk;
> -		u32 div;
> -
> -		/*
> -		 * Compute the clock divisor and select the internal or external
> -		 * dot clock based on the requested frequency.
> -		 */
> -		clk = clk_get_rate(rcrtc->clock);
> -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> -		div = clamp(div, 1U, 64U) - 1;
> -
> -		escr = ESCR_DCLKSEL_CLKS | div;
> -
> -		if (rcrtc->extclock) {
> -			unsigned long extclk;
> -			unsigned long extrate;
> -			unsigned long rate;
> -			u32 extdiv;
> +		struct du_clk_params params = { .diff = (unsigned long)-1 };
> 
> -			extclk = clk_get_rate(rcrtc->extclock);
> -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> +		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
> +				     ESCR_DCLKSEL_CLKS, &params);
> +		if (rcrtc->extclock)
> +			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> +					     ESCR_DCLKSEL_DCLKIN, &params);
> 
> -			extrate = extclk / (extdiv + 1);
> -			rate = clk / (div + 1);
> +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> +			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
> +			params.rate);
> 
> -			if (abs((long)extrate - (long)mode_clock) <
> -			    abs((long)rate - (long)mode_clock))
> -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> -
> -			dev_dbg(rcrtc->group->dev->dev,
> -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> -				mode_clock, extrate, rate, escr);
> -		}
> +		clk_set_rate(params.clk, params.rate);
> +		escr = params.escr;
>  	}
> 
> +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> +
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
>  			    escr);
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[Laurent Pinchart]

Hi Jacopo,

On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> DU channels not equipped with a DPLL use an SoC internal (provided by
> the CPG) or external clock source combined with a DU internal divider to
> generate the desired output dot clock frequency.
> 
> The current clock selection procedure does not fully exploit the ability
> of external clock sources to generate the exact dot clock frequency by
> themselves, but relies instead on tuning the internal DU clock divider
> only, resulting in a less precise clock generation process.
> 
> When possible, and desirable, ask the external clock source for the
> exact output dot clock frequency, and select the clock source that
> produces the frequency closest to the desired output dot clock.
> 
> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> where the DU's input dotclock.in is generated by the versaclock VC5
> clock source, which is capable of generating the exact rate the DU needs
> as pixel clock output.
> 
> This patch fixes higher resolution modes which requires an high pixel
> clock output currently not working on non-HDMI DU channel (such as
> 1920x1080@60Hz on the VGA output).

Just for the record, with this patch the following modes (as printed by 
modetest) on the VGA output now produce correct result with my monitor:

  1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync, nvsync
  1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync, pvsync
  1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync

The second mode used to not display at all, with a message telling that 
timings were out of range, and the other two modes used to produce a displayed 
image partly shifted or scaled out of the screen boundaries.

The following modes still produce an image partly out of the screen 
boundaries.

  1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
  1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
  1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
  1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
  1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync

And this one is reported to be out of range.

  1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync, pvsync

There is thus still room for improvement (some of the issues are possibly due 
to my monitor though), but there's also an improvement, and no noticeable 
regression.

> Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> [Factor out code to a helper function]
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index f8068170905a..2c9405458bbf
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> *rcrtc, best_diff);
>  }
> 
> +struct du_clk_params {
> +	struct clk *clk;
> +	unsigned long rate;
> +	unsigned long diff;
> +	u32 escr;
> +};
> +
> +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> *clk, +				 unsigned long target, u32 escr,
> +				 struct du_clk_params *params)
> +{
> +	unsigned long rate;
> +	unsigned long diff;
> +	u32 div;
> +
> +	/*
> +	 * If the target rate has already been achieved perfectly we can't do
> +	 * better.
> +	 */
> +	if (params->diff == 0)
> +		return;
> +
> +	/*
> +	 * Compute the input clock rate and internal divisor values to obtain
> +	 * the clock rate closest to the target frequency.
> +	 */
> +	rate = clk_round_rate(clk, target);
> +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> +	diff = abs(rate / (div + 1) - target);
> +
> +	/*
> +	 * If the resulting frequency is better than any previously obtained,
> +	 * store the parameters.
> +	 */
> +	if (diff < params->diff) {
> +		params->clk = clk;
> +		params->rate = rate;
> +		params->diff = diff;
> +		params->escr = escr | div;
> +	}
> +}
> +
>  static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
>  	{ .soc_id = "r8a7795", .revision = "ES1.*" },
>  	{ /* sentinel */ }
> @@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc)
> 
>  		escr = ESCR_DCLKSEL_DCLKIN | div;
>  	} else {
> -		unsigned long clk;
> -		u32 div;
> -
> -		/*
> -		 * Compute the clock divisor and select the internal or external
> -		 * dot clock based on the requested frequency.
> -		 */
> -		clk = clk_get_rate(rcrtc->clock);
> -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> -		div = clamp(div, 1U, 64U) - 1;
> -
> -		escr = ESCR_DCLKSEL_CLKS | div;
> -
> -		if (rcrtc->extclock) {
> -			unsigned long extclk;
> -			unsigned long extrate;
> -			unsigned long rate;
> -			u32 extdiv;
> +		struct du_clk_params params = { .diff = (unsigned long)-1 };
> 
> -			extclk = clk_get_rate(rcrtc->extclock);
> -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> +		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
> +				     ESCR_DCLKSEL_CLKS, &params);
> +		if (rcrtc->extclock)
> +			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> +					     ESCR_DCLKSEL_DCLKIN, &params);
> 
> -			extrate = extclk / (extdiv + 1);
> -			rate = clk / (div + 1);
> +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> +			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
> +			params.rate);
> 
> -			if (abs((long)extrate - (long)mode_clock) <
> -			    abs((long)rate - (long)mode_clock))
> -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> -
> -			dev_dbg(rcrtc->group->dev->dev,
> -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> -				mode_clock, extrate, rate, escr);
> -		}
> +		clk_set_rate(params.clk, params.rate);
> +		escr = params.escr;
>  	}
> 
> +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> +
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
>  			    escr);
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[jacopo mondi]

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

HI Laurent,
   thanks for the patch rework

On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > DU channels not equipped with a DPLL use an SoC internal (provided by
> > the CPG) or external clock source combined with a DU internal divider to
> > generate the desired output dot clock frequency.
> >
> > The current clock selection procedure does not fully exploit the ability
> > of external clock sources to generate the exact dot clock frequency by
> > themselves, but relies instead on tuning the internal DU clock divider
> > only, resulting in a less precise clock generation process.
> >
> > When possible, and desirable, ask the external clock source for the
> > exact output dot clock frequency, and select the clock source that
> > produces the frequency closest to the desired output dot clock.
> >
> > This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> > where the DU's input dotclock.in is generated by the versaclock VC5
> > clock source, which is capable of generating the exact rate the DU needs
> > as pixel clock output.
> >
> > This patch fixes higher resolution modes which requires an high pixel
> > clock output currently not working on non-HDMI DU channel (such as
> > 1920x1080@60Hz on the VGA output).
>
> Just for the record, with this patch the following modes (as printed by
> modetest) on the VGA output now produce correct result with my monitor:
>
>   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync, nvsync
>   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync, pvsync
>   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
>
> The second mode used to not display at all, with a message telling that
> timings were out of range, and the other two modes used to produce a displayed
> image partly shifted or scaled out of the screen boundaries.
>
> The following modes still produce an image partly out of the screen
> boundaries.
>
>   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
>   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
>   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
>   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
>   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
>
> And this one is reported to be out of range.
>
>   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync, pvsync
>
> There is thus still room for improvement (some of the issues are possibly due
> to my monitor though), but there's also an improvement, and no noticeable
> regression.
>
> > Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > [Factor out code to a helper function]
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++----------
> >  1 file changed, 55 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index f8068170905a..2c9405458bbf
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> > *rcrtc, best_diff);
> >  }
> >
> > +struct du_clk_params {
> > +	struct clk *clk;
> > +	unsigned long rate;
> > +	unsigned long diff;
> > +	u32 escr;
> > +};
> > +
> > +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> > *clk, +				 unsigned long target, u32 escr,
> > +				 struct du_clk_params *params)

I don't see the rcrtc parameter ever being used in this function.
Do you want to keep it anyhow?

> > +{
> > +	unsigned long rate;
> > +	unsigned long diff;
> > +	u32 div;
> > +
> > +	/*
> > +	 * If the target rate has already been achieved perfectly we can't do
> > +	 * better.
> > +	 */
> > +	if (params->diff == 0)
> > +		return;
> > +
> > +	/*
> > +	 * Compute the input clock rate and internal divisor values to obtain
> > +	 * the clock rate closest to the target frequency.
> > +	 */
> > +	rate = clk_round_rate(clk, target);
> > +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> > +	diff = abs(rate / (div + 1) - target);
> > +
> > +	/*
> > +	 * If the resulting frequency is better than any previously obtained,

s/obtained,/obtained one,/ ?

Will get back with some testing results on a different VGA monitor...

Thanks
  j

> > +	 * store the parameters.
> > +	 */
> > +	if (diff < params->diff) {
> > +		params->clk = clk;
> > +		params->rate = rate;
> > +		params->diff = diff;
> > +		params->escr = escr | div;
> > +	}
> > +}
> > +
> >  static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> >  	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> >  	{ /* sentinel */ }
> > @@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)
> >
> >  		escr = ESCR_DCLKSEL_DCLKIN | div;
> >  	} else {
> > -		unsigned long clk;
> > -		u32 div;
> > -
> > -		/*
> > -		 * Compute the clock divisor and select the internal or external
> > -		 * dot clock based on the requested frequency.
> > -		 */
> > -		clk = clk_get_rate(rcrtc->clock);
> > -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> > -		div = clamp(div, 1U, 64U) - 1;
> > -
> > -		escr = ESCR_DCLKSEL_CLKS | div;
> > -
> > -		if (rcrtc->extclock) {
> > -			unsigned long extclk;
> > -			unsigned long extrate;
> > -			unsigned long rate;
> > -			u32 extdiv;
> > +		struct du_clk_params params = { .diff = (unsigned long)-1 };
> >
> > -			extclk = clk_get_rate(rcrtc->extclock);
> > -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> > -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> > +		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
> > +				     ESCR_DCLKSEL_CLKS, &params);
> > +		if (rcrtc->extclock)
> > +			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> > +					     ESCR_DCLKSEL_DCLKIN, &params);
> >
> > -			extrate = extclk / (extdiv + 1);
> > -			rate = clk / (div + 1);
> > +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> > +			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
> > +			params.rate);
> >
> > -			if (abs((long)extrate - (long)mode_clock) <
> > -			    abs((long)rate - (long)mode_clock))
> > -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> > -
> > -			dev_dbg(rcrtc->group->dev->dev,
> > -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> > -				mode_clock, extrate, rate, escr);
> > -		}
> > +		clk_set_rate(params.clk, params.rate);
> > +		escr = params.escr;
> >  	}
> >
> > +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> > +
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> >  			    escr);
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 7097 bytes --]

HI Laurent,
   thanks for the patch rework

On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > DU channels not equipped with a DPLL use an SoC internal (provided by
> > the CPG) or external clock source combined with a DU internal divider to
> > generate the desired output dot clock frequency.
> >
> > The current clock selection procedure does not fully exploit the ability
> > of external clock sources to generate the exact dot clock frequency by
> > themselves, but relies instead on tuning the internal DU clock divider
> > only, resulting in a less precise clock generation process.
> >
> > When possible, and desirable, ask the external clock source for the
> > exact output dot clock frequency, and select the clock source that
> > produces the frequency closest to the desired output dot clock.
> >
> > This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> > where the DU's input dotclock.in is generated by the versaclock VC5
> > clock source, which is capable of generating the exact rate the DU needs
> > as pixel clock output.
> >
> > This patch fixes higher resolution modes which requires an high pixel
> > clock output currently not working on non-HDMI DU channel (such as
> > 1920x1080@60Hz on the VGA output).
>
> Just for the record, with this patch the following modes (as printed by
> modetest) on the VGA output now produce correct result with my monitor:
>
>   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync, nvsync
>   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync, pvsync
>   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
>
> The second mode used to not display at all, with a message telling that
> timings were out of range, and the other two modes used to produce a displayed
> image partly shifted or scaled out of the screen boundaries.
>
> The following modes still produce an image partly out of the screen
> boundaries.
>
>   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
>   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
>   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
>   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
>   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
>
> And this one is reported to be out of range.
>
>   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync, pvsync
>
> There is thus still room for improvement (some of the issues are possibly due
> to my monitor though), but there's also an improvement, and no noticeable
> regression.
>
> > Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > [Factor out code to a helper function]
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++----------
> >  1 file changed, 55 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index f8068170905a..2c9405458bbf
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> > *rcrtc, best_diff);
> >  }
> >
> > +struct du_clk_params {
> > +	struct clk *clk;
> > +	unsigned long rate;
> > +	unsigned long diff;
> > +	u32 escr;
> > +};
> > +
> > +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> > *clk, +				 unsigned long target, u32 escr,
> > +				 struct du_clk_params *params)

I don't see the rcrtc parameter ever being used in this function.
Do you want to keep it anyhow?

> > +{
> > +	unsigned long rate;
> > +	unsigned long diff;
> > +	u32 div;
> > +
> > +	/*
> > +	 * If the target rate has already been achieved perfectly we can't do
> > +	 * better.
> > +	 */
> > +	if (params->diff == 0)
> > +		return;
> > +
> > +	/*
> > +	 * Compute the input clock rate and internal divisor values to obtain
> > +	 * the clock rate closest to the target frequency.
> > +	 */
> > +	rate = clk_round_rate(clk, target);
> > +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> > +	diff = abs(rate / (div + 1) - target);
> > +
> > +	/*
> > +	 * If the resulting frequency is better than any previously obtained,

s/obtained,/obtained one,/ ?

Will get back with some testing results on a different VGA monitor...

Thanks
  j

> > +	 * store the parameters.
> > +	 */
> > +	if (diff < params->diff) {
> > +		params->clk = clk;
> > +		params->rate = rate;
> > +		params->diff = diff;
> > +		params->escr = escr | div;
> > +	}
> > +}
> > +
> >  static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> >  	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> >  	{ /* sentinel */ }
> > @@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)
> >
> >  		escr = ESCR_DCLKSEL_DCLKIN | div;
> >  	} else {
> > -		unsigned long clk;
> > -		u32 div;
> > -
> > -		/*
> > -		 * Compute the clock divisor and select the internal or external
> > -		 * dot clock based on the requested frequency.
> > -		 */
> > -		clk = clk_get_rate(rcrtc->clock);
> > -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> > -		div = clamp(div, 1U, 64U) - 1;
> > -
> > -		escr = ESCR_DCLKSEL_CLKS | div;
> > -
> > -		if (rcrtc->extclock) {
> > -			unsigned long extclk;
> > -			unsigned long extrate;
> > -			unsigned long rate;
> > -			u32 extdiv;
> > +		struct du_clk_params params = { .diff = (unsigned long)-1 };
> >
> > -			extclk = clk_get_rate(rcrtc->extclock);
> > -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> > -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> > +		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
> > +				     ESCR_DCLKSEL_CLKS, &params);
> > +		if (rcrtc->extclock)
> > +			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> > +					     ESCR_DCLKSEL_DCLKIN, &params);
> >
> > -			extrate = extclk / (extdiv + 1);
> > -			rate = clk / (div + 1);
> > +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> > +			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
> > +			params.rate);
> >
> > -			if (abs((long)extrate - (long)mode_clock) <
> > -			    abs((long)rate - (long)mode_clock))
> > -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> > -
> > -			dev_dbg(rcrtc->group->dev->dev,
> > -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> > -				mode_clock, extrate, rate, escr);
> > -		}
> > +		clk_set_rate(params.clk, params.rate);
> > +		escr = params.escr;
> >  	}
> >
> > +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> > +
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> >  			    escr);
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[Laurent Pinchart]

Hi Jacopo,

(CC'ing Kieran)

On Tuesday, 21 August 2018 10:33:57 EEST jacopo mondi wrote:
> On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> > On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> >> From: Jacopo Mondi <jacopo@jmondi.org>
> >> 
> >> DU channels not equipped with a DPLL use an SoC internal (provided by
> >> the CPG) or external clock source combined with a DU internal divider to
> >> generate the desired output dot clock frequency.
> >> 
> >> The current clock selection procedure does not fully exploit the ability
> >> of external clock sources to generate the exact dot clock frequency by
> >> themselves, but relies instead on tuning the internal DU clock divider
> >> only, resulting in a less precise clock generation process.
> >> 
> >> When possible, and desirable, ask the external clock source for the
> >> exact output dot clock frequency, and select the clock source that
> >> produces the frequency closest to the desired output dot clock.
> >> 
> >> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> >> where the DU's input dotclock.in is generated by the versaclock VC5
> >> clock source, which is capable of generating the exact rate the DU needs
> >> as pixel clock output.
> >> 
> >> This patch fixes higher resolution modes which requires an high pixel
> >> clock output currently not working on non-HDMI DU channel (such as
> >> 1920x1080@60Hz on the VGA output).
> > 
> > Just for the record, with this patch the following modes (as printed by
> > 
> > modetest) on the VGA output now produce correct result with my monitor:
> >   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync,
> >   nvsync
> >   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync,
> >   pvsync
> >   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
> > 
> > The second mode used to not display at all, with a message telling that
> > timings were out of range, and the other two modes used to produce a
> > displayed image partly shifted or scaled out of the screen boundaries.
> > 
> > The following modes still produce an image partly out of the screen
> > boundaries.
> > 
> >   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
> >   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
> >   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
> >   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
> >   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
> > 
> > And this one is reported to be out of range.
> > 
> >   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync,
> >   pvsync
> > 
> > There is thus still room for improvement (some of the issues are possibly
> > due to my monitor though), but there's also an improvement, and no
> > noticeable regression.
> > 
> >> Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel
> >> clock")
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> [Factor out code to a helper function]
> >> Signed-off-by: Laurent Pinchart
> >> <laurent.pinchart+renesas@ideasonboard.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 +++++++++++++++++++---------
> >>  1 file changed, 55 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index
> >> f8068170905a..2c9405458bbf
> >> 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct
> >> rcar_du_crtc *rcrtc,
> >>  	best_diff);
> >>  }
> >> 
> >> +struct du_clk_params {
> >> +	struct clk *clk;
> >> +	unsigned long rate;
> >> +	unsigned long diff;
> >> +	u32 escr;
> >> +};
> >> +
> >> +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> >> *clk,
> >> +				 unsigned long target, u32 escr,
> >> +				 struct du_clk_params *params)
> 
> I don't see the rcrtc parameter ever being used in this function.
> Do you want to keep it anyhow?

You're right, I'll remove it.

> >> +{
> >> +	unsigned long rate;
> >> +	unsigned long diff;
> >> +	u32 div;
> >> +
> >> +	/*
> >> +	 * If the target rate has already been achieved perfectly we can't do
> >> +	 * better.
> >> +	 */
> >> +	if (params->diff == 0)
> >> +		return;
> >> +
> >> +	/*
> >> +	 * Compute the input clock rate and internal divisor values to obtain
> >> +	 * the clock rate closest to the target frequency.
> >> +	 */
> >> +	rate = clk_round_rate(clk, target);
> >> +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> >> +	diff = abs(rate / (div + 1) - target);
> >> +
> >> +	/*
> >> +	 * If the resulting frequency is better than any previously obtained,
> 
> s/obtained,/obtained one,/ ?

Any opinion from a native English speaker ? :-)

> Will get back with some testing results on a different VGA monitor...

Thank you.

> >> +	 * store the parameters.
> >> +	 */
> >> +	if (diff < params->diff) {
> >> +		params->clk = clk;
> >> +		params->rate = rate;
> >> +		params->diff = diff;
> >> +		params->escr = escr | div;
> >> +	}
> >> +}

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[Laurent Pinchart]

Hi Jacopo,

(CC'ing Kieran)

On Tuesday, 21 August 2018 10:33:57 EEST jacopo mondi wrote:
> On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> > On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> >> From: Jacopo Mondi <jacopo@jmondi.org>
> >> 
> >> DU channels not equipped with a DPLL use an SoC internal (provided by
> >> the CPG) or external clock source combined with a DU internal divider to
> >> generate the desired output dot clock frequency.
> >> 
> >> The current clock selection procedure does not fully exploit the ability
> >> of external clock sources to generate the exact dot clock frequency by
> >> themselves, but relies instead on tuning the internal DU clock divider
> >> only, resulting in a less precise clock generation process.
> >> 
> >> When possible, and desirable, ask the external clock source for the
> >> exact output dot clock frequency, and select the clock source that
> >> produces the frequency closest to the desired output dot clock.
> >> 
> >> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> >> where the DU's input dotclock.in is generated by the versaclock VC5
> >> clock source, which is capable of generating the exact rate the DU needs
> >> as pixel clock output.
> >> 
> >> This patch fixes higher resolution modes which requires an high pixel
> >> clock output currently not working on non-HDMI DU channel (such as
> >> 1920x1080@60Hz on the VGA output).
> > 
> > Just for the record, with this patch the following modes (as printed by
> > 
> > modetest) on the VGA output now produce correct result with my monitor:
> >   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync,
> >   nvsync
> >   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync,
> >   pvsync
> >   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
> > 
> > The second mode used to not display at all, with a message telling that
> > timings were out of range, and the other two modes used to produce a
> > displayed image partly shifted or scaled out of the screen boundaries.
> > 
> > The following modes still produce an image partly out of the screen
> > boundaries.
> > 
> >   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
> >   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
> >   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
> >   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
> >   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
> > 
> > And this one is reported to be out of range.
> > 
> >   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync,
> >   pvsync
> > 
> > There is thus still room for improvement (some of the issues are possibly
> > due to my monitor though), but there's also an improvement, and no
> > noticeable regression.
> > 
> >> Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel
> >> clock")
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> [Factor out code to a helper function]
> >> Signed-off-by: Laurent Pinchart
> >> <laurent.pinchart+renesas@ideasonboard.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 +++++++++++++++++++---------
> >>  1 file changed, 55 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index
> >> f8068170905a..2c9405458bbf
> >> 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct
> >> rcar_du_crtc *rcrtc,
> >>  	best_diff);
> >>  }
> >> 
> >> +struct du_clk_params {
> >> +	struct clk *clk;
> >> +	unsigned long rate;
> >> +	unsigned long diff;
> >> +	u32 escr;
> >> +};
> >> +
> >> +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> >> *clk,
> >> +				 unsigned long target, u32 escr,
> >> +				 struct du_clk_params *params)
> 
> I don't see the rcrtc parameter ever being used in this function.
> Do you want to keep it anyhow?

You're right, I'll remove it.

> >> +{
> >> +	unsigned long rate;
> >> +	unsigned long diff;
> >> +	u32 div;
> >> +
> >> +	/*
> >> +	 * If the target rate has already been achieved perfectly we can't do
> >> +	 * better.
> >> +	 */
> >> +	if (params->diff == 0)
> >> +		return;
> >> +
> >> +	/*
> >> +	 * Compute the input clock rate and internal divisor values to obtain
> >> +	 * the clock rate closest to the target frequency.
> >> +	 */
> >> +	rate = clk_round_rate(clk, target);
> >> +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> >> +	diff = abs(rate / (div + 1) - target);
> >> +
> >> +	/*
> >> +	 * If the resulting frequency is better than any previously obtained,
> 
> s/obtained,/obtained one,/ ?

Any opinion from a native English speaker ? :-)

> Will get back with some testing results on a different VGA monitor...

Thank you.

> >> +	 * store the parameters.
> >> +	 */
> >> +	if (diff < params->diff) {
> >> +		params->clk = clk;
> >> +		params->rate = rate;
> >> +		params->diff = diff;
> >> +		params->escr = escr | div;
> >> +	}
> >> +}

[snip]

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[jacopo mondi]

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

Hi Laurent,
   I run some tests, and here below there's a summary of what I see

On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > DU channels not equipped with a DPLL use an SoC internal (provided by
> > the CPG) or external clock source combined with a DU internal divider to
> > generate the desired output dot clock frequency.
> >
> > The current clock selection procedure does not fully exploit the ability
> > of external clock sources to generate the exact dot clock frequency by
> > themselves, but relies instead on tuning the internal DU clock divider
> > only, resulting in a less precise clock generation process.
> >
> > When possible, and desirable, ask the external clock source for the
> > exact output dot clock frequency, and select the clock source that
> > produces the frequency closest to the desired output dot clock.
> >
> > This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> > where the DU's input dotclock.in is generated by the versaclock VC5
> > clock source, which is capable of generating the exact rate the DU needs
> > as pixel clock output.
> >
> > This patch fixes higher resolution modes which requires an high pixel
> > clock output currently not working on non-HDMI DU channel (such as
> > 1920x1080@60Hz on the VGA output).
>
> Just for the record, with this patch the following modes (as printed by
> modetest) on the VGA output now produce correct result with my monitor:
>
>   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync, nvsync
>   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync, pvsync
>   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
>
> The second mode used to not display at all, with a message telling that
> timings were out of range, and the other two modes used to produce a displayed
> image partly shifted or scaled out of the screen boundaries.
>
> The following modes still produce an image partly out of the screen
> boundaries.
>
>   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
>   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
>   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
>   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
>   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
>
> And this one is reported to be out of range.
>
>   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync, pvsync
>
> There is thus still room for improvement (some of the issues are possibly due
> to my monitor though), but there's also an improvement, and no noticeable
> regression.

The following table compares results obtained with the latest
renesas-drivers, with and without this series applied on top.

-------------------------------------------------------------------------------
Legend:
	A = image badly aligned: not all 4 blue/red borders visible
	F = flickering: disturbance in the shown image
	B = broken: mode is not displayed

Results: I = improvement
	 R = regression

	   renesas-drivers   du_clk		result
1024x768
1920x1200	F		A		I
1920x1200	A		A
1920x1080	A		A
1600x1200	B				I
1680x1050
1680x1050
1400x1050
1400x1050
1600x900	A		A
1280x1024
1440x900
1440x900
1280x960
1366x768	A		A
1366x768	A		A
1360x768			A		R
1280x800
1280x768	A		A
1280x768	A		A
1280x768	A		A
1280x720	A		A
800x600
800x600
848x480
640x480
-------------------------------------------------------------------------------

Overall I see two modes that were broken or unusable due to flickering
(1600x1200 and 1920x1200 respectively) to be now (almost) fixed.

There are visible alignement problems on some modes on both versions,
but I only see one 'regression' (the last 1360x768 that is now
slightly not aligned).

I guess monitors play a role here, with each one being different, but
overall I guess our test results match.


>
> > Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > [Factor out code to a helper function]
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++----------
> >  1 file changed, 55 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index f8068170905a..2c9405458bbf
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> > *rcrtc, best_diff);
> >  }
> >
> > +struct du_clk_params {
> > +	struct clk *clk;
> > +	unsigned long rate;
> > +	unsigned long diff;
> > +	u32 escr;
> > +};
> > +
> > +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> > *clk, +				 unsigned long target, u32 escr,
> > +				 struct du_clk_params *params)
> > +{
> > +	unsigned long rate;
> > +	unsigned long diff;
> > +	u32 div;
> > +
> > +	/*
> > +	 * If the target rate has already been achieved perfectly we can't do
> > +	 * better.
> > +	 */
> > +	if (params->diff == 0)
> > +		return;
> > +
> > +	/*
> > +	 * Compute the input clock rate and internal divisor values to obtain
> > +	 * the clock rate closest to the target frequency.
> > +	 */
> > +	rate = clk_round_rate(clk, target);
> > +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> > +	diff = abs(rate / (div + 1) - target);
> > +
> > +	/*
> > +	 * If the resulting frequency is better than any previously obtained,
> > +	 * store the parameters.
> > +	 */
> > +	if (diff < params->diff) {
> > +		params->clk = clk;
> > +		params->rate = rate;
> > +		params->diff = diff;
> > +		params->escr = escr | div;
> > +	}
> > +}
> > +
> >  static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> >  	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> >  	{ /* sentinel */ }
> > @@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)
> >
> >  		escr = ESCR_DCLKSEL_DCLKIN | div;
> >  	} else {
> > -		unsigned long clk;
> > -		u32 div;
> > -
> > -		/*
> > -		 * Compute the clock divisor and select the internal or external
> > -		 * dot clock based on the requested frequency.
> > -		 */
> > -		clk = clk_get_rate(rcrtc->clock);
> > -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> > -		div = clamp(div, 1U, 64U) - 1;
> > -
> > -		escr = ESCR_DCLKSEL_CLKS | div;
> > -
> > -		if (rcrtc->extclock) {
> > -			unsigned long extclk;
> > -			unsigned long extrate;
> > -			unsigned long rate;
> > -			u32 extdiv;
> > +		struct du_clk_params params = { .diff = (unsigned long)-1 };
> >
> > -			extclk = clk_get_rate(rcrtc->extclock);
> > -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> > -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> > +		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
> > +				     ESCR_DCLKSEL_CLKS, &params);
> > +		if (rcrtc->extclock)
> > +			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> > +					     ESCR_DCLKSEL_DCLKIN, &params);
> >
> > -			extrate = extclk / (extdiv + 1);
> > -			rate = clk / (div + 1);
> > +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> > +			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
> > +			params.rate);
> >
> > -			if (abs((long)extrate - (long)mode_clock) <
> > -			    abs((long)rate - (long)mode_clock))
> > -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> > -
> > -			dev_dbg(rcrtc->group->dev->dev,
> > -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> > -				mode_clock, extrate, rate, escr);
> > -		}
> > +		clk_set_rate(params.clk, params.rate);
> > +		escr = params.escr;
> >  	}
> >
> > +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> > +
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> >  			    escr);
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 8153 bytes --]

Hi Laurent,
   I run some tests, and here below there's a summary of what I see

On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > DU channels not equipped with a DPLL use an SoC internal (provided by
> > the CPG) or external clock source combined with a DU internal divider to
> > generate the desired output dot clock frequency.
> >
> > The current clock selection procedure does not fully exploit the ability
> > of external clock sources to generate the exact dot clock frequency by
> > themselves, but relies instead on tuning the internal DU clock divider
> > only, resulting in a less precise clock generation process.
> >
> > When possible, and desirable, ask the external clock source for the
> > exact output dot clock frequency, and select the clock source that
> > produces the frequency closest to the desired output dot clock.
> >
> > This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> > where the DU's input dotclock.in is generated by the versaclock VC5
> > clock source, which is capable of generating the exact rate the DU needs
> > as pixel clock output.
> >
> > This patch fixes higher resolution modes which requires an high pixel
> > clock output currently not working on non-HDMI DU channel (such as
> > 1920x1080@60Hz on the VGA output).
>
> Just for the record, with this patch the following modes (as printed by
> modetest) on the VGA output now produce correct result with my monitor:
>
>   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync, nvsync
>   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync, pvsync
>   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
>
> The second mode used to not display at all, with a message telling that
> timings were out of range, and the other two modes used to produce a displayed
> image partly shifted or scaled out of the screen boundaries.
>
> The following modes still produce an image partly out of the screen
> boundaries.
>
>   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
>   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
>   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
>   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
>   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
>
> And this one is reported to be out of range.
>
>   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync, pvsync
>
> There is thus still room for improvement (some of the issues are possibly due
> to my monitor though), but there's also an improvement, and no noticeable
> regression.

The following table compares results obtained with the latest
renesas-drivers, with and without this series applied on top.

-------------------------------------------------------------------------------
Legend:
	A = image badly aligned: not all 4 blue/red borders visible
	F = flickering: disturbance in the shown image
	B = broken: mode is not displayed

Results: I = improvement
	 R = regression

	   renesas-drivers   du_clk		result
1024x768
1920x1200	F		A		I
1920x1200	A		A
1920x1080	A		A
1600x1200	B				I
1680x1050
1680x1050
1400x1050
1400x1050
1600x900	A		A
1280x1024
1440x900
1440x900
1280x960
1366x768	A		A
1366x768	A		A
1360x768			A		R
1280x800
1280x768	A		A
1280x768	A		A
1280x768	A		A
1280x720	A		A
800x600
800x600
848x480
640x480
-------------------------------------------------------------------------------

Overall I see two modes that were broken or unusable due to flickering
(1600x1200 and 1920x1200 respectively) to be now (almost) fixed.

There are visible alignement problems on some modes on both versions,
but I only see one 'regression' (the last 1360x768 that is now
slightly not aligned).

I guess monitors play a role here, with each one being different, but
overall I guess our test results match.


>
> > Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > [Factor out code to a helper function]
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++----------
> >  1 file changed, 55 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index f8068170905a..2c9405458bbf
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> > *rcrtc, best_diff);
> >  }
> >
> > +struct du_clk_params {
> > +	struct clk *clk;
> > +	unsigned long rate;
> > +	unsigned long diff;
> > +	u32 escr;
> > +};
> > +
> > +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> > *clk, +				 unsigned long target, u32 escr,
> > +				 struct du_clk_params *params)
> > +{
> > +	unsigned long rate;
> > +	unsigned long diff;
> > +	u32 div;
> > +
> > +	/*
> > +	 * If the target rate has already been achieved perfectly we can't do
> > +	 * better.
> > +	 */
> > +	if (params->diff == 0)
> > +		return;
> > +
> > +	/*
> > +	 * Compute the input clock rate and internal divisor values to obtain
> > +	 * the clock rate closest to the target frequency.
> > +	 */
> > +	rate = clk_round_rate(clk, target);
> > +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> > +	diff = abs(rate / (div + 1) - target);
> > +
> > +	/*
> > +	 * If the resulting frequency is better than any previously obtained,
> > +	 * store the parameters.
> > +	 */
> > +	if (diff < params->diff) {
> > +		params->clk = clk;
> > +		params->rate = rate;
> > +		params->diff = diff;
> > +		params->escr = escr | div;
> > +	}
> > +}
> > +
> >  static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> >  	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> >  	{ /* sentinel */ }
> > @@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)
> >
> >  		escr = ESCR_DCLKSEL_DCLKIN | div;
> >  	} else {
> > -		unsigned long clk;
> > -		u32 div;
> > -
> > -		/*
> > -		 * Compute the clock divisor and select the internal or external
> > -		 * dot clock based on the requested frequency.
> > -		 */
> > -		clk = clk_get_rate(rcrtc->clock);
> > -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> > -		div = clamp(div, 1U, 64U) - 1;
> > -
> > -		escr = ESCR_DCLKSEL_CLKS | div;
> > -
> > -		if (rcrtc->extclock) {
> > -			unsigned long extclk;
> > -			unsigned long extrate;
> > -			unsigned long rate;
> > -			u32 extdiv;
> > +		struct du_clk_params params = { .diff = (unsigned long)-1 };
> >
> > -			extclk = clk_get_rate(rcrtc->extclock);
> > -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> > -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> > +		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
> > +				     ESCR_DCLKSEL_CLKS, &params);
> > +		if (rcrtc->extclock)
> > +			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> > +					     ESCR_DCLKSEL_DCLKIN, &params);
> >
> > -			extrate = extclk / (extdiv + 1);
> > -			rate = clk / (div + 1);
> > +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> > +			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
> > +			params.rate);
> >
> > -			if (abs((long)extrate - (long)mode_clock) <
> > -			    abs((long)rate - (long)mode_clock))
> > -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> > -
> > -			dev_dbg(rcrtc->group->dev->dev,
> > -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> > -				mode_clock, extrate, rate, escr);
> > -		}
> > +		clk_set_rate(params.clk, params.rate);
> > +		escr = params.escr;
> >  	}
> >
> > +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> > +
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> >  			    escr);
> >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[jacopo mondi]

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

And for the records, I forgot to add

On Tue, Aug 21, 2018 at 12:35:48PM +0200, jacopo mondi wrote:
> Hi Laurent,
>    I run some tests, and here below there's a summary of what I see
>
> On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> > > From: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > DU channels not equipped with a DPLL use an SoC internal (provided by
> > > the CPG) or external clock source combined with a DU internal divider to
> > > generate the desired output dot clock frequency.
> > >
> > > The current clock selection procedure does not fully exploit the ability
> > > of external clock sources to generate the exact dot clock frequency by
> > > themselves, but relies instead on tuning the internal DU clock divider
> > > only, resulting in a less precise clock generation process.
> > >
> > > When possible, and desirable, ask the external clock source for the
> > > exact output dot clock frequency, and select the clock source that
> > > produces the frequency closest to the desired output dot clock.
> > >
> > > This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> > > where the DU's input dotclock.in is generated by the versaclock VC5
> > > clock source, which is capable of generating the exact rate the DU needs
> > > as pixel clock output.
> > >
> > > This patch fixes higher resolution modes which requires an high pixel
> > > clock output currently not working on non-HDMI DU channel (such as
> > > 1920x1080@60Hz on the VGA output).
> >
> > Just for the record, with this patch the following modes (as printed by
> > modetest) on the VGA output now produce correct result with my monitor:
> >
> >   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync, nvsync
> >   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync, pvsync
> >   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
> >
> > The second mode used to not display at all, with a message telling that
> > timings were out of range, and the other two modes used to produce a displayed
> > image partly shifted or scaled out of the screen boundaries.
> >
> > The following modes still produce an image partly out of the screen
> > boundaries.
> >
> >   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
> >   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
> >   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
> >   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
> >   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
> >
> > And this one is reported to be out of range.
> >
> >   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync, pvsync
> >
> > There is thus still room for improvement (some of the issues are possibly due
> > to my monitor though), but there's also an improvement, and no noticeable
> > regression.
>
> The following table compares results obtained with the latest
> renesas-drivers, with and without this series applied on top.
>
> -------------------------------------------------------------------------------
> Legend:
> 	A = image badly aligned: not all 4 blue/red borders visible
> 	F = flickering: disturbance in the shown image
> 	B = broken: mode is not displayed
>
> Results: I = improvement
> 	 R = regression
>
> 	   renesas-drivers   du_clk		result
> 1024x768
> 1920x1200	F		A		I
> 1920x1200	A		A
> 1920x1080	A		A
> 1600x1200	B				I
> 1680x1050
> 1680x1050
> 1400x1050
> 1400x1050
> 1600x900	A		A
> 1280x1024
> 1440x900
> 1440x900
> 1280x960
> 1366x768	A		A
> 1366x768	A		A
> 1360x768			A		R
> 1280x800
> 1280x768	A		A
> 1280x768	A		A
> 1280x768	A		A
> 1280x720	A		A
> 800x600
> 800x600
> 848x480
> 640x480
> -------------------------------------------------------------------------------
>
> Overall I see two modes that were broken or unusable due to flickering
> (1600x1200 and 1920x1200 respectively) to be now (almost) fixed.
>
> There are visible alignement problems on some modes on both versions,
> but I only see one 'regression' (the last 1360x768 that is now
> slightly not aligned).
>
> I guess monitors play a role here, with each one being different, but
> overall I guess our test results match.
>
>
> >
> > > Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > [Factor out code to a helper function]
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Acked-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

To your re-worked version of the patch!

Thanks
   j

> > > ---
> > >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++----------
> > >  1 file changed, 55 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index f8068170905a..2c9405458bbf
> > > 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> > > *rcrtc, best_diff);
> > >  }
> > >
> > > +struct du_clk_params {
> > > +	struct clk *clk;
> > > +	unsigned long rate;
> > > +	unsigned long diff;
> > > +	u32 escr;
> > > +};
> > > +
> > > +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> > > *clk, +				 unsigned long target, u32 escr,
> > > +				 struct du_clk_params *params)
> > > +{
> > > +	unsigned long rate;
> > > +	unsigned long diff;
> > > +	u32 div;
> > > +
> > > +	/*
> > > +	 * If the target rate has already been achieved perfectly we can't do
> > > +	 * better.
> > > +	 */
> > > +	if (params->diff == 0)
> > > +		return;
> > > +
> > > +	/*
> > > +	 * Compute the input clock rate and internal divisor values to obtain
> > > +	 * the clock rate closest to the target frequency.
> > > +	 */
> > > +	rate = clk_round_rate(clk, target);
> > > +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> > > +	diff = abs(rate / (div + 1) - target);
> > > +
> > > +	/*
> > > +	 * If the resulting frequency is better than any previously obtained,
> > > +	 * store the parameters.
> > > +	 */
> > > +	if (diff < params->diff) {
> > > +		params->clk = clk;
> > > +		params->rate = rate;
> > > +		params->diff = diff;
> > > +		params->escr = escr | div;
> > > +	}
> > > +}
> > > +
> > >  static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> > >  	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> > >  	{ /* sentinel */ }
> > > @@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct
> > > rcar_du_crtc *rcrtc)
> > >
> > >  		escr = ESCR_DCLKSEL_DCLKIN | div;
> > >  	} else {
> > > -		unsigned long clk;
> > > -		u32 div;
> > > -
> > > -		/*
> > > -		 * Compute the clock divisor and select the internal or external
> > > -		 * dot clock based on the requested frequency.
> > > -		 */
> > > -		clk = clk_get_rate(rcrtc->clock);
> > > -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> > > -		div = clamp(div, 1U, 64U) - 1;
> > > -
> > > -		escr = ESCR_DCLKSEL_CLKS | div;
> > > -
> > > -		if (rcrtc->extclock) {
> > > -			unsigned long extclk;
> > > -			unsigned long extrate;
> > > -			unsigned long rate;
> > > -			u32 extdiv;
> > > +		struct du_clk_params params = { .diff = (unsigned long)-1 };
> > >
> > > -			extclk = clk_get_rate(rcrtc->extclock);
> > > -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> > > -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> > > +		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
> > > +				     ESCR_DCLKSEL_CLKS, &params);
> > > +		if (rcrtc->extclock)
> > > +			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> > > +					     ESCR_DCLKSEL_DCLKIN, &params);
> > >
> > > -			extrate = extclk / (extdiv + 1);
> > > -			rate = clk / (div + 1);
> > > +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> > > +			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
> > > +			params.rate);
> > >
> > > -			if (abs((long)extrate - (long)mode_clock) <
> > > -			    abs((long)rate - (long)mode_clock))
> > > -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> > > -
> > > -			dev_dbg(rcrtc->group->dev->dev,
> > > -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> > > -				mode_clock, extrate, rate, escr);
> > > -		}
> > > +		clk_set_rate(params.clk, params.rate);
> > > +		escr = params.escr;
> > >  	}
> > >
> > > +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> > > +
> > >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> > >  			    escr);
> > >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >
> >



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 8830 bytes --]

And for the records, I forgot to add

On Tue, Aug 21, 2018 at 12:35:48PM +0200, jacopo mondi wrote:
> Hi Laurent,
>    I run some tests, and here below there's a summary of what I see
>
> On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
> > > From: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > DU channels not equipped with a DPLL use an SoC internal (provided by
> > > the CPG) or external clock source combined with a DU internal divider to
> > > generate the desired output dot clock frequency.
> > >
> > > The current clock selection procedure does not fully exploit the ability
> > > of external clock sources to generate the exact dot clock frequency by
> > > themselves, but relies instead on tuning the internal DU clock divider
> > > only, resulting in a less precise clock generation process.
> > >
> > > When possible, and desirable, ask the external clock source for the
> > > exact output dot clock frequency, and select the clock source that
> > > produces the frequency closest to the desired output dot clock.
> > >
> > > This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> > > where the DU's input dotclock.in is generated by the versaclock VC5
> > > clock source, which is capable of generating the exact rate the DU needs
> > > as pixel clock output.
> > >
> > > This patch fixes higher resolution modes which requires an high pixel
> > > clock output currently not working on non-HDMI DU channel (such as
> > > 1920x1080@60Hz on the VGA output).
> >
> > Just for the record, with this patch the following modes (as printed by
> > modetest) on the VGA output now produce correct result with my monitor:
> >
> >   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync, nvsync
> >   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync, pvsync
> >   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
> >
> > The second mode used to not display at all, with a message telling that
> > timings were out of range, and the other two modes used to produce a displayed
> > image partly shifted or scaled out of the screen boundaries.
> >
> > The following modes still produce an image partly out of the screen
> > boundaries.
> >
> >   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
> >   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
> >   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
> >   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
> >   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
> >
> > And this one is reported to be out of range.
> >
> >   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync, pvsync
> >
> > There is thus still room for improvement (some of the issues are possibly due
> > to my monitor though), but there's also an improvement, and no noticeable
> > regression.
>
> The following table compares results obtained with the latest
> renesas-drivers, with and without this series applied on top.
>
> -------------------------------------------------------------------------------
> Legend:
> 	A = image badly aligned: not all 4 blue/red borders visible
> 	F = flickering: disturbance in the shown image
> 	B = broken: mode is not displayed
>
> Results: I = improvement
> 	 R = regression
>
> 	   renesas-drivers   du_clk		result
> 1024x768
> 1920x1200	F		A		I
> 1920x1200	A		A
> 1920x1080	A		A
> 1600x1200	B				I
> 1680x1050
> 1680x1050
> 1400x1050
> 1400x1050
> 1600x900	A		A
> 1280x1024
> 1440x900
> 1440x900
> 1280x960
> 1366x768	A		A
> 1366x768	A		A
> 1360x768			A		R
> 1280x800
> 1280x768	A		A
> 1280x768	A		A
> 1280x768	A		A
> 1280x720	A		A
> 800x600
> 800x600
> 848x480
> 640x480
> -------------------------------------------------------------------------------
>
> Overall I see two modes that were broken or unusable due to flickering
> (1600x1200 and 1920x1200 respectively) to be now (almost) fixed.
>
> There are visible alignement problems on some modes on both versions,
> but I only see one 'regression' (the last 1360x768 that is now
> slightly not aligned).
>
> I guess monitors play a role here, with each one being different, but
> overall I guess our test results match.
>
>
> >
> > > Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > [Factor out code to a helper function]
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Acked-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

To your re-worked version of the patch!

Thanks
   j

> > > ---
> > >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 ++++++++++++++++++++----------
> > >  1 file changed, 55 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index f8068170905a..2c9405458bbf
> > > 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> > > *rcrtc, best_diff);
> > >  }
> > >
> > > +struct du_clk_params {
> > > +	struct clk *clk;
> > > +	unsigned long rate;
> > > +	unsigned long diff;
> > > +	u32 escr;
> > > +};
> > > +
> > > +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
> > > *clk, +				 unsigned long target, u32 escr,
> > > +				 struct du_clk_params *params)
> > > +{
> > > +	unsigned long rate;
> > > +	unsigned long diff;
> > > +	u32 div;
> > > +
> > > +	/*
> > > +	 * If the target rate has already been achieved perfectly we can't do
> > > +	 * better.
> > > +	 */
> > > +	if (params->diff == 0)
> > > +		return;
> > > +
> > > +	/*
> > > +	 * Compute the input clock rate and internal divisor values to obtain
> > > +	 * the clock rate closest to the target frequency.
> > > +	 */
> > > +	rate = clk_round_rate(clk, target);
> > > +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
> > > +	diff = abs(rate / (div + 1) - target);
> > > +
> > > +	/*
> > > +	 * If the resulting frequency is better than any previously obtained,
> > > +	 * store the parameters.
> > > +	 */
> > > +	if (diff < params->diff) {
> > > +		params->clk = clk;
> > > +		params->rate = rate;
> > > +		params->diff = diff;
> > > +		params->escr = escr | div;
> > > +	}
> > > +}
> > > +
> > >  static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> > >  	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> > >  	{ /* sentinel */ }
> > > @@ -225,42 +267,24 @@ static void rcar_du_crtc_set_display_timing(struct
> > > rcar_du_crtc *rcrtc)
> > >
> > >  		escr = ESCR_DCLKSEL_DCLKIN | div;
> > >  	} else {
> > > -		unsigned long clk;
> > > -		u32 div;
> > > -
> > > -		/*
> > > -		 * Compute the clock divisor and select the internal or external
> > > -		 * dot clock based on the requested frequency.
> > > -		 */
> > > -		clk = clk_get_rate(rcrtc->clock);
> > > -		div = DIV_ROUND_CLOSEST(clk, mode_clock);
> > > -		div = clamp(div, 1U, 64U) - 1;
> > > -
> > > -		escr = ESCR_DCLKSEL_CLKS | div;
> > > -
> > > -		if (rcrtc->extclock) {
> > > -			unsigned long extclk;
> > > -			unsigned long extrate;
> > > -			unsigned long rate;
> > > -			u32 extdiv;
> > > +		struct du_clk_params params = { .diff = (unsigned long)-1 };
> > >
> > > -			extclk = clk_get_rate(rcrtc->extclock);
> > > -			extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> > > -			extdiv = clamp(extdiv, 1U, 64U) - 1;
> > > +		rcar_du_escr_divider(rcrtc, rcrtc->clock, mode_clock,
> > > +				     ESCR_DCLKSEL_CLKS, &params);
> > > +		if (rcrtc->extclock)
> > > +			rcar_du_escr_divider(rcrtc, rcrtc->extclock, mode_clock,
> > > +					     ESCR_DCLKSEL_DCLKIN, &params);
> > >
> > > -			extrate = extclk / (extdiv + 1);
> > > -			rate = clk / (div + 1);
> > > +		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> > > +			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
> > > +			params.rate);
> > >
> > > -			if (abs((long)extrate - (long)mode_clock) <
> > > -			    abs((long)rate - (long)mode_clock))
> > > -				escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> > > -
> > > -			dev_dbg(rcrtc->group->dev->dev,
> > > -				"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> > > -				mode_clock, extrate, rate, escr);
> > > -		}
> > > +		clk_set_rate(params.clk, params.rate);
> > > +		escr = params.escr;
> > >  	}
> > >
> > > +	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> > > +
> > >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> > >  			    escr);
> > >  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >
> >



[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[Kieran Bingham]

Hi Laurent, Jacopo,

On 21/08/18 09:08, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> (CC'ing Kieran)
> 
> On Tuesday, 21 August 2018 10:33:57 EEST jacopo mondi wrote:
>> On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
>>> On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
>>>> From: Jacopo Mondi <jacopo@jmondi.org>
>>>>
>>>> DU channels not equipped with a DPLL use an SoC internal (provided by
>>>> the CPG) or external clock source combined with a DU internal divider to
>>>> generate the desired output dot clock frequency.
>>>>
>>>> The current clock selection procedure does not fully exploit the ability
>>>> of external clock sources to generate the exact dot clock frequency by
>>>> themselves, but relies instead on tuning the internal DU clock divider
>>>> only, resulting in a less precise clock generation process.
>>>>
>>>> When possible, and desirable, ask the external clock source for the
>>>> exact output dot clock frequency, and select the clock source that
>>>> produces the frequency closest to the desired output dot clock.
>>>>
>>>> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
>>>> where the DU's input dotclock.in is generated by the versaclock VC5
>>>> clock source, which is capable of generating the exact rate the DU needs
>>>> as pixel clock output.
>>>>
>>>> This patch fixes higher resolution modes which requires an high pixel
>>>> clock output currently not working on non-HDMI DU channel (such as
>>>> 1920x1080@60Hz on the VGA output).
>>>
>>> Just for the record, with this patch the following modes (as printed by
>>>
>>> modetest) on the VGA output now produce correct result with my monitor:
>>>   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync,
>>>   nvsync
>>>   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync,
>>>   pvsync
>>>   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
>>>
>>> The second mode used to not display at all, with a message telling that
>>> timings were out of range, and the other two modes used to produce a
>>> displayed image partly shifted or scaled out of the screen boundaries.
>>>
>>> The following modes still produce an image partly out of the screen
>>> boundaries.
>>>
>>>   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
>>>   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
>>>   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
>>>   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
>>>   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
>>>
>>> And this one is reported to be out of range.
>>>
>>>   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync,
>>>   pvsync
>>>
>>> There is thus still room for improvement (some of the issues are possibly
>>> due to my monitor though), but there's also an improvement, and no
>>> noticeable regression.
>>>
>>>> Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel
>>>> clock")
>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>> [Factor out code to a helper function]
>>>> Signed-off-by: Laurent Pinchart
>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>> ---
>>>>
>>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 +++++++++++++++++++---------
>>>>  1 file changed, 55 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index
>>>> f8068170905a..2c9405458bbf
>>>> 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct
>>>> rcar_du_crtc *rcrtc,
>>>>  	best_diff);
>>>>  }
>>>>
>>>> +struct du_clk_params {
>>>> +	struct clk *clk;
>>>> +	unsigned long rate;
>>>> +	unsigned long diff;
>>>> +	u32 escr;
>>>> +};
>>>> +
>>>> +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
>>>> *clk,
>>>> +				 unsigned long target, u32 escr,
>>>> +				 struct du_clk_params *params)
>>
>> I don't see the rcrtc parameter ever being used in this function.
>> Do you want to keep it anyhow?
> 
> You're right, I'll remove it.
> 
>>>> +{
>>>> +	unsigned long rate;
>>>> +	unsigned long diff;
>>>> +	u32 div;
>>>> +
>>>> +	/*
>>>> +	 * If the target rate has already been achieved perfectly we can't do
>>>> +	 * better.
>>>> +	 */
>>>> +	if (params->diff == 0)
>>>> +		return;
>>>> +
>>>> +	/*
>>>> +	 * Compute the input clock rate and internal divisor values to obtain
>>>> +	 * the clock rate closest to the target frequency.
>>>> +	 */
>>>> +	rate = clk_round_rate(clk, target);
>>>> +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
>>>> +	diff = abs(rate / (div + 1) - target);
>>>> +
>>>> +	/*
>>>> +	 * If the resulting frequency is better than any previously obtained,
>>
>> s/obtained,/obtained one,/ ?
> 
> Any opinion from a native English speaker ? :-)

I'd probably write:

Store the parameters if the resulting frequency is better than any
previously calculated value.


>> Will get back with some testing results on a different VGA monitor...
> 
> Thank you.
> 
>>>> +	 * store the parameters.
>>>> +	 */
>>>> +	if (diff < params->diff) {
>>>> +		params->clk = clk;
>>>> +		params->rate = rate;
>>>> +		params->diff = diff;
>>>> +		params->escr = escr | div;
>>>> +	}
>>>> +}
> 
> [snip]
> 

-- 
Regards
--
Kieran

Re: [PATCH] drm: rcar-du: Improve non-DPLL clock selection

[Kieran Bingham]

Hi Laurent, Jacopo,

On 21/08/18 09:08, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> (CC'ing Kieran)
> 
> On Tuesday, 21 August 2018 10:33:57 EEST jacopo mondi wrote:
>> On Tue, Aug 21, 2018 at 01:12:40AM +0300, Laurent Pinchart wrote:
>>> On Tuesday, 21 August 2018 00:49:41 EEST Laurent Pinchart wrote:
>>>> From: Jacopo Mondi <jacopo@jmondi.org>
>>>>
>>>> DU channels not equipped with a DPLL use an SoC internal (provided by
>>>> the CPG) or external clock source combined with a DU internal divider to
>>>> generate the desired output dot clock frequency.
>>>>
>>>> The current clock selection procedure does not fully exploit the ability
>>>> of external clock sources to generate the exact dot clock frequency by
>>>> themselves, but relies instead on tuning the internal DU clock divider
>>>> only, resulting in a less precise clock generation process.
>>>>
>>>> When possible, and desirable, ask the external clock source for the
>>>> exact output dot clock frequency, and select the clock source that
>>>> produces the frequency closest to the desired output dot clock.
>>>>
>>>> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
>>>> where the DU's input dotclock.in is generated by the versaclock VC5
>>>> clock source, which is capable of generating the exact rate the DU needs
>>>> as pixel clock output.
>>>>
>>>> This patch fixes higher resolution modes which requires an high pixel
>>>> clock output currently not working on non-HDMI DU channel (such as
>>>> 1920x1080@60Hz on the VGA output).
>>>
>>> Just for the record, with this patch the following modes (as printed by
>>>
>>> modetest) on the VGA output now produce correct result with my monitor:
>>>   1920x1080 60 1920 2008 2052 2200 1080 1084 1089 1125 flags: nhsync,
>>>   nvsync
>>>   1600x1200 60 1600 1664 1856 2160 1200 1201 1204 1250 flags: phsync,
>>>   pvsync
>>>   1360x768 60 1360 1424 1536 1792 768 771 777 795 flags: phsync, pvsync
>>>
>>> The second mode used to not display at all, with a message telling that
>>> timings were out of range, and the other two modes used to produce a
>>> displayed image partly shifted or scaled out of the screen boundaries.
>>>
>>> The following modes still produce an image partly out of the screen
>>> boundaries.
>>>
>>>   1600x900 60 1600 1624 1704 1800 900 901 904 1000 flags: phsync, pvsync
>>>   1280x960 60 1280 1376 1488 1800 960 961 964 1000 flags: phsync, pvsync
>>>   1366x768 60 1366 1436 1579 1792 768 771 774 798 flags: phsync, pvsync
>>>   1366x768 60 1366 1380 1436 1500 768 769 772 800 flags: phsync, pvsync
>>>   1280x720 60 1280 1390 1430 1650 720 725 730 750 flags: phsync, pvsync
>>>
>>> And this one is reported to be out of range.
>>>
>>>   1920x1200 60 1920 2056 2256 2592 1200 1203 1209 1245 flags: nhsync,
>>>   pvsync
>>>
>>> There is thus still room for improvement (some of the issues are possibly
>>> due to my monitor though), but there's also an improvement, and no
>>> noticeable regression.
>>>
>>>> Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel
>>>> clock")
>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>> [Factor out code to a helper function]
>>>> Signed-off-by: Laurent Pinchart
>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>> ---
>>>>
>>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 86 +++++++++++++++++++---------
>>>>  1 file changed, 55 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index
>>>> f8068170905a..2c9405458bbf
>>>> 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> @@ -165,6 +165,48 @@ static void rcar_du_dpll_divider(struct
>>>> rcar_du_crtc *rcrtc,
>>>>  	best_diff);
>>>>  }
>>>>
>>>> +struct du_clk_params {
>>>> +	struct clk *clk;
>>>> +	unsigned long rate;
>>>> +	unsigned long diff;
>>>> +	u32 escr;
>>>> +};
>>>> +
>>>> +static void rcar_du_escr_divider(struct rcar_du_crtc *rcrtc, struct clk
>>>> *clk,
>>>> +				 unsigned long target, u32 escr,
>>>> +				 struct du_clk_params *params)
>>
>> I don't see the rcrtc parameter ever being used in this function.
>> Do you want to keep it anyhow?
> 
> You're right, I'll remove it.
> 
>>>> +{
>>>> +	unsigned long rate;
>>>> +	unsigned long diff;
>>>> +	u32 div;
>>>> +
>>>> +	/*
>>>> +	 * If the target rate has already been achieved perfectly we can't do
>>>> +	 * better.
>>>> +	 */
>>>> +	if (params->diff == 0)
>>>> +		return;
>>>> +
>>>> +	/*
>>>> +	 * Compute the input clock rate and internal divisor values to obtain
>>>> +	 * the clock rate closest to the target frequency.
>>>> +	 */
>>>> +	rate = clk_round_rate(clk, target);
>>>> +	div = clamp(DIV_ROUND_CLOSEST(rate, target), 1UL, 64UL) - 1;
>>>> +	diff = abs(rate / (div + 1) - target);
>>>> +
>>>> +	/*
>>>> +	 * If the resulting frequency is better than any previously obtained,
>>
>> s/obtained,/obtained one,/ ?
> 
> Any opinion from a native English speaker ? :-)

I'd probably write:

Store the parameters if the resulting frequency is better than any
previously calculated value.


>> Will get back with some testing results on a different VGA monitor...
> 
> Thank you.
> 
>>>> +	 * store the parameters.
>>>> +	 */
>>>> +	if (diff < params->diff) {
>>>> +		params->clk = clk;
>>>> +		params->rate = rate;
>>>> +		params->diff = diff;
>>>> +		params->escr = escr | div;
>>>> +	}
>>>> +}
> 
> [snip]
> 

-- 
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel