Re: [Intel-gfx] [PATCH 6/7] drm/i915/guc: Make GuC log sizes runtime configurable
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
Re: [Intel-gfx] [PATCH 6/7] drm/i915/guc: Make GuC log sizes runtime configurable
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. > > 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. > > I will push a fixup to remove the module parameters, please figure the > rest out in a timely manner. 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. Regards, Joonas
Re: [Intel-gfx] [PATCH 6/7] drm/i915/guc: Make GuC log sizes runtime configurable
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) > >> On 8/24/2022 02:01, Joonas Lahtinen wrote: > >>> NACK on this one. Let's get this reverted or fixed to eliminate > >>> new module parameters. > >>> > >>> What prevents us just from using the maximum sizes? Or alternatively > >>> we could check the already existing drm.debug variable or anything else > >>> but addding 3 new module parameters. > >> We don't want to waste 24MB of memory for all users when 99.999% of them > >> don't care about GuC logs. > > It is not exactly wasting memory if it is what is needed to capture > > the error logs to properly debug a system. And it's definitely not much > > on any modern system where you will have a GPU. You can always leave > > the Kconfig options in place for the cases where it matters. > > > > On the other hand, if 99.999% don't need this, then it could just stay > > as a kernel config time option as well? > No. The point is that we need to able to ask customers to increase the > log size, repro an issue and send us the results. Or we could just ask them to provide the logs for each bug report and save one round trip time. > All on a pre-installed > system where they do not have the option to build a custom kernel. If you argue that way, they don't always have an easy way to change the kernel cmdline options either. Accounting for initrd, update-grub etc. > Either we always allocate the maximum and waste the memory for all end > users or we have a runtime configuration option. Doesn't have to be maximum (as there seems to be limitations in log readback speeds also), just something that is useful for most of the debug scenarios. > Compile time is not > acceptable for some important customers/situations. Yet spending the time discussing how to make the debug logs useful with the issue reporter wouldn't be an issue in such urgency? One can argue what is most convenient way, but there's no way to beat sane default. If somebody has problem with that extra memory usage, then we have the config options to reduce/disable. > >> 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. 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. I will push a fixup to remove the module parameters, please figure the rest out in a timely manner. Regards, Joonas > > Sane for most users is to not allocate 24MB of memory for an internal > debug only buffer they will never use. And which completely swamps any > error capture log with the ASCII encoding of said buffer. > > But as above, we need a way to (very occasionally) get larger GuC logs > from customers without rebuilding the kernel. > > John. > > > > > So for now, let's get the module parameters reverted and go with > > reasonable default buffer sizes when GuC is enabled. The compile time > > options can be left in place. > > > > Thank you in advance. > > > > Regards, Joonas > > > >> John. > >> > >>> For future reference, please do Cc maintainers when adding new uAPI > >>> like module parameters. > >>> > >>> Regards, Joonas > >>> > >>> Quoting john.c.harri...@intel.com (2022-07-28 05:20:27) > From: John Harrison > > The GuC log buffer sizes had to be configured statically at compile > time. This can be quite troublesome when needing to get larger logs > out of a released driver. So re-organise the code to allow a boot time > module parameter override. > > Signed-off-by: John Harrison > --- > drivers/gpu/drm/i915/gt/uc/intel_guc.c| 53 ++ > .../gpu/drm/i915/gt/uc/intel_guc_capture.c| 14 +- > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 176
Re: [Intel-gfx] [PATCH 6/7] drm/i915/guc: Make GuC log sizes runtime configurable
On 8/25/2022 00:15, Joonas Lahtinen wrote:
Quoting John Harrison (2022-08-24 21:45:09)
On 8/24/2022 02:01, Joonas Lahtinen wrote:
NACK on this one. Let's get this reverted or fixed to eliminate
new module parameters.
What prevents us just from using the maximum sizes? Or alternatively
we could check the already existing drm.debug variable or anything else
but addding 3 new module parameters.
We don't want to waste 24MB of memory for all users when 99.999% of them
don't care about GuC logs.
It is not exactly wasting memory if it is what is needed to capture
the error logs to properly debug a system. And it's definitely not much
on any modern system where you will have a GPU. You can always leave
the Kconfig options in place for the cases where it matters.
On the other hand, if 99.999% don't need this, then it could just stay
as a kernel config time option as well?
No. The point is that we need to able to ask customers to increase the
log size, repro an issue and send us the results. All on a pre-installed
system where they do not have the option to build a custom kernel.
Either we always allocate the maximum and waste the memory for all end
users or we have a runtime configuration option. Compile time is not
acceptable for some important customers/situations.
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.
Sane for most users is to not allocate 24MB of memory for an internal
debug only buffer they will never use. And which completely swamps any
error capture log with the ASCII encoding of said buffer.
But as above, we need a way to (very occasionally) get larger GuC logs
from customers without rebuilding the kernel.
John.
So for now, let's get the module parameters reverted and go with
reasonable default buffer sizes when GuC is enabled. The compile time
options can be left in place.
Thank you in advance.
Regards, Joonas
John.
For future reference, please do Cc maintainers when adding new uAPI
like module parameters.
Regards, Joonas
Quoting john.c.harri...@intel.com (2022-07-28 05:20:27)
From: John Harrison
The GuC log buffer sizes had to be configured statically at compile
time. This can be quite troublesome when needing to get larger logs
out of a released driver. So re-organise the code to allow a boot time
module parameter override.
Signed-off-by: John Harrison
---
drivers/gpu/drm/i915/gt/uc/intel_guc.c| 53 ++
.../gpu/drm/i915/gt/uc/intel_guc_capture.c| 14 +-
drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 176 +-
drivers/gpu/drm/i915/gt/uc/intel_guc_log.h| 42 +++--
drivers/gpu/drm/i915/i915_params.c| 12 ++
drivers/gpu/drm/i915/i915_params.h| 3 +
6 files changed, 226 insertions(+), 74 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index ab4aacc516aa4..01f2705cb94a3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -224,53 +224,22 @@ static u32 guc_ctl_feature_flags(struct intel_guc *guc)
static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
{
- u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
- u32 flags;
-
- #if (((CRASH_BUFFER_SIZE) % SZ_1M) == 0)
- #define LOG_UNIT SZ_1M
- #define LOG_FLAG GUC_LOG_LOG_ALLOC_UNITS
- #else
- #define LOG_UNIT SZ_4K
- #define LOG_FLAG 0
- #endif
-
- #if (((CAPTURE_BUFFER_SIZE) % SZ_1M) == 0)
- #define CAPTURE_UNIT SZ_1M
- #define CAPTURE_FLAG GUC_LOG_CAPTURE_ALLOC_UNITS
- #else
- #define CAPTURE_UNIT SZ_4K
- #define CAPTURE_FLAG 0
- #endif
-
- BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
- BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, LOG_UNIT));
- BUILD_BUG_ON(!DEBUG_BUFFER_SIZE);
- BUILD_BUG_ON(!IS_ALIGNED(DEBUG_BUFFER_SIZE, LOG_UNIT));
- BUILD_BUG_ON(!CAPTURE_BUFFER_SIZE);
- BUILD_BUG_ON(!IS_ALIGNED(CAPTURE_BUF
Re: [Intel-gfx] [PATCH 6/7] drm/i915/guc: Make GuC log sizes runtime configurable
Quoting John Harrison (2022-08-24 21:45:09)
> On 8/24/2022 02:01, Joonas Lahtinen wrote:
> > NACK on this one. Let's get this reverted or fixed to eliminate
> > new module parameters.
> >
> > What prevents us just from using the maximum sizes? Or alternatively
> > we could check the already existing drm.debug variable or anything else
> > but addding 3 new module parameters.
> We don't want to waste 24MB of memory for all users when 99.999% of them
> don't care about GuC logs.
It is not exactly wasting memory if it is what is needed to capture
the error logs to properly debug a system. And it's definitely not much
on any modern system where you will have a GPU. You can always leave
the Kconfig options in place for the cases where it matters.
On the other hand, if 99.999% don't need this, then it could just stay
as a kernel config time option as well?
> 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.
So for now, let's get the module parameters reverted and go with
reasonable default buffer sizes when GuC is enabled. The compile time
options can be left in place.
Thank you in advance.
Regards, Joonas
>
> John.
>
> >
> > For future reference, please do Cc maintainers when adding new uAPI
> > like module parameters.
> >
> > Regards, Joonas
> >
> > Quoting john.c.harri...@intel.com (2022-07-28 05:20:27)
> >> From: John Harrison
> >>
> >> The GuC log buffer sizes had to be configured statically at compile
> >> time. This can be quite troublesome when needing to get larger logs
> >> out of a released driver. So re-organise the code to allow a boot time
> >> module parameter override.
> >>
> >> Signed-off-by: John Harrison
> >> ---
> >> drivers/gpu/drm/i915/gt/uc/intel_guc.c| 53 ++
> >> .../gpu/drm/i915/gt/uc/intel_guc_capture.c| 14 +-
> >> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 176 +-
> >> drivers/gpu/drm/i915/gt/uc/intel_guc_log.h| 42 +++--
> >> drivers/gpu/drm/i915/i915_params.c| 12 ++
> >> drivers/gpu/drm/i915/i915_params.h| 3 +
> >> 6 files changed, 226 insertions(+), 74 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> >> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> >> index ab4aacc516aa4..01f2705cb94a3 100644
> >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> >> @@ -224,53 +224,22 @@ static u32 guc_ctl_feature_flags(struct intel_guc
> >> *guc)
> >>
> >> static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
> >> {
> >> - u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >>
> >> PAGE_SHIFT;
> >> - u32 flags;
> >> -
> >> - #if (((CRASH_BUFFER_SIZE) % SZ_1M) == 0)
> >> - #define LOG_UNIT SZ_1M
> >> - #define LOG_FLAG GUC_LOG_LOG_ALLOC_UNITS
> >> - #else
> >> - #define LOG_UNIT SZ_4K
> >> - #define LOG_FLAG 0
> >> - #endif
> >> -
> >> - #if (((CAPTURE_BUFFER_SIZE) % SZ_1M) == 0)
> >> - #define CAPTURE_UNIT SZ_1M
> >> - #define CAPTURE_FLAG GUC_LOG_CAPTURE_ALLOC_UNITS
> >> - #else
> >> - #define CAPTURE_UNIT SZ_4K
> >> - #define CAPTURE_FLAG 0
> >> - #endif
> >> -
> >> - BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
> >> - BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, LOG_UNIT));
> >> - BUILD_BUG_ON(!DEBUG_BUFFER_SIZE);
> >> - BUILD_BUG_ON(!IS_ALIGNED(DEBUG_BUFFER_SIZE, LOG_UNIT));
> >> - BUILD_BUG_ON(!CAPTURE_BUFFER_SIZE);
> >> - BUILD_BUG_ON(!IS_ALIGNED(CAPTURE_BUFFER_SIZE, CAPTURE_UNIT));
> >> -
> >> - BUILD_BUG_ON((CRASH_BUFFER_SIZE / LOG_UNIT - 1) >
> >> - (GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT));
> >> - BUILD_BUG_ON((DEBUG_BUFFER_SIZE / LOG_UNIT - 1) >
> >> - (GUC_LOG_DEBUG_MASK >> GUC_LOG_DEBUG_SHIFT));
> >> - BUILD_BUG_ON((CAPTURE_BUFFER_SIZE / CAPTURE_UNIT - 1) >
> >> - (GUC_LOG_CAPTURE_MASK >> GUC_LOG_CAPTURE_SHIFT)
Re: [Intel-gfx] [PATCH 6/7] drm/i915/guc: Make GuC log sizes runtime configurable
NACK on this one. Let's get this reverted or fixed to eliminate
new module parameters.
What prevents us just from using the maximum sizes? Or alternatively
we could check the already existing drm.debug variable or anything else
but addding 3 new module parameters.
For future reference, please do Cc maintainers when adding new uAPI
like module parameters.
Regards, Joonas
Quoting john.c.harri...@intel.com (2022-07-28 05:20:27)
> From: John Harrison
>
> The GuC log buffer sizes had to be configured statically at compile
> time. This can be quite troublesome when needing to get larger logs
> out of a released driver. So re-organise the code to allow a boot time
> module parameter override.
>
> Signed-off-by: John Harrison
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc.c| 53 ++
> .../gpu/drm/i915/gt/uc/intel_guc_capture.c| 14 +-
> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 176 +-
> drivers/gpu/drm/i915/gt/uc/intel_guc_log.h| 42 +++--
> drivers/gpu/drm/i915/i915_params.c| 12 ++
> drivers/gpu/drm/i915/i915_params.h| 3 +
> 6 files changed, 226 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index ab4aacc516aa4..01f2705cb94a3 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -224,53 +224,22 @@ static u32 guc_ctl_feature_flags(struct intel_guc *guc)
>
> static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
> {
> - u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
> - u32 flags;
> -
> - #if (((CRASH_BUFFER_SIZE) % SZ_1M) == 0)
> - #define LOG_UNIT SZ_1M
> - #define LOG_FLAG GUC_LOG_LOG_ALLOC_UNITS
> - #else
> - #define LOG_UNIT SZ_4K
> - #define LOG_FLAG 0
> - #endif
> -
> - #if (((CAPTURE_BUFFER_SIZE) % SZ_1M) == 0)
> - #define CAPTURE_UNIT SZ_1M
> - #define CAPTURE_FLAG GUC_LOG_CAPTURE_ALLOC_UNITS
> - #else
> - #define CAPTURE_UNIT SZ_4K
> - #define CAPTURE_FLAG 0
> - #endif
> -
> - BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
> - BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, LOG_UNIT));
> - BUILD_BUG_ON(!DEBUG_BUFFER_SIZE);
> - BUILD_BUG_ON(!IS_ALIGNED(DEBUG_BUFFER_SIZE, LOG_UNIT));
> - BUILD_BUG_ON(!CAPTURE_BUFFER_SIZE);
> - BUILD_BUG_ON(!IS_ALIGNED(CAPTURE_BUFFER_SIZE, CAPTURE_UNIT));
> -
> - BUILD_BUG_ON((CRASH_BUFFER_SIZE / LOG_UNIT - 1) >
> - (GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT));
> - BUILD_BUG_ON((DEBUG_BUFFER_SIZE / LOG_UNIT - 1) >
> - (GUC_LOG_DEBUG_MASK >> GUC_LOG_DEBUG_SHIFT));
> - BUILD_BUG_ON((CAPTURE_BUFFER_SIZE / CAPTURE_UNIT - 1) >
> - (GUC_LOG_CAPTURE_MASK >> GUC_LOG_CAPTURE_SHIFT));
> + struct intel_guc_log *log = &guc->log;
> + u32 offset, flags;
> +
> + GEM_BUG_ON(!log->sizes_initialised);
> +
> + offset = intel_guc_ggtt_offset(guc, log->vma) >> PAGE_SHIFT;
>
> flags = GUC_LOG_VALID |
> GUC_LOG_NOTIFY_ON_HALF_FULL |
> - CAPTURE_FLAG |
> - LOG_FLAG |
> - ((CRASH_BUFFER_SIZE / LOG_UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
> - ((DEBUG_BUFFER_SIZE / LOG_UNIT - 1) << GUC_LOG_DEBUG_SHIFT) |
> - ((CAPTURE_BUFFER_SIZE / CAPTURE_UNIT - 1) <<
> GUC_LOG_CAPTURE_SHIFT) |
> + log->sizes[GUC_LOG_SECTIONS_DEBUG].flag |
> + log->sizes[GUC_LOG_SECTIONS_CAPTURE].flag |
> + (log->sizes[GUC_LOG_SECTIONS_CRASH].count <<
> GUC_LOG_CRASH_SHIFT) |
> + (log->sizes[GUC_LOG_SECTIONS_DEBUG].count <<
> GUC_LOG_DEBUG_SHIFT) |
> + (log->sizes[GUC_LOG_SECTIONS_CAPTURE].count <<
> GUC_LOG_CAPTURE_SHIFT) |
> (offset << GUC_LOG_BUF_ADDR_SHIFT);
>
> - #undef LOG_UNIT
> - #undef LOG_FLAG
> - #undef CAPTURE_UNIT
> - #undef CAPTURE_FLAG
> -
> return flags;
> }
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> index b54b7883320b1..d2ac53d4f3b6e 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> @@ -656,16 +656,17 @@ static void check_guc_capture_size(struct intel_guc
> *guc)
> struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> int min_size = guc_capture_output_min_size_est(guc);
> int spare_size = min_size * GUC_CAPTURE_OVERBUFFER_MULTIPLIER;
> + u32 buffer_size = intel_guc_log_section_size_capture(&guc->log);
>
> if (min_size < 0)
> drm_warn(&i915->drm, "Failed to calculate GuC error state
> capture buffer minimum size: %d!\n",
> min_size);
> - else if (min_size > CAPTURE_BUFFER_SIZE)
> + else if (min_size > bu
Re: [Intel-gfx] [PATCH 6/7] drm/i915/guc: Make GuC log sizes runtime configurable
Looks good to me.
Reviewed-by: Alan Previn
On Wed, 2022-07-27 at 19:20 -0700, john.c.harri...@intel.com wrote:
> From: John Harrison
>
> The GuC log buffer sizes had to be configured statically at compile
> time. This can be quite troublesome when needing to get larger logs
> out of a released driver. So re-organise the code to allow a boot time
> module parameter override.
>
> Signed-off-by: John Harrison
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc.c| 53 ++
> .../gpu/drm/i915/gt/uc/intel_guc_capture.c| 14 +-
> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 176 +-
> drivers/gpu/drm/i915/gt/uc/intel_guc_log.h| 42 +++--
> drivers/gpu/drm/i915/i915_params.c| 12 ++
> drivers/gpu/drm/i915/i915_params.h| 3 +
> 6 files changed, 226 insertions(+), 74 deletions(-)
>
...
> +static s32 scale_log_param(struct intel_guc_log *log, const struct
> guc_log_section *section,
> +s32 param)
> +{
> + /* -1 means default */
> + if (param < 0)
> + return section->default_val;
> +
> + /* Check for 32-bit overflow */
> + if (param >= SZ_4K) {
> + drm_err(&guc_to_gt(log_to_guc(log))->i915->drm, "Size too large
> for GuC %s log: %dMB!",
> + section->name, param);
> + return section->default_val;
> + }
> +
> + /* Param units are 1MB */
> + return param * SZ_1M;
> +}
> +
[Intel-gfx] [PATCH 6/7] drm/i915/guc: Make GuC log sizes runtime configurable
From: John Harrison
The GuC log buffer sizes had to be configured statically at compile
time. This can be quite troublesome when needing to get larger logs
out of a released driver. So re-organise the code to allow a boot time
module parameter override.
Signed-off-by: John Harrison
---
drivers/gpu/drm/i915/gt/uc/intel_guc.c| 53 ++
.../gpu/drm/i915/gt/uc/intel_guc_capture.c| 14 +-
drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 176 +-
drivers/gpu/drm/i915/gt/uc/intel_guc_log.h| 42 +++--
drivers/gpu/drm/i915/i915_params.c| 12 ++
drivers/gpu/drm/i915/i915_params.h| 3 +
6 files changed, 226 insertions(+), 74 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index ab4aacc516aa4..01f2705cb94a3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -224,53 +224,22 @@ static u32 guc_ctl_feature_flags(struct intel_guc *guc)
static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
{
- u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
- u32 flags;
-
- #if (((CRASH_BUFFER_SIZE) % SZ_1M) == 0)
- #define LOG_UNIT SZ_1M
- #define LOG_FLAG GUC_LOG_LOG_ALLOC_UNITS
- #else
- #define LOG_UNIT SZ_4K
- #define LOG_FLAG 0
- #endif
-
- #if (((CAPTURE_BUFFER_SIZE) % SZ_1M) == 0)
- #define CAPTURE_UNIT SZ_1M
- #define CAPTURE_FLAG GUC_LOG_CAPTURE_ALLOC_UNITS
- #else
- #define CAPTURE_UNIT SZ_4K
- #define CAPTURE_FLAG 0
- #endif
-
- BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
- BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, LOG_UNIT));
- BUILD_BUG_ON(!DEBUG_BUFFER_SIZE);
- BUILD_BUG_ON(!IS_ALIGNED(DEBUG_BUFFER_SIZE, LOG_UNIT));
- BUILD_BUG_ON(!CAPTURE_BUFFER_SIZE);
- BUILD_BUG_ON(!IS_ALIGNED(CAPTURE_BUFFER_SIZE, CAPTURE_UNIT));
-
- BUILD_BUG_ON((CRASH_BUFFER_SIZE / LOG_UNIT - 1) >
- (GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT));
- BUILD_BUG_ON((DEBUG_BUFFER_SIZE / LOG_UNIT - 1) >
- (GUC_LOG_DEBUG_MASK >> GUC_LOG_DEBUG_SHIFT));
- BUILD_BUG_ON((CAPTURE_BUFFER_SIZE / CAPTURE_UNIT - 1) >
- (GUC_LOG_CAPTURE_MASK >> GUC_LOG_CAPTURE_SHIFT));
+ struct intel_guc_log *log = &guc->log;
+ u32 offset, flags;
+
+ GEM_BUG_ON(!log->sizes_initialised);
+
+ offset = intel_guc_ggtt_offset(guc, log->vma) >> PAGE_SHIFT;
flags = GUC_LOG_VALID |
GUC_LOG_NOTIFY_ON_HALF_FULL |
- CAPTURE_FLAG |
- LOG_FLAG |
- ((CRASH_BUFFER_SIZE / LOG_UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
- ((DEBUG_BUFFER_SIZE / LOG_UNIT - 1) << GUC_LOG_DEBUG_SHIFT) |
- ((CAPTURE_BUFFER_SIZE / CAPTURE_UNIT - 1) <<
GUC_LOG_CAPTURE_SHIFT) |
+ log->sizes[GUC_LOG_SECTIONS_DEBUG].flag |
+ log->sizes[GUC_LOG_SECTIONS_CAPTURE].flag |
+ (log->sizes[GUC_LOG_SECTIONS_CRASH].count <<
GUC_LOG_CRASH_SHIFT) |
+ (log->sizes[GUC_LOG_SECTIONS_DEBUG].count <<
GUC_LOG_DEBUG_SHIFT) |
+ (log->sizes[GUC_LOG_SECTIONS_CAPTURE].count <<
GUC_LOG_CAPTURE_SHIFT) |
(offset << GUC_LOG_BUF_ADDR_SHIFT);
- #undef LOG_UNIT
- #undef LOG_FLAG
- #undef CAPTURE_UNIT
- #undef CAPTURE_FLAG
-
return flags;
}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index b54b7883320b1..d2ac53d4f3b6e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -656,16 +656,17 @@ static void check_guc_capture_size(struct intel_guc *guc)
struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
int min_size = guc_capture_output_min_size_est(guc);
int spare_size = min_size * GUC_CAPTURE_OVERBUFFER_MULTIPLIER;
+ u32 buffer_size = intel_guc_log_section_size_capture(&guc->log);
if (min_size < 0)
drm_warn(&i915->drm, "Failed to calculate GuC error state
capture buffer minimum size: %d!\n",
min_size);
- else if (min_size > CAPTURE_BUFFER_SIZE)
+ else if (min_size > buffer_size)
drm_warn(&i915->drm, "GuC error state capture buffer is too
small: %d < %d\n",
-CAPTURE_BUFFER_SIZE, min_size);
- else if (spare_size > CAPTURE_BUFFER_SIZE)
+buffer_size, min_size);
+ else if (spare_size > buffer_size)
drm_notice(&i915->drm, "GuC error state capture buffer maybe
too small: %d < %d (min = %d)\n",
- CAPTURE_BUFFER_SIZE, spare_size, min_size);
+ buffer_size, spare_size, min_size);
}
/*
@@ -1294,7 +1295,8 @@ static void __guc_capture_process_output(struct intel_guc
*guc)
