RE: [PATCH] drm/amdgpu: enable separate timeout setting for every ring type V3

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Fixed in V4.

Regards,
Evan
> -----Original Message-----
> From: Alex Deucher <alexdeucher@xxxxxxxxx>
> Sent: 2019年4月30日 9:34
> To: Quan, Evan <Evan.Quan@xxxxxxx>
> Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Lou, Wentao <Wentao.Lou@xxxxxxx>;
> Daenzer, Michel <Michel.Daenzer@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>
> Subject: Re: [PATCH] drm/amdgpu: enable separate timeout setting for
> every ring type V3
> 
> [CAUTION: External Email]
> 
> On Mon, Apr 29, 2019 at 9:29 PM Quan, Evan <Evan.Quan@xxxxxxx> wrote:
> >
> > For old design, lockup_timeout affects non-compute IPs only(e.g.
> amdgpu.lockup_timeout=1000). Compute jobs always use
> MAX_SCHEDULE_TIMEOUT.
> > For the new design, I would like it be backward compatible. With [Non-
> Compute]:[Compute] format, "amdgpu.lockup_timeout=1000" setting has
> the same effect as old design.
> >
> > > * The format can be [Global] or [GFX,Compute,SDMA,Video].
> > I think using this format will break backward compatibility. Since
> "amdgpu.lockup_timeout=1000" setting will affect all IPs(both non-compute
> and compute).
> 
> Ah, ok.  I forgot we always used a different timeout for compute.  In that
> case, how about
> * The format can be [non-Compute] or [GFX,Compute,SDMA,Video].
> 
> Alex
> 
> >
> > Regards,
> > Evan
> > > -----Original Message-----
> > > From: Alex Deucher <alexdeucher@xxxxxxxxx>
> > > Sent: 2019年4月29日 22:27
> > > To: Quan, Evan <Evan.Quan@xxxxxxx>
> > > Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; Deucher, Alexander
> > > <Alexander.Deucher@xxxxxxx>; Lou, Wentao
> <Wentao.Lou@xxxxxxx>;
> > > Daenzer, Michel <Michel.Daenzer@xxxxxxx>; Koenig, Christian
> > > <Christian.Koenig@xxxxxxx>
> > > Subject: Re: [PATCH] drm/amdgpu: enable separate timeout setting for
> > > every ring type V3
> > >
> > > [CAUTION: External Email]
> > >
> > > On Mon, Apr 29, 2019 at 9:51 AM Evan Quan <evan.quan@xxxxxxx>
> wrote:
> > > >
> > > > Every ring type can have its own timeout setting.
> > > >
> > > >  - V2: update lockup_timeout parameter format and cosmetic fixes
> > > >  - V3: invalidate 0 and negative values
> > > >
> > > > Change-Id: I992f224f36bb33acd560162bffd2c3e987840a7e
> > > > Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
> > > > ---
> > > >  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  7 +-
> > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 17 ++--
> > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 90
> > > ++++++++++++++++++++--
> > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 35 +++++++--
> > > >  drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c      |  2 +-
> > > >  5 files changed, 132 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > > > index f6965b9403eb..c9b44b8c1969 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > > > @@ -118,7 +118,6 @@ extern int amdgpu_disp_priority;  extern int
> > > > amdgpu_hw_i2c;  extern int amdgpu_pcie_gen2;  extern int
> > > > amdgpu_msi; -extern int amdgpu_lockup_timeout;  extern int
> > > > amdgpu_dpm;  extern int amdgpu_fw_load_type;  extern int
> > > > amdgpu_aspm; @@ -428,6 +427,7 @@ struct amdgpu_fpriv {  };
> > > >
> > > >  int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv
> > > > **fpriv);
> > > > +int amdgpu_device_get_job_timeout_settings(struct amdgpu_device
> > > > +*adev);
> > > >
> > > >  int amdgpu_ib_get(struct amdgpu_device *adev, struct amdgpu_vm
> *vm,
> > > >                   unsigned size, struct amdgpu_ib *ib); @@ -1001,6
> > > > +1001,11 @@ struct amdgpu_device {
> > > >         struct work_struct              xgmi_reset_work;
> > > >
> > > >         bool                            in_baco_reset;
> > > > +
> > > > +       long                            gfx_timeout;
> > > > +       long                            sdma_timeout;
> > > > +       long                            video_timeout;
> > > > +       long                            compute_timeout;
> > > >  };
> > > >
> > > >  static inline struct amdgpu_device *amdgpu_ttm_adev(struct
> > > > ttm_bo_device *bdev) diff --git
> > > > a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > index 80bf604019b1..b11af38a0238 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > @@ -912,8 +912,10 @@ static void
> > > amdgpu_device_check_smu_prv_buffer_size(struct amdgpu_device
> *adev)
> > > >   * Validates certain module parameters and updates
> > > >   * the associated values used by the driver (all asics).
> > > >   */
> > > > -static void amdgpu_device_check_arguments(struct amdgpu_device
> > > *adev)
> > > > +static int amdgpu_device_check_arguments(struct amdgpu_device
> > > > +*adev)
> > > >  {
> > > > +       int ret = 0;
> > > > +
> > > >         if (amdgpu_sched_jobs < 4) {
> > > >                 dev_warn(adev->dev, "sched jobs (%d) must be at least 4\n",
> > > >                          amdgpu_sched_jobs); @@ -958,13 +960,16 @@
> > > > static void amdgpu_device_check_arguments(struct amdgpu_device
> > > *adev)
> > > >                 amdgpu_vram_page_split = 1024;
> > > >         }
> > > >
> > > > -       if (amdgpu_lockup_timeout == 0) {
> > > > -               dev_warn(adev->dev, "lockup_timeout msut be > 0, adjusting
> to
> > > 10000\n");
> > > > -               amdgpu_lockup_timeout = 10000;
> > > > +       ret = amdgpu_device_get_job_timeout_settings(adev);
> > > > +       if (ret) {
> > > > +               dev_err(adev->dev, "invalid lockup_timeout
> > > > + parameter
> > > syntax\n");
> > > > +               return ret;
> > > >         }
> > > >
> > > >         adev->firmware.load_type =
> > > > amdgpu_ucode_get_load_type(adev,
> > > amdgpu_fw_load_type);
> > > >         amdgpu_direct_gma_size = min(amdgpu_direct_gma_size, 96);
> > > > +
> > > > +       return ret;
> > > >  }
> > > >
> > > >  /**
> > > > @@ -2468,7 +2473,9 @@ int amdgpu_device_init(struct
> amdgpu_device
> > > *adev,
> > > >         mutex_init(&adev->lock_reset);
> > > >         mutex_init(&adev->virt.dpm_mutex);
> > > >
> > > > -       amdgpu_device_check_arguments(adev);
> > > > +       r = amdgpu_device_check_arguments(adev);
> > > > +       if (r)
> > > > +               return r;
> > > >
> > > >         spin_lock_init(&adev->mmio_idx_lock);
> > > >         spin_lock_init(&adev->smc_idx_lock);
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > index 71df27cd03de..c936e0735a0a 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > @@ -83,6 +83,8 @@
> > > >
> > > >  #define AMDGPU_VERSION         "19.10.9.418"
> > > >
> > > > +#define AMDGPU_MAX_TIMEOUT_PARAM_LENTH 256
> > > > +
> > > >  int amdgpu_vram_limit = 0;
> > > >  int amdgpu_vis_vram_limit = 0;
> > > >  int amdgpu_gart_size = -1; /* auto */ @@ -95,7 +97,7 @@ int
> > > > amdgpu_disp_priority = 0;  int amdgpu_hw_i2c = 0;  int
> > > > amdgpu_pcie_gen2 = -1;  int amdgpu_msi = -1; -int
> > > > amdgpu_lockup_timeout = 10000;
> > > > +char
> > > amdgpu_lockup_timeout[AMDGPU_MAX_TIMEOUT_PARAM_LENTH];
> > > >  int amdgpu_dpm = -1;
> > > >  int amdgpu_fw_load_type = -1;
> > > >  int amdgpu_aspm = -1;
> > > > @@ -232,12 +234,25 @@ MODULE_PARM_DESC(msi, "MSI support (1 =
> > > enable,
> > > > 0 = disable, -1 = auto)");  module_param_named(msi, amdgpu_msi,
> > > > int, 0444);
> > > >
> > > >  /**
> > > > - * DOC: lockup_timeout (int)
> > > > - * Set GPU scheduler timeout value in ms. Value 0 is invalidated,
> > > > will be
> > > adjusted to 10000.
> > > > - * Negative values mean 'infinite timeout' (MAX_JIFFY_OFFSET).
> > > > The
> > > default is 10000.
> > > > + * DOC: lockup_timeout (string)
> > > > + * Set GPU scheduler timeout value in ms.
> > > > + *
> > > > + * The format can be [Non-Compute]:[Compute] or
> > > [GFX,SDMA,[Video]]:[Compute].
> > >
> > > I think this is more complicated than it neeeds to be.  How about:
> > > * The format can be [Global] or [GFX,Compute,SDMA,Video].
> > > Either you just adjust the global value for backwards compat, or
> > > your specify the timeout for all of the engine types.
> > >
> > > Alex
> > >
> > > > + *   - There can be one or multiple values specified before ":". Or there
> is
> > > even no value before ":".
> > > > + *     - With no value specified before ":", the default timeout for all
> non-
> > > compute(GFX, SDMA and Video)
> > > > + *       jobs will be 10000.
> > > > + *     - With one value specified, the setting will apply to all non-
> compute
> > > jobs.
> > > > + *     - With multiple values specified, the first one will be for GFX. The
> > > second one is for SDMA.
> > > > + *       And the third one is for Video.
> > > > + *   - There can be one or no value specified after ":".
> > > > + *     - With no value specified after ":", there will be no timeout
> enforced
> > > on compute jobs.
> > > > + *     - Otherwise, the timeout for compute jobs will be set as specified.
> > > > + *
> > > > + * 0 and negative values are invalidated. They will be adjusted
> > > > + to default
> > > timeout.
> > > >   */
> > > > -MODULE_PARM_DESC(lockup_timeout, "GPU lockup timeout in ms >
> 0
> > > > (default 10000)"); -module_param_named(lockup_timeout,
> > > > amdgpu_lockup_timeout, int, 0444);
> > > > +MODULE_PARM_DESC(lockup_timeout, "GPU lockup timeout in ms
> > > (default: 10000 for non-compute jobs and no timeout for compute jobs),
> "
> > > > +               "format is [Non-Compute]:[Compute] or
> > > > +[GFX,SDMA,Video]:[Compute]");
> module_param_string(lockup_timeout,
> > > > +amdgpu_lockup_timeout, sizeof(amdgpu_lockup_timeout), 0444);
> > > >
> > > >  /**
> > > >   * DOC: dpm (int)
> > > > @@ -1307,6 +1322,69 @@ int amdgpu_file_to_fpriv(struct file *filp,
> > > > struct
> > > amdgpu_fpriv **fpriv)
> > > >         return 0;
> > > >  }
> > > >
> > > > +int amdgpu_device_get_job_timeout_settings(struct amdgpu_device
> > > > +*adev) {
> > > > +       char *input = amdgpu_lockup_timeout;
> > > > +       char *non_compute_setting = NULL;
> > > > +       char *timeout_setting = NULL;
> > > > +       int index = 0;
> > > > +       long timeout;
> > > > +       int ret = 0;
> > > > +
> > > > +       /*
> > > > +        * By default timeout for non compute jobs is 10000.
> > > > +        * And there is no timeout enforced on compute jobs.
> > > > +        */
> > > > +       adev->gfx_timeout = adev->sdma_timeout =
> > > > + adev->video_timeout =
> > > 10000;
> > > > +       adev->compute_timeout = MAX_SCHEDULE_TIMEOUT;
> > > > +
> > > > +       if (strnlen(input, AMDGPU_MAX_TIMEOUT_PARAM_LENTH)) {
> > > > +               non_compute_setting = strsep(&input, ":");
> > > > +               if (input) {
> > > > +                       /* Get compute jobs timeout setting */
> > > > +                       ret = kstrtol(input, 0, &timeout);
> > > > +                       if (ret)
> > > > +                               return ret;
> > > > +                       if (timeout > 0)
> > > > +                               adev->compute_timeout = timeout;
> > > > +               }
> > > > +
> > > > +               while ((timeout_setting = strsep(&non_compute_setting, ","))
> &&
> > > > +                               strnlen(timeout_setting,
> > > AMDGPU_MAX_TIMEOUT_PARAM_LENTH)) {
> > > > +                       ret = kstrtol(timeout_setting, 0, &timeout);
> > > > +                       if (ret)
> > > > +                               return ret;
> > > > +
> > > > +                       if (timeout <= 0) {
> > > > +                               index++;
> > > > +                               continue;
> > > > +                       }
> > > > +
> > > > +                       switch (index++) {
> > > > +                       case 0:
> > > > +                               adev->gfx_timeout = timeout;
> > > > +                               break;
> > > > +                       case 1:
> > > > +                               adev->sdma_timeout = timeout;
> > > > +                               break;
> > > > +                       case 2:
> > > > +                               adev->video_timeout = timeout;
> > > > +                               break;
> > > > +                       default:
> > > > +                               break;
> > > > +                       }
> > > > +               }
> > > > +               /*
> > > > +                * There is only one value specified before ":" and
> > > > +                * it should apply to all non-compute jobs.
> > > > +                */
> > > > +               if (index == 1)
> > > > +                       adev->sdma_timeout = adev->video_timeout =
> > > > + adev-
> > > >gfx_timeout;
> > > > +       }
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > >  static bool
> > > >  amdgpu_get_crtc_scanout_position(struct drm_device *dev, unsigned
> > > > int
> > > pipe,
> > > >                                  bool in_vblank_irq, int *vpos,
> > > > int *hpos, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > index d19ad34bef75..16b7e3a22e89 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > @@ -436,9 +436,13 @@ int amdgpu_fence_driver_start_ring(struct
> > > > amdgpu_ring *ring,  int amdgpu_fence_driver_init_ring(struct
> > > amdgpu_ring *ring,
> > > >                                   unsigned num_hw_submission)  {
> > > > +       struct amdgpu_device *adev = ring->adev;
> > > >         long timeout;
> > > >         int r;
> > > >
> > > > +       if (!adev)
> > > > +               return -EINVAL;
> > > > +
> > > >         /* Check that num_hw_submission is a power of two */
> > > >         if ((num_hw_submission & (num_hw_submission - 1)) != 0)
> > > >                 return -EINVAL;
> > > > @@ -465,12 +469,31 @@ int amdgpu_fence_driver_init_ring(struct
> > > > amdgpu_ring *ring,
> > > >
> > > >         /* No need to setup the GPU scheduler for KIQ ring */
> > > >         if (ring->funcs->type != AMDGPU_RING_TYPE_KIQ) {
> > > > -               /* for non-sriov case, no timeout enforce on compute ring */
> > > > -               if ((ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE)
> > > > -                               && !amdgpu_sriov_vf(ring->adev))
> > > > -                       timeout = MAX_SCHEDULE_TIMEOUT;
> > > > -               else
> > > > -                       timeout = msecs_to_jiffies(amdgpu_lockup_timeout);
> > > > +               switch (ring->funcs->type) {
> > > > +               case AMDGPU_RING_TYPE_GFX:
> > > > +                       timeout = adev->gfx_timeout;
> > > > +                       break;
> > > > +               case AMDGPU_RING_TYPE_COMPUTE:
> > > > +                       /*
> > > > +                        * For non-sriov case, no timeout enforce
> > > > +                        * on compute ring by default. Unless user
> > > > +                        * specifies a timeout for compute ring.
> > > > +                        *
> > > > +                        * For sriov case, always use the timeout
> > > > +                        * as gfx ring
> > > > +                        */
> > > > +                       if (!amdgpu_sriov_vf(ring->adev))
> > > > +                               timeout = adev->compute_timeout;
> > > > +                       else
> > > > +                               timeout = adev->gfx_timeout;
> > > > +                       break;
> > > > +               case AMDGPU_RING_TYPE_SDMA:
> > > > +                       timeout = adev->sdma_timeout;
> > > > +                       break;
> > > > +               default:
> > > > +                       timeout = adev->video_timeout;
> > > > +                       break;
> > > > +               }
> > > >
> > > >                 r = drm_sched_init(&ring->sched, &amdgpu_sched_ops,
> > > >                                    num_hw_submission,
> > > > amdgpu_job_hang_limit, diff --git
> > > > a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
> > > > b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
> > > > index 8dbad496b29f..089952a1e6b0 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
> > > > @@ -343,7 +343,7 @@ static void xgpu_ai_mailbox_flr_work(struct
> > > > work_struct *work)
> > > >
> > > >         /* Trigger recovery for world switch failure if no TDR */
> > > >         if (amdgpu_device_should_recover_gpu(adev)
> > > > -               && amdgpu_lockup_timeout == MAX_SCHEDULE_TIMEOUT)
> > > > +               && adev->sdma_timeout == MAX_SCHEDULE_TIMEOUT)
> > > >                 amdgpu_device_gpu_recover(adev, NULL);  }
> > > >
> > > > --
> > > > 2.21.0
> > > >
> > > > _______________________________________________
> > > > amd-gfx mailing list
> > > > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux