Re: [PATCH 3/8] drm/xe/ggtt: use drm_dev_enter to mark device section

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 29.04.2024 14:14, Matthew Auld wrote:
> Device can be hotunplugged before we start destroying gem objects. In
> such a case don't touch the GGTT entries, trigger any invalidations or
> mess around with rpm.  This should already be taken care of when
> removing the device, we just need to take care of dealing with the
> software state, like removing the mm node.
> 
> References: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1717
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_ggtt.c | 34 ++++++++++++++++++++++------------
>   1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 0d541f55b4fc..7c05d3f7e1a4 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -8,6 +8,7 @@
>   #include <linux/io-64-nonatomic-lo-hi.h>
>   #include <linux/sizes.h>
>   
> +#include <drm/drm_drv.h>
>   #include <drm/drm_managed.h>
>   #include <drm/i915_drm.h>
>   
> @@ -433,18 +434,27 @@ int xe_ggtt_insert_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
>   void xe_ggtt_remove_node(struct xe_ggtt *ggtt, struct drm_mm_node *node,
>   			 bool invalidate)
>   {
> -	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
> -
> -	mutex_lock(&ggtt->lock);
> -	xe_ggtt_clear(ggtt, node->start, node->size);
> -	drm_mm_remove_node(node);
> -	node->size = 0;
> -	mutex_unlock(&ggtt->lock);
> -
> -	if (invalidate)
> -		xe_ggtt_invalidate(ggtt);
> -
> -	xe_pm_runtime_put(tile_to_xe(ggtt->tile));
> +	struct xe_device *xe = tile_to_xe(ggtt->tile);
> +	int idx;
> +
> +	if (drm_dev_enter(&xe->drm, &idx)) {
> +		xe_pm_runtime_get_noresume(xe);
> +		mutex_lock(&ggtt->lock);
> +		xe_ggtt_clear(ggtt, node->start, node->size);
> +		drm_mm_remove_node(node);
> +		node->size = 0;
> +		mutex_unlock(&ggtt->lock);
> +
> +		if (invalidate)
> +			xe_ggtt_invalidate(ggtt);
> +		xe_pm_runtime_put(xe);
> +		drm_dev_exit(idx);
> +	} else {
> +		mutex_lock(&ggtt->lock);
> +		drm_mm_remove_node(node);
> +		node->size = 0;
> +		mutex_unlock(&ggtt->lock);
> +	}


There is little redundancy between both 'if' branches, I wonder
it it wouldn't be better to do sth like:
	bool driver_bound = drm_dev_enter(&xe->drm, &idx);

	if (driver_bound)
		xe_pm_runtime_get_noresume(xe);
	mutex_lock(&ggtt->lock);
	if (driver_bound)
		xe_ggtt_clear(ggtt, node->start, node->size);
	drm_mm_remove_node(node);
	node->size = 0;
	mutex_unlock(&ggtt->lock);
	if (!driver_bound)
		return;

	if (invalidate)
		xe_ggtt_invalidate(ggtt);
	xe_pm_runtime_put(xe);
	drm_dev_exit(idx);

No big feelings.

One related thing: drm_dev_unplug is called from 
xe_device_remove_display, sounds slightly misleading as drm_dev is not 
only about display. Maybe xe_device_remove_display should be 
incorporated into xe_device_remove to make it more clear, and more 
symmetric with drm_dev_register called from xe_device_probe.

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej


>   }
>   
>   void xe_ggtt_remove_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)