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

[Lucas Stach <l.stach@pengutronix.de>]

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@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.
> 
> > 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@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.

Regards,
Lucas

> > +
> >         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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> regards
> Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel