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

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

[Shreeya Patel]

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.

Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
 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;
-- 
2.7.4

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

[Chris Wilson]

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.

> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> ---
>  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.
-Chris

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

[Shreeya Patel]

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.
> 
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> > ---
> >  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

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@gmail.com>
> > > ---
> > >  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