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);
>> }
--
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
