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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Sep 23, 2024 at 06:06:51PM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> 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.

yeap, please let's keep it simply as unsigned int for the flags of the
domains which needs to be returned.

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

The caller doesn't need to know. But the relative put should only put
back what it was taken. Hence the flags.

But no need for any wakeref tracking or magic... it should be simple.

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

I agree wit this as well. Always warn on non attended request and simplify
the callers.

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