Re: [PATCH 85/86] drm: move drm_timeout_abs_to_jiffies to drm_util

[Sam Ravnborg <sam@ravnborg.org>]

Hi Thomas,

On Mon, Jan 23, 2023 at 09:57:13AM +0100, Thomas Zimmermann wrote:
> Hi Sam,
> 
> please see my comment below.
> 
> Am 21.01.23 um 21:09 schrieb Sam Ravnborg via B4 Submission Endpoint:
> > From: Sam Ravnborg <sam@ravnborg.org>
> > 
> > drm_timeout_abs_to_jiffies() was implmented in drm_syncobj where
> > it really did not belong. Create a drm_util file and move the
> > implementation. Likewise move the prototype and update all users.
> > 
> > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> > [https://lore.kernel.org/dri-devel/20190527185311.GS21222@phenom.ffwll.local/]
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > ---
> >   drivers/accel/ivpu/ivpu_gem.c           |  2 +-
> >   drivers/gpu/drm/Makefile                |  1 +
> >   drivers/gpu/drm/drm_syncobj.c           | 34 ----------------------------
> >   drivers/gpu/drm/drm_util.c              | 40 +++++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/lima/lima_gem.c         |  2 +-
> >   drivers/gpu/drm/panfrost/panfrost_drv.c |  2 +-
> >   drivers/gpu/drm/tegra/uapi.c            |  2 +-
> >   include/drm/drm_util.h                  |  1 +
> >   include/drm/drm_utils.h                 |  2 --
> >   9 files changed, 46 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
> > index d1f923971b4c..55aa94ba6c10 100644
> > --- a/drivers/accel/ivpu/ivpu_gem.c
> > +++ b/drivers/accel/ivpu/ivpu_gem.c
> > @@ -12,7 +12,7 @@
> >   #include <drm/drm_cache.h>
> >   #include <drm/drm_debugfs.h>
> >   #include <drm/drm_file.h>
> > -#include <drm/drm_utils.h>
> > +#include <drm/drm_util.h>
> >   #include "ivpu_drv.h"
> >   #include "ivpu_gem.h"
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index ab4460fcd63f..561b93d19685 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -42,6 +42,7 @@ drm-y := \
> >   	drm_syncobj.o \
> >   	drm_sysfs.o \
> >   	drm_trace_points.o \
> > +	drm_util.o \
> >   	drm_vblank.o \
> >   	drm_vblank_work.o \
> >   	drm_vma_manager.o \
> > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> > index 0c2be8360525..35f5416c5cfe 100644
> > --- a/drivers/gpu/drm/drm_syncobj.c
> > +++ b/drivers/gpu/drm/drm_syncobj.c
> > @@ -197,7 +197,6 @@
> >   #include <drm/drm_gem.h>
> >   #include <drm/drm_print.h>
> >   #include <drm/drm_syncobj.h>
> > -#include <drm/drm_utils.h>
> >   #include "drm_internal.h"
> > @@ -1114,39 +1113,6 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> >   	return timeout;
> >   }
> > -/**
> > - * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute value
> > - *
> > - * @timeout_nsec: timeout nsec component in ns, 0 for poll
> > - *
> > - * Calculate the timeout in jiffies from an absolute time in sec/nsec.
> > - */
> > -signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec)

Thanks for the critical look at this!

> 
> This function converts an absolute timeout in nsec to a relative timeout in
> jiffies. (?)
> 
> It appears to me as if this helper should not exist. It uses a mixture of
> different time interfaces; combined with hardcoded policy for 0 and
> MAX_SCHEDULE_TIMEOUT.
> 
> There are only 3 callers of this helper. I think we should consider inlining
> it in each.
> 
> As part of this, maybe the use of ktime could go away. Convert nsecs to
> jiffies and do the rest of the computation in jiffies.

I blindly copied the existing function and did not consider the
implementation. Looking for a helper that do what we needs here turned
up empty. I also looked at your suggestion to do:
nsec in absolute => jiffies in absolute => jiffies in relative
But did not find something that is better than what we have.

I will leave it for now, and focus on the other parts of the patchset.
In the vain hope someone else takes a look.

	Sam