Re: [Intel-gfx] [PATCH 1/4] drm/i915/huc: Use i915_probe_error to report early CTB failures

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 12.10.2021 18:16, Jani Nikula wrote:
> On Mon, 11 Oct 2021, Matthew Brost <matthew.brost@intel.com> 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 <thanneeru.srinivasulu@intel.com>
>>
>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>>
>>> ---
>>>  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
>>>
>