[PATCH 2/4] drm/shmem: Use mutex_trylock in drm_gem_shmem_purge

[Rob Herring]

On Wed, Aug 21, 2019 at 3:23 AM Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Tue, Aug 20, 2019 at 07:35:47AM -0500, Rob Herring wrote:
> > On Tue, Aug 20, 2019 at 4:05 AM Daniel Vetter <daniel at ffwll.ch> wrote:
> > >
> > > On Mon, Aug 19, 2019 at 11:12:02AM -0500, Rob Herring wrote:
> > > > Lockdep reports a circular locking dependency with pages_lock taken in
> > > > the shrinker callback. The deadlock can't actually happen with current
> > > > users at least as a BO will never be purgeable when pages_lock is held.
> > > > To be safe, let's use mutex_trylock() instead and bail if a BO is locked
> > > > already.

[...]

> > > > -void drm_gem_shmem_purge(struct drm_gem_object *obj)
> > > > +bool drm_gem_shmem_purge(struct drm_gem_object *obj)
> > > >  {
> > > >       struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> > > >
> > > > -     mutex_lock(&shmem->pages_lock);
> > > > +     if (!mutex_trylock(&shmem->pages_lock))
> > >
> > > Did you see my ping about cutting all the locking over to dma_resv?
> >
> > Yes, but you didn't reply to Rob C. about it. I guess I'll have to go
> > figure out how reservation objects work...
>
> msm was the last driver that still used struct_mutex. It's a long-term
> dead-end, and I think with all the effort recently to create helpers for
> rendering drivers (shmem, vram, ttm refactoring) we should make a solid
> attempt to get aligned. Or did you mean that Rob Clark had some
> reply/questions that I didn' respond to because it fell through cracks?

I'm not using struct_mutex, so I'm confused as to why you keep
mentioning it. The list of BOs for the shrinker is protected with a
mutex for the list. That list head, list mutex, and the shrinker
instance all have to live at the driver level, so they can't be moved
into shmem as you suggested. Agreed?

Then there is the pages_lock within the shmem BO. I assume that is
what you are suggesting converting to dma_resv? I'm not really sure
what that would look like. You're going to have to spell it out for
me. In my brief look at it, it seems like added complexity and it's
not clear to me what that buys. Also, I think it would mostly be an
internal implementation detail of shmem helpers, though there is one
spot in panfrost that takes the lock (2 before this series). So it's
kind of orthogonal to this series.

Also, I think getting more drivers using shmem is more beneficial than
aligning the implementations of the GEM helpers. We should at least be
able to convert vgem and vkms I would think. Various KMS drivers too,
but there's an issue around kernel mappings (or lack of). There really
should be little reason for most KMS drivers to have a custom BO as
CMA or shmem helpers should work.

Rob