This patchset has more fixes for fixing more issues related to wait for vblank.
Patch 1: add mixer apply function The mixer window enabled flag is being done in both exynos_drm_hdmi as well as exynos_mixer. This patch moves the flag to one place and adds the mixer apply function similar to fimd apply function.
Patch 2: fimd: call win_disable only when window is enabled Adds a condition to call win_disable only if the window is enabled.
Patch 3: enable vblank in fimd wait for vblank If vblank is disabled, then we turn enable vblank temporarily before wait for vblank and disable it afterwards. This ensures that the wait will not timeout.
Patch 4: fimd: clear channel before enabling iommu This patch is required if the fimd window channel is already active before fimd probe. This could happen if we turn on fimd during uboot. This patch ensures that we do not get a page fault upon iommu initialization. If any channel is active, we disable the channel and wait for vblank.
Patch 5: mixer: do not finish a pageflip if layer update in progress Exynos5 supports only 2 layer updates for mixer. This patch ensures each layer is updated only once per vsync. Also, in case a layer update is pending, the irq handler exits early. This ensures there is no corruption on screen as well as there is no page fault during buffer freeing.
Patch 6: add complete_scanout interface Before freeing a framebuffer, we call complete_scanout encoder function which just calls wait for vblank of all the encoders. This is not a very optimized method since a framebuffer might not be in use by a crtc and we end up waiting for vblank unnecessarily. Instead, this patch modifies the wait_for_vblank interface to complete_scanout interface. In this function, each crtc must check if the fb is currently being used. If it is being used, it must wait for vsync (or even disable a window if necessary) and ensure that the buffer is no longer used by crtc before returning. So if a crtc is not actually reading from the buffer, the complete scanout function will just return and not wait for vsync.
Patch 7: hdmi: add complete_scanout function Patch 8: fimd: add complete_scanout function These two patches add the respective complete_scanout functions for fimd and mixer. These functions check the actual base and shadow registers of fimd/mixer to see if a particular dma-address is in use and will wait_for_vblank only if it is in use. Also for mixer, if the base address matches the fb's dma address, then the window is disabled. This is because a layer update is already set when the base register was updated. So unless we disable the window, we cannot free the buffer.
Akshu Agrawal (1): drm/exynos: fimd: clear channel before enabling iommu
Prathyush K (6): drm/exynos: add mixer apply function drm/exynos: fimd: call win_disable only when window is enabled drm/exynos: enable vblank in fimd wait for vblank drm/exynos: add complete_scanout interface drm/exynos: hdmi: add complete_scanout function drm/exynos: fimd: add complete_scanout function
Sean Paul (1): drm/exynos: mixer: do not finish a pageflip if layer update in progress
drivers/gpu/drm/exynos/exynos_drm_drv.h | 7 ++- drivers/gpu/drm/exynos/exynos_drm_encoder.c | 23 ++++---- drivers/gpu/drm/exynos/exynos_drm_encoder.h | 4 +- drivers/gpu/drm/exynos/exynos_drm_fb.c | 13 ++++- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 81 ++++++++++++++++++++++++-- drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 26 +++------ drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 4 +- drivers/gpu/drm/exynos/exynos_mixer.c | 90 ++++++++++++++++++++++++++--- drivers/gpu/drm/exynos/regs-mixer.h | 1 + include/video/samsung_fimd.h | 1 + 10 files changed, 202 insertions(+), 48 deletions(-)
Currently, an enabled flag for each mixer window is being set in both mixer and the common hdmi framework. This patch adds an apply function in mixer and moves the enabled flag inside the mixer. This is required to ensure the enabled flag is updated by the resume flag during dpms.
Signed-off-by: Prathyush K prathyush.k@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 15 ++------------- drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 1 + drivers/gpu/drm/exynos/exynos_mixer.c | 18 +++++++++++++++++- 3 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c index 55793c4..d8ae47e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c @@ -45,8 +45,6 @@ struct drm_hdmi_context { struct exynos_drm_subdrv subdrv; struct exynos_drm_hdmi_context *hdmi_ctx; struct exynos_drm_hdmi_context *mixer_ctx; - - bool enabled[MIXER_WIN_NR]; };
int exynos_platform_device_hdmi_register(void) @@ -250,16 +248,11 @@ static void drm_hdmi_dpms(struct device *subdrv_dev, int mode) static void drm_hdmi_apply(struct device *subdrv_dev) { struct drm_hdmi_context *ctx = to_context(subdrv_dev); - int i;
DRM_DEBUG_KMS("%s\n", __FILE__);
- for (i = 0; i < MIXER_WIN_NR; i++) { - if (!ctx->enabled[i]) - continue; - if (mixer_ops && mixer_ops->win_commit) - mixer_ops->win_commit(ctx->mixer_ctx->ctx, i); - } + if (mixer_ops && mixer_ops->apply) + mixer_ops->apply(ctx->mixer_ctx->ctx);
if (hdmi_ops && hdmi_ops->commit) hdmi_ops->commit(ctx->hdmi_ctx->ctx); @@ -302,8 +295,6 @@ static void drm_mixer_commit(struct device *subdrv_dev, int zpos)
if (mixer_ops && mixer_ops->win_commit) mixer_ops->win_commit(ctx->mixer_ctx->ctx, win); - - ctx->enabled[win] = true; }
static void drm_mixer_disable(struct device *subdrv_dev, int zpos) @@ -320,8 +311,6 @@ static void drm_mixer_disable(struct device *subdrv_dev, int zpos)
if (mixer_ops && mixer_ops->win_disable) mixer_ops->win_disable(ctx->mixer_ctx->ctx, win); - - ctx->enabled[win] = false; }
static struct exynos_drm_overlay_ops drm_hdmi_overlay_ops = { diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h index 784a7e9..4fad00c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h @@ -58,6 +58,7 @@ struct exynos_mixer_ops { void (*win_mode_set)(void *ctx, struct exynos_drm_overlay *overlay); void (*win_commit)(void *ctx, int zpos); void (*win_disable)(void *ctx, int zpos); + void (*apply)(void *ctx); };
void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx); diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 21db895..2506567 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -810,6 +810,20 @@ static void mixer_win_disable(void *ctx, int win) mixer_ctx->win_data[win].enabled = false; }
+static void mixer_apply(void *ctx) +{ + struct mixer_context *mixer_ctx = ctx; + int i; + + DRM_DEBUG_KMS("%s\n", __FILE__); + + for (i = 0; i < MIXER_WIN_NR; i++) { + struct hdmi_win_data *win_data = &mixer_ctx->win_data[i]; + if (win_data->enabled) + mixer_win_commit(ctx, i); + } +} + static void mixer_wait_for_vblank(void *ctx) { struct mixer_context *mixer_ctx = ctx; @@ -841,7 +855,8 @@ static void mixer_window_suspend(struct mixer_context *ctx) for (i = 0; i < MIXER_WIN_NR; i++) { win_data = &ctx->win_data[i]; win_data->resume = win_data->enabled; - mixer_win_disable(ctx, i); + if (win_data->enabled) + mixer_win_disable(ctx, i); } mixer_wait_for_vblank(ctx); } @@ -947,6 +962,7 @@ static struct exynos_mixer_ops mixer_ops = { .win_mode_set = mixer_win_mode_set, .win_commit = mixer_win_commit, .win_disable = mixer_win_disable, + .apply = mixer_apply, };
/* for pageflip event */
This patch adds a check in fimd_window_suspend to call win_disable only if the window is enabled.
Signed-off-by: Prathyush K prathyush.k@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index bf0d9ba..4d1d9c1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -852,8 +852,10 @@ static void fimd_window_suspend(struct device *dev)
for (i = 0; i < WINDOWS_NR; i++) { win_data = &ctx->win_data[i]; - win_data->resume = win_data->enabled; - fimd_win_disable(dev, i); + if (win_data->enabled) { + win_data->resume = win_data->enabled; + fimd_win_disable(dev, i); + } } fimd_wait_for_vblank(dev); }
This patch checks if vblank is enabled inside wait for vblank. If not enabled, vblank is temporarily enabled before waiting and then disabled afterwards. This ensures that the wait never times out.
Signed-off-by: Prathyush K prathyush.k@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 4d1d9c1..f88eaa4 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -335,10 +335,19 @@ static void fimd_disable_vblank(struct device *dev) static void fimd_wait_for_vblank(struct device *dev) { struct fimd_context *ctx = get_fimd_context(dev); + u32 val; + bool vblank_enabled = true;
if (ctx->suspended) return;
+ val = readl(ctx->regs + VIDINTCON0); + + if (!(val & VIDINTCON0_INT_FRAME)) { + vblank_enabled = false; + fimd_enable_vblank(dev); + } + atomic_set(&ctx->wait_vsync_event, 1);
/* @@ -349,6 +358,9 @@ static void fimd_wait_for_vblank(struct device *dev) !atomic_read(&ctx->wait_vsync_event), DRM_HZ/20)) DRM_DEBUG_KMS("vblank wait timed out.\n"); + + if (!vblank_enabled) + fimd_disable_vblank(dev); }
static struct exynos_drm_manager_ops fimd_manager_ops = {
From: Akshu Agrawal akshu.a@samsung.com
If any fimd channel was already active, initializing iommu will result in a PAGE FAULT (e.g. u-boot could have turned on the display and not disabled it before the kernel starts). This patch checks if any channel is active before initializing iommu and disables it.
Signed-off-by: Akshu Agrawal akshu.a@samsung.com Signed-off-by: Prathyush K prathyush.k@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index f88eaa4..3aeedf5 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -733,6 +733,28 @@ out: return IRQ_HANDLED; }
+static void fimd_clear_channel(struct device *dev) +{ + struct fimd_context *ctx = get_fimd_context(dev); + int win, ch_enabled = 0; + + DRM_DEBUG_KMS("%s\n", __FILE__); + + /* Check if any channel is enabled */ + for (win = 0; win < WINDOWS_NR; win++) { + u32 val = readl(ctx->regs + SHADOWCON); + if (val & SHADOWCON_CHx_ENABLE(win)) { + val &= ~SHADOWCON_CHx_ENABLE(win); + writel(val, ctx->regs + SHADOWCON); + ch_enabled = 1; + } + } + + /* Wait for vsync, as disable channel takes effect at next vsync */ + if (ch_enabled) + fimd_wait_for_vblank(dev); +} + static int fimd_subdrv_probe(struct drm_device *drm_dev, struct device *dev) { DRM_DEBUG_KMS("%s\n", __FILE__); @@ -755,9 +777,14 @@ static int fimd_subdrv_probe(struct drm_device *drm_dev, struct device *dev) drm_dev->vblank_disable_allowed = 1;
/* attach this sub driver to iommu mapping if supported. */ - if (is_drm_iommu_supported(drm_dev)) + if (is_drm_iommu_supported(drm_dev)) { + /* + * If any channel is already active, iommu will throw + * a PAGE FAULT when enabled. So clear any channel if enabled. + */ + fimd_clear_channel(dev); drm_iommu_attach_device(drm_dev, dev); - + } return 0; }
2012/12/26 Prathyush K prathyush.k@samsung.com
From: Akshu Agrawal akshu.a@samsung.com
If any fimd channel was already active, initializing iommu will result in a PAGE FAULT (e.g. u-boot could have turned on the display and not disabled it before the kernel starts). This patch checks if any channel is active before initializing iommu and disables it.
Signed-off-by: Akshu Agrawal akshu.a@samsung.com Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index f88eaa4..3aeedf5 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -733,6 +733,28 @@ out: return IRQ_HANDLED; }
+static void fimd_clear_channel(struct device *dev) +{
struct fimd_context *ctx = get_fimd_context(dev);int win, ch_enabled = 0;DRM_DEBUG_KMS("%s\n", __FILE__);/* Check if any channel is enabled */for (win = 0; win < WINDOWS_NR; win++) {u32 val = readl(ctx->regs + SHADOWCON);if (val & SHADOWCON_CHx_ENABLE(win)) {val &= ~SHADOWCON_CHx_ENABLE(win);writel(val, ctx->regs + SHADOWCON);ch_enabled = 1;}}/* Wait for vsync, as disable channel takes effect at next vsync */if (ch_enabled)fimd_wait_for_vblank(dev);+}
static int fimd_subdrv_probe(struct drm_device *drm_dev, struct device *dev) { DRM_DEBUG_KMS("%s\n", __FILE__); @@ -755,9 +777,14 @@ static int fimd_subdrv_probe(struct drm_device *drm_dev, struct device *dev) drm_dev->vblank_disable_allowed = 1;
/* attach this sub driver to iommu mapping if supported. */
if (is_drm_iommu_supported(drm_dev))
if (is_drm_iommu_supported(drm_dev)) {/** If any channel is already active, iommu will throw* a PAGE FAULT when enabled. So clear any channel ifenabled.
*/fimd_clear_channel(dev);
Right, we had this issue that when booted with iommu, page fault occurs. But the reason we didn't disable all dma channel is that this makes the screen to be blinked. Actually, we are using some codes internally to avoid the blinking issue but this code is not generic(enabling iommu at vsync). Anyway, this patch should be applied for now. But let's find a more generic way to avoid that issue later.
drm_iommu_attach_device(drm_dev, dev);
} return 0;}
-- 1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Dec 26, 2012 at 3:27 AM, Prathyush K prathyush.k@samsung.com wrote:
From: Akshu Agrawal akshu.a@samsung.com
If any fimd channel was already active, initializing iommu will result in a PAGE FAULT (e.g. u-boot could have turned on the display and not disabled it before the kernel starts). This patch checks if any channel is active before initializing iommu and disables it.
Will that work if another drm exynos platform driver (let's say hdmi) starts before fimd and enables the iommu?
Stéphane
Signed-off-by: Akshu Agrawal akshu.a@samsung.com Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index f88eaa4..3aeedf5 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -733,6 +733,28 @@ out: return IRQ_HANDLED; }
+static void fimd_clear_channel(struct device *dev) +{
struct fimd_context *ctx = get_fimd_context(dev);int win, ch_enabled = 0;DRM_DEBUG_KMS("%s\n", __FILE__);/* Check if any channel is enabled */for (win = 0; win < WINDOWS_NR; win++) {u32 val = readl(ctx->regs + SHADOWCON);if (val & SHADOWCON_CHx_ENABLE(win)) {val &= ~SHADOWCON_CHx_ENABLE(win);writel(val, ctx->regs + SHADOWCON);ch_enabled = 1;}}/* Wait for vsync, as disable channel takes effect at next vsync */if (ch_enabled)fimd_wait_for_vblank(dev);+}
static int fimd_subdrv_probe(struct drm_device *drm_dev, struct device *dev) { DRM_DEBUG_KMS("%s\n", __FILE__); @@ -755,9 +777,14 @@ static int fimd_subdrv_probe(struct drm_device *drm_dev, struct device *dev) drm_dev->vblank_disable_allowed = 1;
/* attach this sub driver to iommu mapping if supported. */
if (is_drm_iommu_supported(drm_dev))
if (is_drm_iommu_supported(drm_dev)) {/** If any channel is already active, iommu will throw* a PAGE FAULT when enabled. So clear any channel if enabled.*/fimd_clear_channel(dev); drm_iommu_attach_device(drm_dev, dev);
} return 0;}
-- 1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jan 3, 2013 at 2:33 AM, Stéphane Marchesin < stephane.marchesin@gmail.com> wrote:
On Wed, Dec 26, 2012 at 3:27 AM, Prathyush K prathyush.k@samsung.com wrote:
From: Akshu Agrawal akshu.a@samsung.com
If any fimd channel was already active, initializing iommu will result in a PAGE FAULT (e.g. u-boot could have turned on the display and not disabled it before the kernel starts). This patch checks if any channel is active before initializing iommu and disables it.
Will that work if another drm exynos platform driver (let's say hdmi) starts before fimd and enables the iommu?
Stéphane
How can hdmi enable fimd's iommu?
iommu initialization has two parts in drm. A common mapping gets created first. This mapping is common to all devices of drm - fimd, mixer, exynos-drm etc. Then each device - calls attach_device to turn on its own iommu. so even if hdmi starts before fimd, hdmi will turn on its own iommu and will not affect fimd. Hope that clears your doubt.
Regards, Prathyush
Signed-off-by: Akshu Agrawal akshu.a@samsung.com Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 31
+++++++++++++++++++++++++++++--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index f88eaa4..3aeedf5 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -733,6 +733,28 @@ out: return IRQ_HANDLED; }
+static void fimd_clear_channel(struct device *dev) +{
struct fimd_context *ctx = get_fimd_context(dev);int win, ch_enabled = 0;DRM_DEBUG_KMS("%s\n", __FILE__);/* Check if any channel is enabled */for (win = 0; win < WINDOWS_NR; win++) {u32 val = readl(ctx->regs + SHADOWCON);if (val & SHADOWCON_CHx_ENABLE(win)) {val &= ~SHADOWCON_CHx_ENABLE(win);writel(val, ctx->regs + SHADOWCON);ch_enabled = 1;}}/* Wait for vsync, as disable channel takes effect at next vsync*/
if (ch_enabled)fimd_wait_for_vblank(dev);+}
static int fimd_subdrv_probe(struct drm_device *drm_dev, struct device
*dev)
{ DRM_DEBUG_KMS("%s\n", __FILE__); @@ -755,9 +777,14 @@ static int fimd_subdrv_probe(struct drm_device
*drm_dev, struct device *dev)
drm_dev->vblank_disable_allowed = 1; /* attach this sub driver to iommu mapping if supported. */
if (is_drm_iommu_supported(drm_dev))
if (is_drm_iommu_supported(drm_dev)) {/** If any channel is already active, iommu will throw* a PAGE FAULT when enabled. So clear any channel ifenabled.
*/fimd_clear_channel(dev); drm_iommu_attach_device(drm_dev, dev);
} return 0;}
-- 1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Sean Paul seanpaul@chromium.org
We should not finish a pageflip or set wait_for_vblank to zero if a layer update is pending. This might result in a page fault or corruption on screen. This patch adds a check in the irq handler to exit if a layer update is pending. Also, calls layer_update only once per layer per vsync.
Signed-off-by: Sean Paul seanpaul@chromium.org Signed-off-by: Prathyush K prathyush.k@samsung.com --- drivers/gpu/drm/exynos/exynos_mixer.c | 35 ++++++++++++++++++++++++++++++----- drivers/gpu/drm/exynos/regs-mixer.h | 1 + 2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 2506567..3369d57 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -58,6 +58,7 @@ struct hdmi_win_data { unsigned int mode_width; unsigned int mode_height; unsigned int scan_flags; + bool updated; bool enabled; bool resume; }; @@ -486,16 +487,21 @@ static void vp_video_buffer(struct mixer_context *ctx, int win) vp_regs_dump(ctx); }
-static void mixer_layer_update(struct mixer_context *ctx) +static int mixer_get_layer_update_count(struct mixer_context *ctx) { struct mixer_resources *res = &ctx->mixer_res; u32 val;
val = mixer_reg_read(res, MXR_CFG);
- /* allow one update per vsync only */ - if (!(val & MXR_CFG_LAYER_UPDATE_COUNT_MASK)) - mixer_reg_writemask(res, MXR_CFG, ~0, MXR_CFG_LAYER_UPDATE); + return (val & MXR_CFG_LAYER_UPDATE_COUNT_MASK) >> + MXR_CFG_LAYER_UPDATE_COUNT0; +} + +static void mixer_layer_update(struct mixer_context *ctx) +{ + struct mixer_resources *res = &ctx->mixer_res; + mixer_reg_writemask(res, MXR_CFG, ~0, MXR_CFG_LAYER_UPDATE); }
static void mixer_graph_buffer(struct mixer_context *ctx, int win) @@ -547,6 +553,11 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) ctx->interlace = false;
spin_lock_irqsave(&res->reg_slock, flags); + + /* Only allow one update per vsync */ + if (ctx->mxr_ver == MXR_VER_16_0_33_0 && win_data->updated) + goto end; + mixer_vsync_set_update(ctx, false);
/* setup format */ @@ -580,12 +591,15 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) mixer_cfg_layer(ctx, win, true);
/* layer update mandatory for mixer 16.0.33.0 */ - if (ctx->mxr_ver == MXR_VER_16_0_33_0) + if (ctx->mxr_ver == MXR_VER_16_0_33_0) { mixer_layer_update(ctx); + win_data->updated = true; + }
mixer_run(ctx);
mixer_vsync_set_update(ctx, true); +end: spin_unlock_irqrestore(&res->reg_slock, flags); }
@@ -1000,6 +1014,7 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg) struct mixer_context *ctx = drm_hdmi_ctx->ctx; struct mixer_resources *res = &ctx->mixer_res; u32 val, base, shadow; + int i;
spin_lock(&res->reg_slock);
@@ -1022,6 +1037,16 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg) }
drm_handle_vblank(drm_hdmi_ctx->drm_dev, ctx->pipe); + + if (ctx->mxr_ver == MXR_VER_16_0_33_0) { + /* Bail out if a layer update is pending */ + if (mixer_get_layer_update_count(ctx)) + goto out; + + for (i = 0; i < MIXER_WIN_NR; i++) + ctx->win_data[i].updated = false; + } + mixer_finish_pageflip(drm_hdmi_ctx->drm_dev, ctx->pipe);
/* set wait vsync event to zero and wake up queue. */ diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h index 5d8dbc0..bad2b99 100644 --- a/drivers/gpu/drm/exynos/regs-mixer.h +++ b/drivers/gpu/drm/exynos/regs-mixer.h @@ -79,6 +79,7 @@
/* bits for MXR_CFG */ #define MXR_CFG_LAYER_UPDATE (1 << 31) +#define MXR_CFG_LAYER_UPDATE_COUNT0 29 #define MXR_CFG_LAYER_UPDATE_COUNT_MASK (3 << 29) #define MXR_CFG_RGB601_0_255 (0 << 9) #define MXR_CFG_RGB601_16_235 (1 << 9)
Before freeing a framebuffer, we call complete_scanout encoder function which just calls wait for vblank of all the encoders. This is not a very optimized method since a framebuffer might not be in use by a crtc and we end up waiting for vblank unnecessarily.
Instead, this patch modifies the wait_for_vblank interface to complete_scanout interface. In this function, each crtc must check if the fb is currently being used. If it is being used, it must wait for vsync (or even disable a window if necessary) and ensure that the buffer is no longer used by crtc before returning. So if a crtc is not actually reading from the buffer, the complete scanout function will just return and not wait for vsync.
Signed-off-by: Prathyush K prathyush.k@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_drv.h | 7 ++++--- drivers/gpu/drm/exynos/exynos_drm_encoder.c | 23 +++++++++++------------ drivers/gpu/drm/exynos/exynos_drm_encoder.h | 4 +++- drivers/gpu/drm/exynos/exynos_drm_fb.c | 13 +++++++++++-- 4 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index b9e51bc..d351daf 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -167,8 +167,7 @@ struct exynos_drm_display_ops { * @commit: set current hw specific display mode to hw. * @enable_vblank: specific driver callback for enabling vblank interrupt. * @disable_vblank: specific driver callback for disabling vblank interrupt. - * @wait_for_vblank: wait for vblank interrupt to make sure that - * hardware overlay is updated. + * @complete_scanout: complete scan of buffer so that it can be freed. */ struct exynos_drm_manager_ops { void (*dpms)(struct device *subdrv_dev, int mode); @@ -183,7 +182,9 @@ struct exynos_drm_manager_ops { void (*commit)(struct device *subdrv_dev); int (*enable_vblank)(struct device *subdrv_dev); void (*disable_vblank)(struct device *subdrv_dev); - void (*wait_for_vblank)(struct device *subdrv_dev); + void (*complete_scanout)(struct device *subdrv_dev, + dma_addr_t dma_addr, + unsigned long size); };
/* diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c index c63721f..7e772ed 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c @@ -220,28 +220,27 @@ static void exynos_drm_encoder_commit(struct drm_encoder *encoder) exynos_encoder->dpms = DRM_MODE_DPMS_ON; }
-void exynos_drm_encoder_complete_scanout(struct drm_framebuffer *fb) +void exynos_drm_encoder_complete_scanout(struct drm_device *drm_dev, + dma_addr_t dma_addr, + unsigned long size) { struct exynos_drm_encoder *exynos_encoder; struct exynos_drm_manager_ops *ops; - struct drm_device *dev = fb->dev; struct drm_encoder *encoder; + struct device *dev;
- /* - * make sure that overlay data are updated to real hardware - * for all encoders. - */ - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + /* make sure that current framebuffer is not in use for all encoders */ + list_for_each_entry(encoder, &drm_dev->mode_config.encoder_list, head) { exynos_encoder = to_exynos_encoder(encoder); ops = exynos_encoder->manager->ops; + dev = exynos_encoder->manager->dev;
/* - * wait for vblank interrupt - * - this makes sure that overlay data are updated to - * real hardware. + * complete_scanout + * - this makes sure that framebuffer is not in use. */ - if (ops->wait_for_vblank) - ops->wait_for_vblank(exynos_encoder->manager->dev); + if (ops->complete_scanout) + ops->complete_scanout(dev, dma_addr, size); } }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.h b/drivers/gpu/drm/exynos/exynos_drm_encoder.h index 89e2fb0..b85211b 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.h +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.h @@ -32,6 +32,8 @@ void exynos_drm_encoder_plane_mode_set(struct drm_encoder *encoder, void *data); void exynos_drm_encoder_plane_commit(struct drm_encoder *encoder, void *data); void exynos_drm_encoder_plane_enable(struct drm_encoder *encoder, void *data); void exynos_drm_encoder_plane_disable(struct drm_encoder *encoder, void *data); -void exynos_drm_encoder_complete_scanout(struct drm_framebuffer *fb); +void exynos_drm_encoder_complete_scanout(struct drm_device *drm_dev, + dma_addr_t dma_addr, + unsigned long size);
#endif diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 294c051..1ef684a 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -69,11 +69,20 @@ static void exynos_drm_fb_destroy(struct drm_framebuffer *fb) { struct exynos_drm_fb *exynos_fb = to_exynos_fb(fb); unsigned int i; + dma_addr_t dma_addr; + unsigned long size;
DRM_DEBUG_KMS("%s\n", __FILE__);
- /* make sure that overlay data are updated before relesing fb. */ - exynos_drm_encoder_complete_scanout(fb); + for (i = 0; i < ARRAY_SIZE(exynos_fb->exynos_gem_obj); i++) { + if (exynos_fb->exynos_gem_obj[i] == NULL) + continue; + + dma_addr = exynos_fb->exynos_gem_obj[i]->buffer->dma_addr; + size = exynos_fb->exynos_gem_obj[i]->buffer->size; + + exynos_drm_encoder_complete_scanout(fb->dev, dma_addr, size); + }
drm_framebuffer_cleanup(fb);
The wait_for_vblank interface is modified to the complete_scanout function in hdmi. This inturn calls the complete_scanout mixer op. This patch also adds the mixer_complete_scanout function.
Inside mixer_complete_scanout, we read the base register and shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the base register, the mixer window is disabled. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 11 ++++++---- drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 3 ++- drivers/gpu/drm/exynos/exynos_mixer.c | 37 +++++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c index d8ae47e..e32eb1c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c @@ -177,14 +177,17 @@ static void drm_hdmi_disable_vblank(struct device *subdrv_dev) return mixer_ops->disable_vblank(ctx->mixer_ctx->ctx); }
-static void drm_hdmi_wait_for_vblank(struct device *subdrv_dev) +static void drm_hdmi_complete_scanout(struct device *subdrv_dev, + dma_addr_t dma_addr, + unsigned long size) { struct drm_hdmi_context *ctx = to_context(subdrv_dev);
DRM_DEBUG_KMS("%s\n", __FILE__);
- if (mixer_ops && mixer_ops->wait_for_vblank) - mixer_ops->wait_for_vblank(ctx->mixer_ctx->ctx); + if (mixer_ops && mixer_ops->complete_scanout) + mixer_ops->complete_scanout(ctx->mixer_ctx->ctx, + dma_addr, size); }
static void drm_hdmi_mode_fixup(struct device *subdrv_dev, @@ -263,7 +266,7 @@ static struct exynos_drm_manager_ops drm_hdmi_manager_ops = { .apply = drm_hdmi_apply, .enable_vblank = drm_hdmi_enable_vblank, .disable_vblank = drm_hdmi_disable_vblank, - .wait_for_vblank = drm_hdmi_wait_for_vblank, + .complete_scanout = drm_hdmi_complete_scanout, .mode_fixup = drm_hdmi_mode_fixup, .mode_set = drm_hdmi_mode_set, .get_max_resol = drm_hdmi_get_max_resol, diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h index 4fad00c..7d5bf00 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h @@ -51,7 +51,8 @@ struct exynos_mixer_ops { int (*iommu_on)(void *ctx, bool enable); int (*enable_vblank)(void *ctx, int pipe); void (*disable_vblank)(void *ctx); - void (*wait_for_vblank)(void *ctx); + void (*complete_scanout)(void *ctx, dma_addr_t dma_addr, + unsigned long size); void (*dpms)(void *ctx, int mode);
/* overlay */ diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 3369d57..151e13f 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -861,6 +861,41 @@ static void mixer_wait_for_vblank(void *ctx) DRM_DEBUG_KMS("vblank wait timed out.\n"); }
+static void mixer_complete_scanout(void *ctx, dma_addr_t dma_addr, + unsigned long size) +{ + struct mixer_context *mixer_ctx = ctx; + struct mixer_resources *res = &mixer_ctx->mixer_res; + int win; + bool in_use = false; + bool in_use_s = false; + + if (!mixer_ctx->powered) + return; + + for (win = 0; win < MIXER_WIN_NR; win++) { + dma_addr_t dma_addr_in_use; + + if (!mixer_ctx->win_data[win].enabled) + continue; + + dma_addr_in_use = mixer_reg_read(res, MXR_GRAPHIC_BASE(win)); + if (dma_addr_in_use >= dma_addr && + dma_addr_in_use < (dma_addr + size)) { + mixer_win_disable(ctx, win); + in_use = true; + } + + dma_addr_in_use = mixer_reg_read(res, MXR_GRAPHIC_BASE_S(win)); + if (dma_addr_in_use >= dma_addr && + dma_addr_in_use < (dma_addr + size)) + in_use_s = true; + } + + if (in_use) + mixer_wait_for_vblank(ctx); +} + static void mixer_window_suspend(struct mixer_context *ctx) { struct hdmi_win_data *win_data; @@ -969,7 +1004,7 @@ static struct exynos_mixer_ops mixer_ops = { .iommu_on = mixer_iommu_on, .enable_vblank = mixer_enable_vblank, .disable_vblank = mixer_disable_vblank, - .wait_for_vblank = mixer_wait_for_vblank, + .complete_scanout = mixer_complete_scanout, .dpms = mixer_dpms,
/* overlay */
On Wed, Dec 26, 2012 at 6:27 AM, Prathyush K prathyush.k@samsung.com wrote:
The wait_for_vblank interface is modified to the complete_scanout function in hdmi. This inturn calls the complete_scanout mixer op. This patch also adds the mixer_complete_scanout function.
Inside mixer_complete_scanout, we read the base register and shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the base register, the mixer window is disabled. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 11 ++++++---- drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 3 ++- drivers/gpu/drm/exynos/exynos_mixer.c | 37 +++++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c index d8ae47e..e32eb1c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c @@ -177,14 +177,17 @@ static void drm_hdmi_disable_vblank(struct device *subdrv_dev) return mixer_ops->disable_vblank(ctx->mixer_ctx->ctx); }
-static void drm_hdmi_wait_for_vblank(struct device *subdrv_dev) +static void drm_hdmi_complete_scanout(struct device *subdrv_dev,
dma_addr_t dma_addr,unsigned long size){ struct drm_hdmi_context *ctx = to_context(subdrv_dev);
DRM_DEBUG_KMS("%s\n", __FILE__);
if (mixer_ops && mixer_ops->wait_for_vblank)mixer_ops->wait_for_vblank(ctx->mixer_ctx->ctx);
if (mixer_ops && mixer_ops->complete_scanout)mixer_ops->complete_scanout(ctx->mixer_ctx->ctx,dma_addr, size);}
static void drm_hdmi_mode_fixup(struct device *subdrv_dev, @@ -263,7 +266,7 @@ static struct exynos_drm_manager_ops drm_hdmi_manager_ops = { .apply = drm_hdmi_apply, .enable_vblank = drm_hdmi_enable_vblank, .disable_vblank = drm_hdmi_disable_vblank,
.wait_for_vblank = drm_hdmi_wait_for_vblank,
.complete_scanout = drm_hdmi_complete_scanout, .mode_fixup = drm_hdmi_mode_fixup, .mode_set = drm_hdmi_mode_set, .get_max_resol = drm_hdmi_get_max_resol,diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h index 4fad00c..7d5bf00 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h @@ -51,7 +51,8 @@ struct exynos_mixer_ops { int (*iommu_on)(void *ctx, bool enable); int (*enable_vblank)(void *ctx, int pipe); void (*disable_vblank)(void *ctx);
void (*wait_for_vblank)(void *ctx);
void (*complete_scanout)(void *ctx, dma_addr_t dma_addr,unsigned long size); void (*dpms)(void *ctx, int mode); /* overlay */diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 3369d57..151e13f 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -861,6 +861,41 @@ static void mixer_wait_for_vblank(void *ctx) DRM_DEBUG_KMS("vblank wait timed out.\n"); }
+static void mixer_complete_scanout(void *ctx, dma_addr_t dma_addr,
unsigned long size)+{
struct mixer_context *mixer_ctx = ctx;struct mixer_resources *res = &mixer_ctx->mixer_res;int win;bool in_use = false;bool in_use_s = false;if (!mixer_ctx->powered)
Accesses to mixer_ctx->powered are always protected by mixer_mutex, this should probably stay consistent (either way).
return;for (win = 0; win < MIXER_WIN_NR; win++) {dma_addr_t dma_addr_in_use;if (!mixer_ctx->win_data[win].enabled)continue;dma_addr_in_use = mixer_reg_read(res, MXR_GRAPHIC_BASE(win));if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size)) {mixer_win_disable(ctx, win);in_use = true;}dma_addr_in_use = mixer_reg_read(res, MXR_GRAPHIC_BASE_S(win));if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size))in_use_s = true;
You don't use this anywhere in the code. I think bad things will happen if the framebuffer is in a shadow register and we free it.
I also wonder if you should wrap this all with mixer_vsync_set_update so things don't change while you're doing this.
Sean
}if (in_use)mixer_wait_for_vblank(ctx);+}
static void mixer_window_suspend(struct mixer_context *ctx) { struct hdmi_win_data *win_data; @@ -969,7 +1004,7 @@ static struct exynos_mixer_ops mixer_ops = { .iommu_on = mixer_iommu_on, .enable_vblank = mixer_enable_vblank, .disable_vblank = mixer_disable_vblank,
.wait_for_vblank = mixer_wait_for_vblank,
.complete_scanout = mixer_complete_scanout, .dpms = mixer_dpms, /* overlay */-- 1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jan 2, 2013 at 10:12 PM, Sean Paul seanpaul@chromium.org wrote:
On Wed, Dec 26, 2012 at 6:27 AM, Prathyush K prathyush.k@samsung.com wrote:
The wait_for_vblank interface is modified to the complete_scanout function in hdmi. This inturn calls the complete_scanout mixer op. This patch also adds the mixer_complete_scanout function.
Inside mixer_complete_scanout, we read the base register and shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the base register, the mixer window is disabled. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 11 ++++++---- drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 3 ++- drivers/gpu/drm/exynos/exynos_mixer.c | 37
+++++++++++++++++++++++++++++++-
3 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
index d8ae47e..e32eb1c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c @@ -177,14 +177,17 @@ static void drm_hdmi_disable_vblank(struct device
*subdrv_dev)
return mixer_ops->disable_vblank(ctx->mixer_ctx->ctx);}
-static void drm_hdmi_wait_for_vblank(struct device *subdrv_dev) +static void drm_hdmi_complete_scanout(struct device *subdrv_dev,
dma_addr_t dma_addr,unsigned long size){ struct drm_hdmi_context *ctx = to_context(subdrv_dev);
DRM_DEBUG_KMS("%s\n", __FILE__);
if (mixer_ops && mixer_ops->wait_for_vblank)mixer_ops->wait_for_vblank(ctx->mixer_ctx->ctx);
if (mixer_ops && mixer_ops->complete_scanout)mixer_ops->complete_scanout(ctx->mixer_ctx->ctx,dma_addr, size);}
static void drm_hdmi_mode_fixup(struct device *subdrv_dev, @@ -263,7 +266,7 @@ static struct exynos_drm_manager_ops
drm_hdmi_manager_ops = {
.apply = drm_hdmi_apply, .enable_vblank = drm_hdmi_enable_vblank, .disable_vblank = drm_hdmi_disable_vblank,
.wait_for_vblank = drm_hdmi_wait_for_vblank,
.complete_scanout = drm_hdmi_complete_scanout, .mode_fixup = drm_hdmi_mode_fixup, .mode_set = drm_hdmi_mode_set, .get_max_resol = drm_hdmi_get_max_resol,diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
index 4fad00c..7d5bf00 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h @@ -51,7 +51,8 @@ struct exynos_mixer_ops { int (*iommu_on)(void *ctx, bool enable); int (*enable_vblank)(void *ctx, int pipe); void (*disable_vblank)(void *ctx);
void (*wait_for_vblank)(void *ctx);
void (*complete_scanout)(void *ctx, dma_addr_t dma_addr,unsigned long size); void (*dpms)(void *ctx, int mode); /* overlay */diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
b/drivers/gpu/drm/exynos/exynos_mixer.c
index 3369d57..151e13f 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -861,6 +861,41 @@ static void mixer_wait_for_vblank(void *ctx) DRM_DEBUG_KMS("vblank wait timed out.\n"); }
+static void mixer_complete_scanout(void *ctx, dma_addr_t dma_addr,
unsigned long size)+{
struct mixer_context *mixer_ctx = ctx;struct mixer_resources *res = &mixer_ctx->mixer_res;int win;bool in_use = false;bool in_use_s = false;if (!mixer_ctx->powered)Accesses to mixer_ctx->powered are always protected by mixer_mutex, this should probably stay consistent (either way).
Hi Sean,
Right, thanks for pointing that out. I'll update in next patch set.
return;for (win = 0; win < MIXER_WIN_NR; win++) {dma_addr_t dma_addr_in_use;if (!mixer_ctx->win_data[win].enabled)continue;dma_addr_in_use = mixer_reg_read(res,MXR_GRAPHIC_BASE(win));
if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size)) {mixer_win_disable(ctx, win);in_use = true;}dma_addr_in_use = mixer_reg_read(res,MXR_GRAPHIC_BASE_S(win));
if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size))in_use_s = true;You don't use this anywhere in the code. I think bad things will happen if the framebuffer is in a shadow register and we free it.
I don't get your point here. What is not used anywhere in the code?
Please check the comments on the fimd patch.
I also wonder if you should wrap this all with mixer_vsync_set_update so things don't change while you're doing this.
Haven't thought of that. Will definitely consider that for next patch set.
Thanks for reviewing,
Regards, Prathyush
Sean
}if (in_use)mixer_wait_for_vblank(ctx);+}
static void mixer_window_suspend(struct mixer_context *ctx) { struct hdmi_win_data *win_data; @@ -969,7 +1004,7 @@ static struct exynos_mixer_ops mixer_ops = { .iommu_on = mixer_iommu_on, .enable_vblank = mixer_enable_vblank, .disable_vblank = mixer_disable_vblank,
.wait_for_vblank = mixer_wait_for_vblank,
.complete_scanout = mixer_complete_scanout, .dpms = mixer_dpms, /* overlay */-- 1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jan 3, 2013 at 7:58 AM, Prathyush K prathyush@chromium.org wrote:
On Wed, Jan 2, 2013 at 10:12 PM, Sean Paul seanpaul@chromium.org wrote:
On Wed, Dec 26, 2012 at 6:27 AM, Prathyush K prathyush.k@samsung.com wrote:
The wait_for_vblank interface is modified to the complete_scanout function in hdmi. This inturn calls the complete_scanout mixer op. This patch also adds the mixer_complete_scanout function.
Inside mixer_complete_scanout, we read the base register and shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the base register, the mixer window is disabled. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 11 ++++++---- drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 3 ++- drivers/gpu/drm/exynos/exynos_mixer.c | 37 +++++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c index d8ae47e..e32eb1c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c @@ -177,14 +177,17 @@ static void drm_hdmi_disable_vblank(struct device *subdrv_dev) return mixer_ops->disable_vblank(ctx->mixer_ctx->ctx); }
-static void drm_hdmi_wait_for_vblank(struct device *subdrv_dev) +static void drm_hdmi_complete_scanout(struct device *subdrv_dev,
dma_addr_t dma_addr,unsigned long size){ struct drm_hdmi_context *ctx = to_context(subdrv_dev);
DRM_DEBUG_KMS("%s\n", __FILE__);
if (mixer_ops && mixer_ops->wait_for_vblank)mixer_ops->wait_for_vblank(ctx->mixer_ctx->ctx);
if (mixer_ops && mixer_ops->complete_scanout)mixer_ops->complete_scanout(ctx->mixer_ctx->ctx,dma_addr, size);}
static void drm_hdmi_mode_fixup(struct device *subdrv_dev, @@ -263,7 +266,7 @@ static struct exynos_drm_manager_ops drm_hdmi_manager_ops = { .apply = drm_hdmi_apply, .enable_vblank = drm_hdmi_enable_vblank, .disable_vblank = drm_hdmi_disable_vblank,
.wait_for_vblank = drm_hdmi_wait_for_vblank,
.complete_scanout = drm_hdmi_complete_scanout, .mode_fixup = drm_hdmi_mode_fixup, .mode_set = drm_hdmi_mode_set, .get_max_resol = drm_hdmi_get_max_resol,diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h index 4fad00c..7d5bf00 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h @@ -51,7 +51,8 @@ struct exynos_mixer_ops { int (*iommu_on)(void *ctx, bool enable); int (*enable_vblank)(void *ctx, int pipe); void (*disable_vblank)(void *ctx);
void (*wait_for_vblank)(void *ctx);
void (*complete_scanout)(void *ctx, dma_addr_t dma_addr,unsigned long size); void (*dpms)(void *ctx, int mode); /* overlay */diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 3369d57..151e13f 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -861,6 +861,41 @@ static void mixer_wait_for_vblank(void *ctx) DRM_DEBUG_KMS("vblank wait timed out.\n"); }
+static void mixer_complete_scanout(void *ctx, dma_addr_t dma_addr,
unsigned long size)+{
struct mixer_context *mixer_ctx = ctx;struct mixer_resources *res = &mixer_ctx->mixer_res;int win;bool in_use = false;bool in_use_s = false;if (!mixer_ctx->powered)Accesses to mixer_ctx->powered are always protected by mixer_mutex, this should probably stay consistent (either way).
Hi Sean,
Right, thanks for pointing that out. I'll update in next patch set.
return;for (win = 0; win < MIXER_WIN_NR; win++) {dma_addr_t dma_addr_in_use;if (!mixer_ctx->win_data[win].enabled)continue;dma_addr_in_use = mixer_reg_read(res,MXR_GRAPHIC_BASE(win));
if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size)) {mixer_win_disable(ctx, win);in_use = true;}dma_addr_in_use = mixer_reg_read(res,MXR_GRAPHIC_BASE_S(win));
if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size))in_use_s = true;You don't use this anywhere in the code. I think bad things will happen if the framebuffer is in a shadow register and we free it.
I don't get your point here. What is not used anywhere in the code?
The variable in_use_s isn't checked anywhere in your function. You define it as false, set it to true if the fb is in the shadow register, and then never actually check it. You check in_use below (and wait for vblank), but not in_use_s.
Sean
Please check the comments on the fimd patch.
I also wonder if you should wrap this all with mixer_vsync_set_update so things don't change while you're doing this.
Haven't thought of that. Will definitely consider that for next patch set.
Thanks for reviewing,
Regards, Prathyush
Sean
}if (in_use)mixer_wait_for_vblank(ctx);+}
static void mixer_window_suspend(struct mixer_context *ctx) { struct hdmi_win_data *win_data; @@ -969,7 +1004,7 @@ static struct exynos_mixer_ops mixer_ops = { .iommu_on = mixer_iommu_on, .enable_vblank = mixer_enable_vblank, .disable_vblank = mixer_disable_vblank,
.wait_for_vblank = mixer_wait_for_vblank,
.complete_scanout = mixer_complete_scanout, .dpms = mixer_dpms, /* overlay */-- 1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
The wait_for_vblank interface is modified to the complete_scanout function in fimd. This patch adds the fimd_complete_scanout function
Inside fimd_complete_scanout, we read the shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 32 +++++++++++++++++++++++++++++++- include/video/samsung_fimd.h | 1 + 2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 3aeedf5..190ffde9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -46,6 +46,7 @@ #define VIDOSD_D(win) (VIDOSD_BASE + 0x0C + (win) * 16)
#define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8) +#define VIDWx_BUF_START_S(win, buf) (VIDW_BUF_START_S(buf) + (win) * 8) #define VIDWx_BUF_END(win, buf) (VIDW_BUF_END(buf) + (win) * 8) #define VIDWx_BUF_SIZE(win, buf) (VIDW_BUF_SIZE(buf) + (win) * 4)
@@ -363,13 +364,42 @@ static void fimd_wait_for_vblank(struct device *dev) fimd_disable_vblank(dev); }
+static void fimd_complete_scanout(struct device *dev, dma_addr_t dma_addr, + unsigned long size) +{ + struct fimd_context *ctx = get_fimd_context(dev); + int win; + bool in_use = false; + + if (ctx->suspended) + return; + + for (win = 0; win < WINDOWS_NR; win++) { + dma_addr_t dma_addr_in_use; + + if (!ctx->win_data[win].enabled) + continue; + + dma_addr_in_use = readl(ctx->regs + VIDWx_BUF_START_S(win, 0)); + if (dma_addr_in_use >= dma_addr && + dma_addr_in_use < (dma_addr + size)) { + in_use = true; + break; + } + } + + if (in_use) + fimd_wait_for_vblank(dev); + return; +} + static struct exynos_drm_manager_ops fimd_manager_ops = { .dpms = fimd_dpms, .apply = fimd_apply, .commit = fimd_commit, .enable_vblank = fimd_enable_vblank, .disable_vblank = fimd_disable_vblank, - .wait_for_vblank = fimd_wait_for_vblank, + .complete_scanout = fimd_complete_scanout, };
static void fimd_win_mode_set(struct device *dev, diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h index 7ae6c07..382eaec 100644 --- a/include/video/samsung_fimd.h +++ b/include/video/samsung_fimd.h @@ -274,6 +274,7 @@
/* Video buffer addresses */ #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) +#define VIDW_BUF_START_S(_buff) (0x40A0 + ((_buff) * 8)) #define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8)) #define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))
On Wed, Dec 26, 2012 at 6:27 AM, Prathyush K prathyush.k@samsung.com wrote:
The wait_for_vblank interface is modified to the complete_scanout function in fimd. This patch adds the fimd_complete_scanout function
Inside fimd_complete_scanout, we read the shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 32 +++++++++++++++++++++++++++++++- include/video/samsung_fimd.h | 1 + 2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 3aeedf5..190ffde9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -46,6 +46,7 @@ #define VIDOSD_D(win) (VIDOSD_BASE + 0x0C + (win) * 16)
#define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8) +#define VIDWx_BUF_START_S(win, buf) (VIDW_BUF_START_S(buf) + (win) * 8) #define VIDWx_BUF_END(win, buf) (VIDW_BUF_END(buf) + (win) * 8) #define VIDWx_BUF_SIZE(win, buf) (VIDW_BUF_SIZE(buf) + (win) * 4)
@@ -363,13 +364,42 @@ static void fimd_wait_for_vblank(struct device *dev) fimd_disable_vblank(dev); }
+static void fimd_complete_scanout(struct device *dev, dma_addr_t dma_addr,
unsigned long size)+{
struct fimd_context *ctx = get_fimd_context(dev);int win;bool in_use = false;if (ctx->suspended)return;
Is this really possible? If it is, I think there's potential for trouble. It seems possible that an fb is current, but we're suspended. If we exit early here, the fb will be freed and we'll be sad when we come out of suspend.
for (win = 0; win < WINDOWS_NR; win++) {dma_addr_t dma_addr_in_use;if (!ctx->win_data[win].enabled)continue;dma_addr_in_use = readl(ctx->regs + VIDWx_BUF_START_S(win, 0));if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size)) {in_use = true;break;}
I think this has the opposite problem as your mixer patch. You're checking if the framebuffer is in the shadow register, but I don't think this code will wait if the fb is currently being scanned out. I hope I'm just missing something :)
Sean
}if (in_use)fimd_wait_for_vblank(dev);return;+}
static struct exynos_drm_manager_ops fimd_manager_ops = { .dpms = fimd_dpms, .apply = fimd_apply, .commit = fimd_commit, .enable_vblank = fimd_enable_vblank, .disable_vblank = fimd_disable_vblank,
.wait_for_vblank = fimd_wait_for_vblank,
.complete_scanout = fimd_complete_scanout,};
static void fimd_win_mode_set(struct device *dev, diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h index 7ae6c07..382eaec 100644 --- a/include/video/samsung_fimd.h +++ b/include/video/samsung_fimd.h @@ -274,6 +274,7 @@
/* Video buffer addresses */ #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) +#define VIDW_BUF_START_S(_buff) (0x40A0 + ((_buff) * 8)) #define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8))
#define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))
1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jan 2, 2013 at 10:24 PM, Sean Paul seanpaul@chromium.org wrote:
On Wed, Dec 26, 2012 at 6:27 AM, Prathyush K prathyush.k@samsung.com wrote:
The wait_for_vblank interface is modified to the complete_scanout function in fimd. This patch adds the fimd_complete_scanout function
Inside fimd_complete_scanout, we read the shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 32
+++++++++++++++++++++++++++++++-
include/video/samsung_fimd.h | 1 + 2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 3aeedf5..190ffde9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -46,6 +46,7 @@ #define VIDOSD_D(win) (VIDOSD_BASE + 0x0C + (win) * 16)
#define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8) +#define VIDWx_BUF_START_S(win, buf) (VIDW_BUF_START_S(buf) + (win) *
#define VIDWx_BUF_END(win, buf) (VIDW_BUF_END(buf) +
(win) * 8)
#define VIDWx_BUF_SIZE(win, buf) (VIDW_BUF_SIZE(buf) + (win) * 4)
@@ -363,13 +364,42 @@ static void fimd_wait_for_vblank(struct device
*dev)
fimd_disable_vblank(dev);}
+static void fimd_complete_scanout(struct device *dev, dma_addr_t
dma_addr,
unsigned long size)+{
struct fimd_context *ctx = get_fimd_context(dev);int win;bool in_use = false;if (ctx->suspended)return;Is this really possible? If it is, I think there's potential for trouble. It seems possible that an fb is current, but we're suspended. If we exit early here, the fb will be freed and we'll be sad when we come out of suspend.
Hi Sean,
Before suspend, we disable all the windows (and wait for vsync) and keep track of which windows need to be resumed.
If a fb is being removed during suspend, and if that fb was previously set to fimd, we modify the resume flag so that the window will not be resumed during fimd_resume.
for (win = 0; win < WINDOWS_NR; win++) {dma_addr_t dma_addr_in_use;if (!ctx->win_data[win].enabled)continue;dma_addr_in_use = readl(ctx->regs +VIDWx_BUF_START_S(win, 0));
if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size)) {in_use = true;break;}I think this has the opposite problem as your mixer patch. You're checking if the framebuffer is in the shadow register, but I don't think this code will wait if the fb is currently being scanned out. I hope I'm just missing something :)
Right, I do not check for the base register here. This is because I also
consider the disable_crtc function.
Consider the following two cases:
1> fb1 set to fimd and fimd is reading from fb1 (so both base register and shadow register have fb1's dma addr) call remove fb1
since crtc->fb == fb1, the "exynos_drm_crtc_disable" gets called. This will disable the window and also turn off fimd with DPMS_OFF.
fimd dpms off will ensure that the window actually gets disabled by waiting for vblank.
Now before removing fb1, we call complete_scanout. Since fimd is already suspended, this will just return. No issue.
2> fb1 set to fimd, fimd reading from fb1. fb2 set to fimd call remove fb1 (now base register has fb2, shadow register has fb1)
since crtc->fb == fb2, crtc_disable wont be called.
In complete scanout, we check only shadow register (which has fb1). So we wait for vsync, so that next dma starts from fb2 and then we go ahead and remove fb1.
I tried to apply the same logic for mixer: In mixer, it is slightly more complex, since we have layer updates.
Consider the following cases for mixer:
1> fb1 set to mixer (base register has fb1) - also fb1 gets queued with a layer update. mixer is reading from fb1 (shadow register has fb1). remove fb1
crtc->fb == fb1, so win_disable gets called followed by crtc off. This will wait for vsync.
complete_scanout will just return. remove fb1. no issue.
2> mixer reading from fb1 fb2 set to mixer (layer update) remove fb1
In this case, base has fb2, shadow has fb1.
crtc->fb == fb2, so no win_disable.
complete scanout will wait for vsync and ensure dma is from fb2. remove fb1. no issue.
3> mixer reading from fb1. shadow reg has fb1 fb2 set to mixer (layer update - base reg has fb2) fb3 set to mixer (since layer update is pending, win_commit returns) remove fb2
crtc->fb == fb3, so no win_disable
complete scanout: base == fb2 so win_disable and then wait for vsync. remove fb1. no issue
Hope this answers your question. If there is any additional scenario which I missed, do let me know :)
With the above approach, there is no crash in either fimd or hdmi after quite a few attempts at multiple scenarios. I am working on a patch set v2 which I'll post tomorrow with a couple of additional fixes.
Regards, Prathyush
Sean
}if (in_use)fimd_wait_for_vblank(dev);return;+}
static struct exynos_drm_manager_ops fimd_manager_ops = { .dpms = fimd_dpms, .apply = fimd_apply, .commit = fimd_commit, .enable_vblank = fimd_enable_vblank, .disable_vblank = fimd_disable_vblank,
.wait_for_vblank = fimd_wait_for_vblank,
.complete_scanout = fimd_complete_scanout,};
static void fimd_win_mode_set(struct device *dev, diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h index 7ae6c07..382eaec 100644 --- a/include/video/samsung_fimd.h +++ b/include/video/samsung_fimd.h @@ -274,6 +274,7 @@
/* Video buffer addresses */ #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) +#define VIDW_BUF_START_S(_buff) (0x40A0 +
((_buff) * 8))
#define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8))
#define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))
1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jan 3, 2013 at 7:56 AM, Prathyush K prathyush@chromium.org wrote:
On Wed, Jan 2, 2013 at 10:24 PM, Sean Paul seanpaul@chromium.org wrote:
On Wed, Dec 26, 2012 at 6:27 AM, Prathyush K prathyush.k@samsung.com wrote:
The wait_for_vblank interface is modified to the complete_scanout function in fimd. This patch adds the fimd_complete_scanout function
Inside fimd_complete_scanout, we read the shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 32 +++++++++++++++++++++++++++++++- include/video/samsung_fimd.h | 1 + 2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 3aeedf5..190ffde9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -46,6 +46,7 @@ #define VIDOSD_D(win) (VIDOSD_BASE + 0x0C + (win) * 16)
#define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8) +#define VIDWx_BUF_START_S(win, buf) (VIDW_BUF_START_S(buf) + (win) * 8) #define VIDWx_BUF_END(win, buf) (VIDW_BUF_END(buf) + (win) * 8) #define VIDWx_BUF_SIZE(win, buf) (VIDW_BUF_SIZE(buf) + (win) * 4)
@@ -363,13 +364,42 @@ static void fimd_wait_for_vblank(struct device *dev) fimd_disable_vblank(dev); }
+static void fimd_complete_scanout(struct device *dev, dma_addr_t dma_addr,
unsigned long size)+{
struct fimd_context *ctx = get_fimd_context(dev);int win;bool in_use = false;if (ctx->suspended)return;Is this really possible? If it is, I think there's potential for trouble. It seems possible that an fb is current, but we're suspended. If we exit early here, the fb will be freed and we'll be sad when we come out of suspend.
Hi Sean,
Before suspend, we disable all the windows (and wait for vsync) and keep track of which windows need to be resumed.
If a fb is being removed during suspend, and if that fb was previously set to fimd, we modify the resume flag so that the window will not be resumed during fimd_resume.
for (win = 0; win < WINDOWS_NR; win++) {dma_addr_t dma_addr_in_use;if (!ctx->win_data[win].enabled)continue;dma_addr_in_use = readl(ctx->regs +VIDWx_BUF_START_S(win, 0));
if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size)) {in_use = true;break;}I think this has the opposite problem as your mixer patch. You're checking if the framebuffer is in the shadow register, but I don't think this code will wait if the fb is currently being scanned out. I hope I'm just missing something :)
Right, I do not check for the base register here. This is because I also consider the disable_crtc function.
Consider the following two cases:
1> fb1 set to fimd and fimd is reading from fb1 (so both base register and shadow register have fb1's dma addr) call remove fb1
since crtc->fb == fb1, the "exynos_drm_crtc_disable" gets called. This will disable the window and also turn off fimd with DPMS_OFF.
fimd dpms off will ensure that the window actually gets disabled by waiting for vblank.
Now before removing fb1, we call complete_scanout. Since fimd is already suspended, this will just return. No issue.
I see the code path you're talking about. I think I was confused by the "suspended" name, it also covers the disabled/off case.
2> fb1 set to fimd, fimd reading from fb1. fb2 set to fimd call remove fb1 (now base register has fb2, shadow register has fb1)
since crtc->fb == fb2, crtc_disable wont be called.
In complete scanout, we check only shadow register (which has fb1). So we wait for vsync, so that next dma starts from fb2 and then we go ahead and remove fb1.
Ok, I think I've straightened out my mental model of how this works. Thanks for the explanation
I tried to apply the same logic for mixer: In mixer, it is slightly more complex, since we have layer updates.
Consider the following cases for mixer:
1> fb1 set to mixer (base register has fb1) - also fb1 gets queued with a layer update. mixer is reading from fb1 (shadow register has fb1). remove fb1
crtc->fb == fb1, so win_disable gets called followed by crtc off. This will wait for vsync.
complete_scanout will just return. remove fb1. no issue.
2> mixer reading from fb1 fb2 set to mixer (layer update) remove fb1
In this case, base has fb2, shadow has fb1.
crtc->fb == fb2, so no win_disable.
complete scanout will wait for vsync and ensure dma is from fb2. remove fb1. no issue.
3> mixer reading from fb1. shadow reg has fb1 fb2 set to mixer (layer update - base reg has fb2) fb3 set to mixer (since layer update is pending, win_commit returns) remove fb2
crtc->fb == fb3, so no win_disable
complete scanout: base == fb2 so win_disable and then wait for vsync. remove fb1. no issue
I think I see where you're going with this. I think you have a bug in your other patch which I'll reply to separately.
Hope this answers your question. If there is any additional scenario which I missed, do let me know :)
In general, this seems like it's a pretty complex solution to something that should be more simple, but maybe I'm just being dense :)
Sean
With the above approach, there is no crash in either fimd or hdmi after quite a few attempts at multiple scenarios. I am working on a patch set v2 which I'll post tomorrow with a couple of additional fixes.
Regards, Prathyush
Sean
}if (in_use)fimd_wait_for_vblank(dev);return;+}
static struct exynos_drm_manager_ops fimd_manager_ops = { .dpms = fimd_dpms, .apply = fimd_apply, .commit = fimd_commit, .enable_vblank = fimd_enable_vblank, .disable_vblank = fimd_disable_vblank,
.wait_for_vblank = fimd_wait_for_vblank,
.complete_scanout = fimd_complete_scanout,};
static void fimd_win_mode_set(struct device *dev, diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h index 7ae6c07..382eaec 100644 --- a/include/video/samsung_fimd.h +++ b/include/video/samsung_fimd.h @@ -274,6 +274,7 @@
/* Video buffer addresses */ #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) +#define VIDW_BUF_START_S(_buff) (0x40A0 + ((_buff) * 8)) #define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8))
#define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))
1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Dec 26, 2012 at 3:27 AM, Prathyush K prathyush.k@samsung.com wrote:
The wait_for_vblank interface is modified to the complete_scanout function in fimd. This patch adds the fimd_complete_scanout function
With this series, you have a race if the rmfb happens before the flip has completed.
Why not just use reference counts as is done in the i915 driver: see unpin_work. The reference counts also allow you to avoid blocking in rmfb.
Regards, Mandeep
Inside fimd_complete_scanout, we read the shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 32 +++++++++++++++++++++++++++++++- include/video/samsung_fimd.h | 1 + 2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 3aeedf5..190ffde9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -46,6 +46,7 @@ #define VIDOSD_D(win) (VIDOSD_BASE + 0x0C + (win) * 16)
#define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8) +#define VIDWx_BUF_START_S(win, buf) (VIDW_BUF_START_S(buf) + (win) * 8) #define VIDWx_BUF_END(win, buf) (VIDW_BUF_END(buf) + (win) * 8) #define VIDWx_BUF_SIZE(win, buf) (VIDW_BUF_SIZE(buf) + (win) * 4)
@@ -363,13 +364,42 @@ static void fimd_wait_for_vblank(struct device *dev) fimd_disable_vblank(dev); }
+static void fimd_complete_scanout(struct device *dev, dma_addr_t dma_addr,
unsigned long size)+{
struct fimd_context *ctx = get_fimd_context(dev);int win;bool in_use = false;if (ctx->suspended)return;for (win = 0; win < WINDOWS_NR; win++) {dma_addr_t dma_addr_in_use;if (!ctx->win_data[win].enabled)continue;dma_addr_in_use = readl(ctx->regs + VIDWx_BUF_START_S(win, 0));if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size)) {in_use = true;break;}}if (in_use)fimd_wait_for_vblank(dev);return;+}
static struct exynos_drm_manager_ops fimd_manager_ops = { .dpms = fimd_dpms, .apply = fimd_apply, .commit = fimd_commit, .enable_vblank = fimd_enable_vblank, .disable_vblank = fimd_disable_vblank,
.wait_for_vblank = fimd_wait_for_vblank,
.complete_scanout = fimd_complete_scanout,};
static void fimd_win_mode_set(struct device *dev, diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h index 7ae6c07..382eaec 100644 --- a/include/video/samsung_fimd.h +++ b/include/video/samsung_fimd.h @@ -274,6 +274,7 @@
/* Video buffer addresses */ #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) +#define VIDW_BUF_START_S(_buff) (0x40A0 + ((_buff) * 8)) #define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8))
#define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))
1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Jan 8, 2013 at 4:18 AM, Mandeep Singh Baines msb@chromium.orgwrote:
On Wed, Dec 26, 2012 at 3:27 AM, Prathyush K prathyush.k@samsung.com wrote:
The wait_for_vblank interface is modified to the complete_scanout function in fimd. This patch adds the fimd_complete_scanout function
With this series, you have a race if the rmfb happens before the flip has completed.
How will there be a race? Can you explain the scenario in more detail?
If the current fb (fb1) is being removed, (i.e the shadow register has fb1), then we wait for vblank before returning. This ensures that the flip has been handled before remove fb is complete.
Why not just use reference counts as is done in the i915 driver: see unpin_work. The reference counts also allow you to avoid blocking in rmfb.
Will look into it. Thanks.
Regards, Prathyush
Regards, Mandeep
Inside fimd_complete_scanout, we read the shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 32
+++++++++++++++++++++++++++++++-
include/video/samsung_fimd.h | 1 + 2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 3aeedf5..190ffde9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -46,6 +46,7 @@ #define VIDOSD_D(win) (VIDOSD_BASE + 0x0C + (win) * 16)
#define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8) +#define VIDWx_BUF_START_S(win, buf) (VIDW_BUF_START_S(buf) + (win) *
#define VIDWx_BUF_END(win, buf) (VIDW_BUF_END(buf) +
(win) * 8)
#define VIDWx_BUF_SIZE(win, buf) (VIDW_BUF_SIZE(buf) + (win) * 4)
@@ -363,13 +364,42 @@ static void fimd_wait_for_vblank(struct device
*dev)
fimd_disable_vblank(dev);}
+static void fimd_complete_scanout(struct device *dev, dma_addr_t
dma_addr,
unsigned long size)+{
struct fimd_context *ctx = get_fimd_context(dev);int win;bool in_use = false;if (ctx->suspended)return;for (win = 0; win < WINDOWS_NR; win++) {dma_addr_t dma_addr_in_use;if (!ctx->win_data[win].enabled)continue;dma_addr_in_use = readl(ctx->regs +VIDWx_BUF_START_S(win, 0));
if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size)) {in_use = true;break;}}if (in_use)fimd_wait_for_vblank(dev);return;+}
static struct exynos_drm_manager_ops fimd_manager_ops = { .dpms = fimd_dpms, .apply = fimd_apply, .commit = fimd_commit, .enable_vblank = fimd_enable_vblank, .disable_vblank = fimd_disable_vblank,
.wait_for_vblank = fimd_wait_for_vblank,
.complete_scanout = fimd_complete_scanout,};
static void fimd_win_mode_set(struct device *dev, diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h index 7ae6c07..382eaec 100644 --- a/include/video/samsung_fimd.h +++ b/include/video/samsung_fimd.h @@ -274,6 +274,7 @@
/* Video buffer addresses */ #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) +#define VIDW_BUF_START_S(_buff) (0x40A0 +
((_buff) * 8))
#define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8))
#define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))
1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Jan 8, 2013 at 9:43 PM, Prathyush K prathyush@chromium.org wrote:
On Tue, Jan 8, 2013 at 4:18 AM, Mandeep Singh Baines msb@chromium.org wrote:
On Wed, Dec 26, 2012 at 3:27 AM, Prathyush K prathyush.k@samsung.com wrote:
The wait_for_vblank interface is modified to the complete_scanout function in fimd. This patch adds the fimd_complete_scanout function
With this series, you have a race if the rmfb happens before the flip has completed.
How will there be a race? Can you explain the scenario in more detail?
I'm sorry. There's no race. I had misunderstood something.
Reference counting might still be preferable to avoid blocking the Xserver till vblank.
You can implement it by grabbing a reference on page_flip and release the reference on finish_page_flip (when flipping to a new fb).
Regards, Mandeep
If the current fb (fb1) is being removed, (i.e the shadow register has fb1), then we wait for vblank before returning. This ensures that the flip has been handled before remove fb is complete.
Why not just use reference counts as is done in the i915 driver: see unpin_work. The reference counts also allow you to avoid blocking in rmfb.
Will look into it. Thanks.
Regards, Prathyush
Regards, Mandeep
Inside fimd_complete_scanout, we read the shadow register for each window and compare it with the dma address of the framebuffer. If the dma_address is in the shadow register, then the function waits for the next vblank and returns.
Signed-off-by: Prathyush K prathyush.k@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 32 +++++++++++++++++++++++++++++++- include/video/samsung_fimd.h | 1 + 2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 3aeedf5..190ffde9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -46,6 +46,7 @@ #define VIDOSD_D(win) (VIDOSD_BASE + 0x0C + (win) * 16)
#define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8) +#define VIDWx_BUF_START_S(win, buf) (VIDW_BUF_START_S(buf) + (win) * 8) #define VIDWx_BUF_END(win, buf) (VIDW_BUF_END(buf) + (win) * 8) #define VIDWx_BUF_SIZE(win, buf) (VIDW_BUF_SIZE(buf) + (win) * 4)
@@ -363,13 +364,42 @@ static void fimd_wait_for_vblank(struct device *dev) fimd_disable_vblank(dev); }
+static void fimd_complete_scanout(struct device *dev, dma_addr_t dma_addr,
unsigned long size)+{
struct fimd_context *ctx = get_fimd_context(dev);int win;bool in_use = false;if (ctx->suspended)return;for (win = 0; win < WINDOWS_NR; win++) {dma_addr_t dma_addr_in_use;if (!ctx->win_data[win].enabled)continue;dma_addr_in_use = readl(ctx->regs +VIDWx_BUF_START_S(win, 0));
if (dma_addr_in_use >= dma_addr &&dma_addr_in_use < (dma_addr + size)) {in_use = true;break;}}if (in_use)fimd_wait_for_vblank(dev);return;+}
static struct exynos_drm_manager_ops fimd_manager_ops = { .dpms = fimd_dpms, .apply = fimd_apply, .commit = fimd_commit, .enable_vblank = fimd_enable_vblank, .disable_vblank = fimd_disable_vblank,
.wait_for_vblank = fimd_wait_for_vblank,
.complete_scanout = fimd_complete_scanout,};
static void fimd_win_mode_set(struct device *dev, diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h index 7ae6c07..382eaec 100644 --- a/include/video/samsung_fimd.h +++ b/include/video/samsung_fimd.h @@ -274,6 +274,7 @@
/* Video buffer addresses */ #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) +#define VIDW_BUF_START_S(_buff) (0x40A0 + ((_buff) * 8)) #define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8))
#define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))
1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Good patch set. will apply them after review and test.
Thanks, Inki Dae
2012/12/26 Prathyush K prathyush.k@samsung.com
This patchset has more fixes for fixing more issues related to wait for vblank.
Patch 1: add mixer apply function The mixer window enabled flag is being done in both exynos_drm_hdmi as well as exynos_mixer. This patch moves the flag to one place and adds the mixer apply function similar to fimd apply function.
Patch 2: fimd: call win_disable only when window is enabled Adds a condition to call win_disable only if the window is enabled.
Patch 3: enable vblank in fimd wait for vblank If vblank is disabled, then we turn enable vblank temporarily before wait for vblank and disable it afterwards. This ensures that the wait will not timeout.
Patch 4: fimd: clear channel before enabling iommu This patch is required if the fimd window channel is already active before fimd probe. This could happen if we turn on fimd during uboot. This patch ensures that we do not get a page fault upon iommu initialization. If any channel is active, we disable the channel and wait for vblank.
Patch 5: mixer: do not finish a pageflip if layer update in progress Exynos5 supports only 2 layer updates for mixer. This patch ensures each layer is updated only once per vsync. Also, in case a layer update is pending, the irq handler exits early. This ensures there is no corruption on screen as well as there is no page fault during buffer freeing.
Patch 6: add complete_scanout interface Before freeing a framebuffer, we call complete_scanout encoder function which just calls wait for vblank of all the encoders. This is not a very optimized method since a framebuffer might not be in use by a crtc and we end up waiting for vblank unnecessarily. Instead, this patch modifies the wait_for_vblank interface to complete_scanout interface. In this function, each crtc must check if the fb is currently being used. If it is being used, it must wait for vsync (or even disable a window if necessary) and ensure that the buffer is no longer used by crtc before returning. So if a crtc is not actually reading from the buffer, the complete scanout function will just return and not wait for vsync.
Patch 7: hdmi: add complete_scanout function Patch 8: fimd: add complete_scanout function These two patches add the respective complete_scanout functions for fimd and mixer. These functions check the actual base and shadow registers of fimd/mixer to see if a particular dma-address is in use and will wait_for_vblank only if it is in use. Also for mixer, if the base address matches the fb's dma address, then the window is disabled. This is because a layer update is already set when the base register was updated. So unless we disable the window, we cannot free the buffer.
Akshu Agrawal (1): drm/exynos: fimd: clear channel before enabling iommu
Prathyush K (6): drm/exynos: add mixer apply function drm/exynos: fimd: call win_disable only when window is enabled drm/exynos: enable vblank in fimd wait for vblank drm/exynos: add complete_scanout interface drm/exynos: hdmi: add complete_scanout function drm/exynos: fimd: add complete_scanout function
Sean Paul (1): drm/exynos: mixer: do not finish a pageflip if layer update in progress
drivers/gpu/drm/exynos/exynos_drm_drv.h | 7 ++- drivers/gpu/drm/exynos/exynos_drm_encoder.c | 23 ++++---- drivers/gpu/drm/exynos/exynos_drm_encoder.h | 4 +- drivers/gpu/drm/exynos/exynos_drm_fb.c | 13 ++++- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 81 ++++++++++++++++++++++++-- drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 26 +++------ drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 4 +- drivers/gpu/drm/exynos/exynos_mixer.c | 90 ++++++++++++++++++++++++++--- drivers/gpu/drm/exynos/regs-mixer.h | 1 + include/video/samsung_fimd.h | 1 + 10 files changed, 202 insertions(+), 48 deletions(-)
-- 1.8.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Inki Dae -
Mandeep Singh Baines -
Prathyush K -
Prathyush K -
Sean Paul -
Stéphane Marchesin