Re: [PATCH v2 01/23] drm/xe: Error handling in xe_force_wake_get()

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 19-09-2024 17:06, Jani Nikula wrote:
> On Thu, 19 Sep 2024, "Nilawar, Badal" <badal.nilawar@intel.com> wrote:
>> On 18-09-2024 12:49, Jani Nikula wrote:
>>> On Wed, 18 Sep 2024, "Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com> wrote:
>>>> On 18-09-2024 00:20, Matthew Brost wrote:
>>>>> On Tue, Sep 17, 2024 at 11:18:47AM +0530, Nilawar, Badal wrote:
>>>>>> On 13-09-2024 18:47, Ghimiray, Himal Prasad wrote:
>>>>>>> Agreed implementation/usage will be same, will use explicit type for
>>>>>>> clarity.
>>>>>>> IMO typedef unsigned int xe_wakeref_t is sufficient instead of
>>>>>>> typedef unsigned long xe_wakeref_t;
>>>>>>
>>>>>> I agree with this.
>>>>>>
>>>>>
>>>>> What? Really? I thought it was pretty clear rule in kernel programing
>>>>> not use typedefs [1]. Reading through conditions acceptable and I don't
>>>>> use anything applies to this series, maybe a) applies but not really
>>>>> convinced. The example in a) is a pte_t which can likely change based on
>>>>> platform target whereas here we only have one target and see no reason
>>>>> this needs to be opaque.
>>>>>
>>>>> Matt
>>>>>
>>>>> [1] https://www.kernel.org/doc/html/v4.14/process/coding-style.html#typedefs
>>>>
>>>>
>>>> While running checkpatch on my changes, patchwork had also issued a
>>>> WARNING: NEW_TYPEDEFS: do not add new typedefs. I reviewed the usage in
>>>> the Linux kernel tree and found it used in many places, which led me to
>>>> assume it was safe. I now realize that I should have been more careful
>>>> in understanding the context of its usage and referred to the kernel
>>>> coding guidelines. This was an oversight on my part.
>>>>
>>>> Since this doesn’t impact the CI or runtime, I will avoid reverting to
>>>> unsigned int immediately and will hold off until I receive the other
>>>> review comments. I will incorporate the changes to revert it in
>>>> subsequent versions while also addressing the other review comments.
>>>> Thank you for bringing this to the attention.
>>>
>>> If you end up replicating intel_wakeref_t from i915, and go as deep as
>>> the rabbit hole goes, you'll realize intel_wakeref_t is a pointer
>>> disguised as an unsigned long. It's a struct ref_tracker * when you have
>>> certain configs enabled.
>>>
>>> You could just use struct ref_tracker * everywhere. It's an opaque type
>>> to start with.
>>
>> The original idea of using typedef for the fw return mask was for the
>> sake of clarity. However, Matt B pointed that the use of typedef in this
>> instance is not in accordance with the Linux kernel coding standards.
>> Additionally, I agree with Matt B that there is no need for the fw
>> return mask to be opaque; therefore, it is preferable to maintain the
>> use of unsigned int.
> 
> I'm not sure it's a hot idea to explicitly state that the return value
> is a domain mask. The callers shouldn't need to care, should they?
> 
> For example:
> 
>   +	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>   +	if (fw_ref != XE_FORCEWAKE_ALL) {
> 
> Under what conditions do you expect this to happen? Shouldn't

If any of the requested domain is not refcounted (not awake) above 
condition will happen.

> xe_force_wake_get() flag cases where it couldn't deliver what you asked?

Internally xe_force_wake_get prints drm_notice when requested domain set 
ack times out. In the driver currently caller is sometime returning 
there is domain ack failure.

usage: where XE_WARN_ON(fw_ref != XE_FORCEWAKE_ALL) is used, which looks 
redundant to me it can be moved inside xe_force_wake_get.

case a)
	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
	XE_WARN_ON(fw_ref != XE_FORCEWAKE_ALL)

	//Here caller doesn't bother about all the domains are awake and 	continues
	func_b()
	
	xe_force_wake_put((gt_to_fw(gt), fw_ref); // Puts only domains awake by 
xe_force_wake_get.

case b)
	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
	if(fw_ref != XE_FORCEWAKE_ALL) {
		xe_force_wake_put((gt_to_fw(gt), fw_ref); // Puts only domains awake 
by xe_force_wake_get.
		return -ETIMEDOUT;
	}

	func_b()
	
	xe_force_wake_put((gt_to_fw(gt), fw_ref);


As of now driver have both usages and this patch series caters both.
	
Regards,
Badal

> 
> BR,
> Jani.
> 
>