Hi Lucas,
On Wed, Dec 19, 2018 at 4:40 PM Lucas Stach <l.stach@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
