Re: [PATCH 3/3] drm/v3d: Add job timeout module param

[Eric Anholt <eric@anholt.net>]

On Thu, Feb 4, 2021 at 10:09 AM Chema Casanova <jmcasanova@igalia.com> wrote:
>
> On 3/9/20 18:48, Yukimasa Sugizaki wrote:
> > From: Yukimasa Sugizaki <ysugi@idein.jp>
> >
> > The default timeout is 500 ms which is too short for some workloads
> > including Piglit.  Adding this parameter will help users to run heavier
> > tasks.
> >
> > Signed-off-by: Yukimasa Sugizaki <ysugi@idein.jp>
> > ---
> >   drivers/gpu/drm/v3d/v3d_sched.c | 24 +++++++++++++-----------
> >   1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> > index feef0c749e68..983efb018560 100644
> > --- a/drivers/gpu/drm/v3d/v3d_sched.c
> > +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> > @@ -19,11 +19,17 @@
> >    */
> >
> >   #include <linux/kthread.h>
> > +#include <linux/moduleparam.h>
> >
> >   #include "v3d_drv.h"
> >   #include "v3d_regs.h"
> >   #include "v3d_trace.h"
> >
> > +static uint timeout = 500;
> > +module_param(timeout, uint, 0444);
> > +MODULE_PARM_DESC(timeout,
> > +     "Timeout for a job in ms (0 means infinity and default is 500 ms)");
> > +
>
> I think that  parameter definition could be included at v3d_drv.c as
> other drivers do. Then we would have all future parameters together. In
> that case we would need also to include the extern definition at
> v3d_drv.h for the driver variable.
>
> The param name could be more descriptive like "sched_timeout_ms" in the
> lima driver.
>
> I wonder if it would make sense to have different timeouts parameters
> for different type of jobs we have. At least this series come from the
> need additional time to complete compute jobs. This is what amdgpu does,
> but we probably don't need something such complex.
>
> I think it would also make sense to increase our default value for the
> compute jobs.
>
> What do you think?
>
> In any case I think that having this general scheduling timeout
> parameter as other drivers do is a good idea.

Having a param for being able to test if the job completes eventually
is a good idea, but if tests are triggering timeouts, then you
probably need to investigate what's going on in the driver -- it's not
like you want .5second unpreemptible jobs to be easy to produce.

Also, for CS, I wonder if we have another reg besides CSD_CURRENT_CFG4
we could be looking at for "we're making progress on the job".  Half a
second with no discernible progress sounds like a bug.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel