Re: [PATCH v3 4/4] drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm

[Stefan Agner]

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

--
Stefan

> 
> Looking through the docs there seems to be a lot of complexity behind
> vblank events so maybe I'm missing something.