Re: [PATCH 2/2] drm/xe: Make a standalone snapshot to that survives unbind

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 30 Jan 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Instead of having the coredump embedded to the xe device,
> let's dynamically allocate that and remove only when
> requested by devcoredump.

[snip]

> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 50dac1a5b053..4372f5cc98b6 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -214,7 +214,7 @@ struct xe_device {
>  	struct drm_device drm;
>  
>  	/** @devcoredump: device coredump */
> -	struct xe_devcoredump devcoredump;
> +	struct xe_devcoredump *devcoredump;

Yes! I *much* prefer having pointer members in struct xe_device over
embedding structs, for anything where pointer chasing is not a
performance issue. Which is most things, really. Ditto in struct
drm_i915_private, but converting now is a lot of churn.

Anyway, with dynamic allocation you also don't need the type definition
here. You can just forward declare, and drop the include. This file here
is included by absolutely everywhere, and by proxy, so is
xe_devcoredump_types.h. Modify xe_devcoredump_types.h, and you need to
rebuild the entire driver, including all of display. It adds up.

For added bonus, move the struct definitions inside xe_devcoredump.c,
and make struct xe_devcoredump an opaque pointer. This is one of the few
things C has to enforce use of interfaces to access data. There's no
reason for anyone outside of xe_devcoredump.c to directly access
xe->devcoredump. And making the pointer opaque guarantees nobody
bypasses the interfaces in a whim. Those things happen, "I just quickly
...", and you end up in a mess.


BR,
Jani.


-- 
Jani Nikula, Intel