On Thu, Jan 3, 2013 at 7:58 AM, Prathyush K <prathyush@xxxxxxxxxxxx> wrote:
>
>
>
> On Wed, Jan 2, 2013 at 10:12 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 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 <prathyush.k@xxxxxxxxxxx>
>> > ---
>> > 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?
>
The variable in_use_s isn't checked anywhere in your function. You
define it as false, set it to true if the fb is in the shadow
register, and then never actually check it. You check in_use below
(and wait for vblank), but not in_use_s.
Sean
> 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@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
