[RFC,v5,6/6] drm: exynos: mixer: Add interconnect support
diff mbox series

Message ID 20200529163200.18031-7-s.nawrocki@samsung.com
State Superseded
Headers show
Series
  • Exynos: Simple QoS for exynos-bus using interconnect
Related show

Commit Message

Sylwester Nawrocki May 29, 2020, 4:32 p.m. UTC
From: Marek Szyprowski <m.szyprowski@samsung.com>

This patch adds interconnect support to exynos-mixer. The mixer works
the same as before when CONFIG_INTERCONNECT is 'n'.

For proper operation of the video mixer block we need to ensure the
interconnect busses like DMC or LEFTBUS provide enough bandwidth so
as to avoid DMA buffer underruns in the mixer block. i.e we need to
prevent those busses from operating in low perfomance OPPs when
the mixer is running.
In this patch the bus bandwidth request is done through the interconnect
API, the bandiwidth value is calculated from selected DRM mode, i.e.
video plane width, height, refresh rate and pixel format.

Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
[s.nawrocki: renamed soc_path variable to icc_path, edited commit desc.]
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes for v5:
 - renamed soc_path variable to icc_path
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 73 ++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 5 deletions(-)

Comments

Chanwoo Choi June 1, 2020, 7:58 a.m. UTC | #1
Hi Sylwester,

On 5/30/20 1:32 AM, Sylwester Nawrocki wrote:
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> This patch adds interconnect support to exynos-mixer. The mixer works
> the same as before when CONFIG_INTERCONNECT is 'n'.
> 
> For proper operation of the video mixer block we need to ensure the
> interconnect busses like DMC or LEFTBUS provide enough bandwidth so
> as to avoid DMA buffer underruns in the mixer block. i.e we need to
> prevent those busses from operating in low perfomance OPPs when
> the mixer is running.
> In this patch the bus bandwidth request is done through the interconnect
> API, the bandiwidth value is calculated from selected DRM mode, i.e.
> video plane width, height, refresh rate and pixel format.
> 
> Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> [s.nawrocki: renamed soc_path variable to icc_path, edited commit desc.]
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v5:
>  - renamed soc_path variable to icc_path
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 73 ++++++++++++++++++++++++++++++++---
>  1 file changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 21b726b..bdae683 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -13,6 +13,7 @@
>  #include <linux/component.h>
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/interconnect.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
> @@ -98,6 +99,7 @@ struct mixer_context {
>  	struct exynos_drm_crtc	*crtc;
>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
>  	unsigned long		flags;
> +	struct icc_path		*icc_path;
>  
>  	int			irq;
>  	void __iomem		*mixer_regs;
> @@ -934,6 +936,42 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
>  	mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
>  }
>  
> +static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc)
> +{
> +	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
> +	struct mixer_context *ctx = crtc->ctx;
> +	unsigned long bw, bandwidth = 0;
> +	u32 avg_bw, peak_bw;
> +	int i, j, sub;
> +
> +	if (!ctx->icc_path)
> +		return;
> +
> +	for (i = 0; i < MIXER_WIN_NR; i++) {
> +		struct drm_plane *plane = &ctx->planes[i].base;
> +		const struct drm_format_info *format;
> +
> +		if (plane->state && plane->state->crtc && plane->state->fb) {
> +			format = plane->state->fb->format;
> +			bw = mode->hdisplay * mode->vdisplay *
> +							drm_mode_vrefresh(mode);
> +			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +				bw /= 2;
> +			for (j = 0; j < format->num_planes; j++) {
> +				sub = j ? (format->vsub * format->hsub) : 1;
> +				bandwidth += format->cpp[j] * bw / sub;

First of all, I agree this approach.

Could you please add more detailed comments for understadning
about this calculation? As you commented, it seems that
the final bandwidth contains the width/height/refresh rate
and pixel format. If you add one real example, it will be very helpful.


(snip)
Sylwester Nawrocki June 3, 2020, 10:04 a.m. UTC | #2
Hi Chanwoo,

On 01.06.2020 09:58, Chanwoo Choi wrote:
> On 5/30/20 1:32 AM, Sylwester Nawrocki wrote:
>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> This patch adds interconnect support to exynos-mixer. The mixer works
>> the same as before when CONFIG_INTERCONNECT is 'n'.
>>
>> For proper operation of the video mixer block we need to ensure the
>> interconnect busses like DMC or LEFTBUS provide enough bandwidth so
>> as to avoid DMA buffer underruns in the mixer block. i.e we need to
>> prevent those busses from operating in low perfomance OPPs when
>> the mixer is running.
>> In this patch the bus bandwidth request is done through the interconnect
>> API, the bandiwidth value is calculated from selected DRM mode, i.e.
>> video plane width, height, refresh rate and pixel format.
>>
>> Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> [s.nawrocki: renamed soc_path variable to icc_path, edited commit desc.]
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

>>  drivers/gpu/drm/exynos/exynos_mixer.c | 73 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 68 insertions(+), 5 deletions(-)
  
>> +static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc)
>> +{
>> +	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>> +	struct mixer_context *ctx = crtc->ctx;
>> +	unsigned long bw, bandwidth = 0;
>> +	u32 avg_bw, peak_bw;
>> +	int i, j, sub;
>> +
>> +	if (!ctx->icc_path)
>> +		return;
>> +
>> +	for (i = 0; i < MIXER_WIN_NR; i++) {
>> +		struct drm_plane *plane = &ctx->planes[i].base;
>> +		const struct drm_format_info *format;
>> +
>> +		if (plane->state && plane->state->crtc && plane->state->fb) {
>> +			format = plane->state->fb->format;
>> +			bw = mode->hdisplay * mode->vdisplay *
>> +							drm_mode_vrefresh(mode);
>> +			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> +				bw /= 2;
>> +			for (j = 0; j < format->num_planes; j++) {
>> +				sub = j ? (format->vsub * format->hsub) : 1;
>> +				bandwidth += format->cpp[j] * bw / sub;
> 
> First of all, I agree this approach.
> 
> Could you please add more detailed comments for understadning
> about this calculation? As you commented, it seems that
> the final bandwidth contains the width/height/refresh rate
> and pixel format. If you add one real example, it will be very helpful.

OK, I will improve the commit message and add a comment to the function.
As far as I understand this simply calculates amount of data in bytes that
needs to be read from the system memory per second for given video stream,
by summing values for each mixer window and for each color plane within 
a window.

Anyway, the patch will to be changed as after some more tests it seems
the calculation works for LEFTBUS but DMC clock on Odroid U3 needs to
be set to 400 MHz, rather than to at least 160 MHz. With any lower value 
the mixer underflow error interrupts are being triggered and eventually 
the IOMMU page fault occurs.

--
Regards,
Sylwester

Patch
diff mbox series

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 21b726b..bdae683 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -13,6 +13,7 @@ 
 #include <linux/component.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/interconnect.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>
@@ -98,6 +99,7 @@  struct mixer_context {
 	struct exynos_drm_crtc	*crtc;
 	struct exynos_drm_plane	planes[MIXER_WIN_NR];
 	unsigned long		flags;
+	struct icc_path		*icc_path;
 
 	int			irq;
 	void __iomem		*mixer_regs;
@@ -934,6 +936,42 @@  static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
 	mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
 }
 
+static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc)
+{
+	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
+	struct mixer_context *ctx = crtc->ctx;
+	unsigned long bw, bandwidth = 0;
+	u32 avg_bw, peak_bw;
+	int i, j, sub;
+
+	if (!ctx->icc_path)
+		return;
+
+	for (i = 0; i < MIXER_WIN_NR; i++) {
+		struct drm_plane *plane = &ctx->planes[i].base;
+		const struct drm_format_info *format;
+
+		if (plane->state && plane->state->crtc && plane->state->fb) {
+			format = plane->state->fb->format;
+			bw = mode->hdisplay * mode->vdisplay *
+							drm_mode_vrefresh(mode);
+			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+				bw /= 2;
+			for (j = 0; j < format->num_planes; j++) {
+				sub = j ? (format->vsub * format->hsub) : 1;
+				bandwidth += format->cpp[j] * bw / sub;
+			}
+		}
+	}
+
+	/* add 20% safety margin */
+	bandwidth = bandwidth / 4 * 5;
+	dev_dbg(ctx->dev, "exynos-mixer: safe bandwidth %ld Bps\n", bandwidth);
+
+	avg_bw = peak_bw = Bps_to_icc(bandwidth);
+	icc_set_bw(ctx->icc_path, avg_bw, peak_bw);
+}
+
 static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
 {
 	struct mixer_context *ctx = crtc->ctx;
@@ -985,6 +1023,7 @@  static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
 
+	mixer_set_memory_bandwidth(crtc);
 	mixer_enable_sync(mixer_ctx);
 	exynos_crtc_handle_event(crtc);
 }
@@ -1032,6 +1071,7 @@  static void mixer_atomic_disable(struct exynos_drm_crtc *crtc)
 	for (i = 0; i < MIXER_WIN_NR; i++)
 		mixer_disable_plane(crtc, &ctx->planes[i]);
 
+	mixer_set_memory_bandwidth(crtc);
 	exynos_drm_pipe_clk_enable(crtc, false);
 
 	pm_runtime_put(ctx->dev);
@@ -1223,12 +1263,22 @@  static int mixer_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	const struct mixer_drv_data *drv;
 	struct mixer_context *ctx;
+	struct icc_path *path;
 	int ret;
 
+	/*
+	 * Returns NULL if CONFIG_INTERCONNECT is disabled.
+	 * May return ERR_PTR(-EPROBE_DEFER).
+	 */
+	path = of_icc_get(dev, NULL);
+	if (IS_ERR(path))
+		return PTR_ERR(path);
+
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx) {
 		DRM_DEV_ERROR(dev, "failed to alloc mixer context.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	drv = of_device_get_match_data(dev);
@@ -1236,6 +1286,7 @@  static int mixer_probe(struct platform_device *pdev)
 	ctx->pdev = pdev;
 	ctx->dev = dev;
 	ctx->mxr_ver = drv->version;
+	ctx->icc_path = path;
 
 	if (drv->is_vp_enabled)
 		__set_bit(MXR_BIT_VP_ENABLED, &ctx->flags);
@@ -1245,17 +1296,29 @@  static int mixer_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, ctx);
 
 	ret = component_add(&pdev->dev, &mixer_component_ops);
-	if (!ret)
-		pm_runtime_enable(dev);
+	if (ret < 0)
+		goto err;
+
+	pm_runtime_enable(dev);
+
+	return 0;
+
+err:
+	icc_put(path);
 
 	return ret;
 }
 
 static int mixer_remove(struct platform_device *pdev)
 {
-	pm_runtime_disable(&pdev->dev);
+	struct device *dev = &pdev->dev;
+	struct mixer_context *ctx = dev_get_drvdata(dev);
+
+	pm_runtime_disable(dev);
+
+	component_del(dev, &mixer_component_ops);
 
-	component_del(&pdev->dev, &mixer_component_ops);
+	icc_put(ctx->icc_path);
 
 	return 0;
 }