| Message ID | 1472196644-30563-5-git-send-email-gnuiyl@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Freitag, den 26.08.2016, 15:30 +0800 schrieb Liu Ying: > According to basic tests, it looks there is no issue if we don't wait for > DMFC FIFO to clear when disabling DMFC channel. NXP BSP doesn't do that, > either. This patch is needed to avoid the annoying warning caused by a > timeout on waiting for the FIFO to clear after we add the new > DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET flag to the imx-drm driver > which changes the procedure to disable display channel slightly. I suppose the reason this happens is that now DC/DI are disabled first, so the DC can't drain the FIFO anymore. If the FIFO is properly reset when reenabling the DMFC, this shouldn't have any ill effects. regards Philipp
On Mon, Aug 29, 2016 at 4:46 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Am Freitag, den 26.08.2016, 15:30 +0800 schrieb Liu Ying: >> According to basic tests, it looks there is no issue if we don't wait for >> DMFC FIFO to clear when disabling DMFC channel. NXP BSP doesn't do that, >> either. This patch is needed to avoid the annoying warning caused by a >> timeout on waiting for the FIFO to clear after we add the new >> DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET flag to the imx-drm driver >> which changes the procedure to disable display channel slightly. > > I suppose the reason this happens is that now DC/DI are disabled first, > so the DC can't drain the FIFO anymore. If the FIFO is properly reset > when reenabling the DMFC, this shouldn't have any ill effects. I found the timeout warning issue by blanking the framebuffer. Ofc, the framebuffer is supported by the fbdev emulation. Before applying this patch set, the planes are not even disabled when the framebuffer is blanked, that is to say, plane_funcs-> atomic_disable is not called - the CRTC is disabled alone. After applying this patch set, the planes are always disabled together with the CRTC. And, yes, DC/DI are disabled first, then the timeout warning happens. Please note the warning happens when the planes are disabled instead of reenabled. So, I don't get your point by resetting FIFO when _reenabling_ DMFC. And, I don't see the way to reset FIFO. Regards, Liu Ying > > regards > Philipp >
Am Montag, den 29.08.2016, 17:36 +0800 schrieb Ying Liu: > On Mon, Aug 29, 2016 at 4:46 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Am Freitag, den 26.08.2016, 15:30 +0800 schrieb Liu Ying: > >> According to basic tests, it looks there is no issue if we don't wait for > >> DMFC FIFO to clear when disabling DMFC channel. NXP BSP doesn't do that, > >> either. This patch is needed to avoid the annoying warning caused by a > >> timeout on waiting for the FIFO to clear after we add the new > >> DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET flag to the imx-drm driver > >> which changes the procedure to disable display channel slightly. > > > > I suppose the reason this happens is that now DC/DI are disabled first, > > so the DC can't drain the FIFO anymore. If the FIFO is properly reset > > when reenabling the DMFC, this shouldn't have any ill effects. > > I found the timeout warning issue by blanking the framebuffer. > Ofc, the framebuffer is supported by the fbdev emulation. > Before applying this patch set, the planes are not even disabled > when the framebuffer is blanked, that is to say, plane_funcs-> > atomic_disable is not called - the CRTC is disabled alone. > After applying this patch set, the planes are always disabled > together with the CRTC. And, yes, DC/DI are disabled first, > then the timeout warning happens. > > Please note the warning happens when the planes are disabled > instead of reenabled. So, I don't get your point by resetting > FIFO when _reenabling_ DMFC. If we disable the DMFC with data still in the FIFO and then reenable it and the DC first reads the stale data from the FIFO, that should cause a visible artifact in the first frame after reenabling the plane. If that doesn't happen, I trust that the hardware resets the FIFO state somewhere along the way. > And, I don't see the way to reset FIFO. We could reset the DMFC_WR memory after disabling the DMFC, but I'm not sure this is necessary at all. regards Philipp
On Mon, Aug 29, 2016 at 5:46 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Am Montag, den 29.08.2016, 17:36 +0800 schrieb Ying Liu: >> On Mon, Aug 29, 2016 at 4:46 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > Am Freitag, den 26.08.2016, 15:30 +0800 schrieb Liu Ying: >> >> According to basic tests, it looks there is no issue if we don't wait for >> >> DMFC FIFO to clear when disabling DMFC channel. NXP BSP doesn't do that, >> >> either. This patch is needed to avoid the annoying warning caused by a >> >> timeout on waiting for the FIFO to clear after we add the new >> >> DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET flag to the imx-drm driver >> >> which changes the procedure to disable display channel slightly. >> > >> > I suppose the reason this happens is that now DC/DI are disabled first, >> > so the DC can't drain the FIFO anymore. If the FIFO is properly reset >> > when reenabling the DMFC, this shouldn't have any ill effects. >> >> I found the timeout warning issue by blanking the framebuffer. >> Ofc, the framebuffer is supported by the fbdev emulation. >> Before applying this patch set, the planes are not even disabled >> when the framebuffer is blanked, that is to say, plane_funcs-> >> atomic_disable is not called - the CRTC is disabled alone. >> After applying this patch set, the planes are always disabled >> together with the CRTC. And, yes, DC/DI are disabled first, >> then the timeout warning happens. >> >> Please note the warning happens when the planes are disabled >> instead of reenabled. So, I don't get your point by resetting >> FIFO when _reenabling_ DMFC. > > If we disable the DMFC with data still in the FIFO and then reenable it > and the DC first reads the stale data from the FIFO, that should cause a > visible artifact in the first frame after reenabling the plane. If that > doesn't happen, I trust that the hardware resets the FIFO state > somewhere along the way. Not sure if the hardware resets the FIFO automatically... The NXP driver waits for some hardware status/irqs when disabling the channels, but it doesn't wait for DMFC status. > >> And, I don't see the way to reset FIFO. > > We could reset the DMFC_WR memory after disabling the DMFC, but I'm not > sure this is necessary at all. You mean the register DMFC_WR_CHAN, for instance? I still don't see how we could reset the FIFO. Regards, Liu Ying > > regards > Philipp >
Am Montag, den 29.08.2016, 17:57 +0800 schrieb Ying Liu: > On Mon, Aug 29, 2016 at 5:46 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Am Montag, den 29.08.2016, 17:36 +0800 schrieb Ying Liu: > >> On Mon, Aug 29, 2016 at 4:46 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > >> > Am Freitag, den 26.08.2016, 15:30 +0800 schrieb Liu Ying: > >> >> According to basic tests, it looks there is no issue if we don't wait for > >> >> DMFC FIFO to clear when disabling DMFC channel. NXP BSP doesn't do that, > >> >> either. This patch is needed to avoid the annoying warning caused by a > >> >> timeout on waiting for the FIFO to clear after we add the new > >> >> DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET flag to the imx-drm driver > >> >> which changes the procedure to disable display channel slightly. > >> > > >> > I suppose the reason this happens is that now DC/DI are disabled first, > >> > so the DC can't drain the FIFO anymore. If the FIFO is properly reset > >> > when reenabling the DMFC, this shouldn't have any ill effects. > >> > >> I found the timeout warning issue by blanking the framebuffer. > >> Ofc, the framebuffer is supported by the fbdev emulation. > >> Before applying this patch set, the planes are not even disabled > >> when the framebuffer is blanked, that is to say, plane_funcs-> > >> atomic_disable is not called - the CRTC is disabled alone. > >> After applying this patch set, the planes are always disabled > >> together with the CRTC. And, yes, DC/DI are disabled first, > >> then the timeout warning happens. > >> > >> Please note the warning happens when the planes are disabled > >> instead of reenabled. So, I don't get your point by resetting > >> FIFO when _reenabling_ DMFC. > > > > If we disable the DMFC with data still in the FIFO and then reenable it > > and the DC first reads the stale data from the FIFO, that should cause a > > visible artifact in the first frame after reenabling the plane. If that > > doesn't happen, I trust that the hardware resets the FIFO state > > somewhere along the way. > > Not sure if the hardware resets the FIFO automatically... > The NXP driver waits for some hardware status/irqs when disabling > the channels, but it doesn't wait for DMFC status. > > > > >> And, I don't see the way to reset FIFO. > > > > We could reset the DMFC_WR memory after disabling the DMFC, but I'm not > > sure this is necessary at all. > > You mean the register DMFC_WR_CHAN, for instance? > I still don't see how we could reset the FIFO. I mean via IPU_MEM_RST. I'll send a patch to illustrate, but as I said, I don't know if this is really useful. regards Philipp
diff --git a/drivers/gpu/ipu-v3/ipu-dmfc.c b/drivers/gpu/ipu-v3/ipu-dmfc.c index 42705bb..a40f211 100644 --- a/drivers/gpu/ipu-v3/ipu-dmfc.c +++ b/drivers/gpu/ipu-v3/ipu-dmfc.c @@ -123,20 +123,6 @@ int ipu_dmfc_enable_channel(struct dmfc_channel *dmfc) } EXPORT_SYMBOL_GPL(ipu_dmfc_enable_channel); -static void ipu_dmfc_wait_fifos(struct ipu_dmfc_priv *priv) -{ - unsigned long timeout = jiffies + msecs_to_jiffies(1000); - - while ((readl(priv->base + DMFC_STAT) & 0x02fff000) != 0x02fff000) { - if (time_after(jiffies, timeout)) { - dev_warn(priv->dev, - "Timeout waiting for DMFC FIFOs to clear\n"); - break; - } - cpu_relax(); - } -} - void ipu_dmfc_disable_channel(struct dmfc_channel *dmfc) { struct ipu_dmfc_priv *priv = dmfc->priv; @@ -145,10 +131,8 @@ void ipu_dmfc_disable_channel(struct dmfc_channel *dmfc) priv->use_count--; - if (!priv->use_count) { - ipu_dmfc_wait_fifos(priv); + if (!priv->use_count) ipu_module_disable(priv->ipu, IPU_CONF_DMFC_EN); - } if (priv->use_count < 0) priv->use_count = 0;
According to basic tests, it looks there is no issue if we don't wait for DMFC FIFO to clear when disabling DMFC channel. NXP BSP doesn't do that, either. This patch is needed to avoid the annoying warning caused by a timeout on waiting for the FIFO to clear after we add the new DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET flag to the imx-drm driver which changes the procedure to disable display channel slightly. Cc: Philipp Zabel <p.zabel@pengutronix.de> Cc: David Airlie <airlied@linux.ie> Cc: Russell King <linux@armlinux.org.uk> Cc: Peter Senna Tschudin <peter.senna@gmail.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Liu Ying <gnuiyl@gmail.com> --- v4: * Newly introduced in v4. drivers/gpu/ipu-v3/ipu-dmfc.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-)