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