On Wed, Jan 2, 2013 at 10:24 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
Is this really possible? If it is, I think there's potential forOn Wed, Dec 26, 2012 at 6:27 AM, Prathyush K <prathyush.k@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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;
> +
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++) {I think this has the opposite problem as your mixer patch. You're
> + 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;
> + }
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
