Re: [Intel-gfx] [PATCH 1/4] drm/i915/huc: Use i915_probe_error to report early CTB failures
On 12.10.2021 18:16, Jani Nikula wrote: > On Mon, 11 Oct 2021, Matthew Brost wrote: >> On Mon, Oct 11, 2021 at 08:51:03PM +0530, Thanneeru Srinivasulu wrote: >>> Replace DRM_ERROR with CT_PROBE_ERROR to report early CTB failures. >>> >>> Signed-off-by: Thanneeru Srinivasulu >> >> Reviewed-by: Matthew Brost >> >>> --- >>> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >>> index 0a3504bc0b61..83764db0fd6d 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >>> @@ -191,8 +191,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, >>> u32 type, >>> err = guc_action_register_ct_buffer(ct_to_guc(ct), type, >>> desc_addr, buff_addr, size); >>> if (unlikely(err)) >>> - CT_ERROR(ct, "Failed to register %s buffer (%pe)\n", >>> -guc_ct_buffer_type_to_str(type), ERR_PTR(err)); >>> + CT_PROBE_ERROR(ct, "Failed to register %s buffer (%pe)\n", >>> + guc_ct_buffer_type_to_str(type), ERR_PTR(err)); > > Please tell me why we are adding not just i915-specific logging helpers, > but file specific ones? > > To be honest I'd like to see all of the CT_ERROR, CT_DEBUG, > CT_PROBE_ERROR macros just gone. the reason for CT_DEBUG is that it can be quite noisy so we must have an easy option to compile it out on non-debug configs, can't just replace that helper with drm_dbg or i915_dbg (that we don't have) as it will be available likely on I915_DEBUG config, while we want more fine control. use of file (or component) level helpers allows us to simplify the code (no need to repeat long i915->drm lookup from component pointer) and we may provide common prefix and/or classification of the messages. extra bonus, especially useful after introduction of multi-gt support, will be possibility of augmenting message to include gt identifier, without the need to update all existing places if they were using i915- or drm- level functions directly. for this last feature, likely "gt" specific intel_gt_err|probe_err|dbg helpers will do the job as well, so if someone introduce them, I'm happy to convert CT_ERROR calls to these new helpers if really really needed. -Michal > > > BR, > Jani. > > >>> return err; >>> } >>> >>> -- >>> 2.25.1 >>> >
Re: [Intel-gfx] [PATCH 1/4] drm/i915/huc: Use i915_probe_error to report early CTB failures
On Tue, 12 Oct 2021, Jani Nikula wrote: > On Mon, 11 Oct 2021, Matthew Brost wrote: >> On Mon, Oct 11, 2021 at 08:51:03PM +0530, Thanneeru Srinivasulu wrote: >>> Replace DRM_ERROR with CT_PROBE_ERROR to report early CTB failures. >>> >>> Signed-off-by: Thanneeru Srinivasulu >> >> Reviewed-by: Matthew Brost >> >>> --- >>> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >>> index 0a3504bc0b61..83764db0fd6d 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >>> @@ -191,8 +191,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, >>> u32 type, >>> err = guc_action_register_ct_buffer(ct_to_guc(ct), type, >>> desc_addr, buff_addr, size); >>> if (unlikely(err)) >>> - CT_ERROR(ct, "Failed to register %s buffer (%pe)\n", >>> -guc_ct_buffer_type_to_str(type), ERR_PTR(err)); >>> + CT_PROBE_ERROR(ct, "Failed to register %s buffer (%pe)\n", >>> + guc_ct_buffer_type_to_str(type), ERR_PTR(err)); > > Please tell me why we are adding not just i915-specific logging helpers, > but file specific ones? > > To be honest I'd like to see all of the CT_ERROR, CT_DEBUG, > CT_PROBE_ERROR macros just gone. For that matter, why has GEM_BUG_ON spread like a disease to display and intel_uncore.c too? BR, Jani. > > > BR, > Jani. > > >>> return err; >>> } >>> >>> -- >>> 2.25.1 >>> -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 1/4] drm/i915/huc: Use i915_probe_error to report early CTB failures
On Mon, 11 Oct 2021, Matthew Brost wrote: > On Mon, Oct 11, 2021 at 08:51:03PM +0530, Thanneeru Srinivasulu wrote: >> Replace DRM_ERROR with CT_PROBE_ERROR to report early CTB failures. >> >> Signed-off-by: Thanneeru Srinivasulu > > Reviewed-by: Matthew Brost > >> --- >> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> index 0a3504bc0b61..83764db0fd6d 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> @@ -191,8 +191,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, >> u32 type, >> err = guc_action_register_ct_buffer(ct_to_guc(ct), type, >> desc_addr, buff_addr, size); >> if (unlikely(err)) >> -CT_ERROR(ct, "Failed to register %s buffer (%pe)\n", >> - guc_ct_buffer_type_to_str(type), ERR_PTR(err)); >> +CT_PROBE_ERROR(ct, "Failed to register %s buffer (%pe)\n", >> + guc_ct_buffer_type_to_str(type), ERR_PTR(err)); Please tell me why we are adding not just i915-specific logging helpers, but file specific ones? To be honest I'd like to see all of the CT_ERROR, CT_DEBUG, CT_PROBE_ERROR macros just gone. BR, Jani. >> return err; >> } >> >> -- >> 2.25.1 >> -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 1/4] drm/i915/huc: Use i915_probe_error to report early CTB failures
On Mon, Oct 11, 2021 at 08:51:03PM +0530, Thanneeru Srinivasulu wrote: > Replace DRM_ERROR with CT_PROBE_ERROR to report early CTB failures. > > Signed-off-by: Thanneeru Srinivasulu Reviewed-by: Matthew Brost > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > index 0a3504bc0b61..83764db0fd6d 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > @@ -191,8 +191,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, > u32 type, > err = guc_action_register_ct_buffer(ct_to_guc(ct), type, > desc_addr, buff_addr, size); > if (unlikely(err)) > - CT_ERROR(ct, "Failed to register %s buffer (%pe)\n", > - guc_ct_buffer_type_to_str(type), ERR_PTR(err)); > + CT_PROBE_ERROR(ct, "Failed to register %s buffer (%pe)\n", > +guc_ct_buffer_type_to_str(type), ERR_PTR(err)); > return err; > } > > -- > 2.25.1 >
[PATCH 1/4] drm/i915/huc: Use i915_probe_error to report early CTB failures
Replace DRM_ERROR with CT_PROBE_ERROR to report early CTB failures. Signed-off-by: Thanneeru Srinivasulu --- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 0a3504bc0b61..83764db0fd6d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -191,8 +191,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, err = guc_action_register_ct_buffer(ct_to_guc(ct), type, desc_addr, buff_addr, size); if (unlikely(err)) - CT_ERROR(ct, "Failed to register %s buffer (%pe)\n", -guc_ct_buffer_type_to_str(type), ERR_PTR(err)); + CT_PROBE_ERROR(ct, "Failed to register %s buffer (%pe)\n", + guc_ct_buffer_type_to_str(type), ERR_PTR(err)); return err; } -- 2.25.1
