| Message ID | 1354805166-29365-8-git-send-email-prathyush.k@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2012/12/6 Prathyush K <prathyush.k@samsung.com> > When fimd is turned off, we disable the clocks which will stop > the dma. Now if we remove the current framebuffer, we cannot > disable the overlay but the current framebuffer will still be freed. > When fimd resumes, the dma will continue from where it left off > and will throw a PAGE FAULT since the memory was freed. > > This patch fixes the above problem by disabling the fimd windows > before disabling the fimd clocks. It also keeps track of which > windows were currently active by setting the 'resume' flag. When > fimd resumes, the window with a resume flag set is enabled again. > > Now if a current fb is removed when fimd is off, fimd_win_disable > will set the 'resume' flag of that window to zero and return. > So when fimd resumes, that window will not be resumed. > > Signed-off-by: Prathyush K <prathyush.k@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 40 > +++++++++++++++++++++++++++++- > 1 files changed, 39 insertions(+), 1 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > index 1517d15..7e66032 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > @@ -83,6 +83,7 @@ struct fimd_win_data { > unsigned int buf_offsize; > unsigned int line_size; /* bytes */ > bool enabled; > + bool resume; > }; > > struct fimd_context { > @@ -596,6 +597,12 @@ static void fimd_win_disable(struct device *dev, int > zpos) > > win_data = &ctx->win_data[win]; > + if (ctx->suspended) { > Ok, add the below comment, "if disabling hw overlay is tried when fimd was disabled already then do not access the hardware." > + /* do not resume this window*/ > + win_data->resume = false; > + return; > + } > + > /* protect windows */ > val = readl(ctx->regs + SHADOWCON); > val |= SHADOWCON_WINx_PROTECT(win); > @@ -809,11 +816,38 @@ static int fimd_clock(struct fimd_context *ctx, bool > enable) > return 0; > } > > +static void fimd_window_suspend(struct device *dev) > +{ > + struct fimd_context *ctx = get_fimd_context(dev); > + struct fimd_win_data *win_data; > + int i; > + > + for (i = 0; i < WINDOWS_NR; i++) { > + win_data = &ctx->win_data[i]; > + win_data->resume = win_data->enabled; > + fimd_win_disable(dev, i); > + } > + fimd_wait_for_vblank(dev); > +} > + > +static void fimd_window_resume(struct device *dev) > +{ > + struct fimd_context *ctx = get_fimd_context(dev); > + struct fimd_win_data *win_data; > + int i; > + > + for (i = 0; i < WINDOWS_NR; i++) { > + win_data = &ctx->win_data[i]; > + win_data->enabled = win_data->resume; > + win_data->resume = false; > shouldn't the below code be added? fimd_win_enable(dev, i); you backed up overlay status before suspended but this patch doesn't enable the overlays again. you supposed that the registers would be restored by SoC specific pm framework? + } > +} > + > static int fimd_activate(struct fimd_context *ctx, bool enable) > { > + struct device *dev = ctx->subdrv.dev; > if (enable) { > int ret; > - struct device *dev = ctx->subdrv.dev; > > ret = fimd_clock(ctx, true); > if (ret < 0) > @@ -824,7 +858,11 @@ static int fimd_activate(struct fimd_context *ctx, > bool enable) > /* if vblank was enabled status, enable it again. */ > if (test_and_clear_bit(0, &ctx->irq_flags)) > fimd_enable_vblank(dev); > + > + fimd_window_resume(dev); > } else { > + fimd_window_suspend(dev); > + > fimd_clock(ctx, false); > ctx->suspended = true; > } > -- > 1.7.0.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
2012/12/6 Prathyush K <prathyush.k@samsung.com> > When fimd is turned off, we disable the clocks which will stop > the dma. Now if we remove the current framebuffer, we cannot > disable the overlay but the current framebuffer will still be freed. > I think that if fimd was turned off by dpms operation (off only clock and display power) then rmfb request from user should be failed. Otherwise, someone(drm core or specific driver) should turn hw power on and disable hardware overlay and then turn hw power off again so that such exception codes aren't added to specific driver. I'm not sure but there is something to should be considered in drm core. > When fimd resumes, the dma will continue from where it left off > and will throw a PAGE FAULT since the memory was freed. > > This patch fixes the above problem by disabling the fimd windows > before disabling the fimd clocks. It also keeps track of which > windows were currently active by setting the 'resume' flag. When > fimd resumes, the window with a resume flag set is enabled again. > > Now if a current fb is removed when fimd is off, fimd_win_disable > will set the 'resume' flag of that window to zero and return. > So when fimd resumes, that window will not be resumed. > > Signed-off-by: Prathyush K <prathyush.k@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 40 > +++++++++++++++++++++++++++++- > 1 files changed, 39 insertions(+), 1 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > index 1517d15..7e66032 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > @@ -83,6 +83,7 @@ struct fimd_win_data { > unsigned int buf_offsize; > unsigned int line_size; /* bytes */ > bool enabled; > + bool resume; > }; > > struct fimd_context { > @@ -596,6 +597,12 @@ static void fimd_win_disable(struct device *dev, int > zpos) > > win_data = &ctx->win_data[win]; > > + if (ctx->suspended) { > + /* do not resume this window*/ > + win_data->resume = false; > + return; > + } > + > /* protect windows */ > val = readl(ctx->regs + SHADOWCON); > val |= SHADOWCON_WINx_PROTECT(win); > @@ -809,11 +816,38 @@ static int fimd_clock(struct fimd_context *ctx, bool > enable) > return 0; > } > > +static void fimd_window_suspend(struct device *dev) > +{ > + struct fimd_context *ctx = get_fimd_context(dev); > + struct fimd_win_data *win_data; > + int i; > + > + for (i = 0; i < WINDOWS_NR; i++) { > + win_data = &ctx->win_data[i]; > + win_data->resume = win_data->enabled; > + fimd_win_disable(dev, i); > + } > + fimd_wait_for_vblank(dev); > +} > + > +static void fimd_window_resume(struct device *dev) > +{ > + struct fimd_context *ctx = get_fimd_context(dev); > + struct fimd_win_data *win_data; > + int i; > + > + for (i = 0; i < WINDOWS_NR; i++) { > + win_data = &ctx->win_data[i]; > + win_data->enabled = win_data->resume; > + win_data->resume = false; > + } > +} > + > static int fimd_activate(struct fimd_context *ctx, bool enable) > { > + struct device *dev = ctx->subdrv.dev; > if (enable) { > int ret; > - struct device *dev = ctx->subdrv.dev; > > ret = fimd_clock(ctx, true); > if (ret < 0) > @@ -824,7 +858,11 @@ static int fimd_activate(struct fimd_context *ctx, > bool enable) > /* if vblank was enabled status, enable it again. */ > if (test_and_clear_bit(0, &ctx->irq_flags)) > fimd_enable_vblank(dev); > + > + fimd_window_resume(dev); > } else { > + fimd_window_suspend(dev); > + > fimd_clock(ctx, false); > ctx->suspended = true; > } > -- > 1.7.0.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Fri, Dec 7, 2012 at 10:37 AM, Inki Dae <inki.dae@samsung.com> wrote: > > > 2012/12/6 Prathyush K <prathyush.k@samsung.com> > >> When fimd is turned off, we disable the clocks which will stop >> the dma. Now if we remove the current framebuffer, we cannot >> disable the overlay but the current framebuffer will still be freed. >> > > I think that if fimd was turned off by dpms operation (off only clock and > display power) then rmfb request from user should be failed. Otherwise, > someone(drm core or specific driver) should turn hw power on and disable > hardware overlay and then turn hw power off again so that such exception > codes aren't added to specific driver. I'm not sure but there is something > to should be considered in drm core. > > Ideally that should true. But we never check for return value of rmFB in our application. If we do not free our framebuffer, application will not try to free it again. So that memory will be locked till drm gets released. As for your previous question: The overlays are enabled again by the encoder DPMS function. Initially I was resuming the windows by calling fimd_win_commit inside fimd_window_resume. But I saw that the overlays were getting updated twice. So I removed that from fimd_window_resume. In case of suspend/resume, the fimd_resume function is calling fimd_apply after it has called fimd_activate. This ensures that the overlays get updated upon fimd_resume as well as DPMS_ON. Regards, Prathyush > When fimd resumes, the dma will continue from where it left off >> and will throw a PAGE FAULT since the memory was freed. >> >> This patch fixes the above problem by disabling the fimd windows >> before disabling the fimd clocks. It also keeps track of which >> windows were currently active by setting the 'resume' flag. When >> fimd resumes, the window with a resume flag set is enabled again. >> >> Now if a current fb is removed when fimd is off, fimd_win_disable >> will set the 'resume' flag of that window to zero and return. >> So when fimd resumes, that window will not be resumed. >> >> Signed-off-by: Prathyush K <prathyush.k@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 40 >> +++++++++++++++++++++++++++++- >> 1 files changed, 39 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c >> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >> index 1517d15..7e66032 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >> @@ -83,6 +83,7 @@ struct fimd_win_data { >> unsigned int buf_offsize; >> unsigned int line_size; /* bytes */ >> bool enabled; >> + bool resume; >> }; >> >> struct fimd_context { >> @@ -596,6 +597,12 @@ static void fimd_win_disable(struct device *dev, int >> zpos) >> >> win_data = &ctx->win_data[win]; >> >> + if (ctx->suspended) { >> + /* do not resume this window*/ >> + win_data->resume = false; >> + return; >> + } >> + >> /* protect windows */ >> val = readl(ctx->regs + SHADOWCON); >> val |= SHADOWCON_WINx_PROTECT(win); >> @@ -809,11 +816,38 @@ static int fimd_clock(struct fimd_context *ctx, >> bool enable) >> return 0; >> } >> >> +static void fimd_window_suspend(struct device *dev) >> +{ >> + struct fimd_context *ctx = get_fimd_context(dev); >> + struct fimd_win_data *win_data; >> + int i; >> + >> + for (i = 0; i < WINDOWS_NR; i++) { >> + win_data = &ctx->win_data[i]; >> + win_data->resume = win_data->enabled; >> + fimd_win_disable(dev, i); >> + } >> + fimd_wait_for_vblank(dev); >> +} >> + >> +static void fimd_window_resume(struct device *dev) >> +{ >> + struct fimd_context *ctx = get_fimd_context(dev); >> + struct fimd_win_data *win_data; >> + int i; >> + >> + for (i = 0; i < WINDOWS_NR; i++) { >> + win_data = &ctx->win_data[i]; >> + win_data->enabled = win_data->resume; >> + win_data->resume = false; >> + } >> +} >> + >> static int fimd_activate(struct fimd_context *ctx, bool enable) >> { >> + struct device *dev = ctx->subdrv.dev; >> if (enable) { >> int ret; >> - struct device *dev = ctx->subdrv.dev; >> >> ret = fimd_clock(ctx, true); >> if (ret < 0) >> @@ -824,7 +858,11 @@ static int fimd_activate(struct fimd_context *ctx, >> bool enable) >> /* if vblank was enabled status, enable it again. */ >> if (test_and_clear_bit(0, &ctx->irq_flags)) >> fimd_enable_vblank(dev); >> + >> + fimd_window_resume(dev); >> } else { >> + fimd_window_suspend(dev); >> + >> fimd_clock(ctx, false); >> ctx->suspended = true; >> } >> -- >> 1.7.0.4 >> >> _______________________________________________ >> 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 Fri, Dec 7, 2012 at 1:04 PM, Prathyush K <prathyush@chromium.org> wrote: > > > > On Fri, Dec 7, 2012 at 10:37 AM, Inki Dae <inki.dae@samsung.com> wrote: > >> >> >> 2012/12/6 Prathyush K <prathyush.k@samsung.com> >> >>> When fimd is turned off, we disable the clocks which will stop >>> the dma. Now if we remove the current framebuffer, we cannot >>> disable the overlay but the current framebuffer will still be freed. >>> >> >> I think that if fimd was turned off by dpms operation (off only clock and >> display power) then rmfb request from user should be failed. Otherwise, >> someone(drm core or specific driver) should turn hw power on and disable >> hardware overlay and then turn hw power off again so that such exception >> codes aren't added to specific driver. I'm not sure but there is something >> to should be considered in drm core. >> > Yes I thought of that option i.e. to power on fimd, disable overlay, wait for vblank and then power off fimd. But this will cause a flicker on the screen as it'll come on for a brief instant and then go off again. I don't think that is correct. > >> > > Ideally that should true. But we never check for return value of rmFB in > our application. > If we do not free our framebuffer, application will not try to free it > again. So that memory will be locked till drm gets released. > > As for your previous question: > > The overlays are enabled again by the encoder DPMS function. Initially I > was resuming the windows by calling fimd_win_commit inside > fimd_window_resume. > But I saw that the overlays were getting updated twice. So I removed that > from fimd_window_resume. > > In case of suspend/resume, the fimd_resume function is calling fimd_apply > after it has called fimd_activate. > This ensures that the overlays get updated upon fimd_resume as well as > DPMS_ON. > > Regards, > Prathyush > > > >> When fimd resumes, the dma will continue from where it left off >>> and will throw a PAGE FAULT since the memory was freed. >>> >>> This patch fixes the above problem by disabling the fimd windows >>> before disabling the fimd clocks. It also keeps track of which >>> windows were currently active by setting the 'resume' flag. When >>> fimd resumes, the window with a resume flag set is enabled again. >>> >>> Now if a current fb is removed when fimd is off, fimd_win_disable >>> will set the 'resume' flag of that window to zero and return. >>> So when fimd resumes, that window will not be resumed. >>> >>> Signed-off-by: Prathyush K <prathyush.k@samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 40 >>> +++++++++++++++++++++++++++++- >>> 1 files changed, 39 insertions(+), 1 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> index 1517d15..7e66032 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> @@ -83,6 +83,7 @@ struct fimd_win_data { >>> unsigned int buf_offsize; >>> unsigned int line_size; /* bytes */ >>> bool enabled; >>> + bool resume; >>> }; >>> >>> struct fimd_context { >>> @@ -596,6 +597,12 @@ static void fimd_win_disable(struct device *dev, >>> int zpos) >>> >>> win_data = &ctx->win_data[win]; >>> >>> + if (ctx->suspended) { >>> + /* do not resume this window*/ >>> + win_data->resume = false; >>> + return; >>> + } >>> + >>> /* protect windows */ >>> val = readl(ctx->regs + SHADOWCON); >>> val |= SHADOWCON_WINx_PROTECT(win); >>> @@ -809,11 +816,38 @@ static int fimd_clock(struct fimd_context *ctx, >>> bool enable) >>> return 0; >>> } >>> >>> +static void fimd_window_suspend(struct device *dev) >>> +{ >>> + struct fimd_context *ctx = get_fimd_context(dev); >>> + struct fimd_win_data *win_data; >>> + int i; >>> + >>> + for (i = 0; i < WINDOWS_NR; i++) { >>> + win_data = &ctx->win_data[i]; >>> + win_data->resume = win_data->enabled; >>> + fimd_win_disable(dev, i); >>> + } >>> + fimd_wait_for_vblank(dev); >>> +} >>> + >>> +static void fimd_window_resume(struct device *dev) >>> +{ >>> + struct fimd_context *ctx = get_fimd_context(dev); >>> + struct fimd_win_data *win_data; >>> + int i; >>> + >>> + for (i = 0; i < WINDOWS_NR; i++) { >>> + win_data = &ctx->win_data[i]; >>> + win_data->enabled = win_data->resume; >>> + win_data->resume = false; >>> + } >>> +} >>> + >>> static int fimd_activate(struct fimd_context *ctx, bool enable) >>> { >>> + struct device *dev = ctx->subdrv.dev; >>> if (enable) { >>> int ret; >>> - struct device *dev = ctx->subdrv.dev; >>> >>> ret = fimd_clock(ctx, true); >>> if (ret < 0) >>> @@ -824,7 +858,11 @@ static int fimd_activate(struct fimd_context *ctx, >>> bool enable) >>> /* if vblank was enabled status, enable it again. */ >>> if (test_and_clear_bit(0, &ctx->irq_flags)) >>> fimd_enable_vblank(dev); >>> + >>> + fimd_window_resume(dev); >>> } else { >>> + fimd_window_suspend(dev); >>> + >>> fimd_clock(ctx, false); >>> ctx->suspended = true; >>> } >>> -- >>> 1.7.0.4 >>> >>> _______________________________________________ >>> 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 >> >> >
2012/12/7 Prathyush K <prathyush@chromium.org> > > > > On Fri, Dec 7, 2012 at 1:04 PM, Prathyush K <prathyush@chromium.org>wrote: > >> >> >> >> On Fri, Dec 7, 2012 at 10:37 AM, Inki Dae <inki.dae@samsung.com> wrote: >> >>> >>> >>> 2012/12/6 Prathyush K <prathyush.k@samsung.com> >>> >>>> When fimd is turned off, we disable the clocks which will stop >>>> the dma. Now if we remove the current framebuffer, we cannot >>>> disable the overlay but the current framebuffer will still be freed. >>>> >>> >>> I think that if fimd was turned off by dpms operation (off only clock >>> and display power) then rmfb request from user should be failed. Otherwise, >>> someone(drm core or specific driver) should turn hw power on and disable >>> hardware overlay and then turn hw power off again so that such exception >>> codes aren't added to specific driver. I'm not sure but there is something >>> to should be considered in drm core. >>> >> > > Yes I thought of that option i.e. to power on fimd, disable overlay, wait > for vblank and then power off fimd. > > But this will cause a flicker on the screen as it'll come on for a brief > instant and then go off again. > I don't think that is correct. > > Right. but at least, we could avoid the flicker on the screen in case of Exynos drm framework because we could control lcd and display controller power respectively. i.e. it turns lcd power off and display controller power on. Thus, Exynos framework can do it instead of specific drivers. Anyway it's a good as is. But I hope more generic way would come up later. Now your patch set are being tested and will be merged if no problem because it's best for now. Thanks, Inki Dae > >>> >> >> Ideally that should true. But we never check for return value of rmFB in >> our application. >> If we do not free our framebuffer, application will not try to free it >> again. So that memory will be locked till drm gets released. >> >> As for your previous question: >> >> The overlays are enabled again by the encoder DPMS function. Initially I >> was resuming the windows by calling fimd_win_commit inside >> fimd_window_resume. >> But I saw that the overlays were getting updated twice. So I removed that >> from fimd_window_resume. >> >> In case of suspend/resume, the fimd_resume function is calling fimd_apply >> after it has called fimd_activate. >> This ensures that the overlays get updated upon fimd_resume as well as >> DPMS_ON. >> >> Regards, >> Prathyush >> >> >> >>> When fimd resumes, the dma will continue from where it left off >>>> and will throw a PAGE FAULT since the memory was freed. >>>> >>>> This patch fixes the above problem by disabling the fimd windows >>>> before disabling the fimd clocks. It also keeps track of which >>>> windows were currently active by setting the 'resume' flag. When >>>> fimd resumes, the window with a resume flag set is enabled again. >>>> >>>> Now if a current fb is removed when fimd is off, fimd_win_disable >>>> will set the 'resume' flag of that window to zero and return. >>>> So when fimd resumes, that window will not be resumed. >>>> >>>> Signed-off-by: Prathyush K <prathyush.k@samsung.com> >>>> --- >>>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 40 >>>> +++++++++++++++++++++++++++++- >>>> 1 files changed, 39 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>>> index 1517d15..7e66032 100644 >>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>>> @@ -83,6 +83,7 @@ struct fimd_win_data { >>>> unsigned int buf_offsize; >>>> unsigned int line_size; /* bytes */ >>>> bool enabled; >>>> + bool resume; >>>> }; >>>> >>>> struct fimd_context { >>>> @@ -596,6 +597,12 @@ static void fimd_win_disable(struct device *dev, >>>> int zpos) >>>> >>>> win_data = &ctx->win_data[win]; >>>> >>>> + if (ctx->suspended) { >>>> + /* do not resume this window*/ >>>> + win_data->resume = false; >>>> + return; >>>> + } >>>> + >>>> /* protect windows */ >>>> val = readl(ctx->regs + SHADOWCON); >>>> val |= SHADOWCON_WINx_PROTECT(win); >>>> @@ -809,11 +816,38 @@ static int fimd_clock(struct fimd_context *ctx, >>>> bool enable) >>>> return 0; >>>> } >>>> >>>> +static void fimd_window_suspend(struct device *dev) >>>> +{ >>>> + struct fimd_context *ctx = get_fimd_context(dev); >>>> + struct fimd_win_data *win_data; >>>> + int i; >>>> + >>>> + for (i = 0; i < WINDOWS_NR; i++) { >>>> + win_data = &ctx->win_data[i]; >>>> + win_data->resume = win_data->enabled; >>>> + fimd_win_disable(dev, i); >>>> + } >>>> + fimd_wait_for_vblank(dev); >>>> +} >>>> + >>>> +static void fimd_window_resume(struct device *dev) >>>> +{ >>>> + struct fimd_context *ctx = get_fimd_context(dev); >>>> + struct fimd_win_data *win_data; >>>> + int i; >>>> + >>>> + for (i = 0; i < WINDOWS_NR; i++) { >>>> + win_data = &ctx->win_data[i]; >>>> + win_data->enabled = win_data->resume; >>>> + win_data->resume = false; >>>> + } >>>> +} >>>> + >>>> static int fimd_activate(struct fimd_context *ctx, bool enable) >>>> { >>>> + struct device *dev = ctx->subdrv.dev; >>>> if (enable) { >>>> int ret; >>>> - struct device *dev = ctx->subdrv.dev; >>>> >>>> ret = fimd_clock(ctx, true); >>>> if (ret < 0) >>>> @@ -824,7 +858,11 @@ static int fimd_activate(struct fimd_context *ctx, >>>> bool enable) >>>> /* if vblank was enabled status, enable it again. */ >>>> if (test_and_clear_bit(0, &ctx->irq_flags)) >>>> fimd_enable_vblank(dev); >>>> + >>>> + fimd_window_resume(dev); >>>> } else { >>>> + fimd_window_suspend(dev); >>>> + >>>> fimd_clock(ctx, false); >>>> ctx->suspended = true; >>>> } >>>> -- >>>> 1.7.0.4 >>>> >>>> _______________________________________________ >>>> 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 >>> >>> >> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > >
On Thu, Dec 6, 2012 at 6:46 AM, Prathyush K <prathyush.k@samsung.com> wrote: > When fimd is turned off, we disable the clocks which will stop > the dma. Now if we remove the current framebuffer, we cannot > disable the overlay but the current framebuffer will still be freed. > When fimd resumes, the dma will continue from where it left off > and will throw a PAGE FAULT since the memory was freed. > > This patch fixes the above problem by disabling the fimd windows > before disabling the fimd clocks. It also keeps track of which > windows were currently active by setting the 'resume' flag. When > fimd resumes, the window with a resume flag set is enabled again. > > Now if a current fb is removed when fimd is off, fimd_win_disable > will set the 'resume' flag of that window to zero and return. > So when fimd resumes, that window will not be resumed. > > Signed-off-by: Prathyush K <prathyush.k@samsung.com> Hi Prathyush, I think you removed my signed-off/ownership on this patch. See: http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=commitdiff;h=cfa22e49b7408547c73532c4bb03de47cc034a05 Stephane > --- > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 40 +++++++++++++++++++++++++++++- > 1 files changed, 39 insertions(+), 1 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > index 1517d15..7e66032 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > @@ -83,6 +83,7 @@ struct fimd_win_data { > unsigned int buf_offsize; > unsigned int line_size; /* bytes */ > bool enabled; > + bool resume; > }; > > struct fimd_context { > @@ -596,6 +597,12 @@ static void fimd_win_disable(struct device *dev, int zpos) > > win_data = &ctx->win_data[win]; > > + if (ctx->suspended) { > + /* do not resume this window*/ > + win_data->resume = false; > + return; > + } > + > /* protect windows */ > val = readl(ctx->regs + SHADOWCON); > val |= SHADOWCON_WINx_PROTECT(win); > @@ -809,11 +816,38 @@ static int fimd_clock(struct fimd_context *ctx, bool enable) > return 0; > } > > +static void fimd_window_suspend(struct device *dev) > +{ > + struct fimd_context *ctx = get_fimd_context(dev); > + struct fimd_win_data *win_data; > + int i; > + > + for (i = 0; i < WINDOWS_NR; i++) { > + win_data = &ctx->win_data[i]; > + win_data->resume = win_data->enabled; > + fimd_win_disable(dev, i); > + } > + fimd_wait_for_vblank(dev); > +} > + > +static void fimd_window_resume(struct device *dev) > +{ > + struct fimd_context *ctx = get_fimd_context(dev); > + struct fimd_win_data *win_data; > + int i; > + > + for (i = 0; i < WINDOWS_NR; i++) { > + win_data = &ctx->win_data[i]; > + win_data->enabled = win_data->resume; > + win_data->resume = false; > + } > +} > + > static int fimd_activate(struct fimd_context *ctx, bool enable) > { > + struct device *dev = ctx->subdrv.dev; > if (enable) { > int ret; > - struct device *dev = ctx->subdrv.dev; > > ret = fimd_clock(ctx, true); > if (ret < 0) > @@ -824,7 +858,11 @@ static int fimd_activate(struct fimd_context *ctx, bool enable) > /* if vblank was enabled status, enable it again. */ > if (test_and_clear_bit(0, &ctx->irq_flags)) > fimd_enable_vblank(dev); > + > + fimd_window_resume(dev); > } else { > + fimd_window_suspend(dev); > + > fimd_clock(ctx, false); > ctx->suspended = true; > } > -- > 1.7.0.4 > > _______________________________________________ > 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 1517d15..7e66032 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -83,6 +83,7 @@ struct fimd_win_data { unsigned int buf_offsize; unsigned int line_size; /* bytes */ bool enabled; + bool resume; }; struct fimd_context { @@ -596,6 +597,12 @@ static void fimd_win_disable(struct device *dev, int zpos) win_data = &ctx->win_data[win]; + if (ctx->suspended) { + /* do not resume this window*/ + win_data->resume = false; + return; + } + /* protect windows */ val = readl(ctx->regs + SHADOWCON); val |= SHADOWCON_WINx_PROTECT(win); @@ -809,11 +816,38 @@ static int fimd_clock(struct fimd_context *ctx, bool enable) return 0; } +static void fimd_window_suspend(struct device *dev) +{ + struct fimd_context *ctx = get_fimd_context(dev); + struct fimd_win_data *win_data; + int i; + + for (i = 0; i < WINDOWS_NR; i++) { + win_data = &ctx->win_data[i]; + win_data->resume = win_data->enabled; + fimd_win_disable(dev, i); + } + fimd_wait_for_vblank(dev); +} + +static void fimd_window_resume(struct device *dev) +{ + struct fimd_context *ctx = get_fimd_context(dev); + struct fimd_win_data *win_data; + int i; + + for (i = 0; i < WINDOWS_NR; i++) { + win_data = &ctx->win_data[i]; + win_data->enabled = win_data->resume; + win_data->resume = false; + } +} + static int fimd_activate(struct fimd_context *ctx, bool enable) { + struct device *dev = ctx->subdrv.dev; if (enable) { int ret; - struct device *dev = ctx->subdrv.dev; ret = fimd_clock(ctx, true); if (ret < 0) @@ -824,7 +858,11 @@ static int fimd_activate(struct fimd_context *ctx, bool enable) /* if vblank was enabled status, enable it again. */ if (test_and_clear_bit(0, &ctx->irq_flags)) fimd_enable_vblank(dev); + + fimd_window_resume(dev); } else { + fimd_window_suspend(dev); + fimd_clock(ctx, false); ctx->suspended = true; }
When fimd is turned off, we disable the clocks which will stop the dma. Now if we remove the current framebuffer, we cannot disable the overlay but the current framebuffer will still be freed. When fimd resumes, the dma will continue from where it left off and will throw a PAGE FAULT since the memory was freed. This patch fixes the above problem by disabling the fimd windows before disabling the fimd clocks. It also keeps track of which windows were currently active by setting the 'resume' flag. When fimd resumes, the window with a resume flag set is enabled again. Now if a current fb is removed when fimd is off, fimd_win_disable will set the 'resume' flag of that window to zero and return. So when fimd resumes, that window will not be resumed. Signed-off-by: Prathyush K <prathyush.k@samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 40 +++++++++++++++++++++++++++++- 1 files changed, 39 insertions(+), 1 deletions(-)