On Wed, Jan 2, 2013 at 10:12 PM, Sean Paul <seanp...@chromium.org> wrote:
> On Wed, Dec 26, 2012 at 6:27 AM, Prathyush K <prathyus...@samsung.com>
> wrote:
> > The wait_for_vblank interface is modified to the complete_scanout
> > function in hdmi. This inturn calls the complete_scanout mixer op.
> > This patch also adds the mixer_complete_scanout function.
> >
> > Inside mixer_complete_scanout, we read the base register and shadow
> > register for each window and compare it with the dma address of the
> > framebuffer. If the dma_address is in the base register, the mixer
> > window is disabled. If the dma_address is in the shadow register,
> > then the function waits for the next vblank and returns.
> >
> > Signed-off-by: Prathyush K <prathyus...@samsung.com>
> > ---
> > drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 11 ++++++----
> > drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 3 ++-
> > drivers/gpu/drm/exynos/exynos_mixer.c | 37
> +++++++++++++++++++++++++++++++-
> > 3 files changed, 45 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> > index d8ae47e..e32eb1c 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> > @@ -177,14 +177,17 @@ static void drm_hdmi_disable_vblank(struct device
> *subdrv_dev)
> > return mixer_ops->disable_vblank(ctx->mixer_ctx->ctx);
> > }
> >
> > -static void drm_hdmi_wait_for_vblank(struct device *subdrv_dev)
> > +static void drm_hdmi_complete_scanout(struct device *subdrv_dev,
> > + dma_addr_t dma_addr,
> > + unsigned long size)
> > {
> > struct drm_hdmi_context *ctx = to_context(subdrv_dev);
> >
> > DRM_DEBUG_KMS("%s\n", __FILE__);
> >
> > - if (mixer_ops && mixer_ops->wait_for_vblank)
> > - mixer_ops->wait_for_vblank(ctx->mixer_ctx->ctx);
> > + if (mixer_ops && mixer_ops->complete_scanout)
> > + mixer_ops->complete_scanout(ctx->mixer_ctx->ctx,
> > + dma_addr, size);
> > }
> >
> > static void drm_hdmi_mode_fixup(struct device *subdrv_dev,
> > @@ -263,7 +266,7 @@ static struct exynos_drm_manager_ops
> drm_hdmi_manager_ops = {
> > .apply = drm_hdmi_apply,
> > .enable_vblank = drm_hdmi_enable_vblank,
> > .disable_vblank = drm_hdmi_disable_vblank,
> > - .wait_for_vblank = drm_hdmi_wait_for_vblank,
> > + .complete_scanout = drm_hdmi_complete_scanout,
> > .mode_fixup = drm_hdmi_mode_fixup,
> > .mode_set = drm_hdmi_mode_set,
> > .get_max_resol = drm_hdmi_get_max_resol,
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> > index 4fad00c..7d5bf00 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> > @@ -51,7 +51,8 @@ struct exynos_mixer_ops {
> > int (*iommu_on)(void *ctx, bool enable);
> > int (*enable_vblank)(void *ctx, int pipe);
> > void (*disable_vblank)(void *ctx);
> > - void (*wait_for_vblank)(void *ctx);
> > + void (*complete_scanout)(void *ctx, dma_addr_t dma_addr,
> > + unsigned long size);
> > void (*dpms)(void *ctx, int mode);
> >
> > /* overlay */
> > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
> b/drivers/gpu/drm/exynos/exynos_mixer.c
> > index 3369d57..151e13f 100644
> > --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> > @@ -861,6 +861,41 @@ static void mixer_wait_for_vblank(void *ctx)
> > DRM_DEBUG_KMS("vblank wait timed out.\n");
> > }
> >
> > +static void mixer_complete_scanout(void *ctx, dma_addr_t dma_addr,
> > + unsigned long size)
> > +{
> > + struct mixer_context *mixer_ctx = ctx;
> > + struct mixer_resources *res = &mixer_ctx->mixer_res;
> > + int win;
> > + bool in_use = false;
> > + bool in_use_s = false;
> > +
> > + if (!mixer_ctx->powered)
>
> Accesses to mixer_ctx->powered are always protected by mixer_mutex,
> this should probably stay consistent (either way).
>
>
Hi Sean,
Right, thanks for pointing that out. I'll update in next patch set.
> > + return;
> > +
> > + for (win = 0; win < MIXER_WIN_NR; win++) {
> > + dma_addr_t dma_addr_in_use;
> > +
> > + if (!mixer_ctx->win_data[win].enabled)
> > + continue;
> > +
> > + dma_addr_in_use = mixer_reg_read(res,
> MXR_GRAPHIC_BASE(win));
> > + if (dma_addr_in_use >= dma_addr &&
> > + dma_addr_in_use < (dma_addr + size)) {
> > + mixer_win_disable(ctx, win);
> > + in_use = true;
> > + }
> > +
> > + dma_addr_in_use = mixer_reg_read(res,
> MXR_GRAPHIC_BASE_S(win));
> > + if (dma_addr_in_use >= dma_addr &&
> > + dma_addr_in_use < (dma_addr + size))
> > + in_use_s = true;
>
> You don't use this anywhere in the code. I think bad things will
> happen if the framebuffer is in a shadow register and we free it.
>
> I don't get your point here. What is not used anywhere in the code?
Please check the comments on the fimd patch.
> I also wonder if you should wrap this all with mixer_vsync_set_update
> so things don't change while you're doing this.
>
> Haven't thought of that. Will definitely consider that for next patch set.
Thanks for reviewing,
Regards,
Prathyush
> Sean
>
>
> > + }
> > +
> > + if (in_use)
> > + mixer_wait_for_vblank(ctx);
> > +}
> > +
> > static void mixer_window_suspend(struct mixer_context *ctx)
> > {
> > struct hdmi_win_data *win_data;
> > @@ -969,7 +1004,7 @@ static struct exynos_mixer_ops mixer_ops = {
> > .iommu_on = mixer_iommu_on,
> > .enable_vblank = mixer_enable_vblank,
> > .disable_vblank = mixer_disable_vblank,
> > - .wait_for_vblank = mixer_wait_for_vblank,
> > + .complete_scanout = mixer_complete_scanout,
> > .dpms = mixer_dpms,
> >
> > /* overlay */
> > --
> > 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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
