| Message ID | 20200529163200.18031-7-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Exynos: Simple QoS for exynos-bus using interconnect | expand |
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)
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
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; }