[Intel-xe] [PATCH v11 04/12] drm/xe/mmio: grab mem_access in xe_mmio_ioctl

[Matthew Auld]

On 12/06/2023 19:13, Matthew Brost wrote:
> On Mon, Jun 12, 2023 at 06:12:17PM +0100, Matthew Auld wrote:
>> Any kind of device memory access should first ensure the device is not
>> suspended, mmio included.
>>
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_mmio.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>> index f7a7f996b37f..cc5ccbef85a4 100644
>> --- a/drivers/gpu/drm/xe/xe_mmio.c
>> +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> @@ -476,6 +476,7 @@ int xe_mmio_ioctl(struct drm_device *dev, void *data,
>>   	 */
>>   	reg = XE_REG(args->addr);
>>   
>> +	xe_device_mem_access_get(xe);
> 
> What about moving xe_device_mem_access_get/put to xe_force_wake_get/put?
> Just an idea, but this patch LGTM so I'll leave it up to you if you want
> to keep it as is.

Yeah, I think Rodrigo also wanted something similar. However the 
pm_runtime_get_sync() in xe_device_mem_access_get() is scary since it 
allocates memory, plus if it ever reaches our callback 
xe_pm_runtime_resume(), we might grab various locks like ggtt->lock, 
ct->lock etc. That is the main motivation for the patch that adds the 
lockdep annotation. The xe_force_wake_get() is called in some pretty low 
level places AFAICT so my worry is that we will just see a big stream of 
lockdep splats, like already holding ggtt->lock etc. My feeling is that 
xe_device_mem_access_get() should be a fairly "outer" thing, and 
xe_force_wake_get() etc should be more an "inner" thing when looking at 
the hierarchy. What do you think?

But perhaps we can add some helper like xe_force_wake_mem_access_get() 
for the cases where we are close to the ioctl like in this patch?

> 
> With that:
> Reviewed-by: Matthew Brost <matthew.brost at intel.com>
> 
>>   	xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>   
>>   	if (args->flags & DRM_XE_MMIO_WRITE) {
>> @@ -519,6 +520,7 @@ int xe_mmio_ioctl(struct drm_device *dev, void *data,
>>   
>>   exit:
>>   	xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> +	xe_device_mem_access_put(xe);
>>   
>>   	return ret;
>>   }
>> -- 
>> 2.40.1
>>