Re: [Intel-gfx] [PATCH 6/7] drm/i915/guc: Make GuC log sizes runtime configurable

[John Harrison <john.c.harrison@intel.com>]

On 9/12/2022 00:12, Joonas Lahtinen wrote:
> Quoting Joonas Lahtinen (2022-08-26 09:23:08)
>> Quoting John Harrison (2022-08-25 19:31:39)
>>> On 8/25/2022 00:15, Joonas Lahtinen wrote:
>>>> Quoting John Harrison (2022-08-24 21:45:09)
>>>>> We also don't want to tie the GuC logging buffer size to the DRM
>>>>> debugging output. Enabling kernel debug output can change timings and
>>>>> prevent the issue that one is trying to capture in the GuC log. And it
>>>>> seems unlikely we could add an entire new top level DRM debug flag just
>>>>> for an internal i915 only log buffer size. Plus drm.debug is explicitly
>>>>> stated as being dynamically changeable via sysfs at any time. The GuC
>>>>> log buffer size cannot be changed without reloading the i915 driver. Or
>>>>> at least, not without reloading the GuC, but we definitely don't want to
>>>>> create a UAPI for arbitrarily reloading the GuC.
>>>>>
>>>>> We can make these parameters 'unsafe' so that they taint the kernel if
>>>>> used. But this is exactly what module parameters are for - configuration
>>>>> that we don't want to hardcode as CONFIG options but which must be set
>>>>> at module load time.
>>>> It's better to have sane defaults. And if somebody wants something
>>>> strange, they can have a Kconfig behind EXPERT option. But even then
>>>> there should really be need for it.
>>> Define sane.
>> I was hoping you would be the expert on that as you've suggested the
>> patch to begin with.
And as the 'expert' I suggested an approach that everyone was happy with 
except for yourself.

>>
>> Try to aim to cover >90% of the debug scenarios (that are only 0.001%) and
>> we're already only needing to recompile kernel in 1 per million cases.
>>
>> We can live with that.
This is not about how many instances of debug scenarios need a rebuild. 
It's about whether or not the person doing the repro has the ability to 
rebuild a custom kernel.

>>
>> I will push a fixup to remove the module parameters, please figure the
>> rest out in a timely manner.
Your fixup was evidently not tested because it broke non-debug builds.

> John, what is the status of the followup patch here to configure those
> reasonable defaults?
>
> We shouldn't be ignoring this and proceed to pile other GuC patches
> on top.
Being out of office is not ignoring.

And I don't see what other options we have. Setting a large default 
means that the vast majority of people who don't care about GuC will 
have their error capture logs filled with apparent gibberish in the form 
of a huge ASCII dump of the GuC log binary data. Which is something that 
people will surely complain about. Whereas setting a 'sensibly small' 
default means that we won't be able to use the GuC logs in many of the 
cases where we actually need to.

So right now, it seems the only choice we have is to fix up the broken 
driver caused by your incomplete removal and then re-add the module 
parameter to our internal tree so that our internal customers can at 
least use it.

John.


>
> Regards, Joonas