Re: [PATCH] drm/msm: Preemption timeout should be greater than hangcheck

On Sun, Jul 26, 2026 at 10:57 AM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The preemption timeout calculation fails to account for the hangcheck progress multiplier, causing premature job termination for long-running jobs.
> --
>
> --- Patch [1]: [PATCH] drm/msm: Preemption timeout should be greater than hangcheck ---
> commit 5454ddf4018869efeec2d7c8f2767056ad2612d9
> Author: Rob Clark <[email protected]>
>
> drm/msm: Preemption timeout should be greater than hangcheck
>
> This commit changes the preemption timeout from a hard-coded 10 seconds to
> the maximum of 10 seconds and the configured hangcheck period. This is
> intended to prevent the preemption timer from prematurely killing
> long-running jobs that are otherwise compliant with the hangcheck timeout.
>
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_preempt.h b/drivers/gpu/drm/msm/adreno/a6xx_preempt.h
> > index df36c945b836b..178017aad26e6 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_preempt.h
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_preempt.h
> > @@ -35,6 +35,14 @@ static inline void set_preempt_state(struct a6xx_gpu *gpu,
> >       smp_mb__after_atomic();
> >  }
> >
> > +static inline void set_preempt_timer(struct a6xx_gpu *gpu)
> > +{
> > +     struct msm_drm_private *priv = gpu->base.base.dev->dev_private;
> > +     unsigned t = max(10000, priv->hangcheck_period);
>
> [Severity: Medium]
> Will this under-calculate the true hangcheck timeout when progress detection
> is enabled?
>
> According to msm_ringbuffer.h, when progress detection is enabled, the
> hangcheck timer duration is halved and allows up to
> DRM_MSM_HANGCHECK_PROGRESS_RETRIES extra cycles.
>
> If a user sets a large hangcheck period via debugfs to allow for long
> compute shaders, wouldn't using priv->hangcheck_period directly here result
> in a preemption timeout that is shorter than the actual allowed hangcheck
> time, potentially still killing the job prematurely?
>

yes, but the progress detection is based on detecting that the CP
advanced, which won't be the case for long running compute shaders

BR,
-R

> > +
> > +     mod_timer(&gpu->preempt_timer, jiffies + msecs_to_jiffies(t));
> > +}
> > +
> >  /* Write the most recent wptr for the given ring into the hardware */
> >  static inline void update_wptr(struct a6xx_gpu *a6xx_gpu, struct msm_ringbuffer *ring)
> >  {
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1