[RFC,v3,7/7] drm: exynos: mixer: Add interconnect support
diff mbox series

Message ID 20191220115653.6487-8-a.swigon@samsung.com
State Under Review
Headers show
Series
  • [RFC,v3,1/7] interconnect: Export of_icc_get_from_provider()
Related show

Commit Message

Artur Świgoń Dec. 20, 2019, 11:56 a.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'.

Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 5 deletions(-)

Comments

Chanwoo Choi Dec. 21, 2019, 8:15 p.m. UTC | #1
Hi,

On Fri, Dec 20, 2019 at 9:03 PM Artur Świgoń <a.swigon@samsung.com> 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'.

The patch description doesn't include why interconnect is required
and what to do.

>
> Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++++++++++++++++++++++++--
>  1 file changed, 66 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 6cfdb95fef2f..a7e7240a055f 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>
> @@ -97,6 +98,7 @@ struct mixer_context {
>         struct exynos_drm_crtc  *crtc;
>         struct exynos_drm_plane planes[MIXER_WIN_NR];
>         unsigned long           flags;
> +       struct icc_path         *soc_path;
>
>         int                     irq;
>         void __iomem            *mixer_regs;
> @@ -931,6 +933,40 @@ 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;
> +       int i, j, sub;
> +
> +       if (!ctx->soc_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);
> +       icc_set_bw(ctx->soc_path, Bps_to_icc(bandwidth), 0);
> +}

I recommend that add the role of this function in order to guarantee
the minimum bandwidth to prevent performance drop or h/w issue.

> +
>  static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
>  {
>         struct mixer_context *ctx = crtc->ctx;
> @@ -982,6 +1018,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);
>  }
> @@ -1029,6 +1066,7 @@ static void mixer_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);
> @@ -1220,12 +1258,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);
> @@ -1233,6 +1281,7 @@ static int mixer_probe(struct platform_device *pdev)
>         ctx->pdev = pdev;
>         ctx->dev = dev;
>         ctx->mxr_ver = drv->version;
> +       ctx->soc_path = path;
>
>         if (drv->is_vp_enabled)
>                 __set_bit(MXR_BIT_VP_ENABLED, &ctx->flags);
> @@ -1242,17 +1291,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->soc_path);
>
>         return 0;
>  }
> --
> 2.17.1
>

Basically, I agree this patch about using ICC feature
to guarantee the minimum bandwidth. But, I don't have
the knowledge of exynos-mixer.c. So, just I reviewed
the part of ICC usage. After digging the exynos-mixer.c,
I'll reply the reviewed tag. Thanks.
Inki Dae Dec. 24, 2019, 4:56 a.m. UTC | #2
Hi,

19. 12. 20. 오후 8:56에 Artur Świgoń 이(가) 쓴 글:
> 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'.
> 
> Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++++++++++++++++++++++++--
>  1 file changed, 66 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 6cfdb95fef2f..a7e7240a055f 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>
> @@ -97,6 +98,7 @@ struct mixer_context {
>  	struct exynos_drm_crtc	*crtc;
>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
>  	unsigned long		flags;
> +	struct icc_path		*soc_path;
>  
>  	int			irq;
>  	void __iomem		*mixer_regs;
> @@ -931,6 +933,40 @@ 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;
> +	int i, j, sub;
> +
> +	if (!ctx->soc_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);
> +	icc_set_bw(ctx->soc_path, Bps_to_icc(bandwidth), 0);
> +}
> +
>  static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
>  {
>  	struct mixer_context *ctx = crtc->ctx;
> @@ -982,6 +1018,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);
>  }
> @@ -1029,6 +1066,7 @@ static void mixer_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);

Your intention is to set peak and average bandwidth to 0 at disabling mixer device?

Thanks,
Inki Dae
Artur Świgoń Dec. 30, 2019, 9:35 a.m. UTC | #3
Hi,

On Tue, 2019-12-24 at 13:56 +0900, Inki Dae wrote:
> Hi,
> 
> 19. 12. 20. 오후 8:56에 Artur Świgoń 이(가) 쓴 글:
> > 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'.
> > 
> > Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
> > Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++++++++++++++++++++++++--
> >  1 file changed, 66 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> > index 6cfdb95fef2f..a7e7240a055f 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>
> > @@ -97,6 +98,7 @@ struct mixer_context {
> >  	struct exynos_drm_crtc	*crtc;
> >  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
> >  	unsigned long		flags;
> > +	struct icc_path		*soc_path;
> >  
> >  	int			irq;
> >  	void __iomem		*mixer_regs;
> > @@ -931,6 +933,40 @@ 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;
> > +	int i, j, sub;
> > +
> > +	if (!ctx->soc_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);
> > +	icc_set_bw(ctx->soc_path, Bps_to_icc(bandwidth), 0);
> > +}
> > +
> >  static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
> >  {
> >  	struct mixer_context *ctx = crtc->ctx;
> > @@ -982,6 +1018,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);
> >  }
> > @@ -1029,6 +1066,7 @@ static void mixer_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);
> 
> Your intention is to set peak and average bandwidth to 0 at disabling mixer device?

Yes. In general, setting the requested bandwidth to zero means "do not override
the devfreq setting" because only constraints of type DEV_PM_QOS_MIN_FREQUENCY
are used (cf. patch 05 of this series). I will make sure to reflect that in the
commit message.

Moreover, this RFC does not really make use of the peak bandwidth (yet). It is
set to zero in this patch and ignored in patch 05 (cf. exynos_bus_icc_set()).
Only the average bandwidth is translated to a minimum frequency constraint,
overriding devfreq if necessary.

A possible modification to mixer_set_memory_bandwidth() could be:
- bandwidth = bandwidth / 4 * 5;
+ peak_bandwidth = bandwidth / 4 * 5;
in mixer_set_memory_bandwidth() plus some additional logic in exynos_bus_icc_set().

Best regards,

Patch
diff mbox series

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 6cfdb95fef2f..a7e7240a055f 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>
@@ -97,6 +98,7 @@  struct mixer_context {
 	struct exynos_drm_crtc	*crtc;
 	struct exynos_drm_plane	planes[MIXER_WIN_NR];
 	unsigned long		flags;
+	struct icc_path		*soc_path;
 
 	int			irq;
 	void __iomem		*mixer_regs;
@@ -931,6 +933,40 @@  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;
+	int i, j, sub;
+
+	if (!ctx->soc_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);
+	icc_set_bw(ctx->soc_path, Bps_to_icc(bandwidth), 0);
+}
+
 static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
 {
 	struct mixer_context *ctx = crtc->ctx;
@@ -982,6 +1018,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);
 }
@@ -1029,6 +1066,7 @@  static void mixer_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);
@@ -1220,12 +1258,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);
@@ -1233,6 +1281,7 @@  static int mixer_probe(struct platform_device *pdev)
 	ctx->pdev = pdev;
 	ctx->dev = dev;
 	ctx->mxr_ver = drv->version;
+	ctx->soc_path = path;
 
 	if (drv->is_vp_enabled)
 		__set_bit(MXR_BIT_VP_ENABLED, &ctx->flags);
@@ -1242,17 +1291,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->soc_path);
 
 	return 0;
 }