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

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

On Thu, 29 Jun 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 29 Jun 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> 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?
>
> I think all of the display related initialization from this function
> should be moved within intel_display_device_probe() in follow-up.
>
>> But anyway, LGTM. If it saves you some time before the other time zone 
>> comes online:
>>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

And pushed the series to drm-intel-next, thanks for the reviews
everyone!

BR,
Jani.

>
> Thanks,
> Jani.
>
>>
>> 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;

-- 
Jani Nikula, Intel Open Source Graphics Center