Re: [Intel-gfx] [PATCH v2 6/6] drm/i915: make device info a const pointer to rodata

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/06/2023 16:14, Jani Nikula wrote:
> Finally we can get rid of the pseudo-const write-once device info, and
> convert it into a const pointer to device info in rodata.
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h          |  4 ++--
>   drivers/gpu/drm/i915/intel_device_info.c | 17 ++++-------------
>   2 files changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8947d1201298..682ef2b5c7d5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -203,7 +203,7 @@ struct drm_i915_private {
>   	/* i915 device parameters */
>   	struct i915_params params;
>   
> -	const struct intel_device_info __info; /* Use INTEL_INFO() to access. */
> +	const struct intel_device_info *__info; /* Use INTEL_INFO() to access. */
>   	struct intel_runtime_info __runtime; /* Use RUNTIME_INFO() to access. */
>   	struct intel_driver_caps caps;
>   
> @@ -415,7 +415,7 @@ static inline struct intel_gt *to_gt(struct drm_i915_private *i915)
>   	     (engine__) && (engine__)->uabi_class == (class__); \
>   	     (engine__) = rb_to_uabi_engine(rb_next(&(engine__)->uabi_node)))
>   
> -#define INTEL_INFO(i915)	(&(i915)->__info)
> +#define INTEL_INFO(i915)	((i915)->__info)
>   #define RUNTIME_INFO(i915)	(&(i915)->__runtime)
>   #define DISPLAY_INFO(i915)	((i915)->display.info.__device_info)
>   #define DISPLAY_RUNTIME_INFO(i915)	(&(i915)->display.info.__runtime_info)
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 0740922cd71f..ea0ec6174ce5 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -364,13 +364,6 @@ void intel_device_info_runtime_init_early(struct drm_i915_private *i915)
>   	intel_device_info_subplatform_init(i915);
>   }
>   
> -/* FIXME: Remove this, and make device info a const pointer to rodata. */
> -static struct intel_device_info *
> -mkwrite_device_info(struct drm_i915_private *i915)
> -{
> -	return (struct intel_device_info *)INTEL_INFO(i915);
> -}
> -
>   static const struct intel_display_device_info no_display = {};
>   
>   /**
> @@ -430,26 +423,24 @@ void intel_device_info_driver_create(struct drm_i915_private *i915,
>   				     u16 device_id,
>   				     const struct intel_device_info *match_info)
>   {
> -	struct intel_device_info *info;
>   	struct intel_runtime_info *runtime;
>   	u16 ver, rel, step;
>   
> -	/* Setup the write-once "constant" device info */
> -	info = mkwrite_device_info(i915);
> -	memcpy(info, match_info, sizeof(*info));
> +	/* Setup INTEL_INFO() */
> +	i915->__info = match_info;
>   
>   	/* Initialize initial runtime info from static const data and pdev. */
>   	runtime = RUNTIME_INFO(i915);
>   	memcpy(runtime, &INTEL_INFO(i915)->__runtime, sizeof(*runtime));
>   
>   	/* Probe display support */
> -	i915->display.info.__device_info = intel_display_device_probe(i915, info->has_gmd_id,
> +	i915->display.info.__device_info = intel_display_device_probe(i915, HAS_GMD_ID(i915),

Why does intel_display_device_probe need an explicit has_gmdid when it 
could use HAS_GMD_ID itself?

But anyway, LGTM. If it saves you some time before the other time zone 
comes online:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

>   								      &ver, &rel, &step);
>   	memcpy(DISPLAY_RUNTIME_INFO(i915),
>   	       &DISPLAY_INFO(i915)->__runtime_defaults,
>   	       sizeof(*DISPLAY_RUNTIME_INFO(i915)));
>   
> -	if (info->has_gmd_id) {
> +	if (HAS_GMD_ID(i915)) {
>   		DISPLAY_RUNTIME_INFO(i915)->ip.ver = ver;
>   		DISPLAY_RUNTIME_INFO(i915)->ip.rel = rel;
>   		DISPLAY_RUNTIME_INFO(i915)->ip.step = step;