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@xxxxxxxxxxxx> > > > > 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@xxxxxxxx> > > [https://lore.kernel.org/dri-devel/20190527185311.GS21222@phenom.ffwll.local/] > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > Signed-off-by: Sam Ravnborg <sam@xxxxxxxxxxxx> > > --- > > 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
