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

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

[Lucas Stach]

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

Fix this by remembering the last buffer address and just skip the
double buffer update if it would not change the buffer address.

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;
 };
 
 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;
+
 	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

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

[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?

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

> +
>         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

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

[Lucas Stach]

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

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