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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4493 bytes --]

Hi

Am 23.01.23 um 21:46 schrieb Sam Ravnborg:
> 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.

No problem. You can add

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

to patches 85 and 86 as well.

Best regards
Thomas

> 
> 	Sam

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]