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