| Message ID | 1594137527-15201-1-git-send-email-martin.fuzzey@flowbird.group (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: imx: Fix occasional screen corruption on modeset. | expand |
Hi Martin, On Tue, 2020-07-07 at 17:56 +0200, Martin Fuzzey wrote: > When performing a modeset the atomic core calls > ipu_crtc_atomic_disable() which switches off the DC and DI. > > When we immediately restart as in the modeset case this sometimes > leads to corruption at the bottom of the screen looking like a mirror > image of the top. Could this be just a panel getting confused because the pixel clock is disabled, or is there really an issue with the IPU? Have you tried just keeping clk_di_pixel enabled in ipu_di_disable(), but continuing to disable DI and DC? > The exact reason isn't understood but it seems timing related. > > This was observed on i.MX6DL on a system that does 2 mode > transitions on boot (fbcon->android boot animation->android homescreen) > then no more during normal operation resulting in corruption > about once every 10 boots that lasted for variable durations > from a few seconds to several hours. > > Dumping the buffers confirmed that they were correct in memory, > just the display was wrong. > > For tests the problem was reproduced systematically by forcing > a full modeset every 10 frames even if when not required. > > Leaving the DC and DI on if the CRTC is staying on fixes this. > > Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group> > --- > drivers/gpu/drm/imx/ipuv3-crtc.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c > index 63c0284..9137b64 100644 > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c > @@ -84,8 +84,15 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc, > struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc); > struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent); > > - ipu_dc_disable_channel(ipu_crtc->dc); > - ipu_di_disable(ipu_crtc->di); > + /* > + * If we are just doing a modeset don't disable dc/di as that > + * sometimes leads to corruption at the bottom of the screen > + */ > + if (!crtc->state->active) { > + ipu_dc_disable_channel(ipu_crtc->dc); > + ipu_di_disable(ipu_crtc->di); Just removing ipu_di_disable() leaks a clk_prepare_enable refcount on the di->clk_di_pixel clock. Also this is followed by an ipu_dc_disable(), which should remove the DC module's clock if this is the only display. So the DC should be disabled anyway. > + } > + > /* > * Planes must be disabled before DC clock is removed, as otherwise the > * attached IDMACs will be left in undefined state, possibly hanging regards Philipp
Hi Philipp, thanks for the quick reply. On Wed, 8 Jul 2020 at 10:31, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Could this be just a panel getting confused because the pixel clock is > disabled, or is there really an issue with the IPU? Have you tried just > keeping clk_di_pixel enabled in ipu_di_disable(), but continuing > to disable DI and DC? > I've tried that now but still observed the corruption. > > The exact reason isn't understood but it seems timing related. > > Also just adding a msleep(2) or a mdelay(20) at the end of ipu_crtc_atomic_disable() makes the problem go away too. Obviously I'm not suggesting doing that, just that it may help understand what is going on. > > Just removing ipu_di_disable() leaks a clk_prepare_enable refcount on > the di->clk_di_pixel clock. > > Also this is followed by an ipu_dc_disable(), which should remove the DC > module's clock if this is the only display. So the DC should be disabled > anyway. > True. How about doing just ipu_crtc_disable_planes() and drm_crtc_vblank_off() in the active (modeset) case. and in ipu_crtc_atomic_enable() only doing the stuff (which doesn't touch the planes) if old_state->active == false ? That will fix the clock refcount problem and seems generally better as tearing down everything just to do a modeset seems a bit heavy handed. I've tested that and it works too but it's probably better to discuss this some more before sending a new patch. Regards, Martin
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 63c0284..9137b64 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -84,8 +84,15 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc, struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc); struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent); - ipu_dc_disable_channel(ipu_crtc->dc); - ipu_di_disable(ipu_crtc->di); + /* + * If we are just doing a modeset don't disable dc/di as that + * sometimes leads to corruption at the bottom of the screen + */ + if (!crtc->state->active) { + ipu_dc_disable_channel(ipu_crtc->dc); + ipu_di_disable(ipu_crtc->di); + } + /* * Planes must be disabled before DC clock is removed, as otherwise the * attached IDMACs will be left in undefined state, possibly hanging
When performing a modeset the atomic core calls ipu_crtc_atomic_disable() which switches off the DC and DI. When we immediately restart as in the modeset case this sometimes leads to corruption at the bottom of the screen looking like a mirror image of the top. The exact reason isn't understood but it seems timing related. This was observed on i.MX6DL on a system that does 2 mode transitions on boot (fbcon->android boot animation->android homescreen) then no more during normal operation resulting in corruption about once every 10 boots that lasted for variable durations from a few seconds to several hours. Dumping the buffers confirmed that they were correct in memory, just the display was wrong. For tests the problem was reproduced systematically by forcing a full modeset every 10 frames even if when not required. Leaving the DC and DI on if the CRTC is staying on fixes this. Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group> --- drivers/gpu/drm/imx/ipuv3-crtc.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)