Re: [PATCH 1/2] drm/xe/dgfx: Block rpm for active mmap mappings

[Matthew Auld <matthew.auld@intel.com>]

On 08/12/2023 07:29, Nilawar, Badal wrote:
> 
> 
> On 07-12-2023 18:36, Matthew Auld wrote:
>> On 07/12/2023 11:26, Matthew Auld wrote:
>>> On 06/12/2023 13:34, Badal Nilawar wrote:
>>>> Block rpm for discrete cards when mmap mappings are active.
>>>> Ideally rpm wake ref should be taken in vm_open call and put in 
>>>> vm_close
>>>> call but it is seen that vm_open doesn't get called for xe_gem_vm_ops.
>>>> Therefore rpm wake ref is being get in xe_drm_gem_ttm_mmap and put
>>>> in vm_close.
>>>>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
>>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/xe/xe_bo.c | 35 +++++++++++++++++++++++++++++++++--
>>>>   1 file changed, 33 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>>>> index 72dc4a4eed4e..5741948a2a51 100644
>>>> --- a/drivers/gpu/drm/xe/xe_bo.c
>>>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>>>> @@ -15,6 +15,7 @@
>>>>   #include <drm/ttm/ttm_tt.h>
>>>>   #include <drm/xe_drm.h>
>>>> +#include "i915_drv.h"
>>>
>>> Do we need this?
>>>
>>>>   #include "xe_device.h"
>>>>   #include "xe_dma_buf.h"
>>>>   #include "xe_drm_client.h"
>>>> @@ -1158,17 +1159,47 @@ static vm_fault_t xe_gem_fault(struct 
>>>> vm_fault *vmf)
>>>>       return ret;
>>>>   }
>>>> +static void xe_ttm_bo_vm_close(struct vm_area_struct *vma)
>>>> +{
>>>> +    struct ttm_buffer_object *tbo = vma->vm_private_data;
>>>> +    struct drm_device *ddev = tbo->base.dev;
>>>> +    struct xe_device *xe = to_xe_device(ddev);
>>>> +
>>>> +    ttm_bo_vm_close(vma);
>>>> +
>>>> +    if (tbo->resource->bus.is_iomem)
>>>> +        xe_device_mem_access_put(xe);
>>
>> Are you sure this works as expected? Say if the user partially unmaps 
>> something?
>>
>> map = mmap(obj, size);
>> unmap(map, size/2);
>> unmap(map, size);
>>
>> That would be one mmap but multiple vm_close calls leading to an 
>> imbalance in the RPM ref. I think we need the access_get in the 
>> vm_open also?I haven't tried partial mmap but for single mmap-unmap I 
>> observed
> equal number of xe_drm_gem_ttm_mmap and vm_close call. Will try partial 
> mmap.
> 
> For mem_access_get in vm_open, initially we were trying the same but 
> observed that vm_open never get called.

Yeah, if you do:

mmap(obj, size)
munmap(obj, size)

That will do mmap and one vm_close, no vm_open AFAICT. But that looks to 
be fine here.

> In fact i915 i915_gem_mman.c we found this comment for vm_open.
>          /*
>           * When we install vm_ops for mmap we are too late for
>           * the vm_ops->open() which increases the ref_count of
>           * this obj and then it gets decreased by the vm_ops->close().
>           * To balance this increase the obj ref_count here.
>           */
> Does similar reason applicable for xe vm_open as well?

I think so. If you do:

mmap(obj, size)
munmap(obj, size/2)
munmap(obj, size)

That will do one mmap, one vm_open for the newly split vma and finally 
two vm_closes, AFAICT.

> 
> Regards,
> Badal
>>
>>>> +}
>>>> +
>>>>   static const struct vm_operations_struct xe_gem_vm_ops = {
>>>>       .fault = xe_gem_fault,
>>>>       .open = ttm_bo_vm_open,
>>>> -    .close = ttm_bo_vm_close,
>>>> +    .close = xe_ttm_bo_vm_close,
>>>>       .access = ttm_bo_vm_access
>>>>   };
>>>> +int xe_drm_gem_ttm_mmap(struct drm_gem_object *gem,
>>>> +            struct vm_area_struct *vma)
>>>> +{
>>>> +    struct ttm_buffer_object *tbo = drm_gem_ttm_of_gem(gem);
>>>> +    struct drm_device *ddev = tbo->base.dev;
>>>> +    struct xe_device *xe = to_xe_device(ddev);
>>>> +    int ret;
>>>> +
>>>> +    ret = drm_gem_ttm_mmap(gem, vma);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    if (tbo->resource->bus.is_iomem)
>>>> +        xe_device_mem_access_get(xe);
>>>
>>> Checking is_iomem outside of the usual locking is racy. One issue 
>>> here is that is_iomem can freely change at any point (like at fault 
>>> time) so when vm_close is called you can easily get an an unbalanced 
>>> RPM ref count. For example io_mem is false here but later becomes 
>>> true in bo_vm_close and then we call mem_access_put even though we 
>>> never called mem_access_get.
>>>
>>> Maybe check the possible placements of the object instead since that 
>>> is immutable?
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static const struct drm_gem_object_funcs xe_gem_object_funcs = {
>>>>       .free = xe_gem_object_free,
>>>>       .close = xe_gem_object_close,
>>>> -    .mmap = drm_gem_ttm_mmap,
>>>> +    .mmap = xe_drm_gem_ttm_mmap,
>>>>       .export = xe_gem_prime_export,
>>>>       .vm_ops = &xe_gem_vm_ops,
>>>>   };