Re: [PATCH v5 09/10] drm: rcar-du: Perform group setup from the atomic tail handler

From: Maxime Ripard <maxime@cerno.tech>
To: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	Ulrich Hecht <uli+renesas@fpond.eu>
Subject: Re: [PATCH v5 09/10] drm: rcar-du: Perform group setup from the atomic tail handler
Date: Fri, 26 Mar 2021 15:37:15 +0100	[thread overview]
Message-ID: <20210326143715.tpk4o62xgjvigefe@gilmour> (raw)
In-Reply-To: <20210322163535.1090570-10-kieran.bingham+renesas@ideasonboard.com>

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

Hi Kieran,

On Mon, Mar 22, 2021 at 04:35:34PM +0000, Kieran Bingham wrote:
> Create rcar_du_group_atomic_check() and rcar_du_group_atomic_setup()
> functions to track and apply group state through the DRM atomic state.
> The use_count field is moved from the rcar_du_group structure to an
> enabled field in the rcar_du_group_state structure.
> 
> This allows separating group setup from the configuration of the CRTCs
> that are part of the group, simplifying the CRTC code and improving
> overall readability. The existing rcar_du_group_{get,put}() functions
> are now redundant and removed.
> 
> The groups share clocking with the CRTCs within the group, and so are
> accessible only when one of its CRTCs has been powered through
> rcar_du_crtc_atomic_exit_standby().
> 
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

It's a bit weird to have both your and Laurent's SoB without a
Co-Developped-By or an authorship from him.

However, using a drm_private_obj shared between CRTC has a gotcha: you
don't have any ordering guarantee between commits if they affect
different CRTCs (and they are non-blocking).

Let's assume we have two subsequent commits, commit1 and commit2, both
non-blocking, and affecting different CRTC, plane and connectors. In
this case, commit1 old private state will be commit0 new private state,
and commit 2 old private state will be commit1 new private state.

If commit2 is executed before commit1, then it will free its old state
when done with it (so commit1 new private state), and will thus result
in an use-after-free when commit1 is ran.

In order to fix this, you should store (and get a reference to) the
drm_crtc_commit in your private state in atomic_commit_setup, and call
drm_crtc_commit_wait on that commit as the first part of your
commit_tail to serialize those commits.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]