| Message ID | 20220728022028.2190627-7-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes and improvements to GuC logging and error capture | expand |
Looks good to me. Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com> On Wed, 2022-07-27 at 19:20 -0700, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > 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 <John.C.Harrison@Intel.com> > --- > 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; > +} > +
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.Harrison@Intel.com (2022-07-28 05:20:27) > From: John Harrison <John.C.Harrison@Intel.com> > > 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 <John.C.Harrison@Intel.com> > --- > 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) > > log_buf_state = guc->log.buf_addr + > (sizeof(struct guc_log_buffer_state) * GUC_CAPTURE_LOG_BUFFER); > - src_data = guc->log.buf_addr + intel_guc_get_log_buffer_offset(GUC_CAPTURE_LOG_BUFFER); > + src_data = guc->log.buf_addr + > + intel_guc_get_log_buffer_offset(&guc->log, GUC_CAPTURE_LOG_BUFFER); > > /* > * Make a copy of the state structure, inside GuC log buffer > @@ -1302,7 +1304,7 @@ static void __guc_capture_process_output(struct intel_guc *guc) > * from it multiple times. > */ > memcpy(&log_buf_state_local, log_buf_state, sizeof(struct guc_log_buffer_state)); > - buffer_size = intel_guc_get_log_buffer_size(GUC_CAPTURE_LOG_BUFFER); > + buffer_size = intel_guc_get_log_buffer_size(&guc->log, GUC_CAPTURE_LOG_BUFFER); > read_offset = log_buf_state_local.read_ptr; > write_offset = log_buf_state_local.sampled_write_ptr; > full_count = log_buf_state_local.buffer_full_cnt; > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > index 4722d4b18ed19..890b6853bd609 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > @@ -13,8 +13,158 @@ > #include "intel_guc_capture.h" > #include "intel_guc_log.h" > > +#if defined(CONFIG_DRM_I915_DEBUG_GUC) > +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_2M > +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_16M > +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M > +#elif defined(CONFIG_DRM_I915_DEBUG_GEM) > +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_1M > +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_2M > +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M > +#else > +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_8K > +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_64K > +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_2M > +#endif > + > static void guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log); > > +struct guc_log_section { > + u32 max; > + u32 flag; > + u32 default_val; > + const char *name; > +}; > + > +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; > +} > + > +static void _guc_log_init_sizes(struct intel_guc_log *log) > +{ > + struct intel_guc *guc = log_to_guc(log); > + struct drm_i915_private *i915 = guc_to_gt(guc)->i915; > + static const struct guc_log_section sections[GUC_LOG_SECTIONS_LIMIT] = { > + { > + GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT, > + GUC_LOG_LOG_ALLOC_UNITS, > + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE, > + "crash dump" > + }, > + { > + GUC_LOG_DEBUG_MASK >> GUC_LOG_DEBUG_SHIFT, > + GUC_LOG_LOG_ALLOC_UNITS, > + GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE, > + "debug", > + }, > + { > + GUC_LOG_CAPTURE_MASK >> GUC_LOG_CAPTURE_SHIFT, > + GUC_LOG_CAPTURE_ALLOC_UNITS, > + GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE, > + "capture", > + } > + }; > + s32 params[GUC_LOG_SECTIONS_LIMIT] = { > + i915->params.guc_log_size_crash, > + i915->params.guc_log_size_debug, > + i915->params.guc_log_size_capture, > + }; > + int i; > + > + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) > + log->sizes[i].bytes = scale_log_param(log, sections + i, params[i]); > + > + /* If debug size > 1MB then bump default crash size to keep the same units */ > + if (log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes >= SZ_1M && > + (i915->params.guc_log_size_crash == -1) && > + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE < SZ_1M) > + log->sizes[GUC_LOG_SECTIONS_CRASH].bytes = SZ_1M; > + > + /* Prepare the GuC API structure fields: */ > + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) { > + /* Convert to correct units */ > + if ((log->sizes[i].bytes % SZ_1M) == 0) { > + log->sizes[i].units = SZ_1M; > + log->sizes[i].flag = sections[i].flag; > + } else { > + log->sizes[i].units = SZ_4K; > + log->sizes[i].flag = 0; > + } > + > + if (!IS_ALIGNED(log->sizes[i].bytes, log->sizes[i].units)) > + drm_err(&i915->drm, "Mis-aligned GuC log %s size: 0x%X vs 0x%X!", > + sections[i].name, log->sizes[i].bytes, log->sizes[i].units); > + log->sizes[i].count = log->sizes[i].bytes / log->sizes[i].units; > + > + if (!log->sizes[i].count) { > + drm_err(&i915->drm, "Zero GuC log %s size!", sections[i].name); > + } else { > + /* Size is +1 unit */ > + log->sizes[i].count--; > + } > + > + /* Clip to field size */ > + if (log->sizes[i].count > sections[i].max) { > + drm_err(&i915->drm, "GuC log %s size too large: %d vs %d!", > + sections[i].name, log->sizes[i].count + 1, sections[i].max + 1); > + log->sizes[i].count = sections[i].max; > + } > + } > + > + if (log->sizes[GUC_LOG_SECTIONS_CRASH].units != log->sizes[GUC_LOG_SECTIONS_DEBUG].units) { > + drm_err(&i915->drm, "Unit mis-match for GuC log crash and debug sections: %d vs %d!", > + log->sizes[GUC_LOG_SECTIONS_CRASH].units, > + log->sizes[GUC_LOG_SECTIONS_DEBUG].units); > + log->sizes[GUC_LOG_SECTIONS_CRASH].units = log->sizes[GUC_LOG_SECTIONS_DEBUG].units; > + log->sizes[GUC_LOG_SECTIONS_CRASH].count = 0; > + } > + > + log->sizes_initialised = true; > +} > + > +static void guc_log_init_sizes(struct intel_guc_log *log) > +{ > + if (log->sizes_initialised) > + return; > + > + _guc_log_init_sizes(log); > +} > + > +static u32 intel_guc_log_section_size_crash(struct intel_guc_log *log) > +{ > + guc_log_init_sizes(log); > + > + return log->sizes[GUC_LOG_SECTIONS_CRASH].bytes; > +} > + > +static u32 intel_guc_log_section_size_debug(struct intel_guc_log *log) > +{ > + guc_log_init_sizes(log); > + > + return log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes; > +} > + > +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log) > +{ > + guc_log_init_sizes(log); > + > + return log->sizes[GUC_LOG_SECTIONS_CAPTURE].bytes; > +} > + > static u32 intel_guc_log_size(struct intel_guc_log *log) > { > /* > @@ -38,7 +188,10 @@ static u32 intel_guc_log_size(struct intel_guc_log *log) > * | Capture logs | > * +===============================+ + CAPTURE_SIZE > */ > - return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE + CAPTURE_BUFFER_SIZE; > + return PAGE_SIZE + > + intel_guc_log_section_size_crash(log) + > + intel_guc_log_section_size_debug(log) + > + intel_guc_log_section_size_capture(log); > } > > /** > @@ -165,7 +318,8 @@ static void guc_move_to_next_buf(struct intel_guc_log *log) > smp_wmb(); > > /* All data has been written, so now move the offset of sub buffer. */ > - relay_reserve(log->relay.channel, log->vma->obj->base.size - CAPTURE_BUFFER_SIZE); > + relay_reserve(log->relay.channel, log->vma->obj->base.size - > + intel_guc_log_section_size_capture(log)); > > /* Switch to the next sub buffer */ > relay_flush(log->relay.channel); > @@ -210,15 +364,16 @@ bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, > return overflow; > } > > -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) > +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, > + enum guc_log_buffer_type type) > { > switch (type) { > case GUC_DEBUG_LOG_BUFFER: > - return DEBUG_BUFFER_SIZE; > + return intel_guc_log_section_size_debug(log); > case GUC_CRASH_DUMP_LOG_BUFFER: > - return CRASH_BUFFER_SIZE; > + return intel_guc_log_section_size_crash(log); > case GUC_CAPTURE_LOG_BUFFER: > - return CAPTURE_BUFFER_SIZE; > + return intel_guc_log_section_size_capture(log); > default: > MISSING_CASE(type); > } > @@ -226,7 +381,8 @@ unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) > return 0; > } > > -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) > +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, > + enum guc_log_buffer_type type) > { > enum guc_log_buffer_type i; > size_t offset = PAGE_SIZE;/* for the log_buffer_states */ > @@ -234,7 +390,7 @@ size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) > for (i = GUC_DEBUG_LOG_BUFFER; i < GUC_MAX_LOG_BUFFER; ++i) { > if (i == type) > break; > - offset += intel_guc_get_log_buffer_size(i); > + offset += intel_guc_get_log_buffer_size(log, i); > } > > return offset; > @@ -285,7 +441,7 @@ static void _guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log) > */ > memcpy(&log_buf_state_local, log_buf_state, > sizeof(struct guc_log_buffer_state)); > - buffer_size = intel_guc_get_log_buffer_size(type); > + buffer_size = intel_guc_get_log_buffer_size(log, type); > read_offset = log_buf_state_local.read_ptr; > write_offset = log_buf_state_local.sampled_write_ptr; > full_cnt = log_buf_state_local.buffer_full_cnt; > @@ -400,7 +556,7 @@ static int guc_log_relay_create(struct intel_guc_log *log) > * Keep the size of sub buffers same as shared log buffer > * but GuC log-events excludes the error-state-capture logs > */ > - subbuf_size = log->vma->size - CAPTURE_BUFFER_SIZE; > + subbuf_size = log->vma->size - intel_guc_log_section_size_capture(log); > > /* > * Store up to 8 snapshots, which is large enough to buffer sufficient > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h > index dc9715411d626..02127703be809 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h > @@ -15,20 +15,6 @@ > > struct intel_guc; > > -#if defined(CONFIG_DRM_I915_DEBUG_GUC) > -#define CRASH_BUFFER_SIZE SZ_2M > -#define DEBUG_BUFFER_SIZE SZ_16M > -#define CAPTURE_BUFFER_SIZE SZ_4M > -#elif defined(CONFIG_DRM_I915_DEBUG_GEM) > -#define CRASH_BUFFER_SIZE SZ_1M > -#define DEBUG_BUFFER_SIZE SZ_2M > -#define CAPTURE_BUFFER_SIZE SZ_4M > -#else > -#define CRASH_BUFFER_SIZE SZ_8K > -#define DEBUG_BUFFER_SIZE SZ_64K > -#define CAPTURE_BUFFER_SIZE SZ_2M > -#endif > - > /* > * While we're using plain log level in i915, GuC controls are much more... > * "elaborate"? We have a couple of bits for verbosity, separate bit for actual > @@ -46,10 +32,30 @@ struct intel_guc; > #define GUC_VERBOSITY_TO_LOG_LEVEL(x) ((x) + 2) > #define GUC_LOG_LEVEL_MAX GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX) > > +enum { > + GUC_LOG_SECTIONS_CRASH, > + GUC_LOG_SECTIONS_DEBUG, > + GUC_LOG_SECTIONS_CAPTURE, > + GUC_LOG_SECTIONS_LIMIT > +}; > + > struct intel_guc_log { > u32 level; > + > + /* Allocation settings */ > + struct { > + s32 bytes; /* Size in bytes */ > + s32 units; /* GuC API units - 1MB or 4KB */ > + s32 count; /* Number of API units */ > + u32 flag; /* GuC API units flag */ > + } sizes[GUC_LOG_SECTIONS_LIMIT]; > + bool sizes_initialised; > + > + /* Combined buffer allocation */ > struct i915_vma *vma; > void *buf_addr; > + > + /* RelayFS support */ > struct { > bool buf_in_use; > bool started; > @@ -58,6 +64,7 @@ struct intel_guc_log { > struct mutex lock; > u32 full_count; > } relay; > + > /* logging related stats */ > struct { > u32 sampled_overflow; > @@ -69,8 +76,9 @@ struct intel_guc_log { > void intel_guc_log_init_early(struct intel_guc_log *log); > bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, enum guc_log_buffer_type type, > unsigned int full_cnt); > -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type); > -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type); > +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, > + enum guc_log_buffer_type type); > +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, enum guc_log_buffer_type type); > int intel_guc_log_create(struct intel_guc_log *log); > void intel_guc_log_destroy(struct intel_guc_log *log); > > @@ -92,4 +100,6 @@ void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p); > int intel_guc_log_dump(struct intel_guc_log *log, struct drm_printer *p, > bool dump_load_err); > > +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log); > + > #endif > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 6fc475a5db615..06ca5b8221118 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -171,6 +171,18 @@ i915_param_named(guc_log_level, int, 0400, > "GuC firmware logging level. Requires GuC to be loaded. " > "(-1=auto [default], 0=disable, 1..4=enable with verbosity min..max)"); > > +i915_param_named(guc_log_size_crash, int, 0400, > + "GuC firmware logging buffer size for crash dumps (in MB)" > + "(-1=auto [default], NB: max = 4, other restrictions apply)"); > + > +i915_param_named(guc_log_size_debug, int, 0400, > + "GuC firmware logging buffer size for debug logs (in MB)" > + "(-1=auto [default], NB: max = 16, other restrictions apply)"); > + > +i915_param_named(guc_log_size_capture, int, 0400, > + "GuC error capture register dump buffer size (in MB)" > + "(-1=auto [default], NB: max = 4, other restrictions apply)"); > + > i915_param_named_unsafe(guc_firmware_path, charp, 0400, > "GuC firmware path to use instead of the default one"); > > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > index 2733cb6cfe094..f684d1ab87078 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -61,6 +61,9 @@ struct drm_printer; > param(int, invert_brightness, 0, 0600) \ > param(int, enable_guc, -1, 0400) \ > param(int, guc_log_level, -1, 0400) \ > + param(int, guc_log_size_crash, -1, 0400) \ > + param(int, guc_log_size_debug, -1, 0400) \ > + param(int, guc_log_size_capture, -1, 0400) \ > param(char *, guc_firmware_path, NULL, 0400) \ > param(char *, huc_firmware_path, NULL, 0400) \ > param(char *, dmc_firmware_path, NULL, 0400) \ > -- > 2.37.1 >
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.Harrison@Intel.com (2022-07-28 05:20:27) > >> From: John Harrison <John.C.Harrison@Intel.com> > >> > >> 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 <John.C.Harrison@Intel.com> > >> --- > >> 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) > >> > >> log_buf_state = guc->log.buf_addr + > >> (sizeof(struct guc_log_buffer_state) * GUC_CAPTURE_LOG_BUFFER); > >> - src_data = guc->log.buf_addr + intel_guc_get_log_buffer_offset(GUC_CAPTURE_LOG_BUFFER); > >> + src_data = guc->log.buf_addr + > >> + intel_guc_get_log_buffer_offset(&guc->log, GUC_CAPTURE_LOG_BUFFER); > >> > >> /* > >> * Make a copy of the state structure, inside GuC log buffer > >> @@ -1302,7 +1304,7 @@ static void __guc_capture_process_output(struct intel_guc *guc) > >> * from it multiple times. > >> */ > >> memcpy(&log_buf_state_local, log_buf_state, sizeof(struct guc_log_buffer_state)); > >> - buffer_size = intel_guc_get_log_buffer_size(GUC_CAPTURE_LOG_BUFFER); > >> + buffer_size = intel_guc_get_log_buffer_size(&guc->log, GUC_CAPTURE_LOG_BUFFER); > >> read_offset = log_buf_state_local.read_ptr; > >> write_offset = log_buf_state_local.sampled_write_ptr; > >> full_count = log_buf_state_local.buffer_full_cnt; > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > >> index 4722d4b18ed19..890b6853bd609 100644 > >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > >> @@ -13,8 +13,158 @@ > >> #include "intel_guc_capture.h" > >> #include "intel_guc_log.h" > >> > >> +#if defined(CONFIG_DRM_I915_DEBUG_GUC) > >> +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_2M > >> +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_16M > >> +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M > >> +#elif defined(CONFIG_DRM_I915_DEBUG_GEM) > >> +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_1M > >> +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_2M > >> +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M > >> +#else > >> +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_8K > >> +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_64K > >> +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_2M > >> +#endif > >> + > >> static void guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log); > >> > >> +struct guc_log_section { > >> + u32 max; > >> + u32 flag; > >> + u32 default_val; > >> + const char *name; > >> +}; > >> + > >> +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; > >> +} > >> + > >> +static void _guc_log_init_sizes(struct intel_guc_log *log) > >> +{ > >> + struct intel_guc *guc = log_to_guc(log); > >> + struct drm_i915_private *i915 = guc_to_gt(guc)->i915; > >> + static const struct guc_log_section sections[GUC_LOG_SECTIONS_LIMIT] = { > >> + { > >> + GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT, > >> + GUC_LOG_LOG_ALLOC_UNITS, > >> + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE, > >> + "crash dump" > >> + }, > >> + { > >> + GUC_LOG_DEBUG_MASK >> GUC_LOG_DEBUG_SHIFT, > >> + GUC_LOG_LOG_ALLOC_UNITS, > >> + GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE, > >> + "debug", > >> + }, > >> + { > >> + GUC_LOG_CAPTURE_MASK >> GUC_LOG_CAPTURE_SHIFT, > >> + GUC_LOG_CAPTURE_ALLOC_UNITS, > >> + GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE, > >> + "capture", > >> + } > >> + }; > >> + s32 params[GUC_LOG_SECTIONS_LIMIT] = { > >> + i915->params.guc_log_size_crash, > >> + i915->params.guc_log_size_debug, > >> + i915->params.guc_log_size_capture, > >> + }; > >> + int i; > >> + > >> + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) > >> + log->sizes[i].bytes = scale_log_param(log, sections + i, params[i]); > >> + > >> + /* If debug size > 1MB then bump default crash size to keep the same units */ > >> + if (log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes >= SZ_1M && > >> + (i915->params.guc_log_size_crash == -1) && > >> + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE < SZ_1M) > >> + log->sizes[GUC_LOG_SECTIONS_CRASH].bytes = SZ_1M; > >> + > >> + /* Prepare the GuC API structure fields: */ > >> + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) { > >> + /* Convert to correct units */ > >> + if ((log->sizes[i].bytes % SZ_1M) == 0) { > >> + log->sizes[i].units = SZ_1M; > >> + log->sizes[i].flag = sections[i].flag; > >> + } else { > >> + log->sizes[i].units = SZ_4K; > >> + log->sizes[i].flag = 0; > >> + } > >> + > >> + if (!IS_ALIGNED(log->sizes[i].bytes, log->sizes[i].units)) > >> + drm_err(&i915->drm, "Mis-aligned GuC log %s size: 0x%X vs 0x%X!", > >> + sections[i].name, log->sizes[i].bytes, log->sizes[i].units); > >> + log->sizes[i].count = log->sizes[i].bytes / log->sizes[i].units; > >> + > >> + if (!log->sizes[i].count) { > >> + drm_err(&i915->drm, "Zero GuC log %s size!", sections[i].name); > >> + } else { > >> + /* Size is +1 unit */ > >> + log->sizes[i].count--; > >> + } > >> + > >> + /* Clip to field size */ > >> + if (log->sizes[i].count > sections[i].max) { > >> + drm_err(&i915->drm, "GuC log %s size too large: %d vs %d!", > >> + sections[i].name, log->sizes[i].count + 1, sections[i].max + 1); > >> + log->sizes[i].count = sections[i].max; > >> + } > >> + } > >> + > >> + if (log->sizes[GUC_LOG_SECTIONS_CRASH].units != log->sizes[GUC_LOG_SECTIONS_DEBUG].units) { > >> + drm_err(&i915->drm, "Unit mis-match for GuC log crash and debug sections: %d vs %d!", > >> + log->sizes[GUC_LOG_SECTIONS_CRASH].units, > >> + log->sizes[GUC_LOG_SECTIONS_DEBUG].units); > >> + log->sizes[GUC_LOG_SECTIONS_CRASH].units = log->sizes[GUC_LOG_SECTIONS_DEBUG].units; > >> + log->sizes[GUC_LOG_SECTIONS_CRASH].count = 0; > >> + } > >> + > >> + log->sizes_initialised = true; > >> +} > >> + > >> +static void guc_log_init_sizes(struct intel_guc_log *log) > >> +{ > >> + if (log->sizes_initialised) > >> + return; > >> + > >> + _guc_log_init_sizes(log); > >> +} > >> + > >> +static u32 intel_guc_log_section_size_crash(struct intel_guc_log *log) > >> +{ > >> + guc_log_init_sizes(log); > >> + > >> + return log->sizes[GUC_LOG_SECTIONS_CRASH].bytes; > >> +} > >> + > >> +static u32 intel_guc_log_section_size_debug(struct intel_guc_log *log) > >> +{ > >> + guc_log_init_sizes(log); > >> + > >> + return log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes; > >> +} > >> + > >> +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log) > >> +{ > >> + guc_log_init_sizes(log); > >> + > >> + return log->sizes[GUC_LOG_SECTIONS_CAPTURE].bytes; > >> +} > >> + > >> static u32 intel_guc_log_size(struct intel_guc_log *log) > >> { > >> /* > >> @@ -38,7 +188,10 @@ static u32 intel_guc_log_size(struct intel_guc_log *log) > >> * | Capture logs | > >> * +===============================+ + CAPTURE_SIZE > >> */ > >> - return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE + CAPTURE_BUFFER_SIZE; > >> + return PAGE_SIZE + > >> + intel_guc_log_section_size_crash(log) + > >> + intel_guc_log_section_size_debug(log) + > >> + intel_guc_log_section_size_capture(log); > >> } > >> > >> /** > >> @@ -165,7 +318,8 @@ static void guc_move_to_next_buf(struct intel_guc_log *log) > >> smp_wmb(); > >> > >> /* All data has been written, so now move the offset of sub buffer. */ > >> - relay_reserve(log->relay.channel, log->vma->obj->base.size - CAPTURE_BUFFER_SIZE); > >> + relay_reserve(log->relay.channel, log->vma->obj->base.size - > >> + intel_guc_log_section_size_capture(log)); > >> > >> /* Switch to the next sub buffer */ > >> relay_flush(log->relay.channel); > >> @@ -210,15 +364,16 @@ bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, > >> return overflow; > >> } > >> > >> -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) > >> +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, > >> + enum guc_log_buffer_type type) > >> { > >> switch (type) { > >> case GUC_DEBUG_LOG_BUFFER: > >> - return DEBUG_BUFFER_SIZE; > >> + return intel_guc_log_section_size_debug(log); > >> case GUC_CRASH_DUMP_LOG_BUFFER: > >> - return CRASH_BUFFER_SIZE; > >> + return intel_guc_log_section_size_crash(log); > >> case GUC_CAPTURE_LOG_BUFFER: > >> - return CAPTURE_BUFFER_SIZE; > >> + return intel_guc_log_section_size_capture(log); > >> default: > >> MISSING_CASE(type); > >> } > >> @@ -226,7 +381,8 @@ unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) > >> return 0; > >> } > >> > >> -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) > >> +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, > >> + enum guc_log_buffer_type type) > >> { > >> enum guc_log_buffer_type i; > >> size_t offset = PAGE_SIZE;/* for the log_buffer_states */ > >> @@ -234,7 +390,7 @@ size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) > >> for (i = GUC_DEBUG_LOG_BUFFER; i < GUC_MAX_LOG_BUFFER; ++i) { > >> if (i == type) > >> break; > >> - offset += intel_guc_get_log_buffer_size(i); > >> + offset += intel_guc_get_log_buffer_size(log, i); > >> } > >> > >> return offset; > >> @@ -285,7 +441,7 @@ static void _guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log) > >> */ > >> memcpy(&log_buf_state_local, log_buf_state, > >> sizeof(struct guc_log_buffer_state)); > >> - buffer_size = intel_guc_get_log_buffer_size(type); > >> + buffer_size = intel_guc_get_log_buffer_size(log, type); > >> read_offset = log_buf_state_local.read_ptr; > >> write_offset = log_buf_state_local.sampled_write_ptr; > >> full_cnt = log_buf_state_local.buffer_full_cnt; > >> @@ -400,7 +556,7 @@ static int guc_log_relay_create(struct intel_guc_log *log) > >> * Keep the size of sub buffers same as shared log buffer > >> * but GuC log-events excludes the error-state-capture logs > >> */ > >> - subbuf_size = log->vma->size - CAPTURE_BUFFER_SIZE; > >> + subbuf_size = log->vma->size - intel_guc_log_section_size_capture(log); > >> > >> /* > >> * Store up to 8 snapshots, which is large enough to buffer sufficient > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h > >> index dc9715411d626..02127703be809 100644 > >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h > >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h > >> @@ -15,20 +15,6 @@ > >> > >> struct intel_guc; > >> > >> -#if defined(CONFIG_DRM_I915_DEBUG_GUC) > >> -#define CRASH_BUFFER_SIZE SZ_2M > >> -#define DEBUG_BUFFER_SIZE SZ_16M > >> -#define CAPTURE_BUFFER_SIZE SZ_4M > >> -#elif defined(CONFIG_DRM_I915_DEBUG_GEM) > >> -#define CRASH_BUFFER_SIZE SZ_1M > >> -#define DEBUG_BUFFER_SIZE SZ_2M > >> -#define CAPTURE_BUFFER_SIZE SZ_4M > >> -#else > >> -#define CRASH_BUFFER_SIZE SZ_8K > >> -#define DEBUG_BUFFER_SIZE SZ_64K > >> -#define CAPTURE_BUFFER_SIZE SZ_2M > >> -#endif > >> - > >> /* > >> * While we're using plain log level in i915, GuC controls are much more... > >> * "elaborate"? We have a couple of bits for verbosity, separate bit for actual > >> @@ -46,10 +32,30 @@ struct intel_guc; > >> #define GUC_VERBOSITY_TO_LOG_LEVEL(x) ((x) + 2) > >> #define GUC_LOG_LEVEL_MAX GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX) > >> > >> +enum { > >> + GUC_LOG_SECTIONS_CRASH, > >> + GUC_LOG_SECTIONS_DEBUG, > >> + GUC_LOG_SECTIONS_CAPTURE, > >> + GUC_LOG_SECTIONS_LIMIT > >> +}; > >> + > >> struct intel_guc_log { > >> u32 level; > >> + > >> + /* Allocation settings */ > >> + struct { > >> + s32 bytes; /* Size in bytes */ > >> + s32 units; /* GuC API units - 1MB or 4KB */ > >> + s32 count; /* Number of API units */ > >> + u32 flag; /* GuC API units flag */ > >> + } sizes[GUC_LOG_SECTIONS_LIMIT]; > >> + bool sizes_initialised; > >> + > >> + /* Combined buffer allocation */ > >> struct i915_vma *vma; > >> void *buf_addr; > >> + > >> + /* RelayFS support */ > >> struct { > >> bool buf_in_use; > >> bool started; > >> @@ -58,6 +64,7 @@ struct intel_guc_log { > >> struct mutex lock; > >> u32 full_count; > >> } relay; > >> + > >> /* logging related stats */ > >> struct { > >> u32 sampled_overflow; > >> @@ -69,8 +76,9 @@ struct intel_guc_log { > >> void intel_guc_log_init_early(struct intel_guc_log *log); > >> bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, enum guc_log_buffer_type type, > >> unsigned int full_cnt); > >> -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type); > >> -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type); > >> +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, > >> + enum guc_log_buffer_type type); > >> +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, enum guc_log_buffer_type type); > >> int intel_guc_log_create(struct intel_guc_log *log); > >> void intel_guc_log_destroy(struct intel_guc_log *log); > >> > >> @@ -92,4 +100,6 @@ void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p); > >> int intel_guc_log_dump(struct intel_guc_log *log, struct drm_printer *p, > >> bool dump_load_err); > >> > >> +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log); > >> + > >> #endif > >> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > >> index 6fc475a5db615..06ca5b8221118 100644 > >> --- a/drivers/gpu/drm/i915/i915_params.c > >> +++ b/drivers/gpu/drm/i915/i915_params.c > >> @@ -171,6 +171,18 @@ i915_param_named(guc_log_level, int, 0400, > >> "GuC firmware logging level. Requires GuC to be loaded. " > >> "(-1=auto [default], 0=disable, 1..4=enable with verbosity min..max)"); > >> > >> +i915_param_named(guc_log_size_crash, int, 0400, > >> + "GuC firmware logging buffer size for crash dumps (in MB)" > >> + "(-1=auto [default], NB: max = 4, other restrictions apply)"); > >> + > >> +i915_param_named(guc_log_size_debug, int, 0400, > >> + "GuC firmware logging buffer size for debug logs (in MB)" > >> + "(-1=auto [default], NB: max = 16, other restrictions apply)"); > >> + > >> +i915_param_named(guc_log_size_capture, int, 0400, > >> + "GuC error capture register dump buffer size (in MB)" > >> + "(-1=auto [default], NB: max = 4, other restrictions apply)"); > >> + > >> i915_param_named_unsafe(guc_firmware_path, charp, 0400, > >> "GuC firmware path to use instead of the default one"); > >> > >> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > >> index 2733cb6cfe094..f684d1ab87078 100644 > >> --- a/drivers/gpu/drm/i915/i915_params.h > >> +++ b/drivers/gpu/drm/i915/i915_params.h > >> @@ -61,6 +61,9 @@ struct drm_printer; > >> param(int, invert_brightness, 0, 0600) \ > >> param(int, enable_guc, -1, 0400) \ > >> param(int, guc_log_level, -1, 0400) \ > >> + param(int, guc_log_size_crash, -1, 0400) \ > >> + param(int, guc_log_size_debug, -1, 0400) \ > >> + param(int, guc_log_size_capture, -1, 0400) \ > >> param(char *, guc_firmware_path, NULL, 0400) \ > >> param(char *, huc_firmware_path, NULL, 0400) \ > >> param(char *, dmc_firmware_path, NULL, 0400) \ > >> -- > >> 2.37.1 > >> >
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.Harrison@Intel.com (2022-07-28 05:20:27) >>>> From: John Harrison <John.C.Harrison@Intel.com> >>>> >>>> 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 <John.C.Harrison@Intel.com> >>>> --- >>>> 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) >>>> >>>> log_buf_state = guc->log.buf_addr + >>>> (sizeof(struct guc_log_buffer_state) * GUC_CAPTURE_LOG_BUFFER); >>>> - src_data = guc->log.buf_addr + intel_guc_get_log_buffer_offset(GUC_CAPTURE_LOG_BUFFER); >>>> + src_data = guc->log.buf_addr + >>>> + intel_guc_get_log_buffer_offset(&guc->log, GUC_CAPTURE_LOG_BUFFER); >>>> >>>> /* >>>> * Make a copy of the state structure, inside GuC log buffer >>>> @@ -1302,7 +1304,7 @@ static void __guc_capture_process_output(struct intel_guc *guc) >>>> * from it multiple times. >>>> */ >>>> memcpy(&log_buf_state_local, log_buf_state, sizeof(struct guc_log_buffer_state)); >>>> - buffer_size = intel_guc_get_log_buffer_size(GUC_CAPTURE_LOG_BUFFER); >>>> + buffer_size = intel_guc_get_log_buffer_size(&guc->log, GUC_CAPTURE_LOG_BUFFER); >>>> read_offset = log_buf_state_local.read_ptr; >>>> write_offset = log_buf_state_local.sampled_write_ptr; >>>> full_count = log_buf_state_local.buffer_full_cnt; >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >>>> index 4722d4b18ed19..890b6853bd609 100644 >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >>>> @@ -13,8 +13,158 @@ >>>> #include "intel_guc_capture.h" >>>> #include "intel_guc_log.h" >>>> >>>> +#if defined(CONFIG_DRM_I915_DEBUG_GUC) >>>> +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_2M >>>> +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_16M >>>> +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M >>>> +#elif defined(CONFIG_DRM_I915_DEBUG_GEM) >>>> +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_1M >>>> +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_2M >>>> +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M >>>> +#else >>>> +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_8K >>>> +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_64K >>>> +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_2M >>>> +#endif >>>> + >>>> static void guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log); >>>> >>>> +struct guc_log_section { >>>> + u32 max; >>>> + u32 flag; >>>> + u32 default_val; >>>> + const char *name; >>>> +}; >>>> + >>>> +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; >>>> +} >>>> + >>>> +static void _guc_log_init_sizes(struct intel_guc_log *log) >>>> +{ >>>> + struct intel_guc *guc = log_to_guc(log); >>>> + struct drm_i915_private *i915 = guc_to_gt(guc)->i915; >>>> + static const struct guc_log_section sections[GUC_LOG_SECTIONS_LIMIT] = { >>>> + { >>>> + GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT, >>>> + GUC_LOG_LOG_ALLOC_UNITS, >>>> + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE, >>>> + "crash dump" >>>> + }, >>>> + { >>>> + GUC_LOG_DEBUG_MASK >> GUC_LOG_DEBUG_SHIFT, >>>> + GUC_LOG_LOG_ALLOC_UNITS, >>>> + GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE, >>>> + "debug", >>>> + }, >>>> + { >>>> + GUC_LOG_CAPTURE_MASK >> GUC_LOG_CAPTURE_SHIFT, >>>> + GUC_LOG_CAPTURE_ALLOC_UNITS, >>>> + GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE, >>>> + "capture", >>>> + } >>>> + }; >>>> + s32 params[GUC_LOG_SECTIONS_LIMIT] = { >>>> + i915->params.guc_log_size_crash, >>>> + i915->params.guc_log_size_debug, >>>> + i915->params.guc_log_size_capture, >>>> + }; >>>> + int i; >>>> + >>>> + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) >>>> + log->sizes[i].bytes = scale_log_param(log, sections + i, params[i]); >>>> + >>>> + /* If debug size > 1MB then bump default crash size to keep the same units */ >>>> + if (log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes >= SZ_1M && >>>> + (i915->params.guc_log_size_crash == -1) && >>>> + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE < SZ_1M) >>>> + log->sizes[GUC_LOG_SECTIONS_CRASH].bytes = SZ_1M; >>>> + >>>> + /* Prepare the GuC API structure fields: */ >>>> + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) { >>>> + /* Convert to correct units */ >>>> + if ((log->sizes[i].bytes % SZ_1M) == 0) { >>>> + log->sizes[i].units = SZ_1M; >>>> + log->sizes[i].flag = sections[i].flag; >>>> + } else { >>>> + log->sizes[i].units = SZ_4K; >>>> + log->sizes[i].flag = 0; >>>> + } >>>> + >>>> + if (!IS_ALIGNED(log->sizes[i].bytes, log->sizes[i].units)) >>>> + drm_err(&i915->drm, "Mis-aligned GuC log %s size: 0x%X vs 0x%X!", >>>> + sections[i].name, log->sizes[i].bytes, log->sizes[i].units); >>>> + log->sizes[i].count = log->sizes[i].bytes / log->sizes[i].units; >>>> + >>>> + if (!log->sizes[i].count) { >>>> + drm_err(&i915->drm, "Zero GuC log %s size!", sections[i].name); >>>> + } else { >>>> + /* Size is +1 unit */ >>>> + log->sizes[i].count--; >>>> + } >>>> + >>>> + /* Clip to field size */ >>>> + if (log->sizes[i].count > sections[i].max) { >>>> + drm_err(&i915->drm, "GuC log %s size too large: %d vs %d!", >>>> + sections[i].name, log->sizes[i].count + 1, sections[i].max + 1); >>>> + log->sizes[i].count = sections[i].max; >>>> + } >>>> + } >>>> + >>>> + if (log->sizes[GUC_LOG_SECTIONS_CRASH].units != log->sizes[GUC_LOG_SECTIONS_DEBUG].units) { >>>> + drm_err(&i915->drm, "Unit mis-match for GuC log crash and debug sections: %d vs %d!", >>>> + log->sizes[GUC_LOG_SECTIONS_CRASH].units, >>>> + log->sizes[GUC_LOG_SECTIONS_DEBUG].units); >>>> + log->sizes[GUC_LOG_SECTIONS_CRASH].units = log->sizes[GUC_LOG_SECTIONS_DEBUG].units; >>>> + log->sizes[GUC_LOG_SECTIONS_CRASH].count = 0; >>>> + } >>>> + >>>> + log->sizes_initialised = true; >>>> +} >>>> + >>>> +static void guc_log_init_sizes(struct intel_guc_log *log) >>>> +{ >>>> + if (log->sizes_initialised) >>>> + return; >>>> + >>>> + _guc_log_init_sizes(log); >>>> +} >>>> + >>>> +static u32 intel_guc_log_section_size_crash(struct intel_guc_log *log) >>>> +{ >>>> + guc_log_init_sizes(log); >>>> + >>>> + return log->sizes[GUC_LOG_SECTIONS_CRASH].bytes; >>>> +} >>>> + >>>> +static u32 intel_guc_log_section_size_debug(struct intel_guc_log *log) >>>> +{ >>>> + guc_log_init_sizes(log); >>>> + >>>> + return log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes; >>>> +} >>>> + >>>> +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log) >>>> +{ >>>> + guc_log_init_sizes(log); >>>> + >>>> + return log->sizes[GUC_LOG_SECTIONS_CAPTURE].bytes; >>>> +} >>>> + >>>> static u32 intel_guc_log_size(struct intel_guc_log *log) >>>> { >>>> /* >>>> @@ -38,7 +188,10 @@ static u32 intel_guc_log_size(struct intel_guc_log *log) >>>> * | Capture logs | >>>> * +===============================+ + CAPTURE_SIZE >>>> */ >>>> - return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE + CAPTURE_BUFFER_SIZE; >>>> + return PAGE_SIZE + >>>> + intel_guc_log_section_size_crash(log) + >>>> + intel_guc_log_section_size_debug(log) + >>>> + intel_guc_log_section_size_capture(log); >>>> } >>>> >>>> /** >>>> @@ -165,7 +318,8 @@ static void guc_move_to_next_buf(struct intel_guc_log *log) >>>> smp_wmb(); >>>> >>>> /* All data has been written, so now move the offset of sub buffer. */ >>>> - relay_reserve(log->relay.channel, log->vma->obj->base.size - CAPTURE_BUFFER_SIZE); >>>> + relay_reserve(log->relay.channel, log->vma->obj->base.size - >>>> + intel_guc_log_section_size_capture(log)); >>>> >>>> /* Switch to the next sub buffer */ >>>> relay_flush(log->relay.channel); >>>> @@ -210,15 +364,16 @@ bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, >>>> return overflow; >>>> } >>>> >>>> -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) >>>> +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, >>>> + enum guc_log_buffer_type type) >>>> { >>>> switch (type) { >>>> case GUC_DEBUG_LOG_BUFFER: >>>> - return DEBUG_BUFFER_SIZE; >>>> + return intel_guc_log_section_size_debug(log); >>>> case GUC_CRASH_DUMP_LOG_BUFFER: >>>> - return CRASH_BUFFER_SIZE; >>>> + return intel_guc_log_section_size_crash(log); >>>> case GUC_CAPTURE_LOG_BUFFER: >>>> - return CAPTURE_BUFFER_SIZE; >>>> + return intel_guc_log_section_size_capture(log); >>>> default: >>>> MISSING_CASE(type); >>>> } >>>> @@ -226,7 +381,8 @@ unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) >>>> return 0; >>>> } >>>> >>>> -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) >>>> +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, >>>> + enum guc_log_buffer_type type) >>>> { >>>> enum guc_log_buffer_type i; >>>> size_t offset = PAGE_SIZE;/* for the log_buffer_states */ >>>> @@ -234,7 +390,7 @@ size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) >>>> for (i = GUC_DEBUG_LOG_BUFFER; i < GUC_MAX_LOG_BUFFER; ++i) { >>>> if (i == type) >>>> break; >>>> - offset += intel_guc_get_log_buffer_size(i); >>>> + offset += intel_guc_get_log_buffer_size(log, i); >>>> } >>>> >>>> return offset; >>>> @@ -285,7 +441,7 @@ static void _guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log) >>>> */ >>>> memcpy(&log_buf_state_local, log_buf_state, >>>> sizeof(struct guc_log_buffer_state)); >>>> - buffer_size = intel_guc_get_log_buffer_size(type); >>>> + buffer_size = intel_guc_get_log_buffer_size(log, type); >>>> read_offset = log_buf_state_local.read_ptr; >>>> write_offset = log_buf_state_local.sampled_write_ptr; >>>> full_cnt = log_buf_state_local.buffer_full_cnt; >>>> @@ -400,7 +556,7 @@ static int guc_log_relay_create(struct intel_guc_log *log) >>>> * Keep the size of sub buffers same as shared log buffer >>>> * but GuC log-events excludes the error-state-capture logs >>>> */ >>>> - subbuf_size = log->vma->size - CAPTURE_BUFFER_SIZE; >>>> + subbuf_size = log->vma->size - intel_guc_log_section_size_capture(log); >>>> >>>> /* >>>> * Store up to 8 snapshots, which is large enough to buffer sufficient >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h >>>> index dc9715411d626..02127703be809 100644 >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h >>>> @@ -15,20 +15,6 @@ >>>> >>>> struct intel_guc; >>>> >>>> -#if defined(CONFIG_DRM_I915_DEBUG_GUC) >>>> -#define CRASH_BUFFER_SIZE SZ_2M >>>> -#define DEBUG_BUFFER_SIZE SZ_16M >>>> -#define CAPTURE_BUFFER_SIZE SZ_4M >>>> -#elif defined(CONFIG_DRM_I915_DEBUG_GEM) >>>> -#define CRASH_BUFFER_SIZE SZ_1M >>>> -#define DEBUG_BUFFER_SIZE SZ_2M >>>> -#define CAPTURE_BUFFER_SIZE SZ_4M >>>> -#else >>>> -#define CRASH_BUFFER_SIZE SZ_8K >>>> -#define DEBUG_BUFFER_SIZE SZ_64K >>>> -#define CAPTURE_BUFFER_SIZE SZ_2M >>>> -#endif >>>> - >>>> /* >>>> * While we're using plain log level in i915, GuC controls are much more... >>>> * "elaborate"? We have a couple of bits for verbosity, separate bit for actual >>>> @@ -46,10 +32,30 @@ struct intel_guc; >>>> #define GUC_VERBOSITY_TO_LOG_LEVEL(x) ((x) + 2) >>>> #define GUC_LOG_LEVEL_MAX GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX) >>>> >>>> +enum { >>>> + GUC_LOG_SECTIONS_CRASH, >>>> + GUC_LOG_SECTIONS_DEBUG, >>>> + GUC_LOG_SECTIONS_CAPTURE, >>>> + GUC_LOG_SECTIONS_LIMIT >>>> +}; >>>> + >>>> struct intel_guc_log { >>>> u32 level; >>>> + >>>> + /* Allocation settings */ >>>> + struct { >>>> + s32 bytes; /* Size in bytes */ >>>> + s32 units; /* GuC API units - 1MB or 4KB */ >>>> + s32 count; /* Number of API units */ >>>> + u32 flag; /* GuC API units flag */ >>>> + } sizes[GUC_LOG_SECTIONS_LIMIT]; >>>> + bool sizes_initialised; >>>> + >>>> + /* Combined buffer allocation */ >>>> struct i915_vma *vma; >>>> void *buf_addr; >>>> + >>>> + /* RelayFS support */ >>>> struct { >>>> bool buf_in_use; >>>> bool started; >>>> @@ -58,6 +64,7 @@ struct intel_guc_log { >>>> struct mutex lock; >>>> u32 full_count; >>>> } relay; >>>> + >>>> /* logging related stats */ >>>> struct { >>>> u32 sampled_overflow; >>>> @@ -69,8 +76,9 @@ struct intel_guc_log { >>>> void intel_guc_log_init_early(struct intel_guc_log *log); >>>> bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, enum guc_log_buffer_type type, >>>> unsigned int full_cnt); >>>> -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type); >>>> -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type); >>>> +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, >>>> + enum guc_log_buffer_type type); >>>> +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, enum guc_log_buffer_type type); >>>> int intel_guc_log_create(struct intel_guc_log *log); >>>> void intel_guc_log_destroy(struct intel_guc_log *log); >>>> >>>> @@ -92,4 +100,6 @@ void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p); >>>> int intel_guc_log_dump(struct intel_guc_log *log, struct drm_printer *p, >>>> bool dump_load_err); >>>> >>>> +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log); >>>> + >>>> #endif >>>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c >>>> index 6fc475a5db615..06ca5b8221118 100644 >>>> --- a/drivers/gpu/drm/i915/i915_params.c >>>> +++ b/drivers/gpu/drm/i915/i915_params.c >>>> @@ -171,6 +171,18 @@ i915_param_named(guc_log_level, int, 0400, >>>> "GuC firmware logging level. Requires GuC to be loaded. " >>>> "(-1=auto [default], 0=disable, 1..4=enable with verbosity min..max)"); >>>> >>>> +i915_param_named(guc_log_size_crash, int, 0400, >>>> + "GuC firmware logging buffer size for crash dumps (in MB)" >>>> + "(-1=auto [default], NB: max = 4, other restrictions apply)"); >>>> + >>>> +i915_param_named(guc_log_size_debug, int, 0400, >>>> + "GuC firmware logging buffer size for debug logs (in MB)" >>>> + "(-1=auto [default], NB: max = 16, other restrictions apply)"); >>>> + >>>> +i915_param_named(guc_log_size_capture, int, 0400, >>>> + "GuC error capture register dump buffer size (in MB)" >>>> + "(-1=auto [default], NB: max = 4, other restrictions apply)"); >>>> + >>>> i915_param_named_unsafe(guc_firmware_path, charp, 0400, >>>> "GuC firmware path to use instead of the default one"); >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h >>>> index 2733cb6cfe094..f684d1ab87078 100644 >>>> --- a/drivers/gpu/drm/i915/i915_params.h >>>> +++ b/drivers/gpu/drm/i915/i915_params.h >>>> @@ -61,6 +61,9 @@ struct drm_printer; >>>> param(int, invert_brightness, 0, 0600) \ >>>> param(int, enable_guc, -1, 0400) \ >>>> param(int, guc_log_level, -1, 0400) \ >>>> + param(int, guc_log_size_crash, -1, 0400) \ >>>> + param(int, guc_log_size_debug, -1, 0400) \ >>>> + param(int, guc_log_size_capture, -1, 0400) \ >>>> param(char *, guc_firmware_path, NULL, 0400) \ >>>> param(char *, huc_firmware_path, NULL, 0400) \ >>>> param(char *, dmc_firmware_path, NULL, 0400) \ >>>> -- >>>> 2.37.1 >>>>
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.Harrison@Intel.com (2022-07-28 05:20:27) > >>>> From: John Harrison <John.C.Harrison@Intel.com> > >>>> > >>>> 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 <John.C.Harrison@Intel.com> > >>>> --- > >>>> 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) > >>>> > >>>> log_buf_state = guc->log.buf_addr + > >>>> (sizeof(struct guc_log_buffer_state) * GUC_CAPTURE_LOG_BUFFER); > >>>> - src_data = guc->log.buf_addr + intel_guc_get_log_buffer_offset(GUC_CAPTURE_LOG_BUFFER); > >>>> + src_data = guc->log.buf_addr + > >>>> + intel_guc_get_log_buffer_offset(&guc->log, GUC_CAPTURE_LOG_BUFFER); > >>>> > >>>> /* > >>>> * Make a copy of the state structure, inside GuC log buffer > >>>> @@ -1302,7 +1304,7 @@ static void __guc_capture_process_output(struct intel_guc *guc) > >>>> * from it multiple times. > >>>> */ > >>>> memcpy(&log_buf_state_local, log_buf_state, sizeof(struct guc_log_buffer_state)); > >>>> - buffer_size = intel_guc_get_log_buffer_size(GUC_CAPTURE_LOG_BUFFER); > >>>> + buffer_size = intel_guc_get_log_buffer_size(&guc->log, GUC_CAPTURE_LOG_BUFFER); > >>>> read_offset = log_buf_state_local.read_ptr; > >>>> write_offset = log_buf_state_local.sampled_write_ptr; > >>>> full_count = log_buf_state_local.buffer_full_cnt; > >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > >>>> index 4722d4b18ed19..890b6853bd609 100644 > >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > >>>> @@ -13,8 +13,158 @@ > >>>> #include "intel_guc_capture.h" > >>>> #include "intel_guc_log.h" > >>>> > >>>> +#if defined(CONFIG_DRM_I915_DEBUG_GUC) > >>>> +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_2M > >>>> +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_16M > >>>> +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M > >>>> +#elif defined(CONFIG_DRM_I915_DEBUG_GEM) > >>>> +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_1M > >>>> +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_2M > >>>> +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M > >>>> +#else > >>>> +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_8K > >>>> +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_64K > >>>> +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_2M > >>>> +#endif > >>>> + > >>>> static void guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log); > >>>> > >>>> +struct guc_log_section { > >>>> + u32 max; > >>>> + u32 flag; > >>>> + u32 default_val; > >>>> + const char *name; > >>>> +}; > >>>> + > >>>> +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; > >>>> +} > >>>> + > >>>> +static void _guc_log_init_sizes(struct intel_guc_log *log) > >>>> +{ > >>>> + struct intel_guc *guc = log_to_guc(log); > >>>> + struct drm_i915_private *i915 = guc_to_gt(guc)->i915; > >>>> + static const struct guc_log_section sections[GUC_LOG_SECTIONS_LIMIT] = { > >>>> + { > >>>> + GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT, > >>>> + GUC_LOG_LOG_ALLOC_UNITS, > >>>> + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE, > >>>> + "crash dump" > >>>> + }, > >>>> + { > >>>> + GUC_LOG_DEBUG_MASK >> GUC_LOG_DEBUG_SHIFT, > >>>> + GUC_LOG_LOG_ALLOC_UNITS, > >>>> + GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE, > >>>> + "debug", > >>>> + }, > >>>> + { > >>>> + GUC_LOG_CAPTURE_MASK >> GUC_LOG_CAPTURE_SHIFT, > >>>> + GUC_LOG_CAPTURE_ALLOC_UNITS, > >>>> + GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE, > >>>> + "capture", > >>>> + } > >>>> + }; > >>>> + s32 params[GUC_LOG_SECTIONS_LIMIT] = { > >>>> + i915->params.guc_log_size_crash, > >>>> + i915->params.guc_log_size_debug, > >>>> + i915->params.guc_log_size_capture, > >>>> + }; > >>>> + int i; > >>>> + > >>>> + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) > >>>> + log->sizes[i].bytes = scale_log_param(log, sections + i, params[i]); > >>>> + > >>>> + /* If debug size > 1MB then bump default crash size to keep the same units */ > >>>> + if (log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes >= SZ_1M && > >>>> + (i915->params.guc_log_size_crash == -1) && > >>>> + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE < SZ_1M) > >>>> + log->sizes[GUC_LOG_SECTIONS_CRASH].bytes = SZ_1M; > >>>> + > >>>> + /* Prepare the GuC API structure fields: */ > >>>> + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) { > >>>> + /* Convert to correct units */ > >>>> + if ((log->sizes[i].bytes % SZ_1M) == 0) { > >>>> + log->sizes[i].units = SZ_1M; > >>>> + log->sizes[i].flag = sections[i].flag; > >>>> + } else { > >>>> + log->sizes[i].units = SZ_4K; > >>>> + log->sizes[i].flag = 0; > >>>> + } > >>>> + > >>>> + if (!IS_ALIGNED(log->sizes[i].bytes, log->sizes[i].units)) > >>>> + drm_err(&i915->drm, "Mis-aligned GuC log %s size: 0x%X vs 0x%X!", > >>>> + sections[i].name, log->sizes[i].bytes, log->sizes[i].units); > >>>> + log->sizes[i].count = log->sizes[i].bytes / log->sizes[i].units; > >>>> + > >>>> + if (!log->sizes[i].count) { > >>>> + drm_err(&i915->drm, "Zero GuC log %s size!", sections[i].name); > >>>> + } else { > >>>> + /* Size is +1 unit */ > >>>> + log->sizes[i].count--; > >>>> + } > >>>> + > >>>> + /* Clip to field size */ > >>>> + if (log->sizes[i].count > sections[i].max) { > >>>> + drm_err(&i915->drm, "GuC log %s size too large: %d vs %d!", > >>>> + sections[i].name, log->sizes[i].count + 1, sections[i].max + 1); > >>>> + log->sizes[i].count = sections[i].max; > >>>> + } > >>>> + } > >>>> + > >>>> + if (log->sizes[GUC_LOG_SECTIONS_CRASH].units != log->sizes[GUC_LOG_SECTIONS_DEBUG].units) { > >>>> + drm_err(&i915->drm, "Unit mis-match for GuC log crash and debug sections: %d vs %d!", > >>>> + log->sizes[GUC_LOG_SECTIONS_CRASH].units, > >>>> + log->sizes[GUC_LOG_SECTIONS_DEBUG].units); > >>>> + log->sizes[GUC_LOG_SECTIONS_CRASH].units = log->sizes[GUC_LOG_SECTIONS_DEBUG].units; > >>>> + log->sizes[GUC_LOG_SECTIONS_CRASH].count = 0; > >>>> + } > >>>> + > >>>> + log->sizes_initialised = true; > >>>> +} > >>>> + > >>>> +static void guc_log_init_sizes(struct intel_guc_log *log) > >>>> +{ > >>>> + if (log->sizes_initialised) > >>>> + return; > >>>> + > >>>> + _guc_log_init_sizes(log); > >>>> +} > >>>> + > >>>> +static u32 intel_guc_log_section_size_crash(struct intel_guc_log *log) > >>>> +{ > >>>> + guc_log_init_sizes(log); > >>>> + > >>>> + return log->sizes[GUC_LOG_SECTIONS_CRASH].bytes; > >>>> +} > >>>> + > >>>> +static u32 intel_guc_log_section_size_debug(struct intel_guc_log *log) > >>>> +{ > >>>> + guc_log_init_sizes(log); > >>>> + > >>>> + return log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes; > >>>> +} > >>>> + > >>>> +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log) > >>>> +{ > >>>> + guc_log_init_sizes(log); > >>>> + > >>>> + return log->sizes[GUC_LOG_SECTIONS_CAPTURE].bytes; > >>>> +} > >>>> + > >>>> static u32 intel_guc_log_size(struct intel_guc_log *log) > >>>> { > >>>> /* > >>>> @@ -38,7 +188,10 @@ static u32 intel_guc_log_size(struct intel_guc_log *log) > >>>> * | Capture logs | > >>>> * +===============================+ + CAPTURE_SIZE > >>>> */ > >>>> - return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE + CAPTURE_BUFFER_SIZE; > >>>> + return PAGE_SIZE + > >>>> + intel_guc_log_section_size_crash(log) + > >>>> + intel_guc_log_section_size_debug(log) + > >>>> + intel_guc_log_section_size_capture(log); > >>>> } > >>>> > >>>> /** > >>>> @@ -165,7 +318,8 @@ static void guc_move_to_next_buf(struct intel_guc_log *log) > >>>> smp_wmb(); > >>>> > >>>> /* All data has been written, so now move the offset of sub buffer. */ > >>>> - relay_reserve(log->relay.channel, log->vma->obj->base.size - CAPTURE_BUFFER_SIZE); > >>>> + relay_reserve(log->relay.channel, log->vma->obj->base.size - > >>>> + intel_guc_log_section_size_capture(log)); > >>>> > >>>> /* Switch to the next sub buffer */ > >>>> relay_flush(log->relay.channel); > >>>> @@ -210,15 +364,16 @@ bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, > >>>> return overflow; > >>>> } > >>>> > >>>> -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) > >>>> +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, > >>>> + enum guc_log_buffer_type type) > >>>> { > >>>> switch (type) { > >>>> case GUC_DEBUG_LOG_BUFFER: > >>>> - return DEBUG_BUFFER_SIZE; > >>>> + return intel_guc_log_section_size_debug(log); > >>>> case GUC_CRASH_DUMP_LOG_BUFFER: > >>>> - return CRASH_BUFFER_SIZE; > >>>> + return intel_guc_log_section_size_crash(log); > >>>> case GUC_CAPTURE_LOG_BUFFER: > >>>> - return CAPTURE_BUFFER_SIZE; > >>>> + return intel_guc_log_section_size_capture(log); > >>>> default: > >>>> MISSING_CASE(type); > >>>> } > >>>> @@ -226,7 +381,8 @@ unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) > >>>> return 0; > >>>> } > >>>> > >>>> -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) > >>>> +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, > >>>> + enum guc_log_buffer_type type) > >>>> { > >>>> enum guc_log_buffer_type i; > >>>> size_t offset = PAGE_SIZE;/* for the log_buffer_states */ > >>>> @@ -234,7 +390,7 @@ size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) > >>>> for (i = GUC_DEBUG_LOG_BUFFER; i < GUC_MAX_LOG_BUFFER; ++i) { > >>>> if (i == type) > >>>> break; > >>>> - offset += intel_guc_get_log_buffer_size(i); > >>>> + offset += intel_guc_get_log_buffer_size(log, i); > >>>> } > >>>> > >>>> return offset; > >>>> @@ -285,7 +441,7 @@ static void _guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log) > >>>> */ > >>>> memcpy(&log_buf_state_local, log_buf_state, > >>>> sizeof(struct guc_log_buffer_state)); > >>>> - buffer_size = intel_guc_get_log_buffer_size(type); > >>>> + buffer_size = intel_guc_get_log_buffer_size(log, type); > >>>> read_offset = log_buf_state_local.read_ptr; > >>>> write_offset = log_buf_state_local.sampled_write_ptr; > >>>> full_cnt = log_buf_state_local.buffer_full_cnt; > >>>> @@ -400,7 +556,7 @@ static int guc_log_relay_create(struct intel_guc_log *log) > >>>> * Keep the size of sub buffers same as shared log buffer > >>>> * but GuC log-events excludes the error-state-capture logs > >>>> */ > >>>> - subbuf_size = log->vma->size - CAPTURE_BUFFER_SIZE; > >>>> + subbuf_size = log->vma->size - intel_guc_log_section_size_capture(log); > >>>> > >>>> /* > >>>> * Store up to 8 snapshots, which is large enough to buffer sufficient > >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h > >>>> index dc9715411d626..02127703be809 100644 > >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h > >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h > >>>> @@ -15,20 +15,6 @@ > >>>> > >>>> struct intel_guc; > >>>> > >>>> -#if defined(CONFIG_DRM_I915_DEBUG_GUC) > >>>> -#define CRASH_BUFFER_SIZE SZ_2M > >>>> -#define DEBUG_BUFFER_SIZE SZ_16M > >>>> -#define CAPTURE_BUFFER_SIZE SZ_4M > >>>> -#elif defined(CONFIG_DRM_I915_DEBUG_GEM) > >>>> -#define CRASH_BUFFER_SIZE SZ_1M > >>>> -#define DEBUG_BUFFER_SIZE SZ_2M > >>>> -#define CAPTURE_BUFFER_SIZE SZ_4M > >>>> -#else > >>>> -#define CRASH_BUFFER_SIZE SZ_8K > >>>> -#define DEBUG_BUFFER_SIZE SZ_64K > >>>> -#define CAPTURE_BUFFER_SIZE SZ_2M > >>>> -#endif > >>>> - > >>>> /* > >>>> * While we're using plain log level in i915, GuC controls are much more... > >>>> * "elaborate"? We have a couple of bits for verbosity, separate bit for actual > >>>> @@ -46,10 +32,30 @@ struct intel_guc; > >>>> #define GUC_VERBOSITY_TO_LOG_LEVEL(x) ((x) + 2) > >>>> #define GUC_LOG_LEVEL_MAX GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX) > >>>> > >>>> +enum { > >>>> + GUC_LOG_SECTIONS_CRASH, > >>>> + GUC_LOG_SECTIONS_DEBUG, > >>>> + GUC_LOG_SECTIONS_CAPTURE, > >>>> + GUC_LOG_SECTIONS_LIMIT > >>>> +}; > >>>> + > >>>> struct intel_guc_log { > >>>> u32 level; > >>>> + > >>>> + /* Allocation settings */ > >>>> + struct { > >>>> + s32 bytes; /* Size in bytes */ > >>>> + s32 units; /* GuC API units - 1MB or 4KB */ > >>>> + s32 count; /* Number of API units */ > >>>> + u32 flag; /* GuC API units flag */ > >>>> + } sizes[GUC_LOG_SECTIONS_LIMIT]; > >>>> + bool sizes_initialised; > >>>> + > >>>> + /* Combined buffer allocation */ > >>>> struct i915_vma *vma; > >>>> void *buf_addr; > >>>> + > >>>> + /* RelayFS support */ > >>>> struct { > >>>> bool buf_in_use; > >>>> bool started; > >>>> @@ -58,6 +64,7 @@ struct intel_guc_log { > >>>> struct mutex lock; > >>>> u32 full_count; > >>>> } relay; > >>>> + > >>>> /* logging related stats */ > >>>> struct { > >>>> u32 sampled_overflow; > >>>> @@ -69,8 +76,9 @@ struct intel_guc_log { > >>>> void intel_guc_log_init_early(struct intel_guc_log *log); > >>>> bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, enum guc_log_buffer_type type, > >>>> unsigned int full_cnt); > >>>> -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type); > >>>> -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type); > >>>> +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, > >>>> + enum guc_log_buffer_type type); > >>>> +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, enum guc_log_buffer_type type); > >>>> int intel_guc_log_create(struct intel_guc_log *log); > >>>> void intel_guc_log_destroy(struct intel_guc_log *log); > >>>> > >>>> @@ -92,4 +100,6 @@ void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p); > >>>> int intel_guc_log_dump(struct intel_guc_log *log, struct drm_printer *p, > >>>> bool dump_load_err); > >>>> > >>>> +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log); > >>>> + > >>>> #endif > >>>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > >>>> index 6fc475a5db615..06ca5b8221118 100644 > >>>> --- a/drivers/gpu/drm/i915/i915_params.c > >>>> +++ b/drivers/gpu/drm/i915/i915_params.c > >>>> @@ -171,6 +171,18 @@ i915_param_named(guc_log_level, int, 0400, > >>>> "GuC firmware logging level. Requires GuC to be loaded. " > >>>> "(-1=auto [default], 0=disable, 1..4=enable with verbosity min..max)"); > >>>> > >>>> +i915_param_named(guc_log_size_crash, int, 0400, > >>>> + "GuC firmware logging buffer size for crash dumps (in MB)" > >>>> + "(-1=auto [default], NB: max = 4, other restrictions apply)"); > >>>> + > >>>> +i915_param_named(guc_log_size_debug, int, 0400, > >>>> + "GuC firmware logging buffer size for debug logs (in MB)" > >>>> + "(-1=auto [default], NB: max = 16, other restrictions apply)"); > >>>> + > >>>> +i915_param_named(guc_log_size_capture, int, 0400, > >>>> + "GuC error capture register dump buffer size (in MB)" > >>>> + "(-1=auto [default], NB: max = 4, other restrictions apply)"); > >>>> + > >>>> i915_param_named_unsafe(guc_firmware_path, charp, 0400, > >>>> "GuC firmware path to use instead of the default one"); > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > >>>> index 2733cb6cfe094..f684d1ab87078 100644 > >>>> --- a/drivers/gpu/drm/i915/i915_params.h > >>>> +++ b/drivers/gpu/drm/i915/i915_params.h > >>>> @@ -61,6 +61,9 @@ struct drm_printer; > >>>> param(int, invert_brightness, 0, 0600) \ > >>>> param(int, enable_guc, -1, 0400) \ > >>>> param(int, guc_log_level, -1, 0400) \ > >>>> + param(int, guc_log_size_crash, -1, 0400) \ > >>>> + param(int, guc_log_size_debug, -1, 0400) \ > >>>> + param(int, guc_log_size_capture, -1, 0400) \ > >>>> param(char *, guc_firmware_path, NULL, 0400) \ > >>>> param(char *, huc_firmware_path, NULL, 0400) \ > >>>> param(char *, dmc_firmware_path, NULL, 0400) \ > >>>> -- > >>>> 2.37.1 > >>>> >
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
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
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) log_buf_state = guc->log.buf_addr + (sizeof(struct guc_log_buffer_state) * GUC_CAPTURE_LOG_BUFFER); - src_data = guc->log.buf_addr + intel_guc_get_log_buffer_offset(GUC_CAPTURE_LOG_BUFFER); + src_data = guc->log.buf_addr + + intel_guc_get_log_buffer_offset(&guc->log, GUC_CAPTURE_LOG_BUFFER); /* * Make a copy of the state structure, inside GuC log buffer @@ -1302,7 +1304,7 @@ static void __guc_capture_process_output(struct intel_guc *guc) * from it multiple times. */ memcpy(&log_buf_state_local, log_buf_state, sizeof(struct guc_log_buffer_state)); - buffer_size = intel_guc_get_log_buffer_size(GUC_CAPTURE_LOG_BUFFER); + buffer_size = intel_guc_get_log_buffer_size(&guc->log, GUC_CAPTURE_LOG_BUFFER); read_offset = log_buf_state_local.read_ptr; write_offset = log_buf_state_local.sampled_write_ptr; full_count = log_buf_state_local.buffer_full_cnt; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c index 4722d4b18ed19..890b6853bd609 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c @@ -13,8 +13,158 @@ #include "intel_guc_capture.h" #include "intel_guc_log.h" +#if defined(CONFIG_DRM_I915_DEBUG_GUC) +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_2M +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_16M +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M +#elif defined(CONFIG_DRM_I915_DEBUG_GEM) +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_1M +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_2M +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_4M +#else +#define GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE SZ_8K +#define GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE SZ_64K +#define GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE SZ_2M +#endif + static void guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log); +struct guc_log_section { + u32 max; + u32 flag; + u32 default_val; + const char *name; +}; + +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; +} + +static void _guc_log_init_sizes(struct intel_guc_log *log) +{ + struct intel_guc *guc = log_to_guc(log); + struct drm_i915_private *i915 = guc_to_gt(guc)->i915; + static const struct guc_log_section sections[GUC_LOG_SECTIONS_LIMIT] = { + { + GUC_LOG_CRASH_MASK >> GUC_LOG_CRASH_SHIFT, + GUC_LOG_LOG_ALLOC_UNITS, + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE, + "crash dump" + }, + { + GUC_LOG_DEBUG_MASK >> GUC_LOG_DEBUG_SHIFT, + GUC_LOG_LOG_ALLOC_UNITS, + GUC_LOG_DEFAULT_DEBUG_BUFFER_SIZE, + "debug", + }, + { + GUC_LOG_CAPTURE_MASK >> GUC_LOG_CAPTURE_SHIFT, + GUC_LOG_CAPTURE_ALLOC_UNITS, + GUC_LOG_DEFAULT_CAPTURE_BUFFER_SIZE, + "capture", + } + }; + s32 params[GUC_LOG_SECTIONS_LIMIT] = { + i915->params.guc_log_size_crash, + i915->params.guc_log_size_debug, + i915->params.guc_log_size_capture, + }; + int i; + + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) + log->sizes[i].bytes = scale_log_param(log, sections + i, params[i]); + + /* If debug size > 1MB then bump default crash size to keep the same units */ + if (log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes >= SZ_1M && + (i915->params.guc_log_size_crash == -1) && + GUC_LOG_DEFAULT_CRASH_BUFFER_SIZE < SZ_1M) + log->sizes[GUC_LOG_SECTIONS_CRASH].bytes = SZ_1M; + + /* Prepare the GuC API structure fields: */ + for (i = 0; i < GUC_LOG_SECTIONS_LIMIT; i++) { + /* Convert to correct units */ + if ((log->sizes[i].bytes % SZ_1M) == 0) { + log->sizes[i].units = SZ_1M; + log->sizes[i].flag = sections[i].flag; + } else { + log->sizes[i].units = SZ_4K; + log->sizes[i].flag = 0; + } + + if (!IS_ALIGNED(log->sizes[i].bytes, log->sizes[i].units)) + drm_err(&i915->drm, "Mis-aligned GuC log %s size: 0x%X vs 0x%X!", + sections[i].name, log->sizes[i].bytes, log->sizes[i].units); + log->sizes[i].count = log->sizes[i].bytes / log->sizes[i].units; + + if (!log->sizes[i].count) { + drm_err(&i915->drm, "Zero GuC log %s size!", sections[i].name); + } else { + /* Size is +1 unit */ + log->sizes[i].count--; + } + + /* Clip to field size */ + if (log->sizes[i].count > sections[i].max) { + drm_err(&i915->drm, "GuC log %s size too large: %d vs %d!", + sections[i].name, log->sizes[i].count + 1, sections[i].max + 1); + log->sizes[i].count = sections[i].max; + } + } + + if (log->sizes[GUC_LOG_SECTIONS_CRASH].units != log->sizes[GUC_LOG_SECTIONS_DEBUG].units) { + drm_err(&i915->drm, "Unit mis-match for GuC log crash and debug sections: %d vs %d!", + log->sizes[GUC_LOG_SECTIONS_CRASH].units, + log->sizes[GUC_LOG_SECTIONS_DEBUG].units); + log->sizes[GUC_LOG_SECTIONS_CRASH].units = log->sizes[GUC_LOG_SECTIONS_DEBUG].units; + log->sizes[GUC_LOG_SECTIONS_CRASH].count = 0; + } + + log->sizes_initialised = true; +} + +static void guc_log_init_sizes(struct intel_guc_log *log) +{ + if (log->sizes_initialised) + return; + + _guc_log_init_sizes(log); +} + +static u32 intel_guc_log_section_size_crash(struct intel_guc_log *log) +{ + guc_log_init_sizes(log); + + return log->sizes[GUC_LOG_SECTIONS_CRASH].bytes; +} + +static u32 intel_guc_log_section_size_debug(struct intel_guc_log *log) +{ + guc_log_init_sizes(log); + + return log->sizes[GUC_LOG_SECTIONS_DEBUG].bytes; +} + +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log) +{ + guc_log_init_sizes(log); + + return log->sizes[GUC_LOG_SECTIONS_CAPTURE].bytes; +} + static u32 intel_guc_log_size(struct intel_guc_log *log) { /* @@ -38,7 +188,10 @@ static u32 intel_guc_log_size(struct intel_guc_log *log) * | Capture logs | * +===============================+ + CAPTURE_SIZE */ - return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE + CAPTURE_BUFFER_SIZE; + return PAGE_SIZE + + intel_guc_log_section_size_crash(log) + + intel_guc_log_section_size_debug(log) + + intel_guc_log_section_size_capture(log); } /** @@ -165,7 +318,8 @@ static void guc_move_to_next_buf(struct intel_guc_log *log) smp_wmb(); /* All data has been written, so now move the offset of sub buffer. */ - relay_reserve(log->relay.channel, log->vma->obj->base.size - CAPTURE_BUFFER_SIZE); + relay_reserve(log->relay.channel, log->vma->obj->base.size - + intel_guc_log_section_size_capture(log)); /* Switch to the next sub buffer */ relay_flush(log->relay.channel); @@ -210,15 +364,16 @@ bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, return overflow; } -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, + enum guc_log_buffer_type type) { switch (type) { case GUC_DEBUG_LOG_BUFFER: - return DEBUG_BUFFER_SIZE; + return intel_guc_log_section_size_debug(log); case GUC_CRASH_DUMP_LOG_BUFFER: - return CRASH_BUFFER_SIZE; + return intel_guc_log_section_size_crash(log); case GUC_CAPTURE_LOG_BUFFER: - return CAPTURE_BUFFER_SIZE; + return intel_guc_log_section_size_capture(log); default: MISSING_CASE(type); } @@ -226,7 +381,8 @@ unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type) return 0; } -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, + enum guc_log_buffer_type type) { enum guc_log_buffer_type i; size_t offset = PAGE_SIZE;/* for the log_buffer_states */ @@ -234,7 +390,7 @@ size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type) for (i = GUC_DEBUG_LOG_BUFFER; i < GUC_MAX_LOG_BUFFER; ++i) { if (i == type) break; - offset += intel_guc_get_log_buffer_size(i); + offset += intel_guc_get_log_buffer_size(log, i); } return offset; @@ -285,7 +441,7 @@ static void _guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log) */ memcpy(&log_buf_state_local, log_buf_state, sizeof(struct guc_log_buffer_state)); - buffer_size = intel_guc_get_log_buffer_size(type); + buffer_size = intel_guc_get_log_buffer_size(log, type); read_offset = log_buf_state_local.read_ptr; write_offset = log_buf_state_local.sampled_write_ptr; full_cnt = log_buf_state_local.buffer_full_cnt; @@ -400,7 +556,7 @@ static int guc_log_relay_create(struct intel_guc_log *log) * Keep the size of sub buffers same as shared log buffer * but GuC log-events excludes the error-state-capture logs */ - subbuf_size = log->vma->size - CAPTURE_BUFFER_SIZE; + subbuf_size = log->vma->size - intel_guc_log_section_size_capture(log); /* * Store up to 8 snapshots, which is large enough to buffer sufficient diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h index dc9715411d626..02127703be809 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h @@ -15,20 +15,6 @@ struct intel_guc; -#if defined(CONFIG_DRM_I915_DEBUG_GUC) -#define CRASH_BUFFER_SIZE SZ_2M -#define DEBUG_BUFFER_SIZE SZ_16M -#define CAPTURE_BUFFER_SIZE SZ_4M -#elif defined(CONFIG_DRM_I915_DEBUG_GEM) -#define CRASH_BUFFER_SIZE SZ_1M -#define DEBUG_BUFFER_SIZE SZ_2M -#define CAPTURE_BUFFER_SIZE SZ_4M -#else -#define CRASH_BUFFER_SIZE SZ_8K -#define DEBUG_BUFFER_SIZE SZ_64K -#define CAPTURE_BUFFER_SIZE SZ_2M -#endif - /* * While we're using plain log level in i915, GuC controls are much more... * "elaborate"? We have a couple of bits for verbosity, separate bit for actual @@ -46,10 +32,30 @@ struct intel_guc; #define GUC_VERBOSITY_TO_LOG_LEVEL(x) ((x) + 2) #define GUC_LOG_LEVEL_MAX GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX) +enum { + GUC_LOG_SECTIONS_CRASH, + GUC_LOG_SECTIONS_DEBUG, + GUC_LOG_SECTIONS_CAPTURE, + GUC_LOG_SECTIONS_LIMIT +}; + struct intel_guc_log { u32 level; + + /* Allocation settings */ + struct { + s32 bytes; /* Size in bytes */ + s32 units; /* GuC API units - 1MB or 4KB */ + s32 count; /* Number of API units */ + u32 flag; /* GuC API units flag */ + } sizes[GUC_LOG_SECTIONS_LIMIT]; + bool sizes_initialised; + + /* Combined buffer allocation */ struct i915_vma *vma; void *buf_addr; + + /* RelayFS support */ struct { bool buf_in_use; bool started; @@ -58,6 +64,7 @@ struct intel_guc_log { struct mutex lock; u32 full_count; } relay; + /* logging related stats */ struct { u32 sampled_overflow; @@ -69,8 +76,9 @@ struct intel_guc_log { void intel_guc_log_init_early(struct intel_guc_log *log); bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, enum guc_log_buffer_type type, unsigned int full_cnt); -unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type type); -size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type); +unsigned int intel_guc_get_log_buffer_size(struct intel_guc_log *log, + enum guc_log_buffer_type type); +size_t intel_guc_get_log_buffer_offset(struct intel_guc_log *log, enum guc_log_buffer_type type); int intel_guc_log_create(struct intel_guc_log *log); void intel_guc_log_destroy(struct intel_guc_log *log); @@ -92,4 +100,6 @@ void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p); int intel_guc_log_dump(struct intel_guc_log *log, struct drm_printer *p, bool dump_load_err); +u32 intel_guc_log_section_size_capture(struct intel_guc_log *log); + #endif diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 6fc475a5db615..06ca5b8221118 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -171,6 +171,18 @@ i915_param_named(guc_log_level, int, 0400, "GuC firmware logging level. Requires GuC to be loaded. " "(-1=auto [default], 0=disable, 1..4=enable with verbosity min..max)"); +i915_param_named(guc_log_size_crash, int, 0400, + "GuC firmware logging buffer size for crash dumps (in MB)" + "(-1=auto [default], NB: max = 4, other restrictions apply)"); + +i915_param_named(guc_log_size_debug, int, 0400, + "GuC firmware logging buffer size for debug logs (in MB)" + "(-1=auto [default], NB: max = 16, other restrictions apply)"); + +i915_param_named(guc_log_size_capture, int, 0400, + "GuC error capture register dump buffer size (in MB)" + "(-1=auto [default], NB: max = 4, other restrictions apply)"); + i915_param_named_unsafe(guc_firmware_path, charp, 0400, "GuC firmware path to use instead of the default one"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 2733cb6cfe094..f684d1ab87078 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -61,6 +61,9 @@ struct drm_printer; param(int, invert_brightness, 0, 0600) \ param(int, enable_guc, -1, 0400) \ param(int, guc_log_level, -1, 0400) \ + param(int, guc_log_size_crash, -1, 0400) \ + param(int, guc_log_size_debug, -1, 0400) \ + param(int, guc_log_size_capture, -1, 0400) \ param(char *, guc_firmware_path, NULL, 0400) \ param(char *, huc_firmware_path, NULL, 0400) \ param(char *, dmc_firmware_path, NULL, 0400) \