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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 19-09-2024 18:02, Nilawar, Badal wrote:
> 
> 
> 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.

Agreed, the driver should warn, in case of domain ack timeout failure, 
irrespective of whether user wants to continue or not. Will move the 
check inside the forcewake_get itself. Similar to what _put will do in
[v4,02/23] drm/xe: Modify xe_force_wake_put to handle _get returned mask


> 
> 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.
>>
>>