| Message ID | 1356521265-22749-9-git-send-email-prathyush.k@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) * > 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. 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.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? 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 >
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 > >
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))
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(-)