On Thu, Jan 3, 2013 at 7:56 AM, Prathyush K <prathyush@xxxxxxxxxxxx> wrote:
>
>
>
> On Wed, Jan 2, 2013 at 10:24 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
>>
>> On 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;
>> > +
>>
>> 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@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
