Re: [PATCH] gpu/drm/udl: Replace struct_mutex with driver private lock

[Daniel Vetter]

On Sat, Feb 10, 2018 at 06:47:31PM +0530, Shreeya Patel wrote:
> On Fri, 2018-02-09 at 12:18 +0000, Chris Wilson wrote:
> > Quoting Shreeya Patel (2018-02-09 12:10:56)
> > > 
> > > dev->struct_mutex is the Big DRM Lock and the only bit where
> > > itâs mandatory is serializing GEM buffer object destruction.
> > > Which unfortunately means drivers have to keep track of that
> > > lock and either call unreference or unreference_locked
> > > depending upon context. Core GEM doesnât have a need for
> > > struct_mutex any more since kernel 4.8.
> > > 
> > > For drivers that need struct_mutex it should be replaced by a
> > > driver
> > > private lock.
> > In that regard, dev->struct_mutex is already a driver private lock.
> > The
> > impetus is to reduce a big lock into lots of little locks where
> > contention deems prudent.
> 
> Ok. I'll make the changes in the commit message.

udl_driver_load would be a good place. Also, running with
CONFIG_PROVE_LOCKING enabled will catch this stuff. On that note, do you
have the hardware to test your changes? If not we need to find someone who
can do that.

Also please switch udl from the gem_free_object to the
gem_free_object_unlocked hook.
-Daniel

> > 
> > > 
> > > Signed-off-by: Shreeya Patel <shreeya.patel23498@xxxxxxxxx>
> > > ---
> > > Âdrivers/gpu/drm/udl/udl_dmabuf.c | 5 +++--
> > > Âdrivers/gpu/drm/udl/udl_drv.hÂÂÂÂ| 1 +
> > > Â2 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c
> > > b/drivers/gpu/drm/udl/udl_dmabuf.c
> > > index 2867ed1..120d2d9 100644
> > > --- a/drivers/gpu/drm/udl/udl_dmabuf.c
> > > +++ b/drivers/gpu/drm/udl/udl_dmabuf.c
> > > @@ -76,6 +76,7 @@ static struct sg_table *udl_map_dma_buf(struct
> > > dma_buf_attachment *attach,
> > > ÂÂÂÂÂÂÂÂstruct udl_drm_dmabuf_attachment *udl_attach = attach-
> > > >priv;
> > > ÂÂÂÂÂÂÂÂstruct udl_gem_object *obj = to_udl_bo(attach->dmabuf-
> > > >priv);
> > > ÂÂÂÂÂÂÂÂstruct drm_device *dev = obj->base.dev;
> > > +ÂÂÂÂÂÂÂstruct udl_device *udl = dev->dev_private;
> > > ÂÂÂÂÂÂÂÂstruct scatterlist *rd, *wr;
> > > ÂÂÂÂÂÂÂÂstruct sg_table *sgt = NULL;
> > > ÂÂÂÂÂÂÂÂunsigned int i;
> > > @@ -112,7 +113,7 @@ static struct sg_table *udl_map_dma_buf(struct
> > > dma_buf_attachment *attach,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ERR_PTR(-ENOMEM);
> > > ÂÂÂÂÂÂÂÂ}
> > > Â
> > > -ÂÂÂÂÂÂÂmutex_lock(&dev->struct_mutex);
> > > +ÂÂÂÂÂÂÂmutex_lock(&udl->urbs.plock);
> > > Â
> > > ÂÂÂÂÂÂÂÂrd = obj->sg->sgl;
> > > ÂÂÂÂÂÂÂÂwr = sgt->sgl;
> > > @@ -137,7 +138,7 @@ static struct sg_table *udl_map_dma_buf(struct
> > > dma_buf_attachment *attach,
> > > ÂÂÂÂÂÂÂÂattach->priv = udl_attach;
> > > Â
> > > Âerr_unlock:
> > > -ÂÂÂÂÂÂÂmutex_unlock(&dev->struct_mutex);
> > > +ÂÂÂÂÂÂÂmutex_unlock(&udl->urbs.plock);
> > > ÂÂÂÂÂÂÂÂreturn sgt;
> > > Â}
> > > Â
> > > diff --git a/drivers/gpu/drm/udl/udl_drv.h
> > > b/drivers/gpu/drm/udl/udl_drv.h
> > > index 2a75ab8..24cca17 100644
> > > --- a/drivers/gpu/drm/udl/udl_drv.h
> > > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > > @@ -39,6 +39,7 @@ struct urb_node {
> > > Â
> > > Âstruct urb_list {
> > > ÂÂÂÂÂÂÂÂstruct list_head list;
> > > +ÂÂÂÂÂÂÂstruct mutex plock;
> > > ÂÂÂÂÂÂÂÂspinlock_t lock;
> > > ÂÂÂÂÂÂÂÂstruct semaphore limit_sem;
> > > ÂÂÂÂÂÂÂÂint available;
> > This hasn't seen much testing as it lacks a mutex_init, and one would
> > wish for a description of what it is guarding.
> 
> Yes, I'll add mutex_init but I am not sure that in which function I
> should add it as there is no probe or init function.
> 
> Also I will add the description for the lock.
> 
> ThanksÂ
> > -Chris

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