<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jan 2, 2013 at 10:24 PM, Sean Paul <span dir="ltr"><<a href="mailto:seanpaul@chromium.org" target="_blank">seanpaul@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>On Wed, Dec 26, 2012 at 6:27 AM, Prathyush K <<a href="mailto:prathyush.k@samsung.com" target="_blank">prathyush.k@samsung.com</a>> wrote:<br>
> The wait_for_vblank interface is modified to the complete_scanout<br>
> function in fimd. This patch adds the fimd_complete_scanout function<br>
><br>
> Inside fimd_complete_scanout, we read the shadow register for each<br>
> window and compare it with the dma address of the framebuffer. If<br>
> the dma_address is in the shadow register, then the function waits<br>
> for the next vblank and returns.<br>
><br>
> Signed-off-by: Prathyush K <<a href="mailto:prathyush.k@samsung.com" target="_blank">prathyush.k@samsung.com</a>><br>
> ---<br>
> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 32 +++++++++++++++++++++++++++++++-<br>
> include/video/samsung_fimd.h | 1 +<br>
> 2 files changed, 32 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> index 3aeedf5..190ffde9 100644<br>
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> @@ -46,6 +46,7 @@<br>
> #define VIDOSD_D(win) (VIDOSD_BASE + 0x0C + (win) * 16)<br>
><br>
> #define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8)<br>
> +#define VIDWx_BUF_START_S(win, buf) (VIDW_BUF_START_S(buf) + (win) * 8)<br>
> #define VIDWx_BUF_END(win, buf) (VIDW_BUF_END(buf) + (win) * 8)<br>
> #define VIDWx_BUF_SIZE(win, buf) (VIDW_BUF_SIZE(buf) + (win) * 4)<br>
><br>
> @@ -363,13 +364,42 @@ static void fimd_wait_for_vblank(struct device *dev)<br>
> fimd_disable_vblank(dev);<br>
> }<br>
><br>
> +static void fimd_complete_scanout(struct device *dev, dma_addr_t dma_addr,<br>
> + unsigned long size)<br>
> +{<br>
> + struct fimd_context *ctx = get_fimd_context(dev);<br>
> + int win;<br>
> + bool in_use = false;<br>
> +<br>
> + if (ctx->suspended)<br>
> + return;<br>
> +<br>
<br>
</div></div>Is this really possible? If it is, I think there's potential for<br>
trouble. It seems possible that an fb is current, but we're suspended.<br>
If we exit early here, the fb will be freed and we'll be sad when we<br>
come out of suspend.<br>
<div><br></div></blockquote><div>Hi Sean,</div><div><br></div><div>Before suspend, we disable all the windows (and wait for vsync) and keep track of which windows need to be resumed.</div><div>
<br></div><div>If a fb is being removed during suspend, and if that fb was previously set to fimd, we modify the resume flag</div><div>so that the window will not be resumed during fimd_resume.</div><div><br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>
> + for (win = 0; win < WINDOWS_NR; win++) {<br>
> + dma_addr_t dma_addr_in_use;<br>
> +<br>
> + if (!ctx->win_data[win].enabled)<br>
> + continue;<br>
> +<br>
> + dma_addr_in_use = readl(ctx->regs + VIDWx_BUF_START_S(win, 0));<br>
> + if (dma_addr_in_use >= dma_addr &&<br>
> + dma_addr_in_use < (dma_addr + size)) {<br>
> + in_use = true;<br>
> + break;<br>
> + }<br>
<br>
</div>I think this has the opposite problem as your mixer patch. You're<br>
checking if the framebuffer is in the shadow register, but I don't<br>
think this code will wait if the fb is currently being scanned out. I<br>
hope I'm just missing something :)<br>
<span><font color="#888888"><br></font></span></blockquote><div>Right, I do not check for the base register here. This is because I also consider the disable_crtc</div><div>function.</div><div>
<br></div><div>Consider the following two cases:</div><div><br></div><div>1> </div><div>fb1 set to fimd and fimd is reading from fb1 (so both base register and shadow register have fb1's dma addr)</div>
<div>call remove fb1</div><div><br></div><div>since crtc->fb == fb1, the "exynos_drm_crtc_disable" gets called. This will disable the window and also turn off fimd with DPMS_OFF.</div><div>
<br></div><div>fimd dpms off will ensure that the window actually gets disabled by waiting for vblank.</div><div><br></div><div>Now before removing fb1, we call complete_scanout. Since fimd is already suspended, this will just return. No issue.<br>
</div><div><br></div><div>2><br></div><div>fb1 set to fimd, fimd reading from fb1.</div><div>fb2 set to fimd</div><div>call remove fb1</div><div>(now base register has fb2, shadow register has fb1)</div>
<div><br></div><div>since crtc->fb == fb2, crtc_disable wont be called.</div><div><br></div><div>In complete scanout, we check only shadow register (which has fb1).</div><div>So we wait for vsync, so that next dma starts from fb2 and then we go ahead and remove fb1.</div>
<div><br></div><div style>I tried to apply the same logic for mixer:</div><div>In mixer, it is slightly more complex, since we have layer updates.<br></div><div><br></div><div>Consider the following cases for mixer:<br></div>
<div><br></div><div>1> fb1 set to mixer (base register has fb1) - also fb1 gets queued with a layer update.</div><div>mixer is reading from fb1 (shadow register has fb1).<br></div><div>remove fb1<br></div><div style><br>
</div><div style>crtc->fb == fb1, so win_disable gets called followed by crtc off. This will wait for vsync.</div><div style><br></div><div style>complete_scanout will just return. remove fb1. no issue.</div><div style>
<br></div><div style><br></div><div style>2> mixer reading from fb1</div><div style>fb2 set to mixer (layer update)</div><div style>remove fb1</div><div style><br></div><div style>In this case, base has fb2, shadow has fb1.</div>
<div style><br></div><div style>crtc->fb == fb2, so no win_disable.</div><div style><br></div><div style>complete scanout will wait for vsync and ensure dma is from fb2. remove fb1. no issue.</div><div style><br></div>
<div style>3> mixer reading from fb1. shadow reg has fb1</div><div style>fb2 set to mixer (layer update - base reg has fb2)</div><div style>fb3 set to mixer (since layer update is pending, win_commit returns)</div><div style>
remove fb2</div><div style><br></div><div style>crtc->fb == fb3, so no win_disable</div><div style><br></div><div style>complete scanout: base == fb2 so win_disable and then wait for vsync. remove fb1. no issue</div><div style>
<br></div><div style><br></div><div style>Hope this answers your question. If there is any additional scenario which I missed, do let me know :)</div><div style><br></div><div style>With the above approach, there is no crash in either fimd or hdmi after quite a few attempts at multiple scenarios.</div>
<div style>I am working on a patch set v2 which I'll post tomorrow with a couple of additional fixes.<br></div><div style><br></div><div style>Regards,</div><div style>Prathyush</div><div style><br></div><div style><br>
</div><div style><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><font color="#888888">
Sean<br>
</font></span><div><div><br>
<br>
<br>
> + }<br>
> +<br>
> + if (in_use)<br>
> + fimd_wait_for_vblank(dev);<br>
> + return;<br>
> +}<br>
> +<br>
> static struct exynos_drm_manager_ops fimd_manager_ops = {<br>
> .dpms = fimd_dpms,<br>
> .apply = fimd_apply,<br>
> .commit = fimd_commit,<br>
> .enable_vblank = fimd_enable_vblank,<br>
> .disable_vblank = fimd_disable_vblank,<br>
> - .wait_for_vblank = fimd_wait_for_vblank,<br>
> + .complete_scanout = fimd_complete_scanout,<br>
> };<br>
><br>
> static void fimd_win_mode_set(struct device *dev,<br>
> diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h<br>
> index 7ae6c07..382eaec 100644<br>
> --- a/include/video/samsung_fimd.h<br>
> +++ b/include/video/samsung_fimd.h<br>
> @@ -274,6 +274,7 @@<br>
><br>
> /* Video buffer addresses */<br>
> #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8))<br>
> +#define VIDW_BUF_START_S(_buff) (0x40A0 + ((_buff) * 8))<br>
> #define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8))<br>
> #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8))<br>
> #define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))<br>
> --<br>
> 1.8.0<br>
><br>
> _______________________________________________<br>
> dri-devel mailing list<br>
> <a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
_______________________________________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
</div></div></blockquote></div><br></div></div>