| Message ID | 20181219153950.6870-1-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpu: ipu-v3: pre: don't trigger update if buffer address doesn't change | expand |
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
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
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
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)) {
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(+)