| Message ID | 20191220115653.6487-8-a.swigon@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC,v3,1/7] interconnect: Export of_icc_get_from_provider() | expand |
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.
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
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,
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; }