linux-kernel - Re: [PATCH v3 4/4] drm/mxsfb: Switch to drm_atomic_helper_commit_tail

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7a2ca85d38f0edc4033a426cca31b04b@agner.ch>
Date:   Wed, 08 Aug 2018 14:30:48 +0200
From:   Stefan Agner <stefan@...er.ch>
To:     Leonard Crestez <leonard.crestez@....com>
Cc:     "A.s. Dong" <aisheng.dong@....com>, marex@...x.de,
        linux-kernel@...r.kernel.org, dl-linux-imx <linux-imx@....com>,
        Robert Chiras <robert.chiras@....com>,
        Marius-cristian Vlad <marius-cristian.vlad@....com>,
        Fabio Estevam <fabio.estevam@....com>, p.zabel@...gutronix.de,
        dri-devel@...ts.freedesktop.org, shawnguo@...nel.org,
        Anson Huang <anson.huang@....com>, kernel@...gutronix.de,
        Mirela Rabulea <mirela.rabulea@....com>
Subject: Re: [PATCH v3 4/4] drm/mxsfb: Switch to
 drm_atomic_helper_commit_tail_rpm

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@...er.ch>

--
Stefan

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