Re: [Freedreno] [PATCH] fixup! drm/msm: Separate locking of buffer resources from struct_mutex

[Sushmita Susheelendra <ssusheel@xxxxxxxxxxxxxx>]

Sounds good.

On 2017-06-16 15:44, Rob Clark wrote:
> On Fri, Jun 16, 2017 at 5:32 PM,  <ssusheel@xxxxxxxxxxxxxx> wrote:
> > Hi Rob,
> >
> > This looks good to me!
> >
> > Just one nit: msm_gem_vunmap becomes very shrinker specific as it 
> > holds the
> > msm_obj->lock with the shrinker class. Should we have the caller i.e.
> > msm_gem_shrinker_vmap hold it instead? Or change it's name to
> > msm_gem_vunmap_shrinker or something like that...?
>
> Hmm, I suppose I could pass the lock class in to msm_gem_vunmap() in
> case we ever want to call it elsewhere (iirc, i915 did something like
> that)
>
> BR,
> -R
>
> > It does make sense to make the madv/priv->inactive_list locking 
> > cleanup
> > separately.
> >
> > Thanks,
> > Sushmita
> >
> >
> >
> > On 2017-06-16 08:22, Rob Clark wrote:
> > > ---
> > > Ok, 2nd fixup to handle vmap shrinking.
> > >
> > >  drivers/gpu/drm/msm/msm_gem.c | 44
> > > +++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 36 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_gem.c 
> > > b/drivers/gpu/drm/msm/msm_gem.c
> > > index f5d1f84..3190e05 100644
> > > --- a/drivers/gpu/drm/msm/msm_gem.c
> > > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > > @@ -42,6 +42,9 @@ enum {
> > >         OBJ_LOCK_SHRINKER,
> > >  };
> > >
> > > +static void msm_gem_vunmap_locked(struct drm_gem_object *obj);
> > > +
> > > +
> > >  static dma_addr_t physaddr(struct drm_gem_object *obj)
> > >  {
> > >         struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > > @@ -484,6 +487,7 @@ int msm_gem_dumb_map_offset(struct drm_file 
> > > *file,
> > > struct drm_device *dev,
> > >  void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> > >  {
> > >         struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > > +       int ret = 0;
> > >
> > >         mutex_lock(&msm_obj->lock);
> > >
> > > @@ -492,22 +496,35 @@ void *msm_gem_get_vaddr(struct drm_gem_object 
> > > *obj)
> > >                 return ERR_PTR(-EBUSY);
> > >         }
> > >
> > > +       /* increment vmap_count *before* vmap() call, so shrinker can
> > > +        * check vmap_count (is_vunmapable()) outside of 
> > > msm_obj->lock.
> > > +        * This guarantees that we won't try to msm_gem_vunmap() this
> > > +        * same object from within the vmap() call (while we already
> > > +        * hold msm_obj->lock)
> > > +        */
> > > +       msm_obj->vmap_count++;
> > > +
> > >         if (!msm_obj->vaddr) {
> > >                 struct page **pages = get_pages(obj);
> > >                 if (IS_ERR(pages)) {
> > > -                       mutex_unlock(&msm_obj->lock);
> > > -                       return ERR_CAST(pages);
> > > +                       ret = PTR_ERR(pages);
> > > +                       goto fail;
> > >                 }
> > >                 msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
> > >                                 VM_MAP, 
> > > pgprot_writecombine(PAGE_KERNEL));
> > >                 if (msm_obj->vaddr == NULL) {
> > > -                       mutex_unlock(&msm_obj->lock);
> > > -                       return ERR_PTR(-ENOMEM);
> > > +                       ret = -ENOMEM;
> > > +                       goto fail;
> > >                 }
> > >         }
> > > -       msm_obj->vmap_count++;
> > > +
> > >         mutex_unlock(&msm_obj->lock);
> > >         return msm_obj->vaddr;
> > > +
> > > +fail:
> > > +       msm_obj->vmap_count--;
> > > +       mutex_unlock(&msm_obj->lock);
> > > +       return ERR_PTR(ret);
> > >  }
> > >
> > >  void msm_gem_put_vaddr(struct drm_gem_object *obj)
> > > @@ -554,7 +571,7 @@ void msm_gem_purge(struct drm_gem_object *obj)
> > >
> > >         put_iova(obj);
> > >
> > > -       msm_gem_vunmap(obj);
> > > +       msm_gem_vunmap_locked(obj);
> > >
> > >         put_pages(obj);
> > >
> > > @@ -576,10 +593,12 @@ void msm_gem_purge(struct drm_gem_object *obj)
> > >         mutex_unlock(&msm_obj->lock);
> > >  }
> > >
> > > -void msm_gem_vunmap(struct drm_gem_object *obj)
> > > +static void msm_gem_vunmap_locked(struct drm_gem_object *obj)
> > >  {
> > >         struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > >
> > > +       WARN_ON(!mutex_is_locked(&msm_obj->lock));
> > > +
> > >         if (!msm_obj->vaddr || WARN_ON(!is_vunmapable(msm_obj)))
> > >                 return;
> > >
> > > @@ -587,6 +606,15 @@ void msm_gem_vunmap(struct drm_gem_object *obj)
> > >         msm_obj->vaddr = NULL;
> > >  }
> > >
> > > +void msm_gem_vunmap(struct drm_gem_object *obj)
> > > +{
> > > +       struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > > +
> > > +       mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
> > > +       msm_gem_vunmap_locked(obj);
> > > +       mutex_unlock(&msm_obj->lock);
> > > +}
> > > +
> > >  /* must be called before _move_to_active().. */
> > >  int msm_gem_sync_object(struct drm_gem_object *obj,
> > >                 struct msm_fence_context *fctx, bool exclusive)
> > > @@ -799,7 +827,7 @@ void msm_gem_free_object(struct drm_gem_object 
> > > *obj)
> > >
> > >                 drm_prime_gem_destroy(obj, msm_obj->sgt);
> > >         } else {
> > > -               msm_gem_vunmap(obj);
> > > +               msm_gem_vunmap_locked(obj);
> > >                 put_pages(obj);
> > >         }
> _______________________________________________
> Freedreno mailing list
> Freedreno@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/freedreno
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html