On Fri, Aug 16, 2024 at 01:33:55PM +0530, apoorva.singh@intel.com wrote:

From: Apoorva Singh <apoorva.singh@intel.com>

- lrc->bo NULL check is not needed in xe_lrc_snapshot_capture() as
  its already been taken care of in xe_lrc_init().

I'm afraid this is not a good reason.
This snapshot capture is coming from other places and apparently
with risks of paths where bo was already freed?!

I didn't think about it while suggesting this while reviewing https://patchwork.freedesktop.org/patch/608210/?series=137289&rev=1

If the bo is null then we will hit NPD while doing xe_lrc_ggtt_addr() in the next like. so I guess it should be then

if (lrc->bo) {

snapshot->context_desc = xe_lrc_ggtt_addr(lrc);

snapshot->lrc_bo = xe_bo_get(lrc->bo);

....

}

Regards,

Nirmoy

why is this check bothering you so much?

Signed-off-by: Apoorva Singh <apoorva.singh@intel.com>
---
 drivers/gpu/drm/xe/xe_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
index 974a9cd8c379..aec7db39c061 100644
--- a/drivers/gpu/drm/xe/xe_lrc.c
+++ b/drivers/gpu/drm/xe/xe_lrc.c
@@ -1649,7 +1649,7 @@ struct xe_lrc_snapshot *xe_lrc_snapshot_capture(struct xe_lrc *lrc)
 	if (!snapshot)
 		return NULL;
 
-	if (lrc->bo && lrc->bo->vm)
+	if (lrc->bo->vm)
 		xe_vm_get(lrc->bo->vm);
 
 	snapshot->context_desc = xe_lrc_ggtt_addr(lrc);
-- 
2.34.1