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
