On 08.08.2018 10:00, Leonard Crestez wrote:
On Tue, 2018-08-07 at 21:01 +0200, Stefan Agner wrote:
On 06.08.2018 21:31, Leonard Crestez wrote:
The lcdif block is only powered on when display is active so plane updates when not enabled are not valid. Writing to an unpowered IP block is mostly ignored but can trigger bus errors on some chips.
Prevent this situation by switching to drm_atomic_helper_commit_tail_rpm and having the drm core ensure atomic_plane_update is only called while the crtc is active. This avoids having to keep track of "enabled" bits inside the mxsfb driver.
This also requires handling the vblank event for disable from ~~mxsfb_pipe_update~~ **mxsfb_pipe_disable**.
Hm, I don't think this is a new requirement. Simple KMS Helper Reference clearly states that it should be called from update.
Probably using drm_atomic_helper_commit_tail_rpm just exacerbates an issue which we haven't seen before...
Since I think it is a general fix, I'd rather prefer have it in a separate commit.
I wrote the commit message wrong, what I meant is that it requires handling the vblank event from *disable*.
Switching to atomic_helper_commit_tail_rpm means atomic_update is no longer called when !state->active so nobody dispatches the last vblank event for disabling the crtc. This causes a warning in drm_atomic_helper_commit_hw_done on disable.
Hm, I see, atomic_helper_commit_tail_rpm() uses DRM_PLANE_COMMIT_ACTIVE_ONLY, which leads to update() not being called for Simple KMS (since simple KMS uses the plane atomic_update() hook).
I was looking in other drivers such as drivers/gpu/drm/pl111/pl111_display.c and was wondering why they do not need that in disable. But it makes sense since they do not use atomic_helper_commit_tail_rpm(), update does get called. Also note that pl111_display_update() uses drm_crtc_send_vblank_event() too if the crtc gets disabled:
if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0) drm_crtc_arm_vblank_event(crtc, event); else drm_crtc_send_vblank_event(crtc, event);
The other users of atomic_helper_commit_tail_rpm(), exynos and sun4i, call drm_crtc_send_vblank_event() in the CRTC atomic_disable() hook. The Simple KMS equivalent of that is the disable hook.
So it is really the change to _rpm() which requires to add drm_crtc_send_vblank_event() in disable. Having this two changes in a single commit indeed makes sense and is correct, sorry about the noise! Just fix the commit, and than I am fine with this.
Reviewed-by: Stefan Agner stefan@agner.ch
-- Stefan
Looking through the docs there seems to be a lot of complexity behind vblank events so maybe I'm missing something.