Re: [PATCH 6/8] drm/vc4: kms: Wait on previous FIFO users before a commit

From: Maxime Ripard <maxime@cerno.tech>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, Tim Gover <tim.gover@raspberrypi.com>,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	David Airlie <airlied@linux.ie>,
	dri-devel@lists.freedesktop.org, Rob Herring <robh+dt@kernel.org>,
	bcm-kernel-feedback-list@broadcom.com,
	linux-rpi-kernel@lists.infradead.org,
	Daniel Vetter <daniel.vetter@intel.com>,
	Frank Rowand <frowand.list@gmail.com>,
	Phil Elwell <phil@raspberrypi.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 6/8] drm/vc4: kms: Wait on previous FIFO users before a commit
Date: Tue, 1 Dec 2020 18:06:49 +0100	[thread overview]
Message-ID: <20201201170649.g2uo6c7ngxdvfw3j@gilmour> (raw)
In-Reply-To: <8c5f37b3-a4ff-d4d0-ebe0-8bc931416293@suse.de>

[-- Attachment #1.1: Type: text/plain, Size: 2646 bytes --]

Hi Thomas,

On Fri, Nov 20, 2020 at 02:19:45PM +0100, Thomas Zimmermann wrote:
> Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> > If we're having two subsequent, non-blocking, commits on two different
> > CRTCs that share no resources, there's no guarantee on the order of
> > execution of both commits.
> 
> Can there only ever be two commits that flip order?

It needs a bit of bad luck, but the patch 2 provides a bit more details.

Basically, if there's two subsequent non-blocking commits, affecting
different CRTCs without anything shared between those CRTCs (so no
plane, encoder or connector in common), you have no guarantee on the
commit order.

Most of the time it's not a big deal precisely because they don't share
anything. However if the private state is shared between the CRTCs then
it becomes an issue and we need to make sure that the ordering is
respected.

> > However, the second one will consider the first one as the old state,
> > and will be in charge of freeing it once that second commit is done.
> > 
> > If the first commit happens after that second commit, it might access
> > some resources related to its state that has been freed, resulting in a
> > use-after-free bug.
> > 
> > The standard DRM objects are protected against this, but our HVS private
> > state isn't so let's make sure we wait for all the previous FIFO users
> > to finish their commit before going with our own.
> 
> I'd appreciate a comment in the code that explains a bit how this works.
> It's sort of clear to me, but not enough to fully get it.

I'm not sure to get what "this" means and what do you want me to comment
there?

> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_kms.c | 118 +++++++++++++++++++++++++++++++++-
> >   1 file changed, 117 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 3034a5a6637e..849bc6b4cea4 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -40,6 +40,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
> >   struct vc4_hvs_state {
> >   	struct drm_private_state base;
> >   	unsigned int unassigned_channels;
> > +
> > +	struct {
> > +		unsigned in_use: 1;
> > +		struct drm_crtc_commit *last_user;
> 
> Can these updates run concurrently? If so, the concurrency control via
> in_use is dubious.

No, there's only ever one commit being done. We just need to make sure
they are in the right order.

I'll address your other comments

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel