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

[Philipp Zabel]

On Mon, 2019-01-07 at 12:58 +0100, Lucas Stach wrote:
> Am Freitag, den 21.12.2018, 12:11 +0100 schrieb 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?
> 
> To me it seems like that. When the address changes, the SDW_UPDATE bit
> is cleared by the time we handle the EOF IRQ. When the address doesn't
> change, it seems the SDW_UPDATE bit clears much later (I've tried the
> new frame IRQ without any success), maybe only at the next EOF,
> effectively doubling the update period if a plane is flipped with the
> same buffer.

Alright, in that case I think it is correct to carry this workaround in
the PRE driver.

> > > 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?
> 
> While we could handle it in ipuv3-plane, this would require more of the
> PRE/PRG state tracking to be moved there. Currently a lot of this is
> hidden behind the simple ipu_prg_channel_configure() call.
> 
> > > 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?
> 
> Nope, this code path is only used when doing no-modeset updates of an
> active plane. When the plane gets disabled the PRE goes through a
> complete reinitialization via ipu_pre_configure() when the channel is
> reenabled.

Let's initialize last_bufaddr in ipu_pre_configure then. I'll amend the
patch as follows:

----------8<----------
--- a/drivers/gpu/ipu-v3/ipu-pre.c
+++ b/drivers/gpu/ipu-v3/ipu-pre.c
@@ -186,6 +186,7 @@ void ipu_pre_configure(struct ipu_pre *pre, unsigned int width,
 
 	writel(bufaddr, pre->regs + IPU_PRE_CUR_BUF);
 	writel(bufaddr, pre->regs + IPU_PRE_NEXT_BUF);
+	pre->last_bufaddr = bufaddr;
 
 	val = IPU_PRE_PREF_ENG_CTRL_INPUT_PIXEL_FORMAT(0) |
 	      IPU_PRE_PREF_ENG_CTRL_INPUT_ACTIVE_BPP(active_bpp) |
---------->8----------

regards
Philipp