RE: [Intel-gfx] [PATCH] drm/i915/guc/slpc: Use i915_probe_error instead of drm_err

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

> -----Original Message-----
> From: Belgaumkar, Vinay <vinay.belgaumkar@intel.com>
> Sent: Thursday, April 14, 2022 8:38 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/guc/slpc: Use i915_probe_error
> instead of drm_err
> 
> 
> On 4/13/2022 11:41 PM, Anshuman Gupta wrote:
> > On 2022-04-13 at 04:18:52 +0530, Vinay Belgaumkar wrote:
> >> This will ensure we don't have false positives when we run error
> >> injection tests.
> >>
> >> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 42 ++++++++++-----------
> >>   1 file changed, 21 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> >> index b170238aa15c..639de3c10545 100644
> >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> >> @@ -152,8 +152,8 @@ static int slpc_query_task_state(struct
> >> intel_guc_slpc *slpc)
> >>
> >>   	ret = guc_action_slpc_query(guc, offset);
> >>   	if (unlikely(ret))
> > As commit logs describe, this code patch can hit, when we run error injection
> test.
> > Do we need unlikely() here?
> > Br,
> > Anshuman Gupta.
> 
> I think we still need the unlikely(). Majority of the time, we still need the
> compiler optimization.
> 
> Only in the rare case of running the error injection test will it not be needed.
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> 
> Thanks,
> 
> Vinay.
> 
> >> -		drm_err(&i915->drm, "Failed to query task state (%pe)\n",
> >> -			ERR_PTR(ret));
> >> +		i915_probe_error(i915, "Failed to query task state (%pe)\n",
> >> +				 ERR_PTR(ret));
> >>
> >>   	drm_clflush_virt_range(slpc->vaddr, SLPC_PAGE_SIZE_BYTES);
> >>
> >> @@ -170,8 +170,8 @@ static int slpc_set_param(struct intel_guc_slpc
> >> *slpc, u8 id, u32 value)
> >>
> >>   	ret = guc_action_slpc_set_param(guc, id, value);
> >>   	if (ret)
> >> -		drm_err(&i915->drm, "Failed to set param %d to %u (%pe)\n",
> >> -			id, value, ERR_PTR(ret));
> >> +		i915_probe_error(i915, "Failed to set param %d to %u (%pe)\n",
> >> +				 id, value, ERR_PTR(ret));
> >>
> >>   	return ret;
> >>   }
> >> @@ -211,8 +211,8 @@ static int slpc_force_min_freq(struct intel_guc_slpc
> *slpc, u32 freq)
> >>
> SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> >>   				     freq);
> >>   		if (ret)
> >> -			drm_err(&i915->drm, "Unable to force min freq to %u:
> %d",
> >> -				freq, ret);
> >> +			i915_probe_error(i915, "Unable to force min freq to
> %u: %d",
> >> +					 freq, ret);
> >>   	}
> >>
> >>   	return ret;
> >> @@ -247,9 +247,9 @@ int intel_guc_slpc_init(struct intel_guc_slpc
> >> *slpc)
> >>
> >>   	err = intel_guc_allocate_and_map_vma(guc, size, &slpc->vma, (void
> **)&slpc->vaddr);
> >>   	if (unlikely(err)) {
> >> -		drm_err(&i915->drm,
> >> -			"Failed to allocate SLPC struct (err=%pe)\n",
> >> -			ERR_PTR(err));
> >> +		i915_probe_error(i915,
> >> +				 "Failed to allocate SLPC struct (err=%pe)\n",
> >> +				 ERR_PTR(err));
> >>   		return err;
> >>   	}
> >>
> >> @@ -316,15 +316,15 @@ static int slpc_reset(struct intel_guc_slpc *slpc)
> >>   	ret = guc_action_slpc_reset(guc, offset);
> >>
> >>   	if (unlikely(ret < 0)) {
> >> -		drm_err(&i915->drm, "SLPC reset action failed (%pe)\n",
> >> -			ERR_PTR(ret));
> >> +		i915_probe_error(i915, "SLPC reset action failed (%pe)\n",
> >> +				 ERR_PTR(ret));
> >>   		return ret;
> >>   	}
> >>
> >>   	if (!ret) {
> >>   		if (wait_for(slpc_is_running(slpc), SLPC_RESET_TIMEOUT_MS)) {
> >> -			drm_err(&i915->drm, "SLPC not enabled! State = %s\n",
> >> -				slpc_get_state_string(slpc));
> >> +			i915_probe_error(i915, "SLPC not enabled! State =
> %s\n",
> >> +					 slpc_get_state_string(slpc));
> >>   			return -EIO;
> >>   		}
> >>   	}
> >> @@ -616,8 +616,8 @@ int intel_guc_slpc_enable(struct intel_guc_slpc
> >> *slpc)
> >>
> >>   	ret = slpc_reset(slpc);
> >>   	if (unlikely(ret < 0)) {
> >> -		drm_err(&i915->drm, "SLPC Reset event returned (%pe)\n",
> >> -			ERR_PTR(ret));
> >> +		i915_probe_error(i915, "SLPC Reset event returned (%pe)\n",
> >> +				 ERR_PTR(ret));
> >>   		return ret;
> >>   	}
> >>
> >> @@ -632,24 +632,24 @@ int intel_guc_slpc_enable(struct intel_guc_slpc
> *slpc)
> >>   	/* Ignore efficient freq and set min to platform min */
> >>   	ret = slpc_ignore_eff_freq(slpc, true);
> >>   	if (unlikely(ret)) {
> >> -		drm_err(&i915->drm, "Failed to set SLPC min to RPn (%pe)\n",
> >> -			ERR_PTR(ret));
> >> +		i915_probe_error(i915, "Failed to set SLPC min to RPn (%pe)\n",
> >> +				 ERR_PTR(ret));
> >>   		return ret;
> >>   	}
> >>
> >>   	/* Set SLPC max limit to RP0 */
> >>   	ret = slpc_use_fused_rp0(slpc);
> >>   	if (unlikely(ret)) {
> >> -		drm_err(&i915->drm, "Failed to set SLPC max to RP0 (%pe)\n",
> >> -			ERR_PTR(ret));
> >> +		i915_probe_error(i915, "Failed to set SLPC max to RP0
> (%pe)\n",
> >> +				 ERR_PTR(ret));
> >>   		return ret;
> >>   	}
> >>
> >>   	/* Revert SLPC min/max to softlimits if necessary */
> >>   	ret = slpc_set_softlimits(slpc);
> >>   	if (unlikely(ret)) {
> >> -		drm_err(&i915->drm, "Failed to set SLPC softlimits (%pe)\n",
> >> -			ERR_PTR(ret));
> >> +		i915_probe_error(i915, "Failed to set SLPC softlimits (%pe)\n",
> >> +				 ERR_PTR(ret));
> >>   		return ret;
> >>   	}
> >>
> >> --
> >> 2.35.1
> >>