[PATCH] gpu: ipu-v3: pre: don't trigger update if buffer address doesn't change

[Philipp Zabel]

Hi Lucas,

On Wed, Dec 19, 2018 at 4:40 PM Lucas Stach <l.stach at pengutronix.de> wrote:
>
> On a NOP double buffer update where current buffer address is the same
> as the next buffer address, the SDW_UPDATE bit clears too late.

What does this mean, exactly? Does the hardware behave differently
if the writel to IPU_PRE_NEXT_BUF doesn't change the value?

> As we
> are now using this bit to determine when it is safe to signal flip
> completion to userspace this will delay completion of atomic commits
> where one plane doesn't change the buffer by a whole frame period.

This sounds like the problem is not the way PRE behaves, but that we are
setting the SDW_UPDATE flag again and then later use it to check whether
the previous buffer was released, resulting in a false negative.

> Fix this by remembering the last buffer address and just skip the
> double buffer update if it would not change the buffer address.

It looks to me like ipu_plane_atomic_update does not have to call
ipu_prg_channel_configure (which then just relays to ipu_pre_update)
at all in this case. Should we just skip this in ipuv3-plane.c already?

> Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
> ---
>  drivers/gpu/ipu-v3/ipu-pre.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-pre.c b/drivers/gpu/ipu-v3/ipu-pre.c
> index f933c1911e65..d383e8dbd6cc 100644
> --- a/drivers/gpu/ipu-v3/ipu-pre.c
> +++ b/drivers/gpu/ipu-v3/ipu-pre.c
> @@ -106,6 +106,7 @@ struct ipu_pre {
>         void                    *buffer_virt;
>         bool                    in_use;
>         unsigned int            safe_window_end;
> +       unsigned int            last_bufaddr;

This looks a lot like plane state.

>  };
>
>  static DEFINE_MUTEX(ipu_pre_list_mutex);
> @@ -242,7 +243,11 @@ void ipu_pre_update(struct ipu_pre *pre, unsigned int bufaddr)
>         unsigned short current_yblock;
>         u32 val;
>
> +       if (bufaddr == pre->last_bufaddr)
> +               return;

Hypothetical question, could this wrongly return if the channel is
first disabled, leaving
last_buffaddr set to X, and then the channel is reenabled, which
doesn't touch last_bufaddr,
and then the first update contains a buffer at the same address X?

> +
>         writel(bufaddr, pre->regs + IPU_PRE_NEXT_BUF);
> +       pre->last_bufaddr = bufaddr;
>
>         do {
>                 if (time_after(jiffies, timeout)) {
> --
> 2.19.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

regards
Philipp