Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Date:   Wed, 08 Aug 2018 14:30:48 +0200
From:   Stefan Agner <stefan@agner.ch>
To:     Leonard Crestez <leonard.crestez@nxp.com>
Cc:     "A.s. Dong" <aisheng.dong@nxp.com>, marex@denx.de,
        linux-kernel@vger.kernel.org, dl-linux-imx <linux-imx@nxp.com>,
        Robert Chiras <robert.chiras@nxp.com>,
        Marius-cristian Vlad <marius-cristian.vlad@nxp.com>,
        Fabio Estevam <fabio.estevam@nxp.com>, p.zabel@pengutronix.de,
        dri-devel@lists.freedesktop.org, shawnguo@kernel.org,
        Anson Huang <anson.huang@nxp.com>, kernel@pengutronix.de,
        Mirela Rabulea <mirela.rabulea@nxp.com>
Subject: Re: [PATCH v3 4/4] drm/mxsfb: Switch to
 drm_atomic_helper_commit_tail_rpm
In-Reply-To: <174c00104b40ebe1f6495e679e115948bb561889.camel@nxp.com>
References: <cover.1533583611.git.leonard.crestez@nxp.com>
 <cd938df7516aee05289dc2d7dc17bc85b219be53.1533583611.git.leonard.crestez@nxp.com>
 <da414ab37c3d7c02f059fd3c0470dac9@agner.ch>
 <174c00104b40ebe1f6495e679e115948bb561889.camel@nxp.com>
Message-ID: <7a2ca85d38f0edc4033a426cca31b04b@agner.ch>
User-Agent: Roundcube Webmail/1.3.4
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.