From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E6594108E1CC for ; Thu, 19 Mar 2026 10:16:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 4B26510E961; Thu, 19 Mar 2026 10:16:00 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=collabora.com header.i=@collabora.com header.b="ItQcJb+k"; dkim-atps=neutral Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) by gabe.freedesktop.org (Postfix) with ESMTPS id CA03910E961 for ; Thu, 19 Mar 2026 10:15:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1773915357; bh=7PYQDNqoLHpBfM5L+vLB506fl91qHRz3/LJFRPlbvVc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=ItQcJb+kmp7Iosdsf/d5UGjsRYl1KVejSGJTj9X9+9Hfvdme4JKBk7kFXWTN8fP3M VQHiKNBEtng6oDxj0Hp6mhuJ3UqeFaFV8Z+dj8ijAWjjsBSLTWNM2ulOfGNQ7xsdyT TZnbwCOcvcv3XpIoLI5NCadYOCarVrkFmaRyIh/G+vaDBHqBOk6lZx39IVKJIfs+nu 2iMxS8kYmAPtcvkXxofzlENC2xasOvpNvleBpKXPwbQF22qOt85pXuEa8SD+zGvbZj 835pkGI4zkl84XKKL/M/MqlQTJYl2wBbN6yQXQc4WSQuMLq1V3p4i83/TgPIG8c0N+ MOQGaGFJhJj7g== Received: from fedora (unknown [IPv6:2a01:e0a:2c:6930:d919:a6e:5ea1:8a9f]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by bali.collaboradmins.com (Postfix) with ESMTPSA id 0901917E10F8; Thu, 19 Mar 2026 11:15:56 +0100 (CET) Date: Thu, 19 Mar 2026 11:15:51 +0100 From: Boris Brezillon To: Marcin Slusarz Cc: Steven Price , Liviu Dudau , dri-devel@lists.freedesktop.org, Chia-I Wu , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Lukas Zapolskas , nd@arm.com Subject: Re: [PATCH v2] drm/panthor: extend timestamp query with flags Message-ID: <20260319111551.4ecc0fa7@fedora> In-Reply-To: <20260319082551.841182-1-marcin.slusarz@arm.com> References: <20260318112952.645160-1-marcin.slusarz@arm.com> <20260319082551.841182-1-marcin.slusarz@arm.com> Organization: Collabora X-Mailer: Claws Mail 4.3.1 (GTK 3.24.51; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Thu, 19 Mar 2026 09:25:51 +0100 Marcin Slusarz wrote: > Flags now control which data user space wants to query, > there is more information sources, and there's ability > to query duration of multiple timestamp reads. > > New sources: > - CPU's monotonic, > - CPU's monotonic raw, > - GPU's cycle count > > These changes should make the implementation of > VK_KHR_calibrated_timestamps more accurate and much simpler. > > Signed-off-by: Marcin Slusarz > --- > Changes in v2: > - added DRM_PANTHOR_TIMESTAMP_CPU_NONE > - cpu_timestamp_nsec extended to u64 > - pad1 removed > - copy_from_user -> copy_struct_from_user > - looking at size replaced by looking at flags > - more comments Looks good to me, just a few minor things, but once addressed, this is Reviewed-by: Boris Brezillon > --- > drivers/gpu/drm/panthor/panthor_drv.c | 125 ++++++++++++++++++++++++-- > include/uapi/drm/panthor_drm.h | 58 +++++++++++- > 2 files changed, 175 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index 165dddfde6ca..b6a100ce389d 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -13,7 +13,9 @@ > #include > #include > #include > +#include > #include > +#include > > #include > #include > @@ -765,18 +767,123 @@ static int panthor_query_timestamp_info(struct panthor_device *ptdev, > struct drm_panthor_timestamp_info *arg) > { > int ret; > + u32 flags; > + unsigned long irq_flags; > + struct timespec64 cpu_ts; > + u64 query_start_time; > + bool minimize_interruption; > + u32 timestamp_types = 0; > + > + if (arg->flags != 0) > + flags = arg->flags; > + else > + flags = DRM_PANTHOR_TIMESTAMP_GPU | > + DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | > + DRM_PANTHOR_TIMESTAMP_FREQ; nit: I like to add explicit {} for multi-line statements under conditional blocks. if (arg->flags != 0) { flags = arg->flags; } else { flags = DRM_PANTHOR_TIMESTAMP_GPU | DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | DRM_PANTHOR_TIMESTAMP_FREQ; } and maybe add a comment to repeat what the uAPI doc says. > + > + switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) { > + case 0: > + break; > + case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: > + case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: > + timestamp_types++; > + break; > + default: > + return -EINVAL; > + } > + > + if (flags & ~(DRM_PANTHOR_TIMESTAMP_GPU | > + DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | > + DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | > + DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | > + DRM_PANTHOR_TIMESTAMP_FREQ | > + DRM_PANTHOR_TIMESTAMP_DURATION)) Let's define a VALID_TIMESTAMP_QUERY_FLAGS like we do for other ioctls. > + return -EINVAL; > + > + if (flags & DRM_PANTHOR_TIMESTAMP_GPU) > + timestamp_types++; > + if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT) > + timestamp_types++; > + > + /* If user asked to obtain timestamps from more than one source, > + * then it very likely means they want them to be as close as possible. > + * If they asked for duration, then that likely means that they > + * want to know how long obtaining timestamp takes, without random > + * events, like process scheduling or interrupts. > + */ > + minimize_interruption = > + (flags & DRM_PANTHOR_TIMESTAMP_DURATION) || > + (timestamp_types >= 2); > > ret = panthor_device_resume_and_get(ptdev); > if (ret) > return ret; > > + if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) { > #ifdef CONFIG_ARM_ARCH_TIMER > - arg->timestamp_frequency = arch_timer_get_cntfrq(); > + arg->timestamp_frequency = arch_timer_get_cntfrq(); > #else > - arg->timestamp_frequency = 0; > + arg->timestamp_frequency = 0; > #endif > - arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP); > - arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET); > + } else { > + arg->timestamp_frequency = 0; > + } > + > + if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET) > + arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET); > + else > + arg->timestamp_offset = 0; > + > + if (minimize_interruption) { > + preempt_disable(); > + local_irq_save(irq_flags); > + } > + > + if (flags & DRM_PANTHOR_TIMESTAMP_DURATION) > + query_start_time = local_clock(); > + else > + query_start_time = 0; > + > + if (flags & DRM_PANTHOR_TIMESTAMP_GPU) > + arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP); > + else > + arg->current_timestamp = 0; > + > + switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) { > + case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: > + ktime_get_ts64(&cpu_ts); > + break; > + case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: > + ktime_get_raw_ts64(&cpu_ts); > + break; > + default: > + break; > + } > + > + if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT) > + arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT); > + else > + arg->cycle_count = 0; > + > + if (flags & DRM_PANTHOR_TIMESTAMP_DURATION) > + arg->duration_nsec = local_clock() - query_start_time; > + else > + arg->duration_nsec = 0; > + > + if (minimize_interruption) { > + local_irq_restore(irq_flags); > + preempt_enable(); > + } > + > + if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) { > + timens_add_monotonic(&cpu_ts); > + > + arg->cpu_timestamp_sec = cpu_ts.tv_sec; > + arg->cpu_timestamp_nsec = cpu_ts.tv_nsec; > + } else { > + arg->cpu_timestamp_sec = 0; > + arg->cpu_timestamp_nsec = 0; > + } > > pm_runtime_put(ptdev->base.dev); > return 0; > @@ -851,8 +958,13 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d > return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info); > > case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO: > - ret = panthor_query_timestamp_info(ptdev, ×tamp_info); > + ret = copy_struct_from_user(×tamp_info, sizeof(timestamp_info), > + (const void __user __force *) args->pointer, u64_to_user_ptr(args->pointer), > + args->size); I wonder if we should have a PANTHOR_UOBJ_GET() that checks the input size against the minimum struct size ever exposed by the driver. Here we are covered by the following PANTHOR_UOBJ_SET() so that's probably not a huge issue, dunno. > + if (ret) > + return ret; > > + ret = panthor_query_timestamp_info(ptdev, ×tamp_info); > if (ret) > return ret; > > @@ -1680,6 +1792,7 @@ static void panthor_debugfs_init(struct drm_minor *minor) > * - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl > * - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl > * - adds drm_panthor_gpu_info::selected_coherency > + * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags > */ > static const struct drm_driver panthor_drm_driver = { > .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ | > @@ -1693,7 +1806,7 @@ static const struct drm_driver panthor_drm_driver = { > .name = "panthor", > .desc = "Panthor DRM driver", > .major = 1, > - .minor = 7, > + .minor = 8, > > .gem_create_object = panthor_gem_create_object, > .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h > index b401ac585d6a..8a46ef040c3d 100644 > --- a/include/uapi/drm/panthor_drm.h > +++ b/include/uapi/drm/panthor_drm.h > @@ -409,6 +409,38 @@ struct drm_panthor_csif_info { > __u32 pad; > }; > > +/** > + * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags > + */ > +enum drm_panthor_timestamp_info_flags { > + /** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */ > + DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0, > + > + /** @DRM_PANTHOR_TIMESTAMP_CPU_NONE: Don't query CPU time. */ > + DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1, > + > + /** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */ > + DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1, > + > + /** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */ > + DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1, > + > + /** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */ > + DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1, > + > + /** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */ > + DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4, > + > + /** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */ > + DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5, > + > + /** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */ > + DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6, > + > + /** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */ > + DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7, > +}; > + > /** > * struct drm_panthor_timestamp_info - Timestamp information > * > @@ -421,11 +453,33 @@ struct drm_panthor_timestamp_info { > */ > __u64 timestamp_frequency; > > - /** @current_timestamp: The current timestamp. */ > + /** @current_timestamp: The current GPU timestamp. */ > __u64 current_timestamp; > > - /** @timestamp_offset: The offset of the timestamp timer. */ > + /** @timestamp_offset: The offset of the GPU timestamp timer. */ > __u64 timestamp_offset; > + > + /** > + * @flags: Bitmask of drm_panthor_timestamp_info_flags. > + * > + * If set to 0, then it is interpreted as: > + * DRM_PANTHOR_TIMESTAMP_GPU | > + * DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | > + * DRM_PANTHOR_TIMESTAMP_FREQ > + */ > + __u32 flags; > + > + /** @duration_nsec: Duration of time query. */ > + __u32 duration_nsec; > + > + /** @cycle_count: Value of GPU_CYCLE_COUNT. */ > + __u64 cycle_count; > + > + /** @cpu_timestamp_sec: Seconds part of CPU timestamp. */ > + __u64 cpu_timestamp_sec; > + > + /** @cpu_timestamp_nsec: Nanseconds part of CPU timestamp. */ > + __u64 cpu_timestamp_nsec; > }; > > /**