Re: [PATCH v4 5/7] drm/panfrost: Add a new ioctl to submit batches

From: Daniel Vetter <daniel@ffwll.ch>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: "Jason Ekstrand" <jason@jlekstrand.net>,
	"Tomeu Vizoso" <tomeu.vizoso@collabora.com>,
	"Christian König" <ckoenig.leichtzumerken@gmail.com>,
	dri-devel@lists.freedesktop.org,
	"Steven Price" <steven.price@arm.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Alyssa Rosenzweig" <alyssa.rosenzweig@collabora.com>,
	"Robin Murphy" <robin.murphy@arm.com>
Subject: Re: [PATCH v4 5/7] drm/panfrost: Add a new ioctl to submit batches
Date: Tue, 27 Jul 2021 11:17:38 +0200	[thread overview]
Message-ID: <YP/PMvbsMS7H/5Sy@phenom.ffwll.local> (raw)
In-Reply-To: <20210726122706.7fb3a8d5@collabora.com>

On Mon, Jul 26, 2021 at 12:27:06PM +0200, Boris Brezillon wrote:
> On Thu, 8 Jul 2021 14:10:45 +0200
> Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
> 
> > >> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > >> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > >> @@ -254,6 +254,9 @@ static int panfrost_acquire_object_fences(struct panfrost_job *job)
> > >>   				return ret;
> > >>   		}
> > >>   
> > >> +		if (job->bo_flags[i] & PANFROST_BO_REF_NO_IMPLICIT_DEP)
> > >> +			continue;  
> > > This breaks dma_resv rules. I'll send out patch set fixing this pattern in
> > > other drivers, I'll ping you on that for what you need to change. Should
> > > go out today or so.
> 
> I guess you're talking about [1]. TBH, I don't quite see the point of
> exposing a 'no-implicit' flag if we end up forcing this implicit dep
> anyway, but I'm probably missing something.

Yeah that's the patch set.

Note that there's better ways to do this, it's just that these better ways
take more typing and need some actual testing (and ideally igts and
everything).

The NO_IMPLICIT flag is still useful even with the hacky solution, as long
as you don't set a write fence too. For that it might be better to look
into the dma_fence import patch from Jason:

https://lore.kernel.org/dri-devel/20210610210925.642582-7-jason@jlekstrand.net/

> > I'm really wondering if the behavior that the exclusive fences replaces 
> > all the shared fences was such a good idea.
> 
> Is that what's done in [1], or are you talking about a different
> patchset/approach?

That's just how dma_resv works for the exclusive slot.
-Daniel

> 
> > 
> > It just allows drivers to mess up things in a way which can be easily 
> > used to compromise the system.
> 
> I must admit I'm a bit lost, so I'm tempted to drop that flag for now
> :-).
> 
> [1]https://patchwork.freedesktop.org/patch/443711/?series=92334&rev=3

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch