Re: [PATCH 1/6] drm/i915: start moving runtime device info to a separate struct

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

On Wed, 02 Jan 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 31/12/2018 14:56, Jani Nikula wrote:
>> @@ -2198,10 +2199,11 @@ intel_info(const struct drm_i915_private *dev_priv)
>>   }
>>   
>>   #define INTEL_INFO(dev_priv)	intel_info((dev_priv))
>> +#define RUNTIME_INFO(dev_priv)	(&(dev_priv)->__runtime)
>
> Do we want to keep the const trick like with INTEL_INFO in order to make 
> accidental modifications harder? Argument is different there than with 
> static info.

I did think about it, but I don't want to repeat mkwrite_device_info().

I understand we have three classes of data:

1) immutable
2) immutable after one-time runtime init
3) mutable

Currently we mix all of them, the intention here is to split out 1 from
2&3, the latter two remaining conflated. I'd like to see how this pans
out before worrying about the difference between 2&3.

>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index 2bd7991ec9af..6238a06b6d4e 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -594,13 +594,14 @@ static void print_error_obj(struct drm_i915_error_state_buf *m,
>>   
>>   static void err_print_capabilities(struct drm_i915_error_state_buf *m,
>>   				   const struct intel_device_info *info,
>> +				   const struct intel_runtime_info *runtime,
>>   				   const struct intel_driver_caps *caps)
>>   {
>>   	struct drm_printer p = i915_error_printer(m);
>>   
>>   	intel_device_info_dump_flags(info, &p);
>
> If I am not missing something here we now miss the runtime flags being 
> dumped.
>
> A bit later: Ah ok, you haven't yet added any flags to runtime info in 
> this patch.

*grin* Like I said, not as complete as your series.

> Looks okay to me.
>
> The only thing which worries me is that one day we end up with too 
> little in static vs runtime and decide having two separate sources of 
> info is only a hassle. (Like if the DCE/LTO path either does not happen, 
> or ends up not needing this completely.)
>
> I suppose it is worth exploring and we can always go back easily if all 
> else fails. I at least want to have another go at the subplatform/devid 
> centralization.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx