On Wed, Apr 24, 2024 at 10:42:59AM +0530, Aravind Iddamsetty wrote:
> 
> On 24/04/24 05:19, Michał Winiarski wrote:
> > On Mon, Apr 22, 2024 at 12:27:56PM +0530, Aravind Iddamsetty wrote:
> >> PCI subsystem provides callbacks to inform the driver about a request to
> >> do function level reset by user, initiated by writing to sysfs entry
> >> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
> >> without the need to do unbind and rebind as the driver needs to
> >> reinitialize the device afresh post FLR.
> >>
> >> v2:
> >> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
> >> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
> >>
> >> v3: declare xe_pci_err_handlers as static(Michal)
> >>
> >> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> >> Cc: Lucas De Marchi <lucas.demar...@intel.com>
> >> Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
> >>
> >> Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> >> Signed-off-by: Aravind Iddamsetty <aravind.iddamse...@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/Makefile          |  1 +
> >>  drivers/gpu/drm/xe/xe_device_types.h |  3 +
> >>  drivers/gpu/drm/xe/xe_guc_pc.c       |  4 ++
> >>  drivers/gpu/drm/xe/xe_pci.c          |  9 ++-
> >>  drivers/gpu/drm/xe/xe_pci.h          |  2 +
> >>  drivers/gpu/drm/xe/xe_pci_err.c      | 88 ++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/xe/xe_pci_err.h      | 13 ++++
> >>  7 files changed, 119 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
> >>  create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
> >>
> >> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> >> index 8bc62bfbc679..693971a1fac0 100644
> >> --- a/drivers/gpu/drm/xe/Makefile
> >> +++ b/drivers/gpu/drm/xe/Makefile
> >> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
> >>    xe_module.o \
> >>    xe_pat.o \
> >>    xe_pci.o \
> >> +  xe_pci_err.o \
> >>    xe_pcode.o \
> >>    xe_pm.o \
> >>    xe_preempt_fence.o \
> >> diff --git a/drivers/gpu/drm/xe/xe_device_types.h 
> >> b/drivers/gpu/drm/xe/xe_device_types.h
> >> index 0a66555229e9..8c749b378a92 100644
> >> --- a/drivers/gpu/drm/xe/xe_device_types.h
> >> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> >> @@ -465,6 +465,9 @@ struct xe_device {
> >>    /** @pci_state: PCI state of device */
> >>    struct pci_saved_state *pci_state;
> >>  
> >> +  /** @pci_device_is_reset: device went through PCIe FLR */
> >> +  bool pci_device_is_reset;
> >> +
> >>    /* private: */
> >>  
> >>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> >> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c 
> >> b/drivers/gpu/drm/xe/xe_guc_pc.c
> >> index 509649d0e65e..efba0fbe2f5c 100644
> >> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> >> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> >> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, 
> >> void *arg)
> >>            return;
> >>    }
> >>  
> >> +  /* We already have done this before going through a reset, so skip here 
> >> */
> >> +  if (xe->pci_device_is_reset)
> >> +          return;
> >> +
> >>    XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
> >>    XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
> >>    XE_WARN_ON(xe_guc_pc_stop(pc));
> >> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> >> index a62300990e19..b5a582afc9e7 100644
> >> --- a/drivers/gpu/drm/xe/xe_pci.c
> >> +++ b/drivers/gpu/drm/xe/xe_pci.c
> >> @@ -23,6 +23,7 @@
> >>  #include "xe_macros.h"
> >>  #include "xe_mmio.h"
> >>  #include "xe_module.h"
> >> +#include "xe_pci_err.h"
> >>  #include "xe_pci_types.h"
> >>  #include "xe_pm.h"
> >>  #include "xe_sriov.h"
> >> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
> >>    pci_set_drvdata(pdev, NULL);
> >>  }
> >>  
> >> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id 
> >> *ent)
> >> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >>  {
> >>    const struct xe_device_desc *desc = (const void *)ent->driver_data;
> >>    const struct xe_subplatform_desc *subplatform_desc;
> >> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
> >>  };
> >>  #endif
> >>  
> >> +static const struct pci_error_handlers xe_pci_err_handlers = {
> >> +  .reset_prepare = xe_pci_reset_prepare,
> >> +  .reset_done = xe_pci_reset_done,
> >> +};
> >> +
> >>  static struct pci_driver xe_pci_driver = {
> >>    .name = DRIVER_NAME,
> >>    .id_table = pciidlist,
> >> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
> >>  #ifdef CONFIG_PM_SLEEP
> >>    .driver.pm = &xe_pm_ops,
> >>  #endif
> >> +  .err_handler = &xe_pci_err_handlers,
> >>  };
> >>  
> >>  int xe_register_pci_driver(void)
> >> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
> >> index 73b90a430d1f..9faf5380a09e 100644
> >> --- a/drivers/gpu/drm/xe/xe_pci.h
> >> +++ b/drivers/gpu/drm/xe/xe_pci.h
> >> @@ -7,8 +7,10 @@
> >>  #define _XE_PCI_H_
> >>  
> >>  struct pci_dev;
> >> +struct pci_device_id;
> >>  
> >>  int xe_register_pci_driver(void);
> >>  void xe_unregister_pci_driver(void);
> >>  void xe_load_pci_state(struct pci_dev *pdev);
> >> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> >>  #endif
> >> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c 
> >> b/drivers/gpu/drm/xe/xe_pci_err.c
> >> new file mode 100644
> >> index 000000000000..5306925ea2fa
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
> >> @@ -0,0 +1,88 @@
> >> +// SPDX-License-Identifier: MIT
> >> +/*
> >> + * Copyright © 2024 Intel Corporation
> >> + */
> >> +
> >> +#include <linux/pci.h>
> >> +#include <drm/drm_drv.h>
> >> +
> >> +#include "xe_device.h"
> >> +#include "xe_gt.h"
> >> +#include "xe_gt_printk.h"
> >> +#include "xe_pci.h"
> >> +#include "xe_pci_err.h"
> >> +#include "xe_pm.h"
> >> +#include "xe_uc.h"
> >> +
> >> +/**
> >> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
> >> + * via /sys/bus/pci/devices/.../reset.
> >> + * @pdev: PCI device struct
> >> + */
> >> +void xe_pci_reset_prepare(struct pci_dev *pdev)
> >> +{
> >> +  struct xe_device *xe = pci_get_drvdata(pdev);
> >> +  struct xe_gt *gt;
> >> +  int id, err;
> >> +
> >> +  pci_warn(pdev, "preparing for PCIe reset\n");
> >> +
> >> +  drm_warn(&xe->drm, "removing device access to userspace\n");
> > Warn? Shouldn't it be info?
> I believe warn is appropriate as this is not a usual path the users apps 
> expect
> to hit and as they loose device connection.

It's an expected path after issuing a reset via /sys/bus/pci/devices/.../reset.
DRM device disappearing - yeah, that's probably not expected, fully
agree on that.

> >
> >> +  drm_dev_unplug(&xe->drm);
> >> +
> >> +  xe_pm_runtime_get(xe);
> >> +  /* idle the GTs */
> >> +  for_each_gt(gt, xe, id) {
> >> +          err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> >> +          if (err)
> >> +                  goto reset;
> >> +          xe_uc_reset_prepare(&gt->uc);
> >> +          xe_gt_idle(gt);
> >> +          err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> >> +          XE_WARN_ON(err);
> >> +  }
> >> +  xe_pm_runtime_put(xe);
> >> +
> >> +reset:
> >> +  pci_disable_device(pdev);
> >> +}
> >> +
> >> +/**
> >> + * xe_pci_reset_done - Called when PCIe reset is done.
> >> + * @pdev: PCI device struct
> >> + */
> >> +void xe_pci_reset_done(struct pci_dev *pdev)
> >> +{
> >> +  const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, 
> >> pdev);
> >> +  struct xe_device *xe = pci_get_drvdata(pdev);
> >> +
> >> +  dev_info(&pdev->dev,
> >> +           "device went through PCIe reset, reenabling the device\n");
> >> +
> >> +  if (pci_enable_device(pdev)) {
> >> +          dev_err(&pdev->dev,
> >> +                  "Cannot re-enable PCI device after reset\n");
> >> +          return;
> >> +  }
> >> +  pci_set_master(pdev);
> >> +  xe_load_pci_state(pdev);
> >> +
> >> +  xe->pci_device_is_reset = true;
> >> +  /*
> >> +   * We want to completely clean the driver and even destroy
> >> +   * the xe private data and reinitialize afresh similar to
> >> +   * probe
> >> +   */
> >> +  pdev->driver->remove(pdev);
> > Do we really want to do that?
> > First of all, that fairly unusual - none of the other PCI drivers makes
> > changes in the device/driver model during reset, which makes me wonder
> > if this is really what the user expects.
> > I would expect that the device is reset, but the driver is not unloaded
> > and previously created FDs are still valid.
> We cannot continue to work with previous instance of driver as post
> reset GuC , LMEM and most of the soc is reset, so  we need to reinitialize
> device afresh like in driver probe, hence we remove to clean the previous
> stale driver state and re-probe again.

GuC, LMEM and most of the soc is reset when we're doing S3 / S4, and
yet, we're not removing the driver in this scenario.

> 
> All applications are expected to reopen the device handles afresh.

Current UMDs are not handling that case.

> 
> >
> > Note that messing with the driver model at reset also makes it difficult
> > to do a proper cleanup if SR-IOV is enabled, as PCI-core expects drivers
> > to remove VF devices during driver->remove.
> > Removal of said devices depends on pci_cfg_access_lock, which is already
> > held for the duration of the reset (wh
> I haven't verified SRIOV use case, i believe we can take this up as next step.
> Also, since this is not an automatic reset but a user/admin initiated i 
> believe
> the onus can be on admin to close all VFs before initiating a reset.

That's not the contract for reset. It's not a "user/admin" interface,
there doesn't necessarily have to be an "interactive" user interfacing
with it through shell.
It's just a regular stable sysfs uAPI - it could very well be called by
application as well.

> > ich includes calling reset_done),
> > which will cause a deadlock.
> >
> > In current form, it also causes WARN if there are open FDs to DRM
> > device during reset.
> I did verify using igt@device_reset@reset-bound which has a open connection 
> haven't seen this error
> but will re verify again but again certain warns are expected but driver 
> reload should be successful.
> similarly here as well we can expect the admin to close any applications 
> using the device before
> initiating a reset.

Can we expect this?
If we can - why are you stating that "all applications are expected to
reopen the device handles afresh"?
I believe that the expectation for reset is that it behaves simiarly to
the PM flows, which is mostly transparent to the users.

Thanks,
-Michał

> 
> Thanks,
> 
> Aravind.
> >
> > [29357.277699] sysfs: cannot create duplicate filename 
> > '/devices/pci0000:00/0000:00:02.0/tile0'                                    
> >                                                                             
> >             01:06:58 [142/47263]
> > [29357.286158] CPU: 7 PID: 3479 Comm: bash Not tainted 6.9.0-rc5-xe+ #78
> > [29357.305870] Call Trace:
> > [29357.308342]  <TASK>
> > [29357.310453]  dump_stack_lvl+0x139/0x1d0
> > [29357.314305]  ? __pfx_dump_stack_lvl+0x10/0x10
> > [29357.318680]  ? __pfx__printk+0x10/0x10
> > [29357.322450]  ? kmalloc_trace+0x1c1/0x3a0
> > [29357.326394]  ? sysfs_create_dir_ns+0x162/0x210
> > [29357.330861]  sysfs_create_dir_ns+0x195/0x210
> > [29357.335154]  ? __pfx_sysfs_create_dir_ns+0x10/0x10
> > [29357.339965]  ? strcmp+0x2f/0x60
> > [29357.343134]  kobject_add_internal+0x2af/0x600
> > [29357.347517]  kobject_add+0xfb/0x190
> > [29357.351028]  ? __link_object+0x1c7/0x280
> > [29357.354976]  ? __pfx_kobject_add+0x10/0x10
> > [29357.359093]  ? __create_object+0x62/0x140
> > [29357.363111]  ? rcu_is_watching+0x20/0x50
> > [29357.367055]  ? kmalloc_trace+0x1c1/0x3a0
> > [29357.370998]  ? xe_tile_sysfs_init+0x4b/0x100 [xe]
> > [29357.376016]  xe_tile_sysfs_init+0x91/0x100 [xe]
> > [29357.380868]  xe_tile_init_noalloc+0xaf/0xc0 [xe]
> > [29357.385936]  xe_device_probe+0x60c/0x750 [xe]
> > [29357.390674]  ? __asan_memcpy+0x40/0x70
> > [29357.394461]  ? __drmm_add_action+0x1ac/0x210 [drm]
> > [29357.399413]  xe_pci_probe+0x5dc/0x680 [xe]
> > [29357.403882]  pci_reset_function+0x108/0x140
> > [29357.408100]  reset_store+0xb5/0x120
> > [29357.411623]  ? __pfx_reset_store+0x10/0x10
> > [29357.415751]  ? __pfx_sysfs_kf_write+0x10/0x10
> > [29357.420149]  kernfs_fop_write_iter+0x1b8/0x260
> > [29357.424620]  vfs_write+0x5ce/0x660
> > [29357.428067]  ? __pfx_vfs_write+0x10/0x10
> > [29357.432028]  ? __rcu_read_unlock+0x60/0x90
> > [29357.436181]  ksys_write+0xe4/0x190
> > [29357.439612]  ? __pfx_ksys_write+0x10/0x10
> > [29357.443657]  do_syscall_64+0x7b/0x120
> > [29357.447348]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > [29357.452423] RIP: 0033:0x7f4f1ff14887
> > [29357.456030] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 
> > 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 
> > <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
> > [29357.474761] RSP: 002b:00007ffe54e95068 EFLAGS: 00000246 ORIG_RAX: 
> > 0000000000000001
> > [29357.482343] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 
> > 00007f4f1ff14887
> > [29357.489487] RDX: 0000000000000002 RSI: 0000559eb4076e30 RDI: 
> > 0000000000000001
> > [29357.496630] RBP: 0000559eb4076e30 R08: 0000000000000000 R09: 
> > 0000559eb4076e30
> > [29357.503775] R10: 0000000000000077 R11: 0000000000000246 R12: 
> > 0000000000000002
> > [29357.510947] R13: 00007f4f2001b780 R14: 00007f4f20017600 R15: 
> > 00007f4f20016a00
> > [29357.518174]  </TASK>
> > [29357.520539] kobject: kobject_add_internal failed for tile0 with -EEXIST, 
> > don't try to register things with the same name in the same directory.
> >
> > -Michał
> >
> >> +  if (pci_dev_msi_enabled(pdev))
> >> +          pci_free_irq_vectors(pdev);
> >> +
> >> +  devm_drm_dev_release_action(&xe->drm);
> >> +  pci_disable_device(pdev);
> >> +
> >> +  /*
> >> +   * if this fails the driver might be in a stale state, only option is
> >> +   * to unbind and rebind
> >> +   */
> >> +  xe_pci_probe(pdev, ent);
> >> +}
> >> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h 
> >> b/drivers/gpu/drm/xe/xe_pci_err.h
> >> new file mode 100644
> >> index 000000000000..95a4c8ce9cf1
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
> >> @@ -0,0 +1,13 @@
> >> +/* SPDX-License-Identifier: MIT */
> >> +/*
> >> + * Copyright © 2024 Intel Corporation
> >> + */
> >> +
> >> +#ifndef _XE_PCI_ERR_H_
> >> +#define _XE_PCI_ERR_H_
> >> +
> >> +struct pci_dev;
> >> +
> >> +void xe_pci_reset_prepare(struct pci_dev *pdev);
> >> +void xe_pci_reset_done(struct pci_dev *pdev);
> >> +#endif
> >> -- 
> >> 2.25.1
> >>

Reply via email to