All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/display: Introduce Display Metrics info
@ 2024-04-05 22:08 Rodrigo Vivi
  2024-04-05 22:36 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Rodrigo Vivi @ 2024-04-05 22:08 UTC (permalink / raw)
  To: intel-gfx
  Cc: Rodrigo Vivi, Uma Shankar, Vandita Kulkarni, Naveen Kumar,
	Jani Nikula, Vinay Belgaumkar

Introduce a display metrics information through debugfs
for a better view of the vblank and page flips, in special
Async flips behavior.

There is currently an overall expectation that whenever
vblank_mode=0 is used with an graphics application, that
automatically async_flips are happening. However, while
implementing the Display Metrics for GuC SLPC, it was observed
that it is not necessarily true for many of the expected cases.

So, while the GuC SLPC side of the metrics doesn't get ready,
let's at least bring the debugfs view of it so we can work
to understand and fix any potential issue around our async vblanks.

Please notice that the every struct here follows exactly the
GuC shared data buffer, so the next step of the integration
would be smooth and almost transparent to this intel_metrics
on the display side.

Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Vandita Kulkarni <vandita.kulkarni@intel.com>
Cc: Naveen Kumar <naveen1.kumar@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/display/intel_display.c  |  15 +-
 .../gpu/drm/i915/display/intel_display_core.h |   2 +
 .../drm/i915/display/intel_display_debugfs.c  |  12 +
 .../drm/i915/display/intel_display_driver.c   |   5 +
 .../gpu/drm/i915/display/intel_display_irq.c  |   3 +
 drivers/gpu/drm/i915/display/intel_metrics.c  | 356 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_metrics.h  |  27 ++
 .../drm/i915/display/skl_universal_plane.c    |   3 +
 drivers/gpu/drm/xe/Makefile                   |   1 +
 10 files changed, 424 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_metrics.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_metrics.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index af9e871daf1d..a3c8d9f5614c 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -291,6 +291,7 @@ i915-y += \
 	display/intel_link_bw.o \
 	display/intel_load_detect.o \
 	display/intel_lpe_audio.o \
+	display/intel_metrics.o \
 	display/intel_modeset_lock.o \
 	display/intel_modeset_setup.o \
 	display/intel_modeset_verify.o \
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a481c9218138..ca30b8d48e1f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -94,6 +94,7 @@
 #include "intel_link_bw.h"
 #include "intel_lvds.h"
 #include "intel_lvds_regs.h"
+#include "intel_metrics.h"
 #include "intel_modeset_setup.h"
 #include "intel_modeset_verify.h"
 #include "intel_overlay.h"
@@ -1021,11 +1022,15 @@ static void intel_post_plane_update(struct intel_atomic_state *state,
 				    struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct intel_display *display = &dev_priv->display;
 	const struct intel_crtc_state *old_crtc_state =
 		intel_atomic_get_old_crtc_state(state, crtc);
 	const struct intel_crtc_state *new_crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
 	enum pipe pipe = crtc->pipe;
+	const struct intel_plane_state __maybe_unused *plane_state;
+	struct intel_plane *plane;
+	int i;
 
 	intel_psr_post_plane_update(state, crtc);
 
@@ -1057,6 +1062,12 @@ static void intel_post_plane_update(struct intel_atomic_state *state,
 
 	if (audio_enabling(old_crtc_state, new_crtc_state))
 		intel_encoders_audio_enable(state, crtc);
+
+	if (!new_crtc_state->do_async_flip) {
+		for_each_new_intel_plane_in_state(state, plane, plane_state, i)
+			intel_metrics_flip(display, new_crtc_state, plane->id,
+					   false);
+	}
 }
 
 static void intel_crtc_enable_flip_done(struct intel_atomic_state *state,
@@ -7139,6 +7150,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 {
 	struct drm_device *dev = state->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_display *display = &dev_priv->display;
 	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
 	struct intel_crtc *crtc;
 	struct intel_power_domain_mask put_domains[I915_MAX_PIPES] = {};
@@ -7261,7 +7273,6 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
 		if (new_crtc_state->do_async_flip)
 			intel_crtc_disable_flip_done(state, crtc);
-
 		intel_color_wait_commit(new_crtc_state);
 	}
 
@@ -7314,6 +7325,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 		 * FIXME get rid of this funny new->old swapping
 		 */
 		old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb);
+
+		intel_metrics_refresh_info(display, new_crtc_state);
 	}
 
 	/* Underruns don't always raise interrupts, so check manually */
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
index 2167dbee5eea..992db9098566 100644
--- a/drivers/gpu/drm/i915/display/intel_display_core.h
+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
@@ -42,6 +42,7 @@ struct intel_cdclk_vals;
 struct intel_color_funcs;
 struct intel_crtc;
 struct intel_crtc_state;
+struct intel_display_metrics;
 struct intel_dmc;
 struct intel_dpll_funcs;
 struct intel_dpll_mgr;
@@ -530,6 +531,7 @@ struct intel_display {
 	struct intel_fbc *fbc[I915_MAX_FBCS];
 	struct intel_frontbuffer_tracking fb_tracking;
 	struct intel_hotplug hotplug;
+	struct intel_display_metrics *metrics;
 	struct intel_opregion *opregion;
 	struct intel_overlay *overlay;
 	struct intel_display_params params;
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 3e364891dcd0..f05b9a9ddee0 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -30,6 +30,7 @@
 #include "intel_hdcp.h"
 #include "intel_hdmi.h"
 #include "intel_hotplug.h"
+#include "intel_metrics.h"
 #include "intel_panel.h"
 #include "intel_psr.h"
 #include "intel_psr_regs.h"
@@ -642,6 +643,16 @@ static int i915_display_capabilities(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static int i915_display_metrics(struct seq_file *m, void *unused)
+{
+	struct drm_i915_private *i915 = node_to_i915(m->private);
+	struct drm_printer p = drm_seq_file_printer(m);
+
+	intel_metrics_show(&i915->display, &p);
+
+	return 0;
+}
+
 static int i915_shared_dplls_info(struct seq_file *m, void *unused)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -1059,6 +1070,7 @@ static const struct drm_info_list intel_display_debugfs_list[] = {
 	{"i915_power_domain_info", i915_power_domain_info, 0},
 	{"i915_display_info", i915_display_info, 0},
 	{"i915_display_capabilities", i915_display_capabilities, 0},
+	{"i915_display_metrics", i915_display_metrics, 0},
 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
 	{"i915_ddb_info", i915_ddb_info, 0},
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
index 87dd07e0d138..767b2d5b3826 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
@@ -46,6 +46,7 @@
 #include "intel_hdcp.h"
 #include "intel_hotplug.h"
 #include "intel_hti.h"
+#include "intel_metrics.h"
 #include "intel_modeset_lock.h"
 #include "intel_modeset_setup.h"
 #include "intel_opregion.h"
@@ -514,6 +515,8 @@ int intel_display_driver_probe(struct drm_i915_private *i915)
 
 	intel_overlay_setup(i915);
 
+	intel_metrics_init(&i915->display);
+
 	ret = intel_fbdev_init(&i915->drm);
 	if (ret)
 		return ret;
@@ -611,6 +614,8 @@ void intel_display_driver_remove_noirq(struct drm_i915_private *i915)
 
 	intel_dp_tunnel_mgr_cleanup(i915);
 
+	intel_metrics_fini(&i915->display);
+
 	intel_overlay_cleanup(i915);
 
 	intel_gmbus_teardown(i915);
diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
index f846c5b108b5..202400a771b2 100644
--- a/drivers/gpu/drm/i915/display/intel_display_irq.c
+++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
@@ -18,6 +18,7 @@
 #include "intel_fifo_underrun.h"
 #include "intel_gmbus.h"
 #include "intel_hotplug_irq.h"
+#include "intel_metrics.h"
 #include "intel_pmdemand.h"
 #include "intel_psr.h"
 #include "intel_psr_regs.h"
@@ -25,8 +26,10 @@
 static void
 intel_handle_vblank(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
+	struct intel_display *display = &dev_priv->display;
 	struct intel_crtc *crtc = intel_crtc_for_pipe(dev_priv, pipe);
 
+	intel_metrics_vblank(display, crtc);
 	drm_crtc_handle_vblank(&crtc->base);
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_metrics.c b/drivers/gpu/drm/i915/display/intel_metrics.c
new file mode 100644
index 000000000000..2cb2b8189b25
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_metrics.c
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#include "intel_metrics.h"
+
+#include <drm/drm_modes.h>
+#include <drm/drm_print.h>
+
+#include "i915_drv.h"
+#include "intel_de.h"
+#include "intel_display_types.h"
+
+/**
+ * Display Metrics
+ *
+ * Provide some display activity overview such as active refresh rates,
+ * vblank activity and page flip activities.
+ * For now it is informative debug only, but later it will be expanded
+ * to be used for GT frequency selection by GuC SLPC.
+ */
+
+/*
+ * An event using an work queue is used to avoid any disturbance in the
+ * critical path that could cause performance impacts.
+ */
+struct display_event {
+	struct work_struct work;
+	struct drm_i915_private *i915;
+	struct intel_display *display;
+	bool is_vblank;
+	int pipe;
+	int plane;
+	bool async_flip;
+};
+
+/*
+ * Although we could simply save this inside our crtc structs, we are
+ * already mimicking the GuC SLPC defition of the display data, for future
+ * usage.
+ */
+#define MAX_PIPES		8
+#define MAX_PLANES_PER_PIPE	8
+
+struct display_global_info {
+	u32 version:8;
+	u32 num_pipes:4;
+	u32 num_planes_per_pipe:4;
+	u32 reserved_1:16;
+	u32 refresh_count:16;
+	u32 vblank_count:16;
+	u32 flip_count:16;
+	u32 reserved_2:16;
+	u32 reserved_3[13];
+} __packed;
+
+struct display_refresh_info {
+	u32 refresh_interval:16;
+	u32 is_variable:1;
+	u32 reserved:15;
+} __packed;
+
+/*
+ * When used with GuC SLPC, the host must update each 32-bit part with a single
+ * atomic write so that SLPC will read the contained bit fields together.
+ * The host must update the two parts in order - total flip count and timestamp
+ * first, vsync and async flip counts second.
+ * Hence, these items are not defined with individual bitfields.
+ */
+#define FLIP_P1_LAST		REG_GENMASK(31, 7)
+#define FLIP_P1_TOTAL_COUNT	REG_GENMASK(6, 0)
+#define FLIP_P2_ASYNC_COUNT	REG_GENMASK(31, 16)
+#define FLIP_P2_VSYNC_COUNT	REG_GENMASK(15, 0)
+
+struct display_flip_metrics {
+	u32 part1;
+	u32 part2;
+} __packed;
+
+/*
+ * When used with GuC SLPC, the host must update each 32-bit part with a single
+ * atomic write, so that SLPC will read the count and timestamp together.
+ * Hence, this item is not defined with individual bitfields.
+ */
+#define VBLANK_LAST	REG_GENMASK(31, 7)
+#define VBLANK_COUNT	REG_GENMASK(6, 0)
+
+struct intel_display_metrics {
+	struct display_global_info global_info;
+	struct display_refresh_info refresh_info[MAX_PIPES];
+	u32 vblank_metrics[MAX_PIPES];
+	struct display_flip_metrics
+	flip_metrics[MAX_PIPES][MAX_PLANES_PER_PIPE];
+} __packed;
+
+/**
+ * intel_metrics_refresh_info - Refresh rate information
+ * @display: Pointer to the intel_display struct that is in use by the gfx device.
+ * @crtc_state: New CRTC state upon a modeset.
+ *
+ * To be called on a modeset. It then saves the current refresh interval in
+ * micro seconds.
+ */
+void intel_metrics_refresh_info(struct intel_display *display,
+				struct intel_crtc_state *crtc_state)
+{
+	struct intel_display_metrics *metrics = display->metrics;
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_display_mode *mode = &crtc_state->hw.adjusted_mode;
+	u32 interval_us;
+
+	if (!display->metrics)
+		return;
+
+	interval_us = crtc_state->hw.active ? DIV_ROUND_UP(1000000,
+						drm_mode_vrefresh(mode)) : 0;
+
+	metrics->refresh_info[crtc->pipe].refresh_interval = interval_us;
+	metrics->refresh_info[crtc->pipe].is_variable = crtc_state->uapi.vrr_enabled;
+	metrics->global_info.refresh_count += 1;
+}
+
+static void metrics_update_vblank(struct intel_display_metrics *metrics, int pipe,
+				  u32 timestamp)
+{
+	u32 vblank;
+
+	vblank = metrics->vblank_metrics[pipe];
+
+	vblank = REG_FIELD_GET(VBLANK_COUNT, vblank);
+	vblank = REG_FIELD_PREP(VBLANK_COUNT, vblank + 1);
+	vblank |= REG_FIELD_PREP(VBLANK_LAST, timestamp);
+
+	/* Write everything at once in preparation for the GuC SLPC requirement */
+	metrics->vblank_metrics[pipe] = vblank;
+	metrics->global_info.vblank_count += 1;
+}
+
+static void metrics_update_flip(struct intel_display_metrics *metrics, int pipe,
+				int plane, bool async_flip, u32 timestamp)
+{
+	u32 part1, part2, count;
+
+	part1 = metrics->flip_metrics[pipe][plane].part1;
+	part2 = metrics->flip_metrics[pipe][plane].part2;
+
+	part1 = REG_FIELD_GET(FLIP_P1_TOTAL_COUNT, part1);
+	part1 = REG_FIELD_PREP(FLIP_P1_TOTAL_COUNT, part1 + 1);
+	part1 |= REG_FIELD_PREP(FLIP_P1_LAST, timestamp);
+
+	if (async_flip) {
+		count = REG_FIELD_GET(FLIP_P2_ASYNC_COUNT, part2);
+		part2 &= ~FLIP_P2_ASYNC_COUNT;
+		part2 |= REG_FIELD_PREP(FLIP_P2_ASYNC_COUNT, count + 1);
+	} else {
+		count = REG_FIELD_GET(FLIP_P2_VSYNC_COUNT, part2);
+		part2 &= ~FLIP_P2_VSYNC_COUNT;
+		part2 |= REG_FIELD_PREP(FLIP_P2_VSYNC_COUNT, count + 1);
+	}
+
+	/*
+	 * Write everything in this way and this order in preparation for the
+	 * GuC SLPC requirement
+	 */
+	metrics->flip_metrics[pipe][plane].part1 = part1;
+	metrics->flip_metrics[pipe][plane].part2 = part2;
+
+	metrics->global_info.flip_count += 1;
+}
+
+/*
+ * Let's use the same register GuC SLPC uses for timestamp.
+ * It uses a register that is outside GT domain so GuC doesn't need
+ * to wake the GT for reading during SLPC loop.
+ * This is a single register regarding the GT, so we can read directly
+ * from here, regarding the GT GuC is in.
+ */
+#define MCHBAR_MIRROR_BASE_SNB	0x140000
+#define MCHBAR_BCLK_COUNT	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5984)
+#define MTL_BCLK_COUNT		_MMIO(0xc28)
+#define   TIMESTAMP_MASK	REG_GENMASK(30, 6)
+
+static u32 bclk_read_timestamp(struct drm_i915_private *i915)
+{
+	u32 timestamp;
+
+	if (DISPLAY_VER(i915) >= 14)
+		timestamp = intel_de_read(i915, MTL_BCLK_COUNT);
+	else
+		timestamp = intel_de_read(i915, MCHBAR_BCLK_COUNT);
+
+	return REG_FIELD_GET(TIMESTAMP_MASK, timestamp);
+}
+
+static void display_event_work(struct work_struct *work)
+{
+	struct display_event *event = container_of(work, struct display_event, work);
+	struct intel_display *display = event->display;
+	u32 timestamp;
+
+	timestamp = bclk_read_timestamp(event->i915);
+
+	if (event->is_vblank) {
+		metrics_update_vblank(display->metrics, event->pipe, timestamp);
+	} else {
+		metrics_update_flip(display->metrics, event->pipe, event->plane,
+				    event->async_flip, timestamp);
+	}
+
+	kfree(event);
+}
+
+void intel_metrics_init(struct intel_display *display)
+{
+	struct intel_display_metrics *metrics;
+
+	metrics = kzalloc(sizeof(*metrics), GFP_KERNEL);
+	if (!metrics)
+		return;
+
+	metrics->global_info.version = 1;
+	metrics->global_info.num_pipes = MAX_PIPES;
+	metrics->global_info.num_planes_per_pipe = MAX_PLANES_PER_PIPE;
+
+	display->metrics = metrics;
+}
+
+void intel_metrics_fini(struct intel_display *display)
+{
+	if (!display->metrics)
+		return;
+
+	kfree(display->metrics);
+}
+
+/**
+ * intel_metrics_vblank - Vblank information
+ * @display: Pointer to the intel_display struct that is in use by the gfx device.
+ * @crtc: The Intel CRTC that received the vblank interrupt.
+ *
+ * To be called when a vblank is passed.
+ */
+void intel_metrics_vblank(struct intel_display *display,
+			  struct intel_crtc *crtc)
+{
+	struct display_event *event;
+
+	if (!display->metrics)
+		return;
+
+	event = kmalloc(sizeof(*event), GFP_ATOMIC);
+	if (!event)
+		return;
+
+	INIT_WORK(&event->work, display_event_work);
+	event->i915 = to_i915(crtc->base.dev);
+	event->display = display;
+	event->is_vblank = true;
+	event->pipe = crtc->pipe;
+	queue_work(system_highpri_wq, &event->work);
+}
+
+/**
+ * intel_metrics_flip - Flip information
+ * @display: Pointer to the intel_display struct that is in use by the gfx device.
+ * @crtc_state: New CRTC state upon a page flip.
+ * @plane: Which plane ID got the page flip.
+ * @async_flip: A boolean to indicate if the page flip was async.
+ *
+ * To be called on a page flip.
+ */
+void intel_metrics_flip(struct intel_display *display,
+			const struct intel_crtc_state *crtc_state,
+			int plane, bool async_flip)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct display_event *event;
+
+	if (!display->metrics)
+		return;
+
+	event = kmalloc(sizeof(*event), GFP_ATOMIC);
+	if (!event)
+		return;
+
+	INIT_WORK(&event->work, display_event_work);
+	event->i915 = to_i915(crtc->base.dev);
+	event->display = display;
+	event->pipe = crtc->pipe;
+	event->plane = plane;
+	event->async_flip = async_flip;
+	queue_work(system_highpri_wq, &event->work);
+}
+
+void intel_metrics_show(struct intel_display *display, struct drm_printer *p)
+{
+	struct intel_display_metrics *metrics = display->metrics;
+	int pipe, plane;
+	u32 val;
+
+	if (!metrics)
+		return;
+
+	drm_printf(p, "\nDisplay Metrics - Globals:\n");
+	drm_printf(p, "\tVersion: %d\n", metrics->global_info.version);
+	drm_printf(p, "\tNum Pipes: %d\n", metrics->global_info.num_pipes);
+	drm_printf(p, "\tNum Planes per Pipe: %d\n",
+		   metrics->global_info.num_planes_per_pipe);
+	drm_printf(p, "\tGlobal Refresh Info Count: %d\n",
+		   metrics->global_info.refresh_count);
+	drm_printf(p, "\tGlobal Vblank Count: %d\n",
+		   metrics->global_info.vblank_count);
+	drm_printf(p, "\tGlobal Flip Count: %d\n",
+		   metrics->global_info.flip_count);
+
+	for (pipe = 0; pipe < MAX_PIPES; pipe++) {
+		if (!metrics->refresh_info[pipe].refresh_interval)
+			continue;
+
+		drm_printf(p, "\nDisplay Metrics - Refresh Info - Pipe[%d]:\n",
+			   pipe);
+		drm_printf(p, "\tRefresh Interval: %d\n",
+			   metrics->refresh_info[pipe].refresh_interval);
+		drm_printf(p, "\tIS VRR: %d\n",
+			   metrics->refresh_info[pipe].is_variable);
+
+		drm_printf(p, "Display Metrics - Vblank Info - Pipe[%d]:\n",
+			   pipe);
+		val = metrics->vblank_metrics[pipe];
+		drm_printf(p, "\tVBlank Last Timestamp: %x\n",
+			   REG_FIELD_GET(VBLANK_LAST, val));
+		drm_printf(p, "\tVBlank Count: %d\n",
+			   REG_FIELD_GET(VBLANK_COUNT, val));
+
+		drm_printf(p, "Display Metrics - Flip Info - Pipe[%d]:\n", pipe);
+		for (plane = 0; plane < MAX_PLANES_PER_PIPE; plane++) {
+			val = metrics->flip_metrics[pipe][plane].part1;
+			if (!val)
+				continue;
+
+			drm_printf(p, "\tFlip Info - Plane[%d]:\n", plane);
+			drm_printf(p, "\t\tFlip Last Timestamp: %x\n",
+				   REG_FIELD_GET(FLIP_P1_LAST, val));
+			drm_printf(p, "\t\tFlip Total Count: %d\n",
+				   REG_FIELD_GET(FLIP_P1_TOTAL_COUNT, val));
+
+			val = metrics->flip_metrics[pipe][plane].part2;
+
+			drm_printf(p, "\t\tFlip Async Count: %d\n",
+				   REG_FIELD_GET(FLIP_P2_ASYNC_COUNT, val));
+			drm_printf(p, "\t\tFlip Vsync Count: %d\n",
+				   REG_FIELD_GET(FLIP_P2_VSYNC_COUNT, val));
+		}
+	}
+}
diff --git a/drivers/gpu/drm/i915/display/intel_metrics.h b/drivers/gpu/drm/i915/display/intel_metrics.h
new file mode 100644
index 000000000000..9e41dc9522f3
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_metrics.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef __INTEL_METRICS_H__
+#define __INTEL_METRICS_H__
+
+#include <linux/types.h>
+
+struct drm_printer;
+struct intel_crtc;
+struct intel_crtc_state;
+struct intel_display;
+
+void intel_metrics_refresh_info(struct intel_display *display,
+				struct intel_crtc_state *crtc_state);
+void intel_metrics_vblank(struct intel_display *display,
+			  struct intel_crtc *intel_crtc);
+void intel_metrics_flip(struct intel_display *display,
+			const struct intel_crtc_state *crtc_state,
+			int plane, bool async_flip);
+void intel_metrics_init(struct intel_display *display);
+void intel_metrics_fini(struct intel_display *display);
+void intel_metrics_show(struct intel_display *display, struct drm_printer *p);
+
+#endif
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 860574d04f88..fdd21a41d79d 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -17,6 +17,7 @@
 #include "intel_fb.h"
 #include "intel_fbc.h"
 #include "intel_frontbuffer.h"
+#include "intel_metrics.h"
 #include "intel_psr.h"
 #include "intel_psr_regs.h"
 #include "skl_scaler.h"
@@ -1392,6 +1393,7 @@ skl_plane_async_flip(struct intel_plane *plane,
 		     bool async_flip)
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	struct intel_display *display = &dev_priv->display;
 	enum plane_id plane_id = plane->id;
 	enum pipe pipe = plane->pipe;
 	u32 plane_ctl = plane_state->ctl;
@@ -1404,6 +1406,7 @@ skl_plane_async_flip(struct intel_plane *plane,
 	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
 	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
 			  skl_plane_surf(plane_state, 0));
+	intel_metrics_flip(display, crtc_state, plane_id, async_flip);
 }
 
 static bool intel_format_is_p01x(u32 format)
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index e5b1715f721e..5133dba2c7de 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -263,6 +263,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
 	i915-display/intel_hti.o \
 	i915-display/intel_link_bw.o \
 	i915-display/intel_lspcon.o \
+	i915-display/intel_metrics.o \
 	i915-display/intel_modeset_lock.o \
 	i915-display/intel_modeset_setup.o \
 	i915-display/intel_modeset_verify.o \
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915/display: Introduce Display Metrics info
  2024-04-05 22:08 [PATCH] drm/i915/display: Introduce Display Metrics info Rodrigo Vivi
@ 2024-04-05 22:36 ` Patchwork
  2024-04-05 22:36 ` ✗ Fi.CI.SPARSE: " Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2024-04-05 22:36 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/display: Introduce Display Metrics info
URL   : https://patchwork.freedesktop.org/series/132108/
State : warning

== Summary ==

Error: dim checkpatch failed
1aaf96febc0d drm/i915/display: Introduce Display Metrics info
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:221: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#221: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 558 lines checked



^ permalink raw reply	[flat|nested] 10+ messages in thread

* ✗ Fi.CI.SPARSE: warning for drm/i915/display: Introduce Display Metrics info
  2024-04-05 22:08 [PATCH] drm/i915/display: Introduce Display Metrics info Rodrigo Vivi
  2024-04-05 22:36 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2024-04-05 22:36 ` Patchwork
  2024-04-05 22:56 ` ✗ Fi.CI.BAT: failure " Patchwork
  2024-05-06 10:03 ` [PATCH] " Kumar, Naveen1
  3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2024-04-05 22:36 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/display: Introduce Display Metrics info
URL   : https://patchwork.freedesktop.org/series/132108/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* ✗ Fi.CI.BAT: failure for drm/i915/display: Introduce Display Metrics info
  2024-04-05 22:08 [PATCH] drm/i915/display: Introduce Display Metrics info Rodrigo Vivi
  2024-04-05 22:36 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
  2024-04-05 22:36 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2024-04-05 22:56 ` Patchwork
  2024-05-06 10:03 ` [PATCH] " Kumar, Naveen1
  3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2024-04-05 22:56 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 19707 bytes --]

== Series Details ==

Series: drm/i915/display: Introduce Display Metrics info
URL   : https://patchwork.freedesktop.org/series/132108/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14537 -> Patchwork_132108v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_132108v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_132108v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/index.html

Participating hosts (33 -> 35)
------------------------------

  Additional (4): fi-glk-j4005 fi-blb-e6850 bat-dg2-11 bat-dg1-7 
  Missing    (2): fi-snb-2520m fi-kbl-8809g 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_132108v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - fi-kbl-x1275:       [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-kbl-x1275/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-kbl-x1275/igt@i915_module_load@load.html
    - fi-kbl-guc:         [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-kbl-guc/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-kbl-guc/igt@i915_module_load@load.html

  * igt@i915_selftest@live@mman:
    - bat-adlm-1:         [PASS][5] -> [INCOMPLETE][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-adlm-1/igt@i915_selftest@live@mman.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-adlm-1/igt@i915_selftest@live@mman.html

  * igt@kms_busy@basic@flip:
    - fi-kbl-7567u:       [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-kbl-7567u/igt@kms_busy@basic@flip.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-kbl-7567u/igt@kms_busy@basic@flip.html
    - fi-elk-e7500:       [PASS][9] -> [INCOMPLETE][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-elk-e7500/igt@kms_busy@basic@flip.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-elk-e7500/igt@kms_busy@basic@flip.html
    - fi-cfl-8109u:       [PASS][11] -> [INCOMPLETE][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-cfl-8109u/igt@kms_busy@basic@flip.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-cfl-8109u/igt@kms_busy@basic@flip.html
    - fi-pnv-d510:        [PASS][13] -> [ABORT][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-pnv-d510/igt@kms_busy@basic@flip.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-pnv-d510/igt@kms_busy@basic@flip.html
    - bat-dg1-7:          NOTRUN -> [ABORT][15]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg1-7/igt@kms_busy@basic@flip.html

  * igt@kms_busy@basic@modeset:
    - bat-adln-1:         [PASS][16] -> [INCOMPLETE][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-adln-1/igt@kms_busy@basic@modeset.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-adln-1/igt@kms_busy@basic@modeset.html
    - bat-rplp-1:         [PASS][18] -> [INCOMPLETE][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-rplp-1/igt@kms_busy@basic@modeset.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-rplp-1/igt@kms_busy@basic@modeset.html
    - bat-adls-6:         [PASS][20] -> [ABORT][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-adls-6/igt@kms_busy@basic@modeset.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-adls-6/igt@kms_busy@basic@modeset.html
    - fi-ilk-650:         [PASS][22] -> [INCOMPLETE][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-ilk-650/igt@kms_busy@basic@modeset.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-ilk-650/igt@kms_busy@basic@modeset.html
    - bat-adlp-6:         [PASS][24] -> [ABORT][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-adlp-6/igt@kms_busy@basic@modeset.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-adlp-6/igt@kms_busy@basic@modeset.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-ivb-3770:        [PASS][26] -> [INCOMPLETE][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-ivb-3770/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-ivb-3770/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-blb-e6850:       NOTRUN -> [INCOMPLETE][28]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-blb-e6850/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-atomic:
    - fi-cfl-8700k:       [PASS][29] -> [INCOMPLETE][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-cfl-8700k/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-cfl-8700k/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - fi-cfl-guc:         [PASS][31] -> [INCOMPLETE][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-cfl-guc/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-cfl-guc/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - bat-jsl-3:          [PASS][33] -> [INCOMPLETE][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-jsl-3/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-jsl-3/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html

  * igt@kms_flip@basic-flip-vs-dpms@b-hdmi-a1:
    - fi-tgl-1115g4:      [PASS][35] -> [INCOMPLETE][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-tgl-1115g4/igt@kms_flip@basic-flip-vs-dpms@b-hdmi-a1.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-tgl-1115g4/igt@kms_flip@basic-flip-vs-dpms@b-hdmi-a1.html

  * igt@kms_force_connector_basic@force-connector-state:
    - bat-rpls-3:         [PASS][37] -> [ABORT][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-rpls-3/igt@kms_force_connector_basic@force-connector-state.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-rpls-3/igt@kms_force_connector_basic@force-connector-state.html

  * igt@kms_pipe_crc_basic@hang-read-crc@pipe-a-hdmi-a-2:
    - bat-dg2-14:         [PASS][39] -> [DMESG-WARN][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-dg2-14/igt@kms_pipe_crc_basic@hang-read-crc@pipe-a-hdmi-a-2.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-14/igt@kms_pipe_crc_basic@hang-read-crc@pipe-a-hdmi-a-2.html

  * igt@kms_pm_rpm@basic-pci-d3-state:
    - bat-arls-1:         [PASS][41] -> [ABORT][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-arls-1/igt@kms_pm_rpm@basic-pci-d3-state.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-arls-1/igt@kms_pm_rpm@basic-pci-d3-state.html

  * igt@kms_pm_rpm@basic-rte:
    - bat-dg2-11:         NOTRUN -> [ABORT][43]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@kms_pm_rpm@basic-rte.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_flip@basic-flip-vs-wf_vblank@a-dp6:
    - {bat-mtlp-9}:       [PASS][44] -> [ABORT][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-mtlp-9/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp6.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-mtlp-9/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp6.html

  
Known issues
------------

  Here are the changes found in Patchwork_132108v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - fi-glk-j4005:       NOTRUN -> [SKIP][46]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-glk-j4005/igt@debugfs_test@basic-hwmon.html

  * igt@gem_exec_parallel@engines@fds:
    - fi-blb-e6850:       NOTRUN -> [SKIP][47] +11 other tests skip
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-blb-e6850/igt@gem_exec_parallel@engines@fds.html

  * igt@gem_huc_copy@huc-copy:
    - fi-glk-j4005:       NOTRUN -> [SKIP][48] ([i915#2190])
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-glk-j4005/igt@gem_huc_copy@huc-copy.html

  * igt@gem_mmap@basic:
    - bat-dg1-7:          NOTRUN -> [SKIP][49] ([i915#4083])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg1-7/igt@gem_mmap@basic.html
    - bat-dg2-11:         NOTRUN -> [SKIP][50] ([i915#4083])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@gem_mmap@basic.html

  * igt@gem_tiled_fence_blits@basic:
    - bat-dg1-7:          NOTRUN -> [SKIP][51] ([i915#4077]) +2 other tests skip
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg1-7/igt@gem_tiled_fence_blits@basic.html
    - bat-dg2-11:         NOTRUN -> [SKIP][52] ([i915#4077]) +2 other tests skip
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@gem_tiled_fence_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-dg1-7:          NOTRUN -> [SKIP][53] ([i915#4079]) +1 other test skip
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg1-7/igt@gem_tiled_pread_basic.html
    - bat-dg2-11:         NOTRUN -> [SKIP][54] ([i915#4079]) +1 other test skip
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@gem_tiled_pread_basic.html

  * igt@kms_addfb_basic@addfb25-x-tiled-mismatch-legacy:
    - bat-dg1-7:          NOTRUN -> [SKIP][55] ([i915#4212]) +7 other tests skip
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg1-7/igt@kms_addfb_basic@addfb25-x-tiled-mismatch-legacy.html
    - bat-dg2-11:         NOTRUN -> [SKIP][56] ([i915#4212]) +7 other tests skip
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@kms_addfb_basic@addfb25-x-tiled-mismatch-legacy.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-dg2-11:         NOTRUN -> [SKIP][57] ([i915#5190])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg1-7:          NOTRUN -> [SKIP][58] ([i915#4215])
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg1-7/igt@kms_addfb_basic@basic-y-tiled-legacy.html
    - bat-dg2-11:         NOTRUN -> [SKIP][59] ([i915#4215] / [i915#5190])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_busy@basic@modeset:
    - fi-glk-j4005:       NOTRUN -> [INCOMPLETE][60] ([i915#10056])
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-glk-j4005/igt@kms_busy@basic@modeset.html
    - bat-adlp-9:         [PASS][61] -> [INCOMPLETE][62] ([i915#10056])
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-adlp-9/igt@kms_busy@basic@modeset.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-adlp-9/igt@kms_busy@basic@modeset.html
    - fi-rkl-11600:       [PASS][63] -> [INCOMPLETE][64] ([i915#10056])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/fi-rkl-11600/igt@kms_busy@basic@modeset.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/fi-rkl-11600/igt@kms_busy@basic@modeset.html
    - bat-jsl-1:          [PASS][65] -> [INCOMPLETE][66] ([i915#10056])
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-jsl-1/igt@kms_busy@basic@modeset.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-jsl-1/igt@kms_busy@basic@modeset.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - bat-dg2-11:         NOTRUN -> [SKIP][67] ([i915#4103] / [i915#4213]) +1 other test skip
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_dsc@dsc-basic:
    - bat-dg2-11:         NOTRUN -> [SKIP][68] ([i915#3555] / [i915#3840])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg2-11:         NOTRUN -> [SKIP][69]
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-dg2-11:         NOTRUN -> [SKIP][70] ([i915#5274])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-dg2-11:         NOTRUN -> [SKIP][71] ([i915#5354])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-dg2-11/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-primary-mmap-gtt@edp-1:
    - bat-arls-2:         NOTRUN -> [SKIP][72] ([i915#10196] / [i915#4077] / [i915#9688])
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-arls-2/igt@kms_psr@psr-primary-mmap-gtt@edp-1.html

  
#### Possible fixes ####

  * igt@dmabuf@all-tests@dma_fence:
    - bat-adlp-11:        [DMESG-FAIL][73] -> [PASS][74]
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-adlp-11/igt@dmabuf@all-tests@dma_fence.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-adlp-11/igt@dmabuf@all-tests@dma_fence.html

  * igt@dmabuf@all-tests@sanitycheck:
    - bat-adlp-11:        [ABORT][75] -> [PASS][76]
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-adlp-11/igt@dmabuf@all-tests@sanitycheck.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-adlp-11/igt@dmabuf@all-tests@sanitycheck.html

  * igt@i915_selftest@live@gt_lrc:
    - bat-arls-2:         [ABORT][77] -> [PASS][78]
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-arls-2/igt@i915_selftest@live@gt_lrc.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-arls-2/igt@i915_selftest@live@gt_lrc.html

  * igt@kms_frontbuffer_tracking@basic:
    - bat-arls-2:         [FAIL][79] -> [PASS][80] +18 other tests pass
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-arls-2/igt@kms_frontbuffer_tracking@basic.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-arls-2/igt@kms_frontbuffer_tracking@basic.html

  
#### Warnings ####

  * igt@i915_pm_rps@basic-api:
    - bat-arls-2:         [FAIL][81] -> [SKIP][82] ([i915#10209])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-arls-2/igt@i915_pm_rps@basic-api.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-arls-2/igt@i915_pm_rps@basic-api.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-arls-2:         [FAIL][83] -> [SKIP][84] ([i915#10202]) +1 other test skip
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-arls-2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-arls-2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-arls-2:         [FAIL][85] -> [SKIP][86] ([i915#9886])
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-arls-2/igt@kms_dsc@dsc-basic.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-arls-2/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-arls-2:         [FAIL][87] -> [SKIP][88] ([i915#10207])
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-arls-2/igt@kms_force_connector_basic@force-load-detect.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-arls-2/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-arls-2:         [FAIL][89] -> [SKIP][90] ([i915#10208] / [i915#8809])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14537/bat-arls-2/igt@kms_setmode@basic-clone-single-crtc.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/bat-arls-2/igt@kms_setmode@basic-clone-single-crtc.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#10056]: https://gitlab.freedesktop.org/drm/intel/issues/10056
  [i915#10196]: https://gitlab.freedesktop.org/drm/intel/issues/10196
  [i915#10202]: https://gitlab.freedesktop.org/drm/intel/issues/10202
  [i915#10207]: https://gitlab.freedesktop.org/drm/intel/issues/10207
  [i915#10208]: https://gitlab.freedesktop.org/drm/intel/issues/10208
  [i915#10209]: https://gitlab.freedesktop.org/drm/intel/issues/10209
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
  [i915#9688]: https://gitlab.freedesktop.org/drm/intel/issues/9688
  [i915#9886]: https://gitlab.freedesktop.org/drm/intel/issues/9886


Build changes
-------------

  * Linux: CI_DRM_14537 -> Patchwork_132108v1

  CI-20190529: 20190529
  CI_DRM_14537: 11155f150a41d028c6ac7a141924fd4c8798721d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7801: 7801
  Patchwork_132108v1: 11155f150a41d028c6ac7a141924fd4c8798721d @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

cb6833dde569 drm/i915/display: Introduce Display Metrics info

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132108v1/index.html

[-- Attachment #2: Type: text/html, Size: 22146 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] drm/i915/display: Introduce Display Metrics info
  2024-04-05 22:08 [PATCH] drm/i915/display: Introduce Display Metrics info Rodrigo Vivi
                   ` (2 preceding siblings ...)
  2024-04-05 22:56 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2024-05-06 10:03 ` Kumar, Naveen1
  2024-05-06 17:22   ` Rodrigo Vivi
  3 siblings, 1 reply; 10+ messages in thread
From: Kumar, Naveen1 @ 2024-05-06 10:03 UTC (permalink / raw)
  To: Vivi, Rodrigo, intel-gfx
  Cc: Shankar, Uma, Kulkarni, Vandita, Nikula, Jani, Belgaumkar, Vinay,
	Borah, Chaitanya Kumar



>-----Original Message-----
>From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Sent: Saturday, April 6, 2024 3:39 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Shankar, Uma
><uma.shankar@intel.com>; Kulkarni, Vandita <vandita.kulkarni@intel.com>;
>Kumar, Naveen1 <naveen1.kumar@intel.com>; Nikula, Jani
><jani.nikula@intel.com>; Belgaumkar, Vinay <vinay.belgaumkar@intel.com>
>Subject: [PATCH] drm/i915/display: Introduce Display Metrics info
>
>Introduce a display metrics information through debugfs for a better view of
>the vblank and page flips, in special Async flips behavior.
>
>There is currently an overall expectation that whenever
>vblank_mode=0 is used with an graphics application, that automatically
>async_flips are happening. However, while implementing the Display Metrics
>for GuC SLPC, it was observed that it is not necessarily true for many of the
>expected cases.
>
>So, while the GuC SLPC side of the metrics doesn't get ready, let's at least bring
>the debugfs view of it so we can work to understand and fix any potential issue
>around our async vblanks.
>
>Please notice that the every struct here follows exactly the GuC shared data
>buffer, so the next step of the integration would be smooth and almost
>transparent to this intel_metrics on the display side.
>
>Cc: Uma Shankar <uma.shankar@intel.com>
>Cc: Vandita Kulkarni <vandita.kulkarni@intel.com>
>Cc: Naveen Kumar <naveen1.kumar@intel.com>
>Cc: Jani Nikula <jani.nikula@intel.com>
>Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>---
> drivers/gpu/drm/i915/Makefile                 |   1 +
> drivers/gpu/drm/i915/display/intel_display.c  |  15 +-
> .../gpu/drm/i915/display/intel_display_core.h |   2 +
> .../drm/i915/display/intel_display_debugfs.c  |  12 +
> .../drm/i915/display/intel_display_driver.c   |   5 +
> .../gpu/drm/i915/display/intel_display_irq.c  |   3 +
> drivers/gpu/drm/i915/display/intel_metrics.c  | 356 ++++++++++++++++++
>drivers/gpu/drm/i915/display/intel_metrics.h  |  27 ++
> .../drm/i915/display/skl_universal_plane.c    |   3 +
> drivers/gpu/drm/xe/Makefile                   |   1 +
> 10 files changed, 424 insertions(+), 1 deletion(-)  create mode 100644
>drivers/gpu/drm/i915/display/intel_metrics.c
> create mode 100644 drivers/gpu/drm/i915/display/intel_metrics.h
>
>diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>index af9e871daf1d..a3c8d9f5614c 100644
>--- a/drivers/gpu/drm/i915/Makefile
>+++ b/drivers/gpu/drm/i915/Makefile
>@@ -291,6 +291,7 @@ i915-y += \
> 	display/intel_link_bw.o \
> 	display/intel_load_detect.o \
> 	display/intel_lpe_audio.o \
>+	display/intel_metrics.o \
> 	display/intel_modeset_lock.o \
> 	display/intel_modeset_setup.o \
> 	display/intel_modeset_verify.o \
>diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>b/drivers/gpu/drm/i915/display/intel_display.c
>index a481c9218138..ca30b8d48e1f 100644
>--- a/drivers/gpu/drm/i915/display/intel_display.c
>+++ b/drivers/gpu/drm/i915/display/intel_display.c
>@@ -94,6 +94,7 @@
> #include "intel_link_bw.h"
> #include "intel_lvds.h"
> #include "intel_lvds_regs.h"
>+#include "intel_metrics.h"
> #include "intel_modeset_setup.h"
> #include "intel_modeset_verify.h"
> #include "intel_overlay.h"
>@@ -1021,11 +1022,15 @@ static void intel_post_plane_update(struct
>intel_atomic_state *state,
> 				    struct intel_crtc *crtc)
> {
> 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>+	struct intel_display *display = &dev_priv->display;
> 	const struct intel_crtc_state *old_crtc_state =
> 		intel_atomic_get_old_crtc_state(state, crtc);
> 	const struct intel_crtc_state *new_crtc_state =
> 		intel_atomic_get_new_crtc_state(state, crtc);
> 	enum pipe pipe = crtc->pipe;
>+	const struct intel_plane_state __maybe_unused *plane_state;
>+	struct intel_plane *plane;
>+	int i;
>
> 	intel_psr_post_plane_update(state, crtc);
>
>@@ -1057,6 +1062,12 @@ static void intel_post_plane_update(struct
>intel_atomic_state *state,
>
> 	if (audio_enabling(old_crtc_state, new_crtc_state))
> 		intel_encoders_audio_enable(state, crtc);
>+
>+	if (!new_crtc_state->do_async_flip) {
>+		for_each_new_intel_plane_in_state(state, plane, plane_state, i)
>+			intel_metrics_flip(display, new_crtc_state, plane->id,
>+					   false);
>+	}
> }
>
> static void intel_crtc_enable_flip_done(struct intel_atomic_state *state, @@ -
>7139,6 +7150,7 @@ static void intel_atomic_commit_tail(struct
>intel_atomic_state *state)  {
> 	struct drm_device *dev = state->base.dev;
> 	struct drm_i915_private *dev_priv = to_i915(dev);
>+	struct intel_display *display = &dev_priv->display;
> 	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> 	struct intel_crtc *crtc;
> 	struct intel_power_domain_mask put_domains[I915_MAX_PIPES] = {};
>@@ -7261,7 +7273,6 @@ static void intel_atomic_commit_tail(struct
>intel_atomic_state *state)
> 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> 		if (new_crtc_state->do_async_flip)
> 			intel_crtc_disable_flip_done(state, crtc);
>-
> 		intel_color_wait_commit(new_crtc_state);
> 	}
>
>@@ -7314,6 +7325,8 @@ static void intel_atomic_commit_tail(struct
>intel_atomic_state *state)
> 		 * FIXME get rid of this funny new->old swapping
> 		 */
> 		old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb);
>+
>+		intel_metrics_refresh_info(display, new_crtc_state);
> 	}
>
> 	/* Underruns don't always raise interrupts, so check manually */ diff --
>git a/drivers/gpu/drm/i915/display/intel_display_core.h
>b/drivers/gpu/drm/i915/display/intel_display_core.h
>index 2167dbee5eea..992db9098566 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_core.h
>+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>@@ -42,6 +42,7 @@ struct intel_cdclk_vals;  struct intel_color_funcs;  struct
>intel_crtc;  struct intel_crtc_state;
>+struct intel_display_metrics;
> struct intel_dmc;
> struct intel_dpll_funcs;
> struct intel_dpll_mgr;
>@@ -530,6 +531,7 @@ struct intel_display {
> 	struct intel_fbc *fbc[I915_MAX_FBCS];
> 	struct intel_frontbuffer_tracking fb_tracking;
> 	struct intel_hotplug hotplug;
>+	struct intel_display_metrics *metrics;
> 	struct intel_opregion *opregion;
> 	struct intel_overlay *overlay;
> 	struct intel_display_params params;
>diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>index 3e364891dcd0..f05b9a9ddee0 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>@@ -30,6 +30,7 @@
> #include "intel_hdcp.h"
> #include "intel_hdmi.h"
> #include "intel_hotplug.h"
>+#include "intel_metrics.h"
> #include "intel_panel.h"
> #include "intel_psr.h"
> #include "intel_psr_regs.h"
>@@ -642,6 +643,16 @@ static int i915_display_capabilities(struct seq_file *m,
>void *unused)
> 	return 0;
> }
>
>+static int i915_display_metrics(struct seq_file *m, void *unused) {
>+	struct drm_i915_private *i915 = node_to_i915(m->private);
>+	struct drm_printer p = drm_seq_file_printer(m);
>+
>+	intel_metrics_show(&i915->display, &p);
>+
>+	return 0;
>+}
>+
> static int i915_shared_dplls_info(struct seq_file *m, void *unused)  {
> 	struct drm_i915_private *dev_priv = node_to_i915(m->private); @@ -
>1059,6 +1070,7 @@ static const struct drm_info_list
>intel_display_debugfs_list[] = {
> 	{"i915_power_domain_info", i915_power_domain_info, 0},
> 	{"i915_display_info", i915_display_info, 0},
> 	{"i915_display_capabilities", i915_display_capabilities, 0},
>+	{"i915_display_metrics", i915_display_metrics, 0},
> 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
> 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
> 	{"i915_ddb_info", i915_ddb_info, 0},
>diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
>b/drivers/gpu/drm/i915/display/intel_display_driver.c
>index 87dd07e0d138..767b2d5b3826 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>@@ -46,6 +46,7 @@
> #include "intel_hdcp.h"
> #include "intel_hotplug.h"
> #include "intel_hti.h"
>+#include "intel_metrics.h"
> #include "intel_modeset_lock.h"
> #include "intel_modeset_setup.h"
> #include "intel_opregion.h"
>@@ -514,6 +515,8 @@ int intel_display_driver_probe(struct
>drm_i915_private *i915)
>
> 	intel_overlay_setup(i915);
>
>+	intel_metrics_init(&i915->display);
>+
> 	ret = intel_fbdev_init(&i915->drm);
> 	if (ret)
> 		return ret;
>@@ -611,6 +614,8 @@ void intel_display_driver_remove_noirq(struct
>drm_i915_private *i915)
>
> 	intel_dp_tunnel_mgr_cleanup(i915);
>
>+	intel_metrics_fini(&i915->display);
>+
> 	intel_overlay_cleanup(i915);
>
> 	intel_gmbus_teardown(i915);
>diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
>b/drivers/gpu/drm/i915/display/intel_display_irq.c
>index f846c5b108b5..202400a771b2 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_irq.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
>@@ -18,6 +18,7 @@
> #include "intel_fifo_underrun.h"
> #include "intel_gmbus.h"
> #include "intel_hotplug_irq.h"
>+#include "intel_metrics.h"
> #include "intel_pmdemand.h"
> #include "intel_psr.h"
> #include "intel_psr_regs.h"
>@@ -25,8 +26,10 @@
> static void
> intel_handle_vblank(struct drm_i915_private *dev_priv, enum pipe pipe)  {
>+	struct intel_display *display = &dev_priv->display;
> 	struct intel_crtc *crtc = intel_crtc_for_pipe(dev_priv, pipe);
>
>+	intel_metrics_vblank(display, crtc);
> 	drm_crtc_handle_vblank(&crtc->base);
> }
>
>diff --git a/drivers/gpu/drm/i915/display/intel_metrics.c
>b/drivers/gpu/drm/i915/display/intel_metrics.c
>new file mode 100644
>index 000000000000..2cb2b8189b25
>--- /dev/null
>+++ b/drivers/gpu/drm/i915/display/intel_metrics.c
>@@ -0,0 +1,356 @@
>+// SPDX-License-Identifier: MIT
>+/*
>+ * Copyright © 2024 Intel Corporation
>+ */
>+
>+#include "intel_metrics.h"
>+
>+#include <drm/drm_modes.h>
>+#include <drm/drm_print.h>
>+
>+#include "i915_drv.h"
>+#include "intel_de.h"
>+#include "intel_display_types.h"
>+
>+/**
>+ * Display Metrics
>+ *
>+ * Provide some display activity overview such as active refresh rates,
>+ * vblank activity and page flip activities.
>+ * For now it is informative debug only, but later it will be expanded
>+ * to be used for GT frequency selection by GuC SLPC.
>+ */
>+
>+/*
>+ * An event using an work queue is used to avoid any disturbance in the
>+ * critical path that could cause performance impacts.
>+ */
>+struct display_event {
>+	struct work_struct work;
>+	struct drm_i915_private *i915;
>+	struct intel_display *display;
>+	bool is_vblank;
>+	int pipe;
>+	int plane;
>+	bool async_flip;
>+};
>+
>+/*
>+ * Although we could simply save this inside our crtc structs, we are
>+ * already mimicking the GuC SLPC defition of the display data, for
>+future
>+ * usage.
>+ */
>+#define MAX_PIPES		8
>+#define MAX_PLANES_PER_PIPE	8
>+
>+struct display_global_info {
>+	u32 version:8;
>+	u32 num_pipes:4;
>+	u32 num_planes_per_pipe:4;
>+	u32 reserved_1:16;
>+	u32 refresh_count:16;
>+	u32 vblank_count:16;
>+	u32 flip_count:16;
>+	u32 reserved_2:16;
>+	u32 reserved_3[13];
>+} __packed;
>+
>+struct display_refresh_info {
>+	u32 refresh_interval:16;
>+	u32 is_variable:1;
>+	u32 reserved:15;
>+} __packed;
>+
>+/*
>+ * When used with GuC SLPC, the host must update each 32-bit part with
>+a single
>+ * atomic write so that SLPC will read the contained bit fields together.
>+ * The host must update the two parts in order - total flip count and
>+timestamp
>+ * first, vsync and async flip counts second.
>+ * Hence, these items are not defined with individual bitfields.
>+ */
>+#define FLIP_P1_LAST		REG_GENMASK(31, 7)
>+#define FLIP_P1_TOTAL_COUNT	REG_GENMASK(6, 0)
>+#define FLIP_P2_ASYNC_COUNT	REG_GENMASK(31, 16)
>+#define FLIP_P2_VSYNC_COUNT	REG_GENMASK(15, 0)
>+
>+struct display_flip_metrics {
>+	u32 part1;
>+	u32 part2;
>+} __packed;
>+
>+/*
>+ * When used with GuC SLPC, the host must update each 32-bit part with
>+a single
>+ * atomic write, so that SLPC will read the count and timestamp together.
>+ * Hence, this item is not defined with individual bitfields.
>+ */
>+#define VBLANK_LAST	REG_GENMASK(31, 7)
>+#define VBLANK_COUNT	REG_GENMASK(6, 0)
>+
>+struct intel_display_metrics {
>+	struct display_global_info global_info;
>+	struct display_refresh_info refresh_info[MAX_PIPES];
>+	u32 vblank_metrics[MAX_PIPES];
>+	struct display_flip_metrics
>+	flip_metrics[MAX_PIPES][MAX_PLANES_PER_PIPE];
>+} __packed;
>+
>+/**
>+ * intel_metrics_refresh_info - Refresh rate information
>+ * @display: Pointer to the intel_display struct that is in use by the gfx device.
>+ * @crtc_state: New CRTC state upon a modeset.
>+ *
>+ * To be called on a modeset. It then saves the current refresh
>+interval in
>+ * micro seconds.
>+ */
>+void intel_metrics_refresh_info(struct intel_display *display,
>+				struct intel_crtc_state *crtc_state) {
>+	struct intel_display_metrics *metrics = display->metrics;
>+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>+	struct drm_display_mode *mode = &crtc_state->hw.adjusted_mode;
>+	u32 interval_us;
>+
>+	if (!display->metrics)
>+		return;
>+
>+	interval_us = crtc_state->hw.active ? DIV_ROUND_UP(1000000,
>+						drm_mode_vrefresh(mode)) :
>0;
>+
>+	metrics->refresh_info[crtc->pipe].refresh_interval = interval_us;
>+	metrics->refresh_info[crtc->pipe].is_variable = crtc_state-
>>uapi.vrr_enabled;
>+	metrics->global_info.refresh_count += 1; }
>+
>+static void metrics_update_vblank(struct intel_display_metrics *metrics, int
>pipe,
>+				  u32 timestamp)
>+{
>+	u32 vblank;
>+
>+	vblank = metrics->vblank_metrics[pipe];
>+
>+	vblank = REG_FIELD_GET(VBLANK_COUNT, vblank);
>+	vblank = REG_FIELD_PREP(VBLANK_COUNT, vblank + 1);
>+	vblank |= REG_FIELD_PREP(VBLANK_LAST, timestamp);
>+
>+	/* Write everything at once in preparation for the GuC SLPC
>requirement */
>+	metrics->vblank_metrics[pipe] = vblank;
>+	metrics->global_info.vblank_count += 1; }
>+
>+static void metrics_update_flip(struct intel_display_metrics *metrics, int pipe,
>+				int plane, bool async_flip, u32 timestamp) {
>+	u32 part1, part2, count;
>+
>+	part1 = metrics->flip_metrics[pipe][plane].part1;
>+	part2 = metrics->flip_metrics[pipe][plane].part2;
>+
>+	part1 = REG_FIELD_GET(FLIP_P1_TOTAL_COUNT, part1);
>+	part1 = REG_FIELD_PREP(FLIP_P1_TOTAL_COUNT, part1 + 1);
>+	part1 |= REG_FIELD_PREP(FLIP_P1_LAST, timestamp);
>+
>+	if (async_flip) {
>+		count = REG_FIELD_GET(FLIP_P2_ASYNC_COUNT, part2);
>+		part2 &= ~FLIP_P2_ASYNC_COUNT;
>+		part2 |= REG_FIELD_PREP(FLIP_P2_ASYNC_COUNT, count + 1);
>+	} else {
>+		count = REG_FIELD_GET(FLIP_P2_VSYNC_COUNT, part2);
>+		part2 &= ~FLIP_P2_VSYNC_COUNT;
>+		part2 |= REG_FIELD_PREP(FLIP_P2_VSYNC_COUNT, count + 1);
>+	}
>+
>+	/*
>+	 * Write everything in this way and this order in preparation for the
>+	 * GuC SLPC requirement
>+	 */
>+	metrics->flip_metrics[pipe][plane].part1 = part1;
>+	metrics->flip_metrics[pipe][plane].part2 = part2;
>+
>+	metrics->global_info.flip_count += 1;
>+}
>+
>+/*
>+ * Let's use the same register GuC SLPC uses for timestamp.
>+ * It uses a register that is outside GT domain so GuC doesn't need
>+ * to wake the GT for reading during SLPC loop.
>+ * This is a single register regarding the GT, so we can read directly
>+ * from here, regarding the GT GuC is in.
>+ */
>+#define MCHBAR_MIRROR_BASE_SNB	0x140000
>+#define MCHBAR_BCLK_COUNT	_MMIO(MCHBAR_MIRROR_BASE_SNB
>+ 0x5984)
>+#define MTL_BCLK_COUNT		_MMIO(0xc28)
>+#define   TIMESTAMP_MASK	REG_GENMASK(30, 6)
>+
>+static u32 bclk_read_timestamp(struct drm_i915_private *i915) {
>+	u32 timestamp;
>+
>+	if (DISPLAY_VER(i915) >= 14)
>+		timestamp = intel_de_read(i915, MTL_BCLK_COUNT);
>+	else
>+		timestamp = intel_de_read(i915, MCHBAR_BCLK_COUNT);
>+
>+	return REG_FIELD_GET(TIMESTAMP_MASK, timestamp); }
>+
>+static void display_event_work(struct work_struct *work) {
>+	struct display_event *event = container_of(work, struct display_event,
>work);
>+	struct intel_display *display = event->display;
>+	u32 timestamp;
>+
>+	timestamp = bclk_read_timestamp(event->i915);
>+
>+	if (event->is_vblank) {
>+		metrics_update_vblank(display->metrics, event->pipe,
>timestamp);
>+	} else {
>+		metrics_update_flip(display->metrics, event->pipe, event-
>>plane,
>+				    event->async_flip, timestamp);
>+	}
>+
>+	kfree(event);
>+}
>+
>+void intel_metrics_init(struct intel_display *display) {
>+	struct intel_display_metrics *metrics;
>+
>+	metrics = kzalloc(sizeof(*metrics), GFP_KERNEL);
>+	if (!metrics)
>+		return;
>+
>+	metrics->global_info.version = 1;
>+	metrics->global_info.num_pipes = MAX_PIPES;
>+	metrics->global_info.num_planes_per_pipe = MAX_PLANES_PER_PIPE;
>+
>+	display->metrics = metrics;
>+}
>+
>+void intel_metrics_fini(struct intel_display *display) {
>+	if (!display->metrics)
>+		return;
>+
>+	kfree(display->metrics);
>+}
>+
>+/**
>+ * intel_metrics_vblank - Vblank information
>+ * @display: Pointer to the intel_display struct that is in use by the gfx device.
>+ * @crtc: The Intel CRTC that received the vblank interrupt.
>+ *
>+ * To be called when a vblank is passed.
>+ */
>+void intel_metrics_vblank(struct intel_display *display,
>+			  struct intel_crtc *crtc)
>+{
>+	struct display_event *event;
>+
>+	if (!display->metrics)
>+		return;
>+
>+	event = kmalloc(sizeof(*event), GFP_ATOMIC);

HI Rodrigo, after changing kmalloc -> kzalloc, it works fine for us. It does correctly prints Global Flip count and Async Flip count.
Prior to this change, event->is_vblank in function display_event_work is always true and hence it never increases flip count.

Thanks Chaitanya for pointing this out.
Regards,
Naveen Kumar

>+	if (!event)
>+		return;
>+
>+	INIT_WORK(&event->work, display_event_work);
>+	event->i915 = to_i915(crtc->base.dev);
>+	event->display = display;
>+	event->is_vblank = true;
>+	event->pipe = crtc->pipe;
>+	queue_work(system_highpri_wq, &event->work); }
>+
>+/**
>+ * intel_metrics_flip - Flip information
>+ * @display: Pointer to the intel_display struct that is in use by the gfx device.
>+ * @crtc_state: New CRTC state upon a page flip.
>+ * @plane: Which plane ID got the page flip.
>+ * @async_flip: A boolean to indicate if the page flip was async.
>+ *
>+ * To be called on a page flip.
>+ */
>+void intel_metrics_flip(struct intel_display *display,
>+			const struct intel_crtc_state *crtc_state,
>+			int plane, bool async_flip)
>+{
>+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>+	struct display_event *event;
>+
>+	if (!display->metrics)
>+		return;
>+
>+	event = kmalloc(sizeof(*event), GFP_ATOMIC);
Same here, had to change kmalloc -> kzalloc

>+	if (!event)
>+		return;
>+
>+	INIT_WORK(&event->work, display_event_work);
>+	event->i915 = to_i915(crtc->base.dev);
>+	event->display = display;
>+	event->pipe = crtc->pipe;
>+	event->plane = plane;
>+	event->async_flip = async_flip;
>+	queue_work(system_highpri_wq, &event->work); }
>+
>+void intel_metrics_show(struct intel_display *display, struct
>+drm_printer *p) {
>+	struct intel_display_metrics *metrics = display->metrics;
>+	int pipe, plane;
>+	u32 val;
>+
>+	if (!metrics)
>+		return;
>+
>+	drm_printf(p, "\nDisplay Metrics - Globals:\n");
>+	drm_printf(p, "\tVersion: %d\n", metrics->global_info.version);
>+	drm_printf(p, "\tNum Pipes: %d\n", metrics->global_info.num_pipes);
>+	drm_printf(p, "\tNum Planes per Pipe: %d\n",
>+		   metrics->global_info.num_planes_per_pipe);
>+	drm_printf(p, "\tGlobal Refresh Info Count: %d\n",
>+		   metrics->global_info.refresh_count);
>+	drm_printf(p, "\tGlobal Vblank Count: %d\n",
>+		   metrics->global_info.vblank_count);
>+	drm_printf(p, "\tGlobal Flip Count: %d\n",
>+		   metrics->global_info.flip_count);
>+
>+	for (pipe = 0; pipe < MAX_PIPES; pipe++) {
>+		if (!metrics->refresh_info[pipe].refresh_interval)
>+			continue;
>+
>+		drm_printf(p, "\nDisplay Metrics - Refresh Info - Pipe[%d]:\n",
>+			   pipe);
>+		drm_printf(p, "\tRefresh Interval: %d\n",
>+			   metrics->refresh_info[pipe].refresh_interval);
>+		drm_printf(p, "\tIS VRR: %d\n",
>+			   metrics->refresh_info[pipe].is_variable);
>+
>+		drm_printf(p, "Display Metrics - Vblank Info - Pipe[%d]:\n",
>+			   pipe);
>+		val = metrics->vblank_metrics[pipe];
>+		drm_printf(p, "\tVBlank Last Timestamp: %x\n",
>+			   REG_FIELD_GET(VBLANK_LAST, val));
>+		drm_printf(p, "\tVBlank Count: %d\n",
>+			   REG_FIELD_GET(VBLANK_COUNT, val));
>+
>+		drm_printf(p, "Display Metrics - Flip Info - Pipe[%d]:\n", pipe);
>+		for (plane = 0; plane < MAX_PLANES_PER_PIPE; plane++) {
>+			val = metrics->flip_metrics[pipe][plane].part1;
>+			if (!val)
>+				continue;
>+
>+			drm_printf(p, "\tFlip Info - Plane[%d]:\n", plane);
>+			drm_printf(p, "\t\tFlip Last Timestamp: %x\n",
>+				   REG_FIELD_GET(FLIP_P1_LAST, val));
>+			drm_printf(p, "\t\tFlip Total Count: %d\n",
>+				   REG_FIELD_GET(FLIP_P1_TOTAL_COUNT,
>val));
>+
>+			val = metrics->flip_metrics[pipe][plane].part2;
>+
>+			drm_printf(p, "\t\tFlip Async Count: %d\n",
>+				   REG_FIELD_GET(FLIP_P2_ASYNC_COUNT,
>val));
>+			drm_printf(p, "\t\tFlip Vsync Count: %d\n",
>+				   REG_FIELD_GET(FLIP_P2_VSYNC_COUNT,
>val));
>+		}
>+	}
>+}
>diff --git a/drivers/gpu/drm/i915/display/intel_metrics.h
>b/drivers/gpu/drm/i915/display/intel_metrics.h
>new file mode 100644
>index 000000000000..9e41dc9522f3
>--- /dev/null
>+++ b/drivers/gpu/drm/i915/display/intel_metrics.h
>@@ -0,0 +1,27 @@
>+/* SPDX-License-Identifier: MIT */
>+/*
>+ * Copyright © 2024 Intel Corporation
>+ */
>+
>+#ifndef __INTEL_METRICS_H__
>+#define __INTEL_METRICS_H__
>+
>+#include <linux/types.h>
>+
>+struct drm_printer;
>+struct intel_crtc;
>+struct intel_crtc_state;
>+struct intel_display;
>+
>+void intel_metrics_refresh_info(struct intel_display *display,
>+				struct intel_crtc_state *crtc_state); void
>+intel_metrics_vblank(struct intel_display *display,
>+			  struct intel_crtc *intel_crtc);
>+void intel_metrics_flip(struct intel_display *display,
>+			const struct intel_crtc_state *crtc_state,
>+			int plane, bool async_flip);
>+void intel_metrics_init(struct intel_display *display); void
>+intel_metrics_fini(struct intel_display *display); void
>+intel_metrics_show(struct intel_display *display, struct drm_printer
>+*p);
>+
>+#endif
>diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>index 860574d04f88..fdd21a41d79d 100644
>--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>@@ -17,6 +17,7 @@
> #include "intel_fb.h"
> #include "intel_fbc.h"
> #include "intel_frontbuffer.h"
>+#include "intel_metrics.h"
> #include "intel_psr.h"
> #include "intel_psr_regs.h"
> #include "skl_scaler.h"
>@@ -1392,6 +1393,7 @@ skl_plane_async_flip(struct intel_plane *plane,
> 		     bool async_flip)
> {
> 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>+	struct intel_display *display = &dev_priv->display;
> 	enum plane_id plane_id = plane->id;
> 	enum pipe pipe = plane->pipe;
> 	u32 plane_ctl = plane_state->ctl;
>@@ -1404,6 +1406,7 @@ skl_plane_async_flip(struct intel_plane *plane,
> 	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> 	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> 			  skl_plane_surf(plane_state, 0));
>+	intel_metrics_flip(display, crtc_state, plane_id, async_flip);
> }
>
> static bool intel_format_is_p01x(u32 format) diff --git
>a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index
>e5b1715f721e..5133dba2c7de 100644
>--- a/drivers/gpu/drm/xe/Makefile
>+++ b/drivers/gpu/drm/xe/Makefile
>@@ -263,6 +263,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> 	i915-display/intel_hti.o \
> 	i915-display/intel_link_bw.o \
> 	i915-display/intel_lspcon.o \
>+	i915-display/intel_metrics.o \
> 	i915-display/intel_modeset_lock.o \
> 	i915-display/intel_modeset_setup.o \
> 	i915-display/intel_modeset_verify.o \
>--
>2.44.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915/display: Introduce Display Metrics info
  2024-05-06 10:03 ` [PATCH] " Kumar, Naveen1
@ 2024-05-06 17:22   ` Rodrigo Vivi
  2024-05-07  3:19     ` Kumar, Naveen1
  0 siblings, 1 reply; 10+ messages in thread
From: Rodrigo Vivi @ 2024-05-06 17:22 UTC (permalink / raw)
  To: Kumar, Naveen1
  Cc: intel-gfx, Shankar, Uma, Kulkarni, Vandita, Nikula, Jani,
	Belgaumkar, Vinay, Borah, Chaitanya Kumar

On Mon, May 06, 2024 at 06:03:17AM -0400, Kumar, Naveen1 wrote:
> 
> 
> >-----Original Message-----
> >From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> >Sent: Saturday, April 6, 2024 3:39 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Shankar, Uma
> ><uma.shankar@intel.com>; Kulkarni, Vandita <vandita.kulkarni@intel.com>;
> >Kumar, Naveen1 <naveen1.kumar@intel.com>; Nikula, Jani
> ><jani.nikula@intel.com>; Belgaumkar, Vinay <vinay.belgaumkar@intel.com>
> >Subject: [PATCH] drm/i915/display: Introduce Display Metrics info
> >
> >Introduce a display metrics information through debugfs for a better view of
> >the vblank and page flips, in special Async flips behavior.
> >
> >There is currently an overall expectation that whenever
> >vblank_mode=0 is used with an graphics application, that automatically
> >async_flips are happening. However, while implementing the Display Metrics
> >for GuC SLPC, it was observed that it is not necessarily true for many of the
> >expected cases.
> >
> >So, while the GuC SLPC side of the metrics doesn't get ready, let's at least bring
> >the debugfs view of it so we can work to understand and fix any potential issue
> >around our async vblanks.
> >
> >Please notice that the every struct here follows exactly the GuC shared data
> >buffer, so the next step of the integration would be smooth and almost
> >transparent to this intel_metrics on the display side.
> >
> >Cc: Uma Shankar <uma.shankar@intel.com>
> >Cc: Vandita Kulkarni <vandita.kulkarni@intel.com>
> >Cc: Naveen Kumar <naveen1.kumar@intel.com>
> >Cc: Jani Nikula <jani.nikula@intel.com>
> >Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >---
> > drivers/gpu/drm/i915/Makefile                 |   1 +
> > drivers/gpu/drm/i915/display/intel_display.c  |  15 +-
> > .../gpu/drm/i915/display/intel_display_core.h |   2 +
> > .../drm/i915/display/intel_display_debugfs.c  |  12 +
> > .../drm/i915/display/intel_display_driver.c   |   5 +
> > .../gpu/drm/i915/display/intel_display_irq.c  |   3 +
> > drivers/gpu/drm/i915/display/intel_metrics.c  | 356 ++++++++++++++++++
> >drivers/gpu/drm/i915/display/intel_metrics.h  |  27 ++
> > .../drm/i915/display/skl_universal_plane.c    |   3 +
> > drivers/gpu/drm/xe/Makefile                   |   1 +
> > 10 files changed, 424 insertions(+), 1 deletion(-)  create mode 100644
> >drivers/gpu/drm/i915/display/intel_metrics.c
> > create mode 100644 drivers/gpu/drm/i915/display/intel_metrics.h
> >
> >diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> >index af9e871daf1d..a3c8d9f5614c 100644
> >--- a/drivers/gpu/drm/i915/Makefile
> >+++ b/drivers/gpu/drm/i915/Makefile
> >@@ -291,6 +291,7 @@ i915-y += \
> > 	display/intel_link_bw.o \
> > 	display/intel_load_detect.o \
> > 	display/intel_lpe_audio.o \
> >+	display/intel_metrics.o \
> > 	display/intel_modeset_lock.o \
> > 	display/intel_modeset_setup.o \
> > 	display/intel_modeset_verify.o \
> >diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >b/drivers/gpu/drm/i915/display/intel_display.c
> >index a481c9218138..ca30b8d48e1f 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display.c
> >+++ b/drivers/gpu/drm/i915/display/intel_display.c
> >@@ -94,6 +94,7 @@
> > #include "intel_link_bw.h"
> > #include "intel_lvds.h"
> > #include "intel_lvds_regs.h"
> >+#include "intel_metrics.h"
> > #include "intel_modeset_setup.h"
> > #include "intel_modeset_verify.h"
> > #include "intel_overlay.h"
> >@@ -1021,11 +1022,15 @@ static void intel_post_plane_update(struct
> >intel_atomic_state *state,
> > 				    struct intel_crtc *crtc)
> > {
> > 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> >+	struct intel_display *display = &dev_priv->display;
> > 	const struct intel_crtc_state *old_crtc_state =
> > 		intel_atomic_get_old_crtc_state(state, crtc);
> > 	const struct intel_crtc_state *new_crtc_state =
> > 		intel_atomic_get_new_crtc_state(state, crtc);
> > 	enum pipe pipe = crtc->pipe;
> >+	const struct intel_plane_state __maybe_unused *plane_state;
> >+	struct intel_plane *plane;
> >+	int i;
> >
> > 	intel_psr_post_plane_update(state, crtc);
> >
> >@@ -1057,6 +1062,12 @@ static void intel_post_plane_update(struct
> >intel_atomic_state *state,
> >
> > 	if (audio_enabling(old_crtc_state, new_crtc_state))
> > 		intel_encoders_audio_enable(state, crtc);
> >+
> >+	if (!new_crtc_state->do_async_flip) {
> >+		for_each_new_intel_plane_in_state(state, plane, plane_state, i)
> >+			intel_metrics_flip(display, new_crtc_state, plane->id,
> >+					   false);
> >+	}
> > }
> >
> > static void intel_crtc_enable_flip_done(struct intel_atomic_state *state, @@ -
> >7139,6 +7150,7 @@ static void intel_atomic_commit_tail(struct
> >intel_atomic_state *state)  {
> > 	struct drm_device *dev = state->base.dev;
> > 	struct drm_i915_private *dev_priv = to_i915(dev);
> >+	struct intel_display *display = &dev_priv->display;
> > 	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > 	struct intel_crtc *crtc;
> > 	struct intel_power_domain_mask put_domains[I915_MAX_PIPES] = {};
> >@@ -7261,7 +7273,6 @@ static void intel_atomic_commit_tail(struct
> >intel_atomic_state *state)
> > 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > 		if (new_crtc_state->do_async_flip)
> > 			intel_crtc_disable_flip_done(state, crtc);
> >-
> > 		intel_color_wait_commit(new_crtc_state);
> > 	}
> >
> >@@ -7314,6 +7325,8 @@ static void intel_atomic_commit_tail(struct
> >intel_atomic_state *state)
> > 		 * FIXME get rid of this funny new->old swapping
> > 		 */
> > 		old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb);
> >+
> >+		intel_metrics_refresh_info(display, new_crtc_state);
> > 	}
> >
> > 	/* Underruns don't always raise interrupts, so check manually */ diff --
> >git a/drivers/gpu/drm/i915/display/intel_display_core.h
> >b/drivers/gpu/drm/i915/display/intel_display_core.h
> >index 2167dbee5eea..992db9098566 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display_core.h
> >+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> >@@ -42,6 +42,7 @@ struct intel_cdclk_vals;  struct intel_color_funcs;  struct
> >intel_crtc;  struct intel_crtc_state;
> >+struct intel_display_metrics;
> > struct intel_dmc;
> > struct intel_dpll_funcs;
> > struct intel_dpll_mgr;
> >@@ -530,6 +531,7 @@ struct intel_display {
> > 	struct intel_fbc *fbc[I915_MAX_FBCS];
> > 	struct intel_frontbuffer_tracking fb_tracking;
> > 	struct intel_hotplug hotplug;
> >+	struct intel_display_metrics *metrics;
> > 	struct intel_opregion *opregion;
> > 	struct intel_overlay *overlay;
> > 	struct intel_display_params params;
> >diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >index 3e364891dcd0..f05b9a9ddee0 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >@@ -30,6 +30,7 @@
> > #include "intel_hdcp.h"
> > #include "intel_hdmi.h"
> > #include "intel_hotplug.h"
> >+#include "intel_metrics.h"
> > #include "intel_panel.h"
> > #include "intel_psr.h"
> > #include "intel_psr_regs.h"
> >@@ -642,6 +643,16 @@ static int i915_display_capabilities(struct seq_file *m,
> >void *unused)
> > 	return 0;
> > }
> >
> >+static int i915_display_metrics(struct seq_file *m, void *unused) {
> >+	struct drm_i915_private *i915 = node_to_i915(m->private);
> >+	struct drm_printer p = drm_seq_file_printer(m);
> >+
> >+	intel_metrics_show(&i915->display, &p);
> >+
> >+	return 0;
> >+}
> >+
> > static int i915_shared_dplls_info(struct seq_file *m, void *unused)  {
> > 	struct drm_i915_private *dev_priv = node_to_i915(m->private); @@ -
> >1059,6 +1070,7 @@ static const struct drm_info_list
> >intel_display_debugfs_list[] = {
> > 	{"i915_power_domain_info", i915_power_domain_info, 0},
> > 	{"i915_display_info", i915_display_info, 0},
> > 	{"i915_display_capabilities", i915_display_capabilities, 0},
> >+	{"i915_display_metrics", i915_display_metrics, 0},
> > 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
> > 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
> > 	{"i915_ddb_info", i915_ddb_info, 0},
> >diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
> >b/drivers/gpu/drm/i915/display/intel_display_driver.c
> >index 87dd07e0d138..767b2d5b3826 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> >+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> >@@ -46,6 +46,7 @@
> > #include "intel_hdcp.h"
> > #include "intel_hotplug.h"
> > #include "intel_hti.h"
> >+#include "intel_metrics.h"
> > #include "intel_modeset_lock.h"
> > #include "intel_modeset_setup.h"
> > #include "intel_opregion.h"
> >@@ -514,6 +515,8 @@ int intel_display_driver_probe(struct
> >drm_i915_private *i915)
> >
> > 	intel_overlay_setup(i915);
> >
> >+	intel_metrics_init(&i915->display);
> >+
> > 	ret = intel_fbdev_init(&i915->drm);
> > 	if (ret)
> > 		return ret;
> >@@ -611,6 +614,8 @@ void intel_display_driver_remove_noirq(struct
> >drm_i915_private *i915)
> >
> > 	intel_dp_tunnel_mgr_cleanup(i915);
> >
> >+	intel_metrics_fini(&i915->display);
> >+
> > 	intel_overlay_cleanup(i915);
> >
> > 	intel_gmbus_teardown(i915);
> >diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
> >b/drivers/gpu/drm/i915/display/intel_display_irq.c
> >index f846c5b108b5..202400a771b2 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> >+++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> >@@ -18,6 +18,7 @@
> > #include "intel_fifo_underrun.h"
> > #include "intel_gmbus.h"
> > #include "intel_hotplug_irq.h"
> >+#include "intel_metrics.h"
> > #include "intel_pmdemand.h"
> > #include "intel_psr.h"
> > #include "intel_psr_regs.h"
> >@@ -25,8 +26,10 @@
> > static void
> > intel_handle_vblank(struct drm_i915_private *dev_priv, enum pipe pipe)  {
> >+	struct intel_display *display = &dev_priv->display;
> > 	struct intel_crtc *crtc = intel_crtc_for_pipe(dev_priv, pipe);
> >
> >+	intel_metrics_vblank(display, crtc);
> > 	drm_crtc_handle_vblank(&crtc->base);
> > }
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_metrics.c
> >b/drivers/gpu/drm/i915/display/intel_metrics.c
> >new file mode 100644
> >index 000000000000..2cb2b8189b25
> >--- /dev/null
> >+++ b/drivers/gpu/drm/i915/display/intel_metrics.c
> >@@ -0,0 +1,356 @@
> >+// SPDX-License-Identifier: MIT
> >+/*
> >+ * Copyright © 2024 Intel Corporation
> >+ */
> >+
> >+#include "intel_metrics.h"
> >+
> >+#include <drm/drm_modes.h>
> >+#include <drm/drm_print.h>
> >+
> >+#include "i915_drv.h"
> >+#include "intel_de.h"
> >+#include "intel_display_types.h"
> >+
> >+/**
> >+ * Display Metrics
> >+ *
> >+ * Provide some display activity overview such as active refresh rates,
> >+ * vblank activity and page flip activities.
> >+ * For now it is informative debug only, but later it will be expanded
> >+ * to be used for GT frequency selection by GuC SLPC.
> >+ */
> >+
> >+/*
> >+ * An event using an work queue is used to avoid any disturbance in the
> >+ * critical path that could cause performance impacts.
> >+ */
> >+struct display_event {
> >+	struct work_struct work;
> >+	struct drm_i915_private *i915;
> >+	struct intel_display *display;
> >+	bool is_vblank;
> >+	int pipe;
> >+	int plane;
> >+	bool async_flip;
> >+};
> >+
> >+/*
> >+ * Although we could simply save this inside our crtc structs, we are
> >+ * already mimicking the GuC SLPC defition of the display data, for
> >+future
> >+ * usage.
> >+ */
> >+#define MAX_PIPES		8
> >+#define MAX_PLANES_PER_PIPE	8
> >+
> >+struct display_global_info {
> >+	u32 version:8;
> >+	u32 num_pipes:4;
> >+	u32 num_planes_per_pipe:4;
> >+	u32 reserved_1:16;
> >+	u32 refresh_count:16;
> >+	u32 vblank_count:16;
> >+	u32 flip_count:16;
> >+	u32 reserved_2:16;
> >+	u32 reserved_3[13];
> >+} __packed;
> >+
> >+struct display_refresh_info {
> >+	u32 refresh_interval:16;
> >+	u32 is_variable:1;
> >+	u32 reserved:15;
> >+} __packed;
> >+
> >+/*
> >+ * When used with GuC SLPC, the host must update each 32-bit part with
> >+a single
> >+ * atomic write so that SLPC will read the contained bit fields together.
> >+ * The host must update the two parts in order - total flip count and
> >+timestamp
> >+ * first, vsync and async flip counts second.
> >+ * Hence, these items are not defined with individual bitfields.
> >+ */
> >+#define FLIP_P1_LAST		REG_GENMASK(31, 7)
> >+#define FLIP_P1_TOTAL_COUNT	REG_GENMASK(6, 0)
> >+#define FLIP_P2_ASYNC_COUNT	REG_GENMASK(31, 16)
> >+#define FLIP_P2_VSYNC_COUNT	REG_GENMASK(15, 0)
> >+
> >+struct display_flip_metrics {
> >+	u32 part1;
> >+	u32 part2;
> >+} __packed;
> >+
> >+/*
> >+ * When used with GuC SLPC, the host must update each 32-bit part with
> >+a single
> >+ * atomic write, so that SLPC will read the count and timestamp together.
> >+ * Hence, this item is not defined with individual bitfields.
> >+ */
> >+#define VBLANK_LAST	REG_GENMASK(31, 7)
> >+#define VBLANK_COUNT	REG_GENMASK(6, 0)
> >+
> >+struct intel_display_metrics {
> >+	struct display_global_info global_info;
> >+	struct display_refresh_info refresh_info[MAX_PIPES];
> >+	u32 vblank_metrics[MAX_PIPES];
> >+	struct display_flip_metrics
> >+	flip_metrics[MAX_PIPES][MAX_PLANES_PER_PIPE];
> >+} __packed;
> >+
> >+/**
> >+ * intel_metrics_refresh_info - Refresh rate information
> >+ * @display: Pointer to the intel_display struct that is in use by the gfx device.
> >+ * @crtc_state: New CRTC state upon a modeset.
> >+ *
> >+ * To be called on a modeset. It then saves the current refresh
> >+interval in
> >+ * micro seconds.
> >+ */
> >+void intel_metrics_refresh_info(struct intel_display *display,
> >+				struct intel_crtc_state *crtc_state) {
> >+	struct intel_display_metrics *metrics = display->metrics;
> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >+	struct drm_display_mode *mode = &crtc_state->hw.adjusted_mode;
> >+	u32 interval_us;
> >+
> >+	if (!display->metrics)
> >+		return;
> >+
> >+	interval_us = crtc_state->hw.active ? DIV_ROUND_UP(1000000,
> >+						drm_mode_vrefresh(mode)) :
> >0;
> >+
> >+	metrics->refresh_info[crtc->pipe].refresh_interval = interval_us;
> >+	metrics->refresh_info[crtc->pipe].is_variable = crtc_state-
> >>uapi.vrr_enabled;
> >+	metrics->global_info.refresh_count += 1; }
> >+
> >+static void metrics_update_vblank(struct intel_display_metrics *metrics, int
> >pipe,
> >+				  u32 timestamp)
> >+{
> >+	u32 vblank;
> >+
> >+	vblank = metrics->vblank_metrics[pipe];
> >+
> >+	vblank = REG_FIELD_GET(VBLANK_COUNT, vblank);
> >+	vblank = REG_FIELD_PREP(VBLANK_COUNT, vblank + 1);
> >+	vblank |= REG_FIELD_PREP(VBLANK_LAST, timestamp);
> >+
> >+	/* Write everything at once in preparation for the GuC SLPC
> >requirement */
> >+	metrics->vblank_metrics[pipe] = vblank;
> >+	metrics->global_info.vblank_count += 1; }
> >+
> >+static void metrics_update_flip(struct intel_display_metrics *metrics, int pipe,
> >+				int plane, bool async_flip, u32 timestamp) {
> >+	u32 part1, part2, count;
> >+
> >+	part1 = metrics->flip_metrics[pipe][plane].part1;
> >+	part2 = metrics->flip_metrics[pipe][plane].part2;
> >+
> >+	part1 = REG_FIELD_GET(FLIP_P1_TOTAL_COUNT, part1);
> >+	part1 = REG_FIELD_PREP(FLIP_P1_TOTAL_COUNT, part1 + 1);
> >+	part1 |= REG_FIELD_PREP(FLIP_P1_LAST, timestamp);
> >+
> >+	if (async_flip) {
> >+		count = REG_FIELD_GET(FLIP_P2_ASYNC_COUNT, part2);
> >+		part2 &= ~FLIP_P2_ASYNC_COUNT;
> >+		part2 |= REG_FIELD_PREP(FLIP_P2_ASYNC_COUNT, count + 1);
> >+	} else {
> >+		count = REG_FIELD_GET(FLIP_P2_VSYNC_COUNT, part2);
> >+		part2 &= ~FLIP_P2_VSYNC_COUNT;
> >+		part2 |= REG_FIELD_PREP(FLIP_P2_VSYNC_COUNT, count + 1);
> >+	}
> >+
> >+	/*
> >+	 * Write everything in this way and this order in preparation for the
> >+	 * GuC SLPC requirement
> >+	 */
> >+	metrics->flip_metrics[pipe][plane].part1 = part1;
> >+	metrics->flip_metrics[pipe][plane].part2 = part2;
> >+
> >+	metrics->global_info.flip_count += 1;
> >+}
> >+
> >+/*
> >+ * Let's use the same register GuC SLPC uses for timestamp.
> >+ * It uses a register that is outside GT domain so GuC doesn't need
> >+ * to wake the GT for reading during SLPC loop.
> >+ * This is a single register regarding the GT, so we can read directly
> >+ * from here, regarding the GT GuC is in.
> >+ */
> >+#define MCHBAR_MIRROR_BASE_SNB	0x140000
> >+#define MCHBAR_BCLK_COUNT	_MMIO(MCHBAR_MIRROR_BASE_SNB
> >+ 0x5984)
> >+#define MTL_BCLK_COUNT		_MMIO(0xc28)
> >+#define   TIMESTAMP_MASK	REG_GENMASK(30, 6)
> >+
> >+static u32 bclk_read_timestamp(struct drm_i915_private *i915) {
> >+	u32 timestamp;
> >+
> >+	if (DISPLAY_VER(i915) >= 14)
> >+		timestamp = intel_de_read(i915, MTL_BCLK_COUNT);
> >+	else
> >+		timestamp = intel_de_read(i915, MCHBAR_BCLK_COUNT);
> >+
> >+	return REG_FIELD_GET(TIMESTAMP_MASK, timestamp); }
> >+
> >+static void display_event_work(struct work_struct *work) {
> >+	struct display_event *event = container_of(work, struct display_event,
> >work);
> >+	struct intel_display *display = event->display;
> >+	u32 timestamp;
> >+
> >+	timestamp = bclk_read_timestamp(event->i915);
> >+
> >+	if (event->is_vblank) {
> >+		metrics_update_vblank(display->metrics, event->pipe,
> >timestamp);
> >+	} else {
> >+		metrics_update_flip(display->metrics, event->pipe, event-
> >>plane,
> >+				    event->async_flip, timestamp);
> >+	}
> >+
> >+	kfree(event);
> >+}
> >+
> >+void intel_metrics_init(struct intel_display *display) {
> >+	struct intel_display_metrics *metrics;
> >+
> >+	metrics = kzalloc(sizeof(*metrics), GFP_KERNEL);
> >+	if (!metrics)
> >+		return;
> >+
> >+	metrics->global_info.version = 1;
> >+	metrics->global_info.num_pipes = MAX_PIPES;
> >+	metrics->global_info.num_planes_per_pipe = MAX_PLANES_PER_PIPE;
> >+
> >+	display->metrics = metrics;
> >+}
> >+
> >+void intel_metrics_fini(struct intel_display *display) {
> >+	if (!display->metrics)
> >+		return;
> >+
> >+	kfree(display->metrics);
> >+}
> >+
> >+/**
> >+ * intel_metrics_vblank - Vblank information
> >+ * @display: Pointer to the intel_display struct that is in use by the gfx device.
> >+ * @crtc: The Intel CRTC that received the vblank interrupt.
> >+ *
> >+ * To be called when a vblank is passed.
> >+ */
> >+void intel_metrics_vblank(struct intel_display *display,
> >+			  struct intel_crtc *crtc)
> >+{
> >+	struct display_event *event;
> >+
> >+	if (!display->metrics)
> >+		return;
> >+
> >+	event = kmalloc(sizeof(*event), GFP_ATOMIC);
> 
> HI Rodrigo, after changing kmalloc -> kzalloc, it works fine for us. It does correctly prints Global Flip count and Async Flip count.
> Prior to this change, event->is_vblank in function display_event_work is always true and hence it never increases flip count.

thanks for pointing that out. kzalloc is indeed better.
There's also another kmalloc down that needs to be changed to kzalloc.

Anyway, when you mentioned that you saw the async flip count increasing,
you are telling about the igt test right?
or do you have any special compositor change required?

In my environment here I still only see the async flip increasing
with IGT or with a very limited cases...

> 
> Thanks Chaitanya for pointing this out.
> Regards,
> Naveen Kumar
> 
> >+	if (!event)
> >+		return;
> >+
> >+	INIT_WORK(&event->work, display_event_work);
> >+	event->i915 = to_i915(crtc->base.dev);
> >+	event->display = display;
> >+	event->is_vblank = true;
> >+	event->pipe = crtc->pipe;
> >+	queue_work(system_highpri_wq, &event->work); }
> >+
> >+/**
> >+ * intel_metrics_flip - Flip information
> >+ * @display: Pointer to the intel_display struct that is in use by the gfx device.
> >+ * @crtc_state: New CRTC state upon a page flip.
> >+ * @plane: Which plane ID got the page flip.
> >+ * @async_flip: A boolean to indicate if the page flip was async.
> >+ *
> >+ * To be called on a page flip.
> >+ */
> >+void intel_metrics_flip(struct intel_display *display,
> >+			const struct intel_crtc_state *crtc_state,
> >+			int plane, bool async_flip)
> >+{
> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >+	struct display_event *event;
> >+
> >+	if (!display->metrics)
> >+		return;
> >+
> >+	event = kmalloc(sizeof(*event), GFP_ATOMIC);
> Same here, had to change kmalloc -> kzalloc
> 
> >+	if (!event)
> >+		return;
> >+
> >+	INIT_WORK(&event->work, display_event_work);
> >+	event->i915 = to_i915(crtc->base.dev);
> >+	event->display = display;
> >+	event->pipe = crtc->pipe;
> >+	event->plane = plane;
> >+	event->async_flip = async_flip;
> >+	queue_work(system_highpri_wq, &event->work); }
> >+
> >+void intel_metrics_show(struct intel_display *display, struct
> >+drm_printer *p) {
> >+	struct intel_display_metrics *metrics = display->metrics;
> >+	int pipe, plane;
> >+	u32 val;
> >+
> >+	if (!metrics)
> >+		return;
> >+
> >+	drm_printf(p, "\nDisplay Metrics - Globals:\n");
> >+	drm_printf(p, "\tVersion: %d\n", metrics->global_info.version);
> >+	drm_printf(p, "\tNum Pipes: %d\n", metrics->global_info.num_pipes);
> >+	drm_printf(p, "\tNum Planes per Pipe: %d\n",
> >+		   metrics->global_info.num_planes_per_pipe);
> >+	drm_printf(p, "\tGlobal Refresh Info Count: %d\n",
> >+		   metrics->global_info.refresh_count);
> >+	drm_printf(p, "\tGlobal Vblank Count: %d\n",
> >+		   metrics->global_info.vblank_count);
> >+	drm_printf(p, "\tGlobal Flip Count: %d\n",
> >+		   metrics->global_info.flip_count);
> >+
> >+	for (pipe = 0; pipe < MAX_PIPES; pipe++) {
> >+		if (!metrics->refresh_info[pipe].refresh_interval)
> >+			continue;
> >+
> >+		drm_printf(p, "\nDisplay Metrics - Refresh Info - Pipe[%d]:\n",
> >+			   pipe);
> >+		drm_printf(p, "\tRefresh Interval: %d\n",
> >+			   metrics->refresh_info[pipe].refresh_interval);
> >+		drm_printf(p, "\tIS VRR: %d\n",
> >+			   metrics->refresh_info[pipe].is_variable);
> >+
> >+		drm_printf(p, "Display Metrics - Vblank Info - Pipe[%d]:\n",
> >+			   pipe);
> >+		val = metrics->vblank_metrics[pipe];
> >+		drm_printf(p, "\tVBlank Last Timestamp: %x\n",
> >+			   REG_FIELD_GET(VBLANK_LAST, val));
> >+		drm_printf(p, "\tVBlank Count: %d\n",
> >+			   REG_FIELD_GET(VBLANK_COUNT, val));
> >+
> >+		drm_printf(p, "Display Metrics - Flip Info - Pipe[%d]:\n", pipe);
> >+		for (plane = 0; plane < MAX_PLANES_PER_PIPE; plane++) {
> >+			val = metrics->flip_metrics[pipe][plane].part1;
> >+			if (!val)
> >+				continue;
> >+
> >+			drm_printf(p, "\tFlip Info - Plane[%d]:\n", plane);
> >+			drm_printf(p, "\t\tFlip Last Timestamp: %x\n",
> >+				   REG_FIELD_GET(FLIP_P1_LAST, val));
> >+			drm_printf(p, "\t\tFlip Total Count: %d\n",
> >+				   REG_FIELD_GET(FLIP_P1_TOTAL_COUNT,
> >val));
> >+
> >+			val = metrics->flip_metrics[pipe][plane].part2;
> >+
> >+			drm_printf(p, "\t\tFlip Async Count: %d\n",
> >+				   REG_FIELD_GET(FLIP_P2_ASYNC_COUNT,
> >val));
> >+			drm_printf(p, "\t\tFlip Vsync Count: %d\n",
> >+				   REG_FIELD_GET(FLIP_P2_VSYNC_COUNT,
> >val));
> >+		}
> >+	}
> >+}
> >diff --git a/drivers/gpu/drm/i915/display/intel_metrics.h
> >b/drivers/gpu/drm/i915/display/intel_metrics.h
> >new file mode 100644
> >index 000000000000..9e41dc9522f3
> >--- /dev/null
> >+++ b/drivers/gpu/drm/i915/display/intel_metrics.h
> >@@ -0,0 +1,27 @@
> >+/* SPDX-License-Identifier: MIT */
> >+/*
> >+ * Copyright © 2024 Intel Corporation
> >+ */
> >+
> >+#ifndef __INTEL_METRICS_H__
> >+#define __INTEL_METRICS_H__
> >+
> >+#include <linux/types.h>
> >+
> >+struct drm_printer;
> >+struct intel_crtc;
> >+struct intel_crtc_state;
> >+struct intel_display;
> >+
> >+void intel_metrics_refresh_info(struct intel_display *display,
> >+				struct intel_crtc_state *crtc_state); void
> >+intel_metrics_vblank(struct intel_display *display,
> >+			  struct intel_crtc *intel_crtc);
> >+void intel_metrics_flip(struct intel_display *display,
> >+			const struct intel_crtc_state *crtc_state,
> >+			int plane, bool async_flip);
> >+void intel_metrics_init(struct intel_display *display); void
> >+intel_metrics_fini(struct intel_display *display); void
> >+intel_metrics_show(struct intel_display *display, struct drm_printer
> >+*p);
> >+
> >+#endif
> >diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >index 860574d04f88..fdd21a41d79d 100644
> >--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >@@ -17,6 +17,7 @@
> > #include "intel_fb.h"
> > #include "intel_fbc.h"
> > #include "intel_frontbuffer.h"
> >+#include "intel_metrics.h"
> > #include "intel_psr.h"
> > #include "intel_psr_regs.h"
> > #include "skl_scaler.h"
> >@@ -1392,6 +1393,7 @@ skl_plane_async_flip(struct intel_plane *plane,
> > 		     bool async_flip)
> > {
> > 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >+	struct intel_display *display = &dev_priv->display;
> > 	enum plane_id plane_id = plane->id;
> > 	enum pipe pipe = plane->pipe;
> > 	u32 plane_ctl = plane_state->ctl;
> >@@ -1404,6 +1406,7 @@ skl_plane_async_flip(struct intel_plane *plane,
> > 	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > 	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> > 			  skl_plane_surf(plane_state, 0));
> >+	intel_metrics_flip(display, crtc_state, plane_id, async_flip);
> > }
> >
> > static bool intel_format_is_p01x(u32 format) diff --git
> >a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index
> >e5b1715f721e..5133dba2c7de 100644
> >--- a/drivers/gpu/drm/xe/Makefile
> >+++ b/drivers/gpu/drm/xe/Makefile
> >@@ -263,6 +263,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> > 	i915-display/intel_hti.o \
> > 	i915-display/intel_link_bw.o \
> > 	i915-display/intel_lspcon.o \
> >+	i915-display/intel_metrics.o \
> > 	i915-display/intel_modeset_lock.o \
> > 	i915-display/intel_modeset_setup.o \
> > 	i915-display/intel_modeset_verify.o \
> >--
> >2.44.0
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] drm/i915/display: Introduce Display Metrics info
  2024-05-06 17:22   ` Rodrigo Vivi
@ 2024-05-07  3:19     ` Kumar, Naveen1
  2024-05-07 12:48       ` Rodrigo Vivi
  0 siblings, 1 reply; 10+ messages in thread
From: Kumar, Naveen1 @ 2024-05-07  3:19 UTC (permalink / raw)
  To: Vivi, Rodrigo
  Cc: intel-gfx, Shankar, Uma, Kulkarni, Vandita, Nikula, Jani,
	Belgaumkar, Vinay, Borah, Chaitanya Kumar



>-----Original Message-----
>From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Sent: Monday, May 6, 2024 10:52 PM
>To: Kumar, Naveen1 <naveen1.kumar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>;
>Kulkarni, Vandita <vandita.kulkarni@intel.com>; Nikula, Jani
><jani.nikula@intel.com>; Belgaumkar, Vinay <vinay.belgaumkar@intel.com>;
>Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
>Subject: Re: [PATCH] drm/i915/display: Introduce Display Metrics info
>
>On Mon, May 06, 2024 at 06:03:17AM -0400, Kumar, Naveen1 wrote:
>>
>>
>> >-----Original Message-----
>> >From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> >Sent: Saturday, April 6, 2024 3:39 AM
>> >To: intel-gfx@lists.freedesktop.org
>> >Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Shankar, Uma
>> ><uma.shankar@intel.com>; Kulkarni, Vandita
>> ><vandita.kulkarni@intel.com>; Kumar, Naveen1
>> ><naveen1.kumar@intel.com>; Nikula, Jani <jani.nikula@intel.com>;
>> >Belgaumkar, Vinay <vinay.belgaumkar@intel.com>
>> >Subject: [PATCH] drm/i915/display: Introduce Display Metrics info
>> >
>> >Introduce a display metrics information through debugfs for a better
>> >view of the vblank and page flips, in special Async flips behavior.
>> >
>> >There is currently an overall expectation that whenever
>> >vblank_mode=0 is used with an graphics application, that
>> >automatically async_flips are happening. However, while implementing
>> >the Display Metrics for GuC SLPC, it was observed that it is not
>> >necessarily true for many of the expected cases.
>> >
>> >So, while the GuC SLPC side of the metrics doesn't get ready, let's
>> >at least bring the debugfs view of it so we can work to understand
>> >and fix any potential issue around our async vblanks.
>> >
>> >Please notice that the every struct here follows exactly the GuC
>> >shared data buffer, so the next step of the integration would be
>> >smooth and almost transparent to this intel_metrics on the display side.
>> >
>> >Cc: Uma Shankar <uma.shankar@intel.com>
>> >Cc: Vandita Kulkarni <vandita.kulkarni@intel.com>
>> >Cc: Naveen Kumar <naveen1.kumar@intel.com>
>> >Cc: Jani Nikula <jani.nikula@intel.com>
>> >Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> >---
>> > drivers/gpu/drm/i915/Makefile                 |   1 +
>> > drivers/gpu/drm/i915/display/intel_display.c  |  15 +-
>> > .../gpu/drm/i915/display/intel_display_core.h |   2 +
>> > .../drm/i915/display/intel_display_debugfs.c  |  12 +
>> > .../drm/i915/display/intel_display_driver.c   |   5 +
>> > .../gpu/drm/i915/display/intel_display_irq.c  |   3 +
>> > drivers/gpu/drm/i915/display/intel_metrics.c  | 356
>> >++++++++++++++++++ drivers/gpu/drm/i915/display/intel_metrics.h  |  27
>++
>> > .../drm/i915/display/skl_universal_plane.c    |   3 +
>> > drivers/gpu/drm/xe/Makefile                   |   1 +
>> > 10 files changed, 424 insertions(+), 1 deletion(-)  create mode
>> >100644 drivers/gpu/drm/i915/display/intel_metrics.c
>> > create mode 100644 drivers/gpu/drm/i915/display/intel_metrics.h
>> >
>> >diff --git a/drivers/gpu/drm/i915/Makefile
>> >b/drivers/gpu/drm/i915/Makefile index af9e871daf1d..a3c8d9f5614c
>> >100644
>> >--- a/drivers/gpu/drm/i915/Makefile
>> >+++ b/drivers/gpu/drm/i915/Makefile
>> >@@ -291,6 +291,7 @@ i915-y += \
>> > 	display/intel_link_bw.o \
>> > 	display/intel_load_detect.o \
>> > 	display/intel_lpe_audio.o \
>> >+	display/intel_metrics.o \
>> > 	display/intel_modeset_lock.o \
>> > 	display/intel_modeset_setup.o \
>> > 	display/intel_modeset_verify.o \
>> >diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>> >b/drivers/gpu/drm/i915/display/intel_display.c
>> >index a481c9218138..ca30b8d48e1f 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_display.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_display.c
>> >@@ -94,6 +94,7 @@
>> > #include "intel_link_bw.h"
>> > #include "intel_lvds.h"
>> > #include "intel_lvds_regs.h"
>> >+#include "intel_metrics.h"
>> > #include "intel_modeset_setup.h"
>> > #include "intel_modeset_verify.h"
>> > #include "intel_overlay.h"
>> >@@ -1021,11 +1022,15 @@ static void intel_post_plane_update(struct
>> >intel_atomic_state *state,
>> > 				    struct intel_crtc *crtc)
>> > {
>> > 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> >+	struct intel_display *display = &dev_priv->display;
>> > 	const struct intel_crtc_state *old_crtc_state =
>> > 		intel_atomic_get_old_crtc_state(state, crtc);
>> > 	const struct intel_crtc_state *new_crtc_state =
>> > 		intel_atomic_get_new_crtc_state(state, crtc);
>> > 	enum pipe pipe = crtc->pipe;
>> >+	const struct intel_plane_state __maybe_unused *plane_state;
>> >+	struct intel_plane *plane;
>> >+	int i;
>> >
>> > 	intel_psr_post_plane_update(state, crtc);
>> >
>> >@@ -1057,6 +1062,12 @@ static void intel_post_plane_update(struct
>> >intel_atomic_state *state,
>> >
>> > 	if (audio_enabling(old_crtc_state, new_crtc_state))
>> > 		intel_encoders_audio_enable(state, crtc);
>> >+
>> >+	if (!new_crtc_state->do_async_flip) {
>> >+		for_each_new_intel_plane_in_state(state, plane, plane_state, i)
>> >+			intel_metrics_flip(display, new_crtc_state, plane->id,
>> >+					   false);
>> >+	}
>> > }
>> >
>> > static void intel_crtc_enable_flip_done(struct intel_atomic_state
>> >*state, @@ -
>> >7139,6 +7150,7 @@ static void intel_atomic_commit_tail(struct
>> >intel_atomic_state *state)  {
>> > 	struct drm_device *dev = state->base.dev;
>> > 	struct drm_i915_private *dev_priv = to_i915(dev);
>> >+	struct intel_display *display = &dev_priv->display;
>> > 	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
>> > 	struct intel_crtc *crtc;
>> > 	struct intel_power_domain_mask put_domains[I915_MAX_PIPES] = {};
>@@
>> >-7261,7 +7273,6 @@ static void intel_atomic_commit_tail(struct
>> >intel_atomic_state *state)
>> > 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>> > 		if (new_crtc_state->do_async_flip)
>> > 			intel_crtc_disable_flip_done(state, crtc);
>> >-
>> > 		intel_color_wait_commit(new_crtc_state);
>> > 	}
>> >
>> >@@ -7314,6 +7325,8 @@ static void intel_atomic_commit_tail(struct
>> >intel_atomic_state *state)
>> > 		 * FIXME get rid of this funny new->old swapping
>> > 		 */
>> > 		old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb);
>> >+
>> >+		intel_metrics_refresh_info(display, new_crtc_state);
>> > 	}
>> >
>> > 	/* Underruns don't always raise interrupts, so check manually */
>> >diff -- git a/drivers/gpu/drm/i915/display/intel_display_core.h
>> >b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >index 2167dbee5eea..992db9098566 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_display_core.h
>> >+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >@@ -42,6 +42,7 @@ struct intel_cdclk_vals;  struct intel_color_funcs;
>> >struct intel_crtc;  struct intel_crtc_state;
>> >+struct intel_display_metrics;
>> > struct intel_dmc;
>> > struct intel_dpll_funcs;
>> > struct intel_dpll_mgr;
>> >@@ -530,6 +531,7 @@ struct intel_display {
>> > 	struct intel_fbc *fbc[I915_MAX_FBCS];
>> > 	struct intel_frontbuffer_tracking fb_tracking;
>> > 	struct intel_hotplug hotplug;
>> >+	struct intel_display_metrics *metrics;
>> > 	struct intel_opregion *opregion;
>> > 	struct intel_overlay *overlay;
>> > 	struct intel_display_params params; diff --git
>> >a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> >b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> >index 3e364891dcd0..f05b9a9ddee0 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> >@@ -30,6 +30,7 @@
>> > #include "intel_hdcp.h"
>> > #include "intel_hdmi.h"
>> > #include "intel_hotplug.h"
>> >+#include "intel_metrics.h"
>> > #include "intel_panel.h"
>> > #include "intel_psr.h"
>> > #include "intel_psr_regs.h"
>> >@@ -642,6 +643,16 @@ static int i915_display_capabilities(struct
>> >seq_file *m, void *unused)
>> > 	return 0;
>> > }
>> >
>> >+static int i915_display_metrics(struct seq_file *m, void *unused) {
>> >+	struct drm_i915_private *i915 = node_to_i915(m->private);
>> >+	struct drm_printer p = drm_seq_file_printer(m);
>> >+
>> >+	intel_metrics_show(&i915->display, &p);
>> >+
>> >+	return 0;
>> >+}
>> >+
>> > static int i915_shared_dplls_info(struct seq_file *m, void *unused)  {
>> > 	struct drm_i915_private *dev_priv = node_to_i915(m->private); @@ -
>> >1059,6 +1070,7 @@ static const struct drm_info_list
>> >intel_display_debugfs_list[] = {
>> > 	{"i915_power_domain_info", i915_power_domain_info, 0},
>> > 	{"i915_display_info", i915_display_info, 0},
>> > 	{"i915_display_capabilities", i915_display_capabilities, 0},
>> >+	{"i915_display_metrics", i915_display_metrics, 0},
>> > 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
>> > 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
>> > 	{"i915_ddb_info", i915_ddb_info, 0}, diff --git
>> >a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> >b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> >index 87dd07e0d138..767b2d5b3826 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> >@@ -46,6 +46,7 @@
>> > #include "intel_hdcp.h"
>> > #include "intel_hotplug.h"
>> > #include "intel_hti.h"
>> >+#include "intel_metrics.h"
>> > #include "intel_modeset_lock.h"
>> > #include "intel_modeset_setup.h"
>> > #include "intel_opregion.h"
>> >@@ -514,6 +515,8 @@ int intel_display_driver_probe(struct
>> >drm_i915_private *i915)
>> >
>> > 	intel_overlay_setup(i915);
>> >
>> >+	intel_metrics_init(&i915->display);
>> >+
>> > 	ret = intel_fbdev_init(&i915->drm);
>> > 	if (ret)
>> > 		return ret;
>> >@@ -611,6 +614,8 @@ void intel_display_driver_remove_noirq(struct
>> >drm_i915_private *i915)
>> >
>> > 	intel_dp_tunnel_mgr_cleanup(i915);
>> >
>> >+	intel_metrics_fini(&i915->display);
>> >+
>> > 	intel_overlay_cleanup(i915);
>> >
>> > 	intel_gmbus_teardown(i915);
>> >diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
>> >b/drivers/gpu/drm/i915/display/intel_display_irq.c
>> >index f846c5b108b5..202400a771b2 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_display_irq.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
>> >@@ -18,6 +18,7 @@
>> > #include "intel_fifo_underrun.h"
>> > #include "intel_gmbus.h"
>> > #include "intel_hotplug_irq.h"
>> >+#include "intel_metrics.h"
>> > #include "intel_pmdemand.h"
>> > #include "intel_psr.h"
>> > #include "intel_psr_regs.h"
>> >@@ -25,8 +26,10 @@
>> > static void
>> > intel_handle_vblank(struct drm_i915_private *dev_priv, enum pipe
>> >pipe)  {
>> >+	struct intel_display *display = &dev_priv->display;
>> > 	struct intel_crtc *crtc = intel_crtc_for_pipe(dev_priv, pipe);
>> >
>> >+	intel_metrics_vblank(display, crtc);
>> > 	drm_crtc_handle_vblank(&crtc->base);
>> > }
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_metrics.c
>> >b/drivers/gpu/drm/i915/display/intel_metrics.c
>> >new file mode 100644
>> >index 000000000000..2cb2b8189b25
>> >--- /dev/null
>> >+++ b/drivers/gpu/drm/i915/display/intel_metrics.c
>> >@@ -0,0 +1,356 @@
>> >+// SPDX-License-Identifier: MIT
>> >+/*
>> >+ * Copyright (c) 2024 Intel Corporation  */
>> >+
>> >+#include "intel_metrics.h"
>> >+
>> >+#include <drm/drm_modes.h>
>> >+#include <drm/drm_print.h>
>> >+
>> >+#include "i915_drv.h"
>> >+#include "intel_de.h"
>> >+#include "intel_display_types.h"
>> >+
>> >+/**
>> >+ * Display Metrics
>> >+ *
>> >+ * Provide some display activity overview such as active refresh
>> >+rates,
>> >+ * vblank activity and page flip activities.
>> >+ * For now it is informative debug only, but later it will be
>> >+expanded
>> >+ * to be used for GT frequency selection by GuC SLPC.
>> >+ */
>> >+
>> >+/*
>> >+ * An event using an work queue is used to avoid any disturbance in
>> >+the
>> >+ * critical path that could cause performance impacts.
>> >+ */
>> >+struct display_event {
>> >+	struct work_struct work;
>> >+	struct drm_i915_private *i915;
>> >+	struct intel_display *display;
>> >+	bool is_vblank;
>> >+	int pipe;
>> >+	int plane;
>> >+	bool async_flip;
>> >+};
>> >+
>> >+/*
>> >+ * Although we could simply save this inside our crtc structs, we
>> >+are
>> >+ * already mimicking the GuC SLPC defition of the display data, for
>> >+future
>> >+ * usage.
>> >+ */
>> >+#define MAX_PIPES		8
>> >+#define MAX_PLANES_PER_PIPE	8
>> >+
>> >+struct display_global_info {
>> >+	u32 version:8;
>> >+	u32 num_pipes:4;
>> >+	u32 num_planes_per_pipe:4;
>> >+	u32 reserved_1:16;
>> >+	u32 refresh_count:16;
>> >+	u32 vblank_count:16;
>> >+	u32 flip_count:16;
>> >+	u32 reserved_2:16;
>> >+	u32 reserved_3[13];
>> >+} __packed;
>> >+
>> >+struct display_refresh_info {
>> >+	u32 refresh_interval:16;
>> >+	u32 is_variable:1;
>> >+	u32 reserved:15;
>> >+} __packed;
>> >+
>> >+/*
>> >+ * When used with GuC SLPC, the host must update each 32-bit part
>> >+with a single
>> >+ * atomic write so that SLPC will read the contained bit fields together.
>> >+ * The host must update the two parts in order - total flip count
>> >+and timestamp
>> >+ * first, vsync and async flip counts second.
>> >+ * Hence, these items are not defined with individual bitfields.
>> >+ */
>> >+#define FLIP_P1_LAST		REG_GENMASK(31, 7)
>> >+#define FLIP_P1_TOTAL_COUNT	REG_GENMASK(6, 0)
>> >+#define FLIP_P2_ASYNC_COUNT	REG_GENMASK(31, 16)
>> >+#define FLIP_P2_VSYNC_COUNT	REG_GENMASK(15, 0)
>> >+
>> >+struct display_flip_metrics {
>> >+	u32 part1;
>> >+	u32 part2;
>> >+} __packed;
>> >+
>> >+/*
>> >+ * When used with GuC SLPC, the host must update each 32-bit part
>> >+with a single
>> >+ * atomic write, so that SLPC will read the count and timestamp together.
>> >+ * Hence, this item is not defined with individual bitfields.
>> >+ */
>> >+#define VBLANK_LAST	REG_GENMASK(31, 7)
>> >+#define VBLANK_COUNT	REG_GENMASK(6, 0)
>> >+
>> >+struct intel_display_metrics {
>> >+	struct display_global_info global_info;
>> >+	struct display_refresh_info refresh_info[MAX_PIPES];
>> >+	u32 vblank_metrics[MAX_PIPES];
>> >+	struct display_flip_metrics
>> >+	flip_metrics[MAX_PIPES][MAX_PLANES_PER_PIPE];
>> >+} __packed;
>> >+
>> >+/**
>> >+ * intel_metrics_refresh_info - Refresh rate information
>> >+ * @display: Pointer to the intel_display struct that is in use by the gfx
>device.
>> >+ * @crtc_state: New CRTC state upon a modeset.
>> >+ *
>> >+ * To be called on a modeset. It then saves the current refresh
>> >+interval in
>> >+ * micro seconds.
>> >+ */
>> >+void intel_metrics_refresh_info(struct intel_display *display,
>> >+				struct intel_crtc_state *crtc_state) {
>> >+	struct intel_display_metrics *metrics = display->metrics;
>> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> >+	struct drm_display_mode *mode = &crtc_state->hw.adjusted_mode;
>> >+	u32 interval_us;
>> >+
>> >+	if (!display->metrics)
>> >+		return;
>> >+
>> >+	interval_us = crtc_state->hw.active ? DIV_ROUND_UP(1000000,
>> >+						drm_mode_vrefresh(mode)) :
>> >0;
>> >+
>> >+	metrics->refresh_info[crtc->pipe].refresh_interval = interval_us;
>> >+	metrics->refresh_info[crtc->pipe].is_variable = crtc_state-
>> >>uapi.vrr_enabled;
>> >+	metrics->global_info.refresh_count += 1; }
>> >+
>> >+static void metrics_update_vblank(struct intel_display_metrics
>> >+*metrics, int
>> >pipe,
>> >+				  u32 timestamp)
>> >+{
>> >+	u32 vblank;
>> >+
>> >+	vblank = metrics->vblank_metrics[pipe];
>> >+
>> >+	vblank = REG_FIELD_GET(VBLANK_COUNT, vblank);
>> >+	vblank = REG_FIELD_PREP(VBLANK_COUNT, vblank + 1);
>> >+	vblank |= REG_FIELD_PREP(VBLANK_LAST, timestamp);
>> >+
>> >+	/* Write everything at once in preparation for the GuC SLPC
>> >requirement */
>> >+	metrics->vblank_metrics[pipe] = vblank;
>> >+	metrics->global_info.vblank_count += 1; }
>> >+
>> >+static void metrics_update_flip(struct intel_display_metrics *metrics, int
>pipe,
>> >+				int plane, bool async_flip, u32 timestamp) {
>> >+	u32 part1, part2, count;
>> >+
>> >+	part1 = metrics->flip_metrics[pipe][plane].part1;
>> >+	part2 = metrics->flip_metrics[pipe][plane].part2;
>> >+
>> >+	part1 = REG_FIELD_GET(FLIP_P1_TOTAL_COUNT, part1);
>> >+	part1 = REG_FIELD_PREP(FLIP_P1_TOTAL_COUNT, part1 + 1);
>> >+	part1 |= REG_FIELD_PREP(FLIP_P1_LAST, timestamp);
>> >+
>> >+	if (async_flip) {
>> >+		count = REG_FIELD_GET(FLIP_P2_ASYNC_COUNT, part2);
>> >+		part2 &= ~FLIP_P2_ASYNC_COUNT;
>> >+		part2 |= REG_FIELD_PREP(FLIP_P2_ASYNC_COUNT, count + 1);
>> >+	} else {
>> >+		count = REG_FIELD_GET(FLIP_P2_VSYNC_COUNT, part2);
>> >+		part2 &= ~FLIP_P2_VSYNC_COUNT;
>> >+		part2 |= REG_FIELD_PREP(FLIP_P2_VSYNC_COUNT, count + 1);
>> >+	}
>> >+
>> >+	/*
>> >+	 * Write everything in this way and this order in preparation for the
>> >+	 * GuC SLPC requirement
>> >+	 */
>> >+	metrics->flip_metrics[pipe][plane].part1 = part1;
>> >+	metrics->flip_metrics[pipe][plane].part2 = part2;
>> >+
>> >+	metrics->global_info.flip_count += 1; }
>> >+
>> >+/*
>> >+ * Let's use the same register GuC SLPC uses for timestamp.
>> >+ * It uses a register that is outside GT domain so GuC doesn't need
>> >+ * to wake the GT for reading during SLPC loop.
>> >+ * This is a single register regarding the GT, so we can read
>> >+directly
>> >+ * from here, regarding the GT GuC is in.
>> >+ */
>> >+#define MCHBAR_MIRROR_BASE_SNB	0x140000
>> >+#define MCHBAR_BCLK_COUNT	_MMIO(MCHBAR_MIRROR_BASE_SNB
>> >+ 0x5984)
>> >+#define MTL_BCLK_COUNT		_MMIO(0xc28)
>> >+#define   TIMESTAMP_MASK	REG_GENMASK(30, 6)
>> >+
>> >+static u32 bclk_read_timestamp(struct drm_i915_private *i915) {
>> >+	u32 timestamp;
>> >+
>> >+	if (DISPLAY_VER(i915) >= 14)
>> >+		timestamp = intel_de_read(i915, MTL_BCLK_COUNT);
>> >+	else
>> >+		timestamp = intel_de_read(i915, MCHBAR_BCLK_COUNT);
>> >+
>> >+	return REG_FIELD_GET(TIMESTAMP_MASK, timestamp); }
>> >+
>> >+static void display_event_work(struct work_struct *work) {
>> >+	struct display_event *event = container_of(work, struct
>> >+display_event,
>> >work);
>> >+	struct intel_display *display = event->display;
>> >+	u32 timestamp;
>> >+
>> >+	timestamp = bclk_read_timestamp(event->i915);
>> >+
>> >+	if (event->is_vblank) {
>> >+		metrics_update_vblank(display->metrics, event->pipe,
>> >timestamp);
>> >+	} else {
>> >+		metrics_update_flip(display->metrics, event->pipe, event-
>> >>plane,
>> >+				    event->async_flip, timestamp);
>> >+	}
>> >+
>> >+	kfree(event);
>> >+}
>> >+
>> >+void intel_metrics_init(struct intel_display *display) {
>> >+	struct intel_display_metrics *metrics;
>> >+
>> >+	metrics = kzalloc(sizeof(*metrics), GFP_KERNEL);
>> >+	if (!metrics)
>> >+		return;
>> >+
>> >+	metrics->global_info.version = 1;
>> >+	metrics->global_info.num_pipes = MAX_PIPES;
>> >+	metrics->global_info.num_planes_per_pipe = MAX_PLANES_PER_PIPE;
>> >+
>> >+	display->metrics = metrics;
>> >+}
>> >+
>> >+void intel_metrics_fini(struct intel_display *display) {
>> >+	if (!display->metrics)
>> >+		return;
>> >+
>> >+	kfree(display->metrics);
>> >+}
>> >+
>> >+/**
>> >+ * intel_metrics_vblank - Vblank information
>> >+ * @display: Pointer to the intel_display struct that is in use by the gfx
>device.
>> >+ * @crtc: The Intel CRTC that received the vblank interrupt.
>> >+ *
>> >+ * To be called when a vblank is passed.
>> >+ */
>> >+void intel_metrics_vblank(struct intel_display *display,
>> >+			  struct intel_crtc *crtc)
>> >+{
>> >+	struct display_event *event;
>> >+
>> >+	if (!display->metrics)
>> >+		return;
>> >+
>> >+	event = kmalloc(sizeof(*event), GFP_ATOMIC);
>>
>> HI Rodrigo, after changing kmalloc -> kzalloc, it works fine for us. It does
>correctly prints Global Flip count and Async Flip count.
>> Prior to this change, event->is_vblank in function display_event_work is
>always true and hence it never increases flip count.
>
>thanks for pointing that out. kzalloc is indeed better.
>There's also another kmalloc down that needs to be changed to kzalloc.
>
>Anyway, when you mentioned that you saw the async flip count increasing,
>you are telling about the igt test right?
>or do you have any special compositor change required?

Hi Rodrigo, async flip count is increasing using both IGT and Weston/wayland client app (https://gitlab.freedesktop.org/wayland/weston/-/blob/main/clients/simple-egl.c#L1294)
Additionally, we had to hack mesa code to use async flip supported modifiers. Mesa needs to handle async flip scenarios and select supported modifiers.

>In my environment here I still only see the async flip increasing with IGT or with
>a very limited cases...
>
>>
>> Thanks Chaitanya for pointing this out.
>> Regards,
>> Naveen Kumar
>>
>> >+	if (!event)
>> >+		return;
>> >+
>> >+	INIT_WORK(&event->work, display_event_work);
>> >+	event->i915 = to_i915(crtc->base.dev);
>> >+	event->display = display;
>> >+	event->is_vblank = true;
>> >+	event->pipe = crtc->pipe;
>> >+	queue_work(system_highpri_wq, &event->work); }
>> >+
>> >+/**
>> >+ * intel_metrics_flip - Flip information
>> >+ * @display: Pointer to the intel_display struct that is in use by the gfx
>device.
>> >+ * @crtc_state: New CRTC state upon a page flip.
>> >+ * @plane: Which plane ID got the page flip.
>> >+ * @async_flip: A boolean to indicate if the page flip was async.
>> >+ *
>> >+ * To be called on a page flip.
>> >+ */
>> >+void intel_metrics_flip(struct intel_display *display,
>> >+			const struct intel_crtc_state *crtc_state,
>> >+			int plane, bool async_flip)
>> >+{
>> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> >+	struct display_event *event;
>> >+
>> >+	if (!display->metrics)
>> >+		return;
>> >+
>> >+	event = kmalloc(sizeof(*event), GFP_ATOMIC);
>> Same here, had to change kmalloc -> kzalloc
>>
>> >+	if (!event)
>> >+		return;
>> >+
>> >+	INIT_WORK(&event->work, display_event_work);
>> >+	event->i915 = to_i915(crtc->base.dev);
>> >+	event->display = display;
>> >+	event->pipe = crtc->pipe;
>> >+	event->plane = plane;
>> >+	event->async_flip = async_flip;
>> >+	queue_work(system_highpri_wq, &event->work); }
>> >+
>> >+void intel_metrics_show(struct intel_display *display, struct
>> >+drm_printer *p) {
>> >+	struct intel_display_metrics *metrics = display->metrics;
>> >+	int pipe, plane;
>> >+	u32 val;
>> >+
>> >+	if (!metrics)
>> >+		return;
>> >+
>> >+	drm_printf(p, "\nDisplay Metrics - Globals:\n");
>> >+	drm_printf(p, "\tVersion: %d\n", metrics->global_info.version);
>> >+	drm_printf(p, "\tNum Pipes: %d\n", metrics->global_info.num_pipes);
>> >+	drm_printf(p, "\tNum Planes per Pipe: %d\n",
>> >+		   metrics->global_info.num_planes_per_pipe);
>> >+	drm_printf(p, "\tGlobal Refresh Info Count: %d\n",
>> >+		   metrics->global_info.refresh_count);
>> >+	drm_printf(p, "\tGlobal Vblank Count: %d\n",
>> >+		   metrics->global_info.vblank_count);
>> >+	drm_printf(p, "\tGlobal Flip Count: %d\n",
>> >+		   metrics->global_info.flip_count);
>> >+
>> >+	for (pipe = 0; pipe < MAX_PIPES; pipe++) {
>> >+		if (!metrics->refresh_info[pipe].refresh_interval)
>> >+			continue;
>> >+
>> >+		drm_printf(p, "\nDisplay Metrics - Refresh Info - Pipe[%d]:\n",
>> >+			   pipe);
>> >+		drm_printf(p, "\tRefresh Interval: %d\n",
>> >+			   metrics->refresh_info[pipe].refresh_interval);
>> >+		drm_printf(p, "\tIS VRR: %d\n",
>> >+			   metrics->refresh_info[pipe].is_variable);
>> >+
>> >+		drm_printf(p, "Display Metrics - Vblank Info - Pipe[%d]:\n",
>> >+			   pipe);
>> >+		val = metrics->vblank_metrics[pipe];
>> >+		drm_printf(p, "\tVBlank Last Timestamp: %x\n",
>> >+			   REG_FIELD_GET(VBLANK_LAST, val));
>> >+		drm_printf(p, "\tVBlank Count: %d\n",
>> >+			   REG_FIELD_GET(VBLANK_COUNT, val));
>> >+
>> >+		drm_printf(p, "Display Metrics - Flip Info - Pipe[%d]:\n", pipe);
>> >+		for (plane = 0; plane < MAX_PLANES_PER_PIPE; plane++) {
>> >+			val = metrics->flip_metrics[pipe][plane].part1;
>> >+			if (!val)
>> >+				continue;
>> >+
>> >+			drm_printf(p, "\tFlip Info - Plane[%d]:\n", plane);
>> >+			drm_printf(p, "\t\tFlip Last Timestamp: %x\n",
>> >+				   REG_FIELD_GET(FLIP_P1_LAST, val));
>> >+			drm_printf(p, "\t\tFlip Total Count: %d\n",
>> >+				   REG_FIELD_GET(FLIP_P1_TOTAL_COUNT,
>> >val));
>> >+
>> >+			val = metrics->flip_metrics[pipe][plane].part2;
>> >+
>> >+			drm_printf(p, "\t\tFlip Async Count: %d\n",
>> >+				   REG_FIELD_GET(FLIP_P2_ASYNC_COUNT,
>> >val));
>> >+			drm_printf(p, "\t\tFlip Vsync Count: %d\n",
>> >+				   REG_FIELD_GET(FLIP_P2_VSYNC_COUNT,
>> >val));
>> >+		}
>> >+	}
>> >+}
>> >diff --git a/drivers/gpu/drm/i915/display/intel_metrics.h
>> >b/drivers/gpu/drm/i915/display/intel_metrics.h
>> >new file mode 100644
>> >index 000000000000..9e41dc9522f3
>> >--- /dev/null
>> >+++ b/drivers/gpu/drm/i915/display/intel_metrics.h
>> >@@ -0,0 +1,27 @@
>> >+/* SPDX-License-Identifier: MIT */
>> >+/*
>> >+ * Copyright (c) 2024 Intel Corporation  */
>> >+
>> >+#ifndef __INTEL_METRICS_H__
>> >+#define __INTEL_METRICS_H__
>> >+
>> >+#include <linux/types.h>
>> >+
>> >+struct drm_printer;
>> >+struct intel_crtc;
>> >+struct intel_crtc_state;
>> >+struct intel_display;
>> >+
>> >+void intel_metrics_refresh_info(struct intel_display *display,
>> >+				struct intel_crtc_state *crtc_state); void
>> >+intel_metrics_vblank(struct intel_display *display,
>> >+			  struct intel_crtc *intel_crtc); void
>intel_metrics_flip(struct
>> >+intel_display *display,
>> >+			const struct intel_crtc_state *crtc_state,
>> >+			int plane, bool async_flip);
>> >+void intel_metrics_init(struct intel_display *display); void
>> >+intel_metrics_fini(struct intel_display *display); void
>> >+intel_metrics_show(struct intel_display *display, struct drm_printer
>> >+*p);
>> >+
>> >+#endif
>> >diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> >b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> >index 860574d04f88..fdd21a41d79d 100644
>> >--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> >+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> >@@ -17,6 +17,7 @@
>> > #include "intel_fb.h"
>> > #include "intel_fbc.h"
>> > #include "intel_frontbuffer.h"
>> >+#include "intel_metrics.h"
>> > #include "intel_psr.h"
>> > #include "intel_psr_regs.h"
>> > #include "skl_scaler.h"
>> >@@ -1392,6 +1393,7 @@ skl_plane_async_flip(struct intel_plane *plane,
>> > 		     bool async_flip)
>> > {
>> > 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>> >+	struct intel_display *display = &dev_priv->display;
>> > 	enum plane_id plane_id = plane->id;
>> > 	enum pipe pipe = plane->pipe;
>> > 	u32 plane_ctl = plane_state->ctl;
>> >@@ -1404,6 +1406,7 @@ skl_plane_async_flip(struct intel_plane *plane,
>> > 	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
>> > 	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
>> > 			  skl_plane_surf(plane_state, 0));
>> >+	intel_metrics_flip(display, crtc_state, plane_id, async_flip);
>> > }
>> >
>> > static bool intel_format_is_p01x(u32 format) diff --git
>> >a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index
>> >e5b1715f721e..5133dba2c7de 100644
>> >--- a/drivers/gpu/drm/xe/Makefile
>> >+++ b/drivers/gpu/drm/xe/Makefile
>> >@@ -263,6 +263,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>> > 	i915-display/intel_hti.o \
>> > 	i915-display/intel_link_bw.o \
>> > 	i915-display/intel_lspcon.o \
>> >+	i915-display/intel_metrics.o \
>> > 	i915-display/intel_modeset_lock.o \
>> > 	i915-display/intel_modeset_setup.o \
>> > 	i915-display/intel_modeset_verify.o \
>> >--
>> >2.44.0
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915/display: Introduce Display Metrics info
  2024-05-07  3:19     ` Kumar, Naveen1
@ 2024-05-07 12:48       ` Rodrigo Vivi
  2024-05-07 17:37         ` Zanoni, Paulo R
  0 siblings, 1 reply; 10+ messages in thread
From: Rodrigo Vivi @ 2024-05-07 12:48 UTC (permalink / raw)
  To: Kumar, Naveen1, jose.souza, Paulo Zanoni
  Cc: intel-gfx, Shankar, Uma, Kulkarni, Vandita, Nikula, Jani,
	Belgaumkar, Vinay, Borah, Chaitanya Kumar

On Mon, May 06, 2024 at 11:19:56PM -0400, Kumar, Naveen1 wrote:
> 
> 
> >-----Original Message-----
> >From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> >Sent: Monday, May 6, 2024 10:52 PM
> >To: Kumar, Naveen1 <naveen1.kumar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>;
> >Kulkarni, Vandita <vandita.kulkarni@intel.com>; Nikula, Jani
> ><jani.nikula@intel.com>; Belgaumkar, Vinay <vinay.belgaumkar@intel.com>;
> >Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
> >Subject: Re: [PATCH] drm/i915/display: Introduce Display Metrics info
> >
> >On Mon, May 06, 2024 at 06:03:17AM -0400, Kumar, Naveen1 wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> >> >Sent: Saturday, April 6, 2024 3:39 AM
> >> >To: intel-gfx@lists.freedesktop.org
> >> >Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Shankar, Uma
> >> ><uma.shankar@intel.com>; Kulkarni, Vandita
> >> ><vandita.kulkarni@intel.com>; Kumar, Naveen1
> >> ><naveen1.kumar@intel.com>; Nikula, Jani <jani.nikula@intel.com>;
> >> >Belgaumkar, Vinay <vinay.belgaumkar@intel.com>
> >> >Subject: [PATCH] drm/i915/display: Introduce Display Metrics info
> >> >
> >> >Introduce a display metrics information through debugfs for a better
> >> >view of the vblank and page flips, in special Async flips behavior.
> >> >
> >> >There is currently an overall expectation that whenever
> >> >vblank_mode=0 is used with an graphics application, that
> >> >automatically async_flips are happening. However, while implementing
> >> >the Display Metrics for GuC SLPC, it was observed that it is not
> >> >necessarily true for many of the expected cases.
> >> >
> >> >So, while the GuC SLPC side of the metrics doesn't get ready, let's
> >> >at least bring the debugfs view of it so we can work to understand
> >> >and fix any potential issue around our async vblanks.
> >> >
> >> >Please notice that the every struct here follows exactly the GuC
> >> >shared data buffer, so the next step of the integration would be
> >> >smooth and almost transparent to this intel_metrics on the display side.
> >> >
> >> >Cc: Uma Shankar <uma.shankar@intel.com>
> >> >Cc: Vandita Kulkarni <vandita.kulkarni@intel.com>
> >> >Cc: Naveen Kumar <naveen1.kumar@intel.com>
> >> >Cc: Jani Nikula <jani.nikula@intel.com>
> >> >Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> >> >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> >---
> >> > drivers/gpu/drm/i915/Makefile                 |   1 +
> >> > drivers/gpu/drm/i915/display/intel_display.c  |  15 +-
> >> > .../gpu/drm/i915/display/intel_display_core.h |   2 +
> >> > .../drm/i915/display/intel_display_debugfs.c  |  12 +
> >> > .../drm/i915/display/intel_display_driver.c   |   5 +
> >> > .../gpu/drm/i915/display/intel_display_irq.c  |   3 +
> >> > drivers/gpu/drm/i915/display/intel_metrics.c  | 356
> >> >++++++++++++++++++ drivers/gpu/drm/i915/display/intel_metrics.h  |  27
> >++
> >> > .../drm/i915/display/skl_universal_plane.c    |   3 +
> >> > drivers/gpu/drm/xe/Makefile                   |   1 +
> >> > 10 files changed, 424 insertions(+), 1 deletion(-)  create mode
> >> >100644 drivers/gpu/drm/i915/display/intel_metrics.c
> >> > create mode 100644 drivers/gpu/drm/i915/display/intel_metrics.h
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/Makefile
> >> >b/drivers/gpu/drm/i915/Makefile index af9e871daf1d..a3c8d9f5614c
> >> >100644
> >> >--- a/drivers/gpu/drm/i915/Makefile
> >> >+++ b/drivers/gpu/drm/i915/Makefile
> >> >@@ -291,6 +291,7 @@ i915-y += \
> >> > 	display/intel_link_bw.o \
> >> > 	display/intel_load_detect.o \
> >> > 	display/intel_lpe_audio.o \
> >> >+	display/intel_metrics.o \
> >> > 	display/intel_modeset_lock.o \
> >> > 	display/intel_modeset_setup.o \
> >> > 	display/intel_modeset_verify.o \
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >> >b/drivers/gpu/drm/i915/display/intel_display.c
> >> >index a481c9218138..ca30b8d48e1f 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_display.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> >@@ -94,6 +94,7 @@
> >> > #include "intel_link_bw.h"
> >> > #include "intel_lvds.h"
> >> > #include "intel_lvds_regs.h"
> >> >+#include "intel_metrics.h"
> >> > #include "intel_modeset_setup.h"
> >> > #include "intel_modeset_verify.h"
> >> > #include "intel_overlay.h"
> >> >@@ -1021,11 +1022,15 @@ static void intel_post_plane_update(struct
> >> >intel_atomic_state *state,
> >> > 				    struct intel_crtc *crtc)
> >> > {
> >> > 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> >> >+	struct intel_display *display = &dev_priv->display;
> >> > 	const struct intel_crtc_state *old_crtc_state =
> >> > 		intel_atomic_get_old_crtc_state(state, crtc);
> >> > 	const struct intel_crtc_state *new_crtc_state =
> >> > 		intel_atomic_get_new_crtc_state(state, crtc);
> >> > 	enum pipe pipe = crtc->pipe;
> >> >+	const struct intel_plane_state __maybe_unused *plane_state;
> >> >+	struct intel_plane *plane;
> >> >+	int i;
> >> >
> >> > 	intel_psr_post_plane_update(state, crtc);
> >> >
> >> >@@ -1057,6 +1062,12 @@ static void intel_post_plane_update(struct
> >> >intel_atomic_state *state,
> >> >
> >> > 	if (audio_enabling(old_crtc_state, new_crtc_state))
> >> > 		intel_encoders_audio_enable(state, crtc);
> >> >+
> >> >+	if (!new_crtc_state->do_async_flip) {
> >> >+		for_each_new_intel_plane_in_state(state, plane, plane_state, i)
> >> >+			intel_metrics_flip(display, new_crtc_state, plane->id,
> >> >+					   false);
> >> >+	}
> >> > }
> >> >
> >> > static void intel_crtc_enable_flip_done(struct intel_atomic_state
> >> >*state, @@ -
> >> >7139,6 +7150,7 @@ static void intel_atomic_commit_tail(struct
> >> >intel_atomic_state *state)  {
> >> > 	struct drm_device *dev = state->base.dev;
> >> > 	struct drm_i915_private *dev_priv = to_i915(dev);
> >> >+	struct intel_display *display = &dev_priv->display;
> >> > 	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> >> > 	struct intel_crtc *crtc;
> >> > 	struct intel_power_domain_mask put_domains[I915_MAX_PIPES] = {};
> >@@
> >> >-7261,7 +7273,6 @@ static void intel_atomic_commit_tail(struct
> >> >intel_atomic_state *state)
> >> > 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> >> > 		if (new_crtc_state->do_async_flip)
> >> > 			intel_crtc_disable_flip_done(state, crtc);
> >> >-
> >> > 		intel_color_wait_commit(new_crtc_state);
> >> > 	}
> >> >
> >> >@@ -7314,6 +7325,8 @@ static void intel_atomic_commit_tail(struct
> >> >intel_atomic_state *state)
> >> > 		 * FIXME get rid of this funny new->old swapping
> >> > 		 */
> >> > 		old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb);
> >> >+
> >> >+		intel_metrics_refresh_info(display, new_crtc_state);
> >> > 	}
> >> >
> >> > 	/* Underruns don't always raise interrupts, so check manually */
> >> >diff -- git a/drivers/gpu/drm/i915/display/intel_display_core.h
> >> >b/drivers/gpu/drm/i915/display/intel_display_core.h
> >> >index 2167dbee5eea..992db9098566 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_display_core.h
> >> >+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> >> >@@ -42,6 +42,7 @@ struct intel_cdclk_vals;  struct intel_color_funcs;
> >> >struct intel_crtc;  struct intel_crtc_state;
> >> >+struct intel_display_metrics;
> >> > struct intel_dmc;
> >> > struct intel_dpll_funcs;
> >> > struct intel_dpll_mgr;
> >> >@@ -530,6 +531,7 @@ struct intel_display {
> >> > 	struct intel_fbc *fbc[I915_MAX_FBCS];
> >> > 	struct intel_frontbuffer_tracking fb_tracking;
> >> > 	struct intel_hotplug hotplug;
> >> >+	struct intel_display_metrics *metrics;
> >> > 	struct intel_opregion *opregion;
> >> > 	struct intel_overlay *overlay;
> >> > 	struct intel_display_params params; diff --git
> >> >a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >> >b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >> >index 3e364891dcd0..f05b9a9ddee0 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> >> >@@ -30,6 +30,7 @@
> >> > #include "intel_hdcp.h"
> >> > #include "intel_hdmi.h"
> >> > #include "intel_hotplug.h"
> >> >+#include "intel_metrics.h"
> >> > #include "intel_panel.h"
> >> > #include "intel_psr.h"
> >> > #include "intel_psr_regs.h"
> >> >@@ -642,6 +643,16 @@ static int i915_display_capabilities(struct
> >> >seq_file *m, void *unused)
> >> > 	return 0;
> >> > }
> >> >
> >> >+static int i915_display_metrics(struct seq_file *m, void *unused) {
> >> >+	struct drm_i915_private *i915 = node_to_i915(m->private);
> >> >+	struct drm_printer p = drm_seq_file_printer(m);
> >> >+
> >> >+	intel_metrics_show(&i915->display, &p);
> >> >+
> >> >+	return 0;
> >> >+}
> >> >+
> >> > static int i915_shared_dplls_info(struct seq_file *m, void *unused)  {
> >> > 	struct drm_i915_private *dev_priv = node_to_i915(m->private); @@ -
> >> >1059,6 +1070,7 @@ static const struct drm_info_list
> >> >intel_display_debugfs_list[] = {
> >> > 	{"i915_power_domain_info", i915_power_domain_info, 0},
> >> > 	{"i915_display_info", i915_display_info, 0},
> >> > 	{"i915_display_capabilities", i915_display_capabilities, 0},
> >> >+	{"i915_display_metrics", i915_display_metrics, 0},
> >> > 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
> >> > 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
> >> > 	{"i915_ddb_info", i915_ddb_info, 0}, diff --git
> >> >a/drivers/gpu/drm/i915/display/intel_display_driver.c
> >> >b/drivers/gpu/drm/i915/display/intel_display_driver.c
> >> >index 87dd07e0d138..767b2d5b3826 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> >> >@@ -46,6 +46,7 @@
> >> > #include "intel_hdcp.h"
> >> > #include "intel_hotplug.h"
> >> > #include "intel_hti.h"
> >> >+#include "intel_metrics.h"
> >> > #include "intel_modeset_lock.h"
> >> > #include "intel_modeset_setup.h"
> >> > #include "intel_opregion.h"
> >> >@@ -514,6 +515,8 @@ int intel_display_driver_probe(struct
> >> >drm_i915_private *i915)
> >> >
> >> > 	intel_overlay_setup(i915);
> >> >
> >> >+	intel_metrics_init(&i915->display);
> >> >+
> >> > 	ret = intel_fbdev_init(&i915->drm);
> >> > 	if (ret)
> >> > 		return ret;
> >> >@@ -611,6 +614,8 @@ void intel_display_driver_remove_noirq(struct
> >> >drm_i915_private *i915)
> >> >
> >> > 	intel_dp_tunnel_mgr_cleanup(i915);
> >> >
> >> >+	intel_metrics_fini(&i915->display);
> >> >+
> >> > 	intel_overlay_cleanup(i915);
> >> >
> >> > 	intel_gmbus_teardown(i915);
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
> >> >b/drivers/gpu/drm/i915/display/intel_display_irq.c
> >> >index f846c5b108b5..202400a771b2 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> >> >@@ -18,6 +18,7 @@
> >> > #include "intel_fifo_underrun.h"
> >> > #include "intel_gmbus.h"
> >> > #include "intel_hotplug_irq.h"
> >> >+#include "intel_metrics.h"
> >> > #include "intel_pmdemand.h"
> >> > #include "intel_psr.h"
> >> > #include "intel_psr_regs.h"
> >> >@@ -25,8 +26,10 @@
> >> > static void
> >> > intel_handle_vblank(struct drm_i915_private *dev_priv, enum pipe
> >> >pipe)  {
> >> >+	struct intel_display *display = &dev_priv->display;
> >> > 	struct intel_crtc *crtc = intel_crtc_for_pipe(dev_priv, pipe);
> >> >
> >> >+	intel_metrics_vblank(display, crtc);
> >> > 	drm_crtc_handle_vblank(&crtc->base);
> >> > }
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_metrics.c
> >> >b/drivers/gpu/drm/i915/display/intel_metrics.c
> >> >new file mode 100644
> >> >index 000000000000..2cb2b8189b25
> >> >--- /dev/null
> >> >+++ b/drivers/gpu/drm/i915/display/intel_metrics.c
> >> >@@ -0,0 +1,356 @@
> >> >+// SPDX-License-Identifier: MIT
> >> >+/*
> >> >+ * Copyright (c) 2024 Intel Corporation  */
> >> >+
> >> >+#include "intel_metrics.h"
> >> >+
> >> >+#include <drm/drm_modes.h>
> >> >+#include <drm/drm_print.h>
> >> >+
> >> >+#include "i915_drv.h"
> >> >+#include "intel_de.h"
> >> >+#include "intel_display_types.h"
> >> >+
> >> >+/**
> >> >+ * Display Metrics
> >> >+ *
> >> >+ * Provide some display activity overview such as active refresh
> >> >+rates,
> >> >+ * vblank activity and page flip activities.
> >> >+ * For now it is informative debug only, but later it will be
> >> >+expanded
> >> >+ * to be used for GT frequency selection by GuC SLPC.
> >> >+ */
> >> >+
> >> >+/*
> >> >+ * An event using an work queue is used to avoid any disturbance in
> >> >+the
> >> >+ * critical path that could cause performance impacts.
> >> >+ */
> >> >+struct display_event {
> >> >+	struct work_struct work;
> >> >+	struct drm_i915_private *i915;
> >> >+	struct intel_display *display;
> >> >+	bool is_vblank;
> >> >+	int pipe;
> >> >+	int plane;
> >> >+	bool async_flip;
> >> >+};
> >> >+
> >> >+/*
> >> >+ * Although we could simply save this inside our crtc structs, we
> >> >+are
> >> >+ * already mimicking the GuC SLPC defition of the display data, for
> >> >+future
> >> >+ * usage.
> >> >+ */
> >> >+#define MAX_PIPES		8
> >> >+#define MAX_PLANES_PER_PIPE	8
> >> >+
> >> >+struct display_global_info {
> >> >+	u32 version:8;
> >> >+	u32 num_pipes:4;
> >> >+	u32 num_planes_per_pipe:4;
> >> >+	u32 reserved_1:16;
> >> >+	u32 refresh_count:16;
> >> >+	u32 vblank_count:16;
> >> >+	u32 flip_count:16;
> >> >+	u32 reserved_2:16;
> >> >+	u32 reserved_3[13];
> >> >+} __packed;
> >> >+
> >> >+struct display_refresh_info {
> >> >+	u32 refresh_interval:16;
> >> >+	u32 is_variable:1;
> >> >+	u32 reserved:15;
> >> >+} __packed;
> >> >+
> >> >+/*
> >> >+ * When used with GuC SLPC, the host must update each 32-bit part
> >> >+with a single
> >> >+ * atomic write so that SLPC will read the contained bit fields together.
> >> >+ * The host must update the two parts in order - total flip count
> >> >+and timestamp
> >> >+ * first, vsync and async flip counts second.
> >> >+ * Hence, these items are not defined with individual bitfields.
> >> >+ */
> >> >+#define FLIP_P1_LAST		REG_GENMASK(31, 7)
> >> >+#define FLIP_P1_TOTAL_COUNT	REG_GENMASK(6, 0)
> >> >+#define FLIP_P2_ASYNC_COUNT	REG_GENMASK(31, 16)
> >> >+#define FLIP_P2_VSYNC_COUNT	REG_GENMASK(15, 0)
> >> >+
> >> >+struct display_flip_metrics {
> >> >+	u32 part1;
> >> >+	u32 part2;
> >> >+} __packed;
> >> >+
> >> >+/*
> >> >+ * When used with GuC SLPC, the host must update each 32-bit part
> >> >+with a single
> >> >+ * atomic write, so that SLPC will read the count and timestamp together.
> >> >+ * Hence, this item is not defined with individual bitfields.
> >> >+ */
> >> >+#define VBLANK_LAST	REG_GENMASK(31, 7)
> >> >+#define VBLANK_COUNT	REG_GENMASK(6, 0)
> >> >+
> >> >+struct intel_display_metrics {
> >> >+	struct display_global_info global_info;
> >> >+	struct display_refresh_info refresh_info[MAX_PIPES];
> >> >+	u32 vblank_metrics[MAX_PIPES];
> >> >+	struct display_flip_metrics
> >> >+	flip_metrics[MAX_PIPES][MAX_PLANES_PER_PIPE];
> >> >+} __packed;
> >> >+
> >> >+/**
> >> >+ * intel_metrics_refresh_info - Refresh rate information
> >> >+ * @display: Pointer to the intel_display struct that is in use by the gfx
> >device.
> >> >+ * @crtc_state: New CRTC state upon a modeset.
> >> >+ *
> >> >+ * To be called on a modeset. It then saves the current refresh
> >> >+interval in
> >> >+ * micro seconds.
> >> >+ */
> >> >+void intel_metrics_refresh_info(struct intel_display *display,
> >> >+				struct intel_crtc_state *crtc_state) {
> >> >+	struct intel_display_metrics *metrics = display->metrics;
> >> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> >+	struct drm_display_mode *mode = &crtc_state->hw.adjusted_mode;
> >> >+	u32 interval_us;
> >> >+
> >> >+	if (!display->metrics)
> >> >+		return;
> >> >+
> >> >+	interval_us = crtc_state->hw.active ? DIV_ROUND_UP(1000000,
> >> >+						drm_mode_vrefresh(mode)) :
> >> >0;
> >> >+
> >> >+	metrics->refresh_info[crtc->pipe].refresh_interval = interval_us;
> >> >+	metrics->refresh_info[crtc->pipe].is_variable = crtc_state-
> >> >>uapi.vrr_enabled;
> >> >+	metrics->global_info.refresh_count += 1; }
> >> >+
> >> >+static void metrics_update_vblank(struct intel_display_metrics
> >> >+*metrics, int
> >> >pipe,
> >> >+				  u32 timestamp)
> >> >+{
> >> >+	u32 vblank;
> >> >+
> >> >+	vblank = metrics->vblank_metrics[pipe];
> >> >+
> >> >+	vblank = REG_FIELD_GET(VBLANK_COUNT, vblank);
> >> >+	vblank = REG_FIELD_PREP(VBLANK_COUNT, vblank + 1);
> >> >+	vblank |= REG_FIELD_PREP(VBLANK_LAST, timestamp);
> >> >+
> >> >+	/* Write everything at once in preparation for the GuC SLPC
> >> >requirement */
> >> >+	metrics->vblank_metrics[pipe] = vblank;
> >> >+	metrics->global_info.vblank_count += 1; }
> >> >+
> >> >+static void metrics_update_flip(struct intel_display_metrics *metrics, int
> >pipe,
> >> >+				int plane, bool async_flip, u32 timestamp) {
> >> >+	u32 part1, part2, count;
> >> >+
> >> >+	part1 = metrics->flip_metrics[pipe][plane].part1;
> >> >+	part2 = metrics->flip_metrics[pipe][plane].part2;
> >> >+
> >> >+	part1 = REG_FIELD_GET(FLIP_P1_TOTAL_COUNT, part1);
> >> >+	part1 = REG_FIELD_PREP(FLIP_P1_TOTAL_COUNT, part1 + 1);
> >> >+	part1 |= REG_FIELD_PREP(FLIP_P1_LAST, timestamp);
> >> >+
> >> >+	if (async_flip) {
> >> >+		count = REG_FIELD_GET(FLIP_P2_ASYNC_COUNT, part2);
> >> >+		part2 &= ~FLIP_P2_ASYNC_COUNT;
> >> >+		part2 |= REG_FIELD_PREP(FLIP_P2_ASYNC_COUNT, count + 1);
> >> >+	} else {
> >> >+		count = REG_FIELD_GET(FLIP_P2_VSYNC_COUNT, part2);
> >> >+		part2 &= ~FLIP_P2_VSYNC_COUNT;
> >> >+		part2 |= REG_FIELD_PREP(FLIP_P2_VSYNC_COUNT, count + 1);
> >> >+	}
> >> >+
> >> >+	/*
> >> >+	 * Write everything in this way and this order in preparation for the
> >> >+	 * GuC SLPC requirement
> >> >+	 */
> >> >+	metrics->flip_metrics[pipe][plane].part1 = part1;
> >> >+	metrics->flip_metrics[pipe][plane].part2 = part2;
> >> >+
> >> >+	metrics->global_info.flip_count += 1; }
> >> >+
> >> >+/*
> >> >+ * Let's use the same register GuC SLPC uses for timestamp.
> >> >+ * It uses a register that is outside GT domain so GuC doesn't need
> >> >+ * to wake the GT for reading during SLPC loop.
> >> >+ * This is a single register regarding the GT, so we can read
> >> >+directly
> >> >+ * from here, regarding the GT GuC is in.
> >> >+ */
> >> >+#define MCHBAR_MIRROR_BASE_SNB	0x140000
> >> >+#define MCHBAR_BCLK_COUNT	_MMIO(MCHBAR_MIRROR_BASE_SNB
> >> >+ 0x5984)
> >> >+#define MTL_BCLK_COUNT		_MMIO(0xc28)
> >> >+#define   TIMESTAMP_MASK	REG_GENMASK(30, 6)
> >> >+
> >> >+static u32 bclk_read_timestamp(struct drm_i915_private *i915) {
> >> >+	u32 timestamp;
> >> >+
> >> >+	if (DISPLAY_VER(i915) >= 14)
> >> >+		timestamp = intel_de_read(i915, MTL_BCLK_COUNT);
> >> >+	else
> >> >+		timestamp = intel_de_read(i915, MCHBAR_BCLK_COUNT);
> >> >+
> >> >+	return REG_FIELD_GET(TIMESTAMP_MASK, timestamp); }
> >> >+
> >> >+static void display_event_work(struct work_struct *work) {
> >> >+	struct display_event *event = container_of(work, struct
> >> >+display_event,
> >> >work);
> >> >+	struct intel_display *display = event->display;
> >> >+	u32 timestamp;
> >> >+
> >> >+	timestamp = bclk_read_timestamp(event->i915);
> >> >+
> >> >+	if (event->is_vblank) {
> >> >+		metrics_update_vblank(display->metrics, event->pipe,
> >> >timestamp);
> >> >+	} else {
> >> >+		metrics_update_flip(display->metrics, event->pipe, event-
> >> >>plane,
> >> >+				    event->async_flip, timestamp);
> >> >+	}
> >> >+
> >> >+	kfree(event);
> >> >+}
> >> >+
> >> >+void intel_metrics_init(struct intel_display *display) {
> >> >+	struct intel_display_metrics *metrics;
> >> >+
> >> >+	metrics = kzalloc(sizeof(*metrics), GFP_KERNEL);
> >> >+	if (!metrics)
> >> >+		return;
> >> >+
> >> >+	metrics->global_info.version = 1;
> >> >+	metrics->global_info.num_pipes = MAX_PIPES;
> >> >+	metrics->global_info.num_planes_per_pipe = MAX_PLANES_PER_PIPE;
> >> >+
> >> >+	display->metrics = metrics;
> >> >+}
> >> >+
> >> >+void intel_metrics_fini(struct intel_display *display) {
> >> >+	if (!display->metrics)
> >> >+		return;
> >> >+
> >> >+	kfree(display->metrics);
> >> >+}
> >> >+
> >> >+/**
> >> >+ * intel_metrics_vblank - Vblank information
> >> >+ * @display: Pointer to the intel_display struct that is in use by the gfx
> >device.
> >> >+ * @crtc: The Intel CRTC that received the vblank interrupt.
> >> >+ *
> >> >+ * To be called when a vblank is passed.
> >> >+ */
> >> >+void intel_metrics_vblank(struct intel_display *display,
> >> >+			  struct intel_crtc *crtc)
> >> >+{
> >> >+	struct display_event *event;
> >> >+
> >> >+	if (!display->metrics)
> >> >+		return;
> >> >+
> >> >+	event = kmalloc(sizeof(*event), GFP_ATOMIC);
> >>
> >> HI Rodrigo, after changing kmalloc -> kzalloc, it works fine for us. It does
> >correctly prints Global Flip count and Async Flip count.
> >> Prior to this change, event->is_vblank in function display_event_work is
> >always true and hence it never increases flip count.
> >
> >thanks for pointing that out. kzalloc is indeed better.
> >There's also another kmalloc down that needs to be changed to kzalloc.
> >
> >Anyway, when you mentioned that you saw the async flip count increasing,
> >you are telling about the igt test right?
> >or do you have any special compositor change required?
> 
> Hi Rodrigo, async flip count is increasing using both IGT and Weston/wayland client app (https://gitlab.freedesktop.org/wayland/weston/-/blob/main/clients/simple-egl.c#L1294)
> Additionally, we had to hack mesa code to use async flip supported modifiers. Mesa needs to handle async flip scenarios and select supported modifiers.

Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

do we have a mesa MR for that?

> 
> >In my environment here I still only see the async flip increasing with IGT or with
> >a very limited cases...
> >
> >>
> >> Thanks Chaitanya for pointing this out.
> >> Regards,
> >> Naveen Kumar
> >>
> >> >+	if (!event)
> >> >+		return;
> >> >+
> >> >+	INIT_WORK(&event->work, display_event_work);
> >> >+	event->i915 = to_i915(crtc->base.dev);
> >> >+	event->display = display;
> >> >+	event->is_vblank = true;
> >> >+	event->pipe = crtc->pipe;
> >> >+	queue_work(system_highpri_wq, &event->work); }
> >> >+
> >> >+/**
> >> >+ * intel_metrics_flip - Flip information
> >> >+ * @display: Pointer to the intel_display struct that is in use by the gfx
> >device.
> >> >+ * @crtc_state: New CRTC state upon a page flip.
> >> >+ * @plane: Which plane ID got the page flip.
> >> >+ * @async_flip: A boolean to indicate if the page flip was async.
> >> >+ *
> >> >+ * To be called on a page flip.
> >> >+ */
> >> >+void intel_metrics_flip(struct intel_display *display,
> >> >+			const struct intel_crtc_state *crtc_state,
> >> >+			int plane, bool async_flip)
> >> >+{
> >> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> >+	struct display_event *event;
> >> >+
> >> >+	if (!display->metrics)
> >> >+		return;
> >> >+
> >> >+	event = kmalloc(sizeof(*event), GFP_ATOMIC);
> >> Same here, had to change kmalloc -> kzalloc
> >>
> >> >+	if (!event)
> >> >+		return;
> >> >+
> >> >+	INIT_WORK(&event->work, display_event_work);
> >> >+	event->i915 = to_i915(crtc->base.dev);
> >> >+	event->display = display;
> >> >+	event->pipe = crtc->pipe;
> >> >+	event->plane = plane;
> >> >+	event->async_flip = async_flip;
> >> >+	queue_work(system_highpri_wq, &event->work); }
> >> >+
> >> >+void intel_metrics_show(struct intel_display *display, struct
> >> >+drm_printer *p) {
> >> >+	struct intel_display_metrics *metrics = display->metrics;
> >> >+	int pipe, plane;
> >> >+	u32 val;
> >> >+
> >> >+	if (!metrics)
> >> >+		return;
> >> >+
> >> >+	drm_printf(p, "\nDisplay Metrics - Globals:\n");
> >> >+	drm_printf(p, "\tVersion: %d\n", metrics->global_info.version);
> >> >+	drm_printf(p, "\tNum Pipes: %d\n", metrics->global_info.num_pipes);
> >> >+	drm_printf(p, "\tNum Planes per Pipe: %d\n",
> >> >+		   metrics->global_info.num_planes_per_pipe);
> >> >+	drm_printf(p, "\tGlobal Refresh Info Count: %d\n",
> >> >+		   metrics->global_info.refresh_count);
> >> >+	drm_printf(p, "\tGlobal Vblank Count: %d\n",
> >> >+		   metrics->global_info.vblank_count);
> >> >+	drm_printf(p, "\tGlobal Flip Count: %d\n",
> >> >+		   metrics->global_info.flip_count);
> >> >+
> >> >+	for (pipe = 0; pipe < MAX_PIPES; pipe++) {
> >> >+		if (!metrics->refresh_info[pipe].refresh_interval)
> >> >+			continue;
> >> >+
> >> >+		drm_printf(p, "\nDisplay Metrics - Refresh Info - Pipe[%d]:\n",
> >> >+			   pipe);
> >> >+		drm_printf(p, "\tRefresh Interval: %d\n",
> >> >+			   metrics->refresh_info[pipe].refresh_interval);
> >> >+		drm_printf(p, "\tIS VRR: %d\n",
> >> >+			   metrics->refresh_info[pipe].is_variable);
> >> >+
> >> >+		drm_printf(p, "Display Metrics - Vblank Info - Pipe[%d]:\n",
> >> >+			   pipe);
> >> >+		val = metrics->vblank_metrics[pipe];
> >> >+		drm_printf(p, "\tVBlank Last Timestamp: %x\n",
> >> >+			   REG_FIELD_GET(VBLANK_LAST, val));
> >> >+		drm_printf(p, "\tVBlank Count: %d\n",
> >> >+			   REG_FIELD_GET(VBLANK_COUNT, val));
> >> >+
> >> >+		drm_printf(p, "Display Metrics - Flip Info - Pipe[%d]:\n", pipe);
> >> >+		for (plane = 0; plane < MAX_PLANES_PER_PIPE; plane++) {
> >> >+			val = metrics->flip_metrics[pipe][plane].part1;
> >> >+			if (!val)
> >> >+				continue;
> >> >+
> >> >+			drm_printf(p, "\tFlip Info - Plane[%d]:\n", plane);
> >> >+			drm_printf(p, "\t\tFlip Last Timestamp: %x\n",
> >> >+				   REG_FIELD_GET(FLIP_P1_LAST, val));
> >> >+			drm_printf(p, "\t\tFlip Total Count: %d\n",
> >> >+				   REG_FIELD_GET(FLIP_P1_TOTAL_COUNT,
> >> >val));
> >> >+
> >> >+			val = metrics->flip_metrics[pipe][plane].part2;
> >> >+
> >> >+			drm_printf(p, "\t\tFlip Async Count: %d\n",
> >> >+				   REG_FIELD_GET(FLIP_P2_ASYNC_COUNT,
> >> >val));
> >> >+			drm_printf(p, "\t\tFlip Vsync Count: %d\n",
> >> >+				   REG_FIELD_GET(FLIP_P2_VSYNC_COUNT,
> >> >val));
> >> >+		}
> >> >+	}
> >> >+}
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_metrics.h
> >> >b/drivers/gpu/drm/i915/display/intel_metrics.h
> >> >new file mode 100644
> >> >index 000000000000..9e41dc9522f3
> >> >--- /dev/null
> >> >+++ b/drivers/gpu/drm/i915/display/intel_metrics.h
> >> >@@ -0,0 +1,27 @@
> >> >+/* SPDX-License-Identifier: MIT */
> >> >+/*
> >> >+ * Copyright (c) 2024 Intel Corporation  */
> >> >+
> >> >+#ifndef __INTEL_METRICS_H__
> >> >+#define __INTEL_METRICS_H__
> >> >+
> >> >+#include <linux/types.h>
> >> >+
> >> >+struct drm_printer;
> >> >+struct intel_crtc;
> >> >+struct intel_crtc_state;
> >> >+struct intel_display;
> >> >+
> >> >+void intel_metrics_refresh_info(struct intel_display *display,
> >> >+				struct intel_crtc_state *crtc_state); void
> >> >+intel_metrics_vblank(struct intel_display *display,
> >> >+			  struct intel_crtc *intel_crtc); void
> >intel_metrics_flip(struct
> >> >+intel_display *display,
> >> >+			const struct intel_crtc_state *crtc_state,
> >> >+			int plane, bool async_flip);
> >> >+void intel_metrics_init(struct intel_display *display); void
> >> >+intel_metrics_fini(struct intel_display *display); void
> >> >+intel_metrics_show(struct intel_display *display, struct drm_printer
> >> >+*p);
> >> >+
> >> >+#endif
> >> >diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> >b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> >index 860574d04f88..fdd21a41d79d 100644
> >> >--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> >+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> >@@ -17,6 +17,7 @@
> >> > #include "intel_fb.h"
> >> > #include "intel_fbc.h"
> >> > #include "intel_frontbuffer.h"
> >> >+#include "intel_metrics.h"
> >> > #include "intel_psr.h"
> >> > #include "intel_psr_regs.h"
> >> > #include "skl_scaler.h"
> >> >@@ -1392,6 +1393,7 @@ skl_plane_async_flip(struct intel_plane *plane,
> >> > 		     bool async_flip)
> >> > {
> >> > 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >> >+	struct intel_display *display = &dev_priv->display;
> >> > 	enum plane_id plane_id = plane->id;
> >> > 	enum pipe pipe = plane->pipe;
> >> > 	u32 plane_ctl = plane_state->ctl;
> >> >@@ -1404,6 +1406,7 @@ skl_plane_async_flip(struct intel_plane *plane,
> >> > 	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> >> > 	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> >> > 			  skl_plane_surf(plane_state, 0));
> >> >+	intel_metrics_flip(display, crtc_state, plane_id, async_flip);
> >> > }
> >> >
> >> > static bool intel_format_is_p01x(u32 format) diff --git
> >> >a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index
> >> >e5b1715f721e..5133dba2c7de 100644
> >> >--- a/drivers/gpu/drm/xe/Makefile
> >> >+++ b/drivers/gpu/drm/xe/Makefile
> >> >@@ -263,6 +263,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> >> > 	i915-display/intel_hti.o \
> >> > 	i915-display/intel_link_bw.o \
> >> > 	i915-display/intel_lspcon.o \
> >> >+	i915-display/intel_metrics.o \
> >> > 	i915-display/intel_modeset_lock.o \
> >> > 	i915-display/intel_modeset_setup.o \
> >> > 	i915-display/intel_modeset_verify.o \
> >> >--
> >> >2.44.0
> >>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915/display: Introduce Display Metrics info
  2024-05-07 12:48       ` Rodrigo Vivi
@ 2024-05-07 17:37         ` Zanoni, Paulo R
  2024-05-09 18:41           ` Kumar, Naveen1
  0 siblings, 1 reply; 10+ messages in thread
From: Zanoni, Paulo R @ 2024-05-07 17:37 UTC (permalink / raw)
  To: Vivi, Rodrigo, Kumar, Naveen1, Souza, Jose
  Cc: Shankar, Uma, Kulkarni, Vandita, Nikula, Jani, intel-gfx,
	Belgaumkar, Vinay, Borah, Chaitanya Kumar

On Tue, 2024-05-07 at 08:48 -0400, Rodrigo Vivi wrote:
> On Mon, May 06, 2024 at 11:19:56PM -0400, Kumar, Naveen1 wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > Sent: Monday, May 6, 2024 10:52 PM
> > > To: Kumar, Naveen1 <naveen1.kumar@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>;
> > > Kulkarni, Vandita <vandita.kulkarni@intel.com>; Nikula, Jani
> > > <jani.nikula@intel.com>; Belgaumkar, Vinay <vinay.belgaumkar@intel.com>;
> > > Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
> > > Subject: Re: [PATCH] drm/i915/display: Introduce Display Metrics info
> > > 
> > > On Mon, May 06, 2024 at 06:03:17AM -0400, Kumar, Naveen1 wrote:
> > > > 
> > > > 
> > > > > -----Original Message-----
> > > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > > Sent: Saturday, April 6, 2024 3:39 AM
> > > > > To: intel-gfx@lists.freedesktop.org
> > > > > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Shankar, Uma
> > > > > <uma.shankar@intel.com>; Kulkarni, Vandita
> > > > > <vandita.kulkarni@intel.com>; Kumar, Naveen1
> > > > > <naveen1.kumar@intel.com>; Nikula, Jani <jani.nikula@intel.com>;
> > > > > Belgaumkar, Vinay <vinay.belgaumkar@intel.com>
> > > > > Subject: [PATCH] drm/i915/display: Introduce Display Metrics info
> > > > > 
> > > > > Introduce a display metrics information through debugfs for a better
> > > > > view of the vblank and page flips, in special Async flips behavior.
> > > > > 
> > > > > There is currently an overall expectation that whenever
> > > > > vblank_mode=0 is used with an graphics application, that
> > > > > automatically async_flips are happening. However, while implementing
> > > > > the Display Metrics for GuC SLPC, it was observed that it is not
> > > > > necessarily true for many of the expected cases.
> > > > > 
> > > > > So, while the GuC SLPC side of the metrics doesn't get ready, let's
> > > > > at least bring the debugfs view of it so we can work to understand
> > > > > and fix any potential issue around our async vblanks.
> > > > > 
> > > > > Please notice that the every struct here follows exactly the GuC
> > > > > shared data buffer, so the next step of the integration would be
> > > > > smooth and almost transparent to this intel_metrics on the display side.
> > > > > 
> > > > > Cc: Uma Shankar <uma.shankar@intel.com>
> > > > > Cc: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > > > > Cc: Naveen Kumar <naveen1.kumar@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/Makefile                 |   1 +
> > > > > drivers/gpu/drm/i915/display/intel_display.c  |  15 +-
> > > > > .../gpu/drm/i915/display/intel_display_core.h |   2 +
> > > > > .../drm/i915/display/intel_display_debugfs.c  |  12 +
> > > > > .../drm/i915/display/intel_display_driver.c   |   5 +
> > > > > .../gpu/drm/i915/display/intel_display_irq.c  |   3 +
> > > > > drivers/gpu/drm/i915/display/intel_metrics.c  | 356
> > > > > ++++++++++++++++++ drivers/gpu/drm/i915/display/intel_metrics.h  |  27
> > > ++
> > > > > .../drm/i915/display/skl_universal_plane.c    |   3 +
> > > > > drivers/gpu/drm/xe/Makefile                   |   1 +
> > > > > 10 files changed, 424 insertions(+), 1 deletion(-)  create mode
> > > > > 100644 drivers/gpu/drm/i915/display/intel_metrics.c
> > > > > create mode 100644 drivers/gpu/drm/i915/display/intel_metrics.h
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/Makefile
> > > > > b/drivers/gpu/drm/i915/Makefile index af9e871daf1d..a3c8d9f5614c
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/i915/Makefile
> > > > > +++ b/drivers/gpu/drm/i915/Makefile
> > > > > @@ -291,6 +291,7 @@ i915-y += \
> > > > > 	display/intel_link_bw.o \
> > > > > 	display/intel_load_detect.o \
> > > > > 	display/intel_lpe_audio.o \
> > > > > +	display/intel_metrics.o \
> > > > > 	display/intel_modeset_lock.o \
> > > > > 	display/intel_modeset_setup.o \
> > > > > 	display/intel_modeset_verify.o \
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index a481c9218138..ca30b8d48e1f 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -94,6 +94,7 @@
> > > > > #include "intel_link_bw.h"
> > > > > #include "intel_lvds.h"
> > > > > #include "intel_lvds_regs.h"
> > > > > +#include "intel_metrics.h"
> > > > > #include "intel_modeset_setup.h"
> > > > > #include "intel_modeset_verify.h"
> > > > > #include "intel_overlay.h"
> > > > > @@ -1021,11 +1022,15 @@ static void intel_post_plane_update(struct
> > > > > intel_atomic_state *state,
> > > > > 				    struct intel_crtc *crtc)
> > > > > {
> > > > > 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > +	struct intel_display *display = &dev_priv->display;
> > > > > 	const struct intel_crtc_state *old_crtc_state =
> > > > > 		intel_atomic_get_old_crtc_state(state, crtc);
> > > > > 	const struct intel_crtc_state *new_crtc_state =
> > > > > 		intel_atomic_get_new_crtc_state(state, crtc);
> > > > > 	enum pipe pipe = crtc->pipe;
> > > > > +	const struct intel_plane_state __maybe_unused *plane_state;
> > > > > +	struct intel_plane *plane;
> > > > > +	int i;
> > > > > 
> > > > > 	intel_psr_post_plane_update(state, crtc);
> > > > > 
> > > > > @@ -1057,6 +1062,12 @@ static void intel_post_plane_update(struct
> > > > > intel_atomic_state *state,
> > > > > 
> > > > > 	if (audio_enabling(old_crtc_state, new_crtc_state))
> > > > > 		intel_encoders_audio_enable(state, crtc);
> > > > > +
> > > > > +	if (!new_crtc_state->do_async_flip) {
> > > > > +		for_each_new_intel_plane_in_state(state, plane, plane_state, i)
> > > > > +			intel_metrics_flip(display, new_crtc_state, plane->id,
> > > > > +					   false);
> > > > > +	}
> > > > > }
> > > > > 
> > > > > static void intel_crtc_enable_flip_done(struct intel_atomic_state
> > > > > *state, @@ -
> > > > > 7139,6 +7150,7 @@ static void intel_atomic_commit_tail(struct
> > > > > intel_atomic_state *state)  {
> > > > > 	struct drm_device *dev = state->base.dev;
> > > > > 	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > +	struct intel_display *display = &dev_priv->display;
> > > > > 	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > > 	struct intel_crtc *crtc;
> > > > > 	struct intel_power_domain_mask put_domains[I915_MAX_PIPES] = {};
> > > @@
> > > > > -7261,7 +7273,6 @@ static void intel_atomic_commit_tail(struct
> > > > > intel_atomic_state *state)
> > > > > 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > > > 		if (new_crtc_state->do_async_flip)
> > > > > 			intel_crtc_disable_flip_done(state, crtc);
> > > > > -
> > > > > 		intel_color_wait_commit(new_crtc_state);
> > > > > 	}
> > > > > 
> > > > > @@ -7314,6 +7325,8 @@ static void intel_atomic_commit_tail(struct
> > > > > intel_atomic_state *state)
> > > > > 		 * FIXME get rid of this funny new->old swapping
> > > > > 		 */
> > > > > 		old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb);
> > > > > +
> > > > > +		intel_metrics_refresh_info(display, new_crtc_state);
> > > > > 	}
> > > > > 
> > > > > 	/* Underruns don't always raise interrupts, so check manually */
> > > > > diff -- git a/drivers/gpu/drm/i915/display/intel_display_core.h
> > > > > b/drivers/gpu/drm/i915/display/intel_display_core.h
> > > > > index 2167dbee5eea..992db9098566 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > > > > @@ -42,6 +42,7 @@ struct intel_cdclk_vals;  struct intel_color_funcs;
> > > > > struct intel_crtc;  struct intel_crtc_state;
> > > > > +struct intel_display_metrics;
> > > > > struct intel_dmc;
> > > > > struct intel_dpll_funcs;
> > > > > struct intel_dpll_mgr;
> > > > > @@ -530,6 +531,7 @@ struct intel_display {
> > > > > 	struct intel_fbc *fbc[I915_MAX_FBCS];
> > > > > 	struct intel_frontbuffer_tracking fb_tracking;
> > > > > 	struct intel_hotplug hotplug;
> > > > > +	struct intel_display_metrics *metrics;
> > > > > 	struct intel_opregion *opregion;
> > > > > 	struct intel_overlay *overlay;
> > > > > 	struct intel_display_params params; diff --git
> > > > > a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > > index 3e364891dcd0..f05b9a9ddee0 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > > @@ -30,6 +30,7 @@
> > > > > #include "intel_hdcp.h"
> > > > > #include "intel_hdmi.h"
> > > > > #include "intel_hotplug.h"
> > > > > +#include "intel_metrics.h"
> > > > > #include "intel_panel.h"
> > > > > #include "intel_psr.h"
> > > > > #include "intel_psr_regs.h"
> > > > > @@ -642,6 +643,16 @@ static int i915_display_capabilities(struct
> > > > > seq_file *m, void *unused)
> > > > > 	return 0;
> > > > > }
> > > > > 
> > > > > +static int i915_display_metrics(struct seq_file *m, void *unused) {
> > > > > +	struct drm_i915_private *i915 = node_to_i915(m->private);
> > > > > +	struct drm_printer p = drm_seq_file_printer(m);
> > > > > +
> > > > > +	intel_metrics_show(&i915->display, &p);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > static int i915_shared_dplls_info(struct seq_file *m, void *unused)  {
> > > > > 	struct drm_i915_private *dev_priv = node_to_i915(m->private); @@ -
> > > > > 1059,6 +1070,7 @@ static const struct drm_info_list
> > > > > intel_display_debugfs_list[] = {
> > > > > 	{"i915_power_domain_info", i915_power_domain_info, 0},
> > > > > 	{"i915_display_info", i915_display_info, 0},
> > > > > 	{"i915_display_capabilities", i915_display_capabilities, 0},
> > > > > +	{"i915_display_metrics", i915_display_metrics, 0},
> > > > > 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
> > > > > 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
> > > > > 	{"i915_ddb_info", i915_ddb_info, 0}, diff --git
> > > > > a/drivers/gpu/drm/i915/display/intel_display_driver.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display_driver.c
> > > > > index 87dd07e0d138..767b2d5b3826 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> > > > > @@ -46,6 +46,7 @@
> > > > > #include "intel_hdcp.h"
> > > > > #include "intel_hotplug.h"
> > > > > #include "intel_hti.h"
> > > > > +#include "intel_metrics.h"
> > > > > #include "intel_modeset_lock.h"
> > > > > #include "intel_modeset_setup.h"
> > > > > #include "intel_opregion.h"
> > > > > @@ -514,6 +515,8 @@ int intel_display_driver_probe(struct
> > > > > drm_i915_private *i915)
> > > > > 
> > > > > 	intel_overlay_setup(i915);
> > > > > 
> > > > > +	intel_metrics_init(&i915->display);
> > > > > +
> > > > > 	ret = intel_fbdev_init(&i915->drm);
> > > > > 	if (ret)
> > > > > 		return ret;
> > > > > @@ -611,6 +614,8 @@ void intel_display_driver_remove_noirq(struct
> > > > > drm_i915_private *i915)
> > > > > 
> > > > > 	intel_dp_tunnel_mgr_cleanup(i915);
> > > > > 
> > > > > +	intel_metrics_fini(&i915->display);
> > > > > +
> > > > > 	intel_overlay_cleanup(i915);
> > > > > 
> > > > > 	intel_gmbus_teardown(i915);
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > > > index f846c5b108b5..202400a771b2 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > > > @@ -18,6 +18,7 @@
> > > > > #include "intel_fifo_underrun.h"
> > > > > #include "intel_gmbus.h"
> > > > > #include "intel_hotplug_irq.h"
> > > > > +#include "intel_metrics.h"
> > > > > #include "intel_pmdemand.h"
> > > > > #include "intel_psr.h"
> > > > > #include "intel_psr_regs.h"
> > > > > @@ -25,8 +26,10 @@
> > > > > static void
> > > > > intel_handle_vblank(struct drm_i915_private *dev_priv, enum pipe
> > > > > pipe)  {
> > > > > +	struct intel_display *display = &dev_priv->display;
> > > > > 	struct intel_crtc *crtc = intel_crtc_for_pipe(dev_priv, pipe);
> > > > > 
> > > > > +	intel_metrics_vblank(display, crtc);
> > > > > 	drm_crtc_handle_vblank(&crtc->base);
> > > > > }
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_metrics.c
> > > > > b/drivers/gpu/drm/i915/display/intel_metrics.c
> > > > > new file mode 100644
> > > > > index 000000000000..2cb2b8189b25
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_metrics.c
> > > > > @@ -0,0 +1,356 @@
> > > > > +// SPDX-License-Identifier: MIT
> > > > > +/*
> > > > > + * Copyright (c) 2024 Intel Corporation  */
> > > > > +
> > > > > +#include "intel_metrics.h"
> > > > > +
> > > > > +#include <drm/drm_modes.h>
> > > > > +#include <drm/drm_print.h>
> > > > > +
> > > > > +#include "i915_drv.h"
> > > > > +#include "intel_de.h"
> > > > > +#include "intel_display_types.h"
> > > > > +
> > > > > +/**
> > > > > + * Display Metrics
> > > > > + *
> > > > > + * Provide some display activity overview such as active refresh
> > > > > +rates,
> > > > > + * vblank activity and page flip activities.
> > > > > + * For now it is informative debug only, but later it will be
> > > > > +expanded
> > > > > + * to be used for GT frequency selection by GuC SLPC.
> > > > > + */
> > > > > +
> > > > > +/*
> > > > > + * An event using an work queue is used to avoid any disturbance in
> > > > > +the
> > > > > + * critical path that could cause performance impacts.
> > > > > + */
> > > > > +struct display_event {
> > > > > +	struct work_struct work;
> > > > > +	struct drm_i915_private *i915;
> > > > > +	struct intel_display *display;
> > > > > +	bool is_vblank;
> > > > > +	int pipe;
> > > > > +	int plane;
> > > > > +	bool async_flip;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Although we could simply save this inside our crtc structs, we
> > > > > +are
> > > > > + * already mimicking the GuC SLPC defition of the display data, for
> > > > > +future
> > > > > + * usage.
> > > > > + */
> > > > > +#define MAX_PIPES		8
> > > > > +#define MAX_PLANES_PER_PIPE	8
> > > > > +
> > > > > +struct display_global_info {
> > > > > +	u32 version:8;
> > > > > +	u32 num_pipes:4;
> > > > > +	u32 num_planes_per_pipe:4;
> > > > > +	u32 reserved_1:16;
> > > > > +	u32 refresh_count:16;
> > > > > +	u32 vblank_count:16;
> > > > > +	u32 flip_count:16;
> > > > > +	u32 reserved_2:16;
> > > > > +	u32 reserved_3[13];
> > > > > +} __packed;
> > > > > +
> > > > > +struct display_refresh_info {
> > > > > +	u32 refresh_interval:16;
> > > > > +	u32 is_variable:1;
> > > > > +	u32 reserved:15;
> > > > > +} __packed;
> > > > > +
> > > > > +/*
> > > > > + * When used with GuC SLPC, the host must update each 32-bit part
> > > > > +with a single
> > > > > + * atomic write so that SLPC will read the contained bit fields together.
> > > > > + * The host must update the two parts in order - total flip count
> > > > > +and timestamp
> > > > > + * first, vsync and async flip counts second.
> > > > > + * Hence, these items are not defined with individual bitfields.
> > > > > + */
> > > > > +#define FLIP_P1_LAST		REG_GENMASK(31, 7)
> > > > > +#define FLIP_P1_TOTAL_COUNT	REG_GENMASK(6, 0)
> > > > > +#define FLIP_P2_ASYNC_COUNT	REG_GENMASK(31, 16)
> > > > > +#define FLIP_P2_VSYNC_COUNT	REG_GENMASK(15, 0)
> > > > > +
> > > > > +struct display_flip_metrics {
> > > > > +	u32 part1;
> > > > > +	u32 part2;
> > > > > +} __packed;
> > > > > +
> > > > > +/*
> > > > > + * When used with GuC SLPC, the host must update each 32-bit part
> > > > > +with a single
> > > > > + * atomic write, so that SLPC will read the count and timestamp together.
> > > > > + * Hence, this item is not defined with individual bitfields.
> > > > > + */
> > > > > +#define VBLANK_LAST	REG_GENMASK(31, 7)
> > > > > +#define VBLANK_COUNT	REG_GENMASK(6, 0)
> > > > > +
> > > > > +struct intel_display_metrics {
> > > > > +	struct display_global_info global_info;
> > > > > +	struct display_refresh_info refresh_info[MAX_PIPES];
> > > > > +	u32 vblank_metrics[MAX_PIPES];
> > > > > +	struct display_flip_metrics
> > > > > +	flip_metrics[MAX_PIPES][MAX_PLANES_PER_PIPE];
> > > > > +} __packed;
> > > > > +
> > > > > +/**
> > > > > + * intel_metrics_refresh_info - Refresh rate information
> > > > > + * @display: Pointer to the intel_display struct that is in use by the gfx
> > > device.
> > > > > + * @crtc_state: New CRTC state upon a modeset.
> > > > > + *
> > > > > + * To be called on a modeset. It then saves the current refresh
> > > > > +interval in
> > > > > + * micro seconds.
> > > > > + */
> > > > > +void intel_metrics_refresh_info(struct intel_display *display,
> > > > > +				struct intel_crtc_state *crtc_state) {
> > > > > +	struct intel_display_metrics *metrics = display->metrics;
> > > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > > +	struct drm_display_mode *mode = &crtc_state->hw.adjusted_mode;
> > > > > +	u32 interval_us;
> > > > > +
> > > > > +	if (!display->metrics)
> > > > > +		return;
> > > > > +
> > > > > +	interval_us = crtc_state->hw.active ? DIV_ROUND_UP(1000000,
> > > > > +						drm_mode_vrefresh(mode)) :
> > > > > 0;
> > > > > +
> > > > > +	metrics->refresh_info[crtc->pipe].refresh_interval = interval_us;
> > > > > +	metrics->refresh_info[crtc->pipe].is_variable = crtc_state-
> > > > > > uapi.vrr_enabled;
> > > > > +	metrics->global_info.refresh_count += 1; }
> > > > > +
> > > > > +static void metrics_update_vblank(struct intel_display_metrics
> > > > > +*metrics, int
> > > > > pipe,
> > > > > +				  u32 timestamp)
> > > > > +{
> > > > > +	u32 vblank;
> > > > > +
> > > > > +	vblank = metrics->vblank_metrics[pipe];
> > > > > +
> > > > > +	vblank = REG_FIELD_GET(VBLANK_COUNT, vblank);
> > > > > +	vblank = REG_FIELD_PREP(VBLANK_COUNT, vblank + 1);
> > > > > +	vblank |= REG_FIELD_PREP(VBLANK_LAST, timestamp);
> > > > > +
> > > > > +	/* Write everything at once in preparation for the GuC SLPC
> > > > > requirement */
> > > > > +	metrics->vblank_metrics[pipe] = vblank;
> > > > > +	metrics->global_info.vblank_count += 1; }
> > > > > +
> > > > > +static void metrics_update_flip(struct intel_display_metrics *metrics, int
> > > pipe,
> > > > > +				int plane, bool async_flip, u32 timestamp) {
> > > > > +	u32 part1, part2, count;
> > > > > +
> > > > > +	part1 = metrics->flip_metrics[pipe][plane].part1;
> > > > > +	part2 = metrics->flip_metrics[pipe][plane].part2;
> > > > > +
> > > > > +	part1 = REG_FIELD_GET(FLIP_P1_TOTAL_COUNT, part1);
> > > > > +	part1 = REG_FIELD_PREP(FLIP_P1_TOTAL_COUNT, part1 + 1);
> > > > > +	part1 |= REG_FIELD_PREP(FLIP_P1_LAST, timestamp);
> > > > > +
> > > > > +	if (async_flip) {
> > > > > +		count = REG_FIELD_GET(FLIP_P2_ASYNC_COUNT, part2);
> > > > > +		part2 &= ~FLIP_P2_ASYNC_COUNT;
> > > > > +		part2 |= REG_FIELD_PREP(FLIP_P2_ASYNC_COUNT, count + 1);
> > > > > +	} else {
> > > > > +		count = REG_FIELD_GET(FLIP_P2_VSYNC_COUNT, part2);
> > > > > +		part2 &= ~FLIP_P2_VSYNC_COUNT;
> > > > > +		part2 |= REG_FIELD_PREP(FLIP_P2_VSYNC_COUNT, count + 1);
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * Write everything in this way and this order in preparation for the
> > > > > +	 * GuC SLPC requirement
> > > > > +	 */
> > > > > +	metrics->flip_metrics[pipe][plane].part1 = part1;
> > > > > +	metrics->flip_metrics[pipe][plane].part2 = part2;
> > > > > +
> > > > > +	metrics->global_info.flip_count += 1; }
> > > > > +
> > > > > +/*
> > > > > + * Let's use the same register GuC SLPC uses for timestamp.
> > > > > + * It uses a register that is outside GT domain so GuC doesn't need
> > > > > + * to wake the GT for reading during SLPC loop.
> > > > > + * This is a single register regarding the GT, so we can read
> > > > > +directly
> > > > > + * from here, regarding the GT GuC is in.
> > > > > + */
> > > > > +#define MCHBAR_MIRROR_BASE_SNB	0x140000
> > > > > +#define MCHBAR_BCLK_COUNT	_MMIO(MCHBAR_MIRROR_BASE_SNB
> > > > > + 0x5984)
> > > > > +#define MTL_BCLK_COUNT		_MMIO(0xc28)
> > > > > +#define   TIMESTAMP_MASK	REG_GENMASK(30, 6)
> > > > > +
> > > > > +static u32 bclk_read_timestamp(struct drm_i915_private *i915) {
> > > > > +	u32 timestamp;
> > > > > +
> > > > > +	if (DISPLAY_VER(i915) >= 14)
> > > > > +		timestamp = intel_de_read(i915, MTL_BCLK_COUNT);
> > > > > +	else
> > > > > +		timestamp = intel_de_read(i915, MCHBAR_BCLK_COUNT);
> > > > > +
> > > > > +	return REG_FIELD_GET(TIMESTAMP_MASK, timestamp); }
> > > > > +
> > > > > +static void display_event_work(struct work_struct *work) {
> > > > > +	struct display_event *event = container_of(work, struct
> > > > > +display_event,
> > > > > work);
> > > > > +	struct intel_display *display = event->display;
> > > > > +	u32 timestamp;
> > > > > +
> > > > > +	timestamp = bclk_read_timestamp(event->i915);
> > > > > +
> > > > > +	if (event->is_vblank) {
> > > > > +		metrics_update_vblank(display->metrics, event->pipe,
> > > > > timestamp);
> > > > > +	} else {
> > > > > +		metrics_update_flip(display->metrics, event->pipe, event-
> > > > > > plane,
> > > > > +				    event->async_flip, timestamp);
> > > > > +	}
> > > > > +
> > > > > +	kfree(event);
> > > > > +}
> > > > > +
> > > > > +void intel_metrics_init(struct intel_display *display) {
> > > > > +	struct intel_display_metrics *metrics;
> > > > > +
> > > > > +	metrics = kzalloc(sizeof(*metrics), GFP_KERNEL);
> > > > > +	if (!metrics)
> > > > > +		return;
> > > > > +
> > > > > +	metrics->global_info.version = 1;
> > > > > +	metrics->global_info.num_pipes = MAX_PIPES;
> > > > > +	metrics->global_info.num_planes_per_pipe = MAX_PLANES_PER_PIPE;
> > > > > +
> > > > > +	display->metrics = metrics;
> > > > > +}
> > > > > +
> > > > > +void intel_metrics_fini(struct intel_display *display) {
> > > > > +	if (!display->metrics)
> > > > > +		return;
> > > > > +
> > > > > +	kfree(display->metrics);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * intel_metrics_vblank - Vblank information
> > > > > + * @display: Pointer to the intel_display struct that is in use by the gfx
> > > device.
> > > > > + * @crtc: The Intel CRTC that received the vblank interrupt.
> > > > > + *
> > > > > + * To be called when a vblank is passed.
> > > > > + */
> > > > > +void intel_metrics_vblank(struct intel_display *display,
> > > > > +			  struct intel_crtc *crtc)
> > > > > +{
> > > > > +	struct display_event *event;
> > > > > +
> > > > > +	if (!display->metrics)
> > > > > +		return;
> > > > > +
> > > > > +	event = kmalloc(sizeof(*event), GFP_ATOMIC);
> > > > 
> > > > HI Rodrigo, after changing kmalloc -> kzalloc, it works fine for us. It does
> > > correctly prints Global Flip count and Async Flip count.
> > > > Prior to this change, event->is_vblank in function display_event_work is
> > > always true and hence it never increases flip count.
> > > 
> > > thanks for pointing that out. kzalloc is indeed better.
> > > There's also another kmalloc down that needs to be changed to kzalloc.
> > > 
> > > Anyway, when you mentioned that you saw the async flip count increasing,
> > > you are telling about the igt test right?
> > > or do you have any special compositor change required?
> > 
> > Hi Rodrigo, async flip count is increasing using both IGT and Weston/wayland client app (https://gitlab.freedesktop.org/wayland/weston/-/blob/main/clients/simple-egl.c#L1294)
> > Additionally, we had to hack mesa code to use async flip supported modifiers. Mesa needs to handle async flip scenarios and select supported modifiers.
> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> do we have a mesa MR for that?

I haven't seen anything. Can you please create a Mesa merge request or
new issue and attach the hack you created and describe the problem?

Thanks,
Paulo

> 
> > 
> > > In my environment here I still only see the async flip increasing with IGT or with
> > > a very limited cases...
> > > 
> > > > 
> > > > Thanks Chaitanya for pointing this out.
> > > > Regards,
> > > > Naveen Kumar
> > > > 
> > > > > +	if (!event)
> > > > > +		return;
> > > > > +
> > > > > +	INIT_WORK(&event->work, display_event_work);
> > > > > +	event->i915 = to_i915(crtc->base.dev);
> > > > > +	event->display = display;
> > > > > +	event->is_vblank = true;
> > > > > +	event->pipe = crtc->pipe;
> > > > > +	queue_work(system_highpri_wq, &event->work); }
> > > > > +
> > > > > +/**
> > > > > + * intel_metrics_flip - Flip information
> > > > > + * @display: Pointer to the intel_display struct that is in use by the gfx
> > > device.
> > > > > + * @crtc_state: New CRTC state upon a page flip.
> > > > > + * @plane: Which plane ID got the page flip.
> > > > > + * @async_flip: A boolean to indicate if the page flip was async.
> > > > > + *
> > > > > + * To be called on a page flip.
> > > > > + */
> > > > > +void intel_metrics_flip(struct intel_display *display,
> > > > > +			const struct intel_crtc_state *crtc_state,
> > > > > +			int plane, bool async_flip)
> > > > > +{
> > > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > > +	struct display_event *event;
> > > > > +
> > > > > +	if (!display->metrics)
> > > > > +		return;
> > > > > +
> > > > > +	event = kmalloc(sizeof(*event), GFP_ATOMIC);
> > > > Same here, had to change kmalloc -> kzalloc
> > > > 
> > > > > +	if (!event)
> > > > > +		return;
> > > > > +
> > > > > +	INIT_WORK(&event->work, display_event_work);
> > > > > +	event->i915 = to_i915(crtc->base.dev);
> > > > > +	event->display = display;
> > > > > +	event->pipe = crtc->pipe;
> > > > > +	event->plane = plane;
> > > > > +	event->async_flip = async_flip;
> > > > > +	queue_work(system_highpri_wq, &event->work); }
> > > > > +
> > > > > +void intel_metrics_show(struct intel_display *display, struct
> > > > > +drm_printer *p) {
> > > > > +	struct intel_display_metrics *metrics = display->metrics;
> > > > > +	int pipe, plane;
> > > > > +	u32 val;
> > > > > +
> > > > > +	if (!metrics)
> > > > > +		return;
> > > > > +
> > > > > +	drm_printf(p, "\nDisplay Metrics - Globals:\n");
> > > > > +	drm_printf(p, "\tVersion: %d\n", metrics->global_info.version);
> > > > > +	drm_printf(p, "\tNum Pipes: %d\n", metrics->global_info.num_pipes);
> > > > > +	drm_printf(p, "\tNum Planes per Pipe: %d\n",
> > > > > +		   metrics->global_info.num_planes_per_pipe);
> > > > > +	drm_printf(p, "\tGlobal Refresh Info Count: %d\n",
> > > > > +		   metrics->global_info.refresh_count);
> > > > > +	drm_printf(p, "\tGlobal Vblank Count: %d\n",
> > > > > +		   metrics->global_info.vblank_count);
> > > > > +	drm_printf(p, "\tGlobal Flip Count: %d\n",
> > > > > +		   metrics->global_info.flip_count);
> > > > > +
> > > > > +	for (pipe = 0; pipe < MAX_PIPES; pipe++) {
> > > > > +		if (!metrics->refresh_info[pipe].refresh_interval)
> > > > > +			continue;
> > > > > +
> > > > > +		drm_printf(p, "\nDisplay Metrics - Refresh Info - Pipe[%d]:\n",
> > > > > +			   pipe);
> > > > > +		drm_printf(p, "\tRefresh Interval: %d\n",
> > > > > +			   metrics->refresh_info[pipe].refresh_interval);
> > > > > +		drm_printf(p, "\tIS VRR: %d\n",
> > > > > +			   metrics->refresh_info[pipe].is_variable);
> > > > > +
> > > > > +		drm_printf(p, "Display Metrics - Vblank Info - Pipe[%d]:\n",
> > > > > +			   pipe);
> > > > > +		val = metrics->vblank_metrics[pipe];
> > > > > +		drm_printf(p, "\tVBlank Last Timestamp: %x\n",
> > > > > +			   REG_FIELD_GET(VBLANK_LAST, val));
> > > > > +		drm_printf(p, "\tVBlank Count: %d\n",
> > > > > +			   REG_FIELD_GET(VBLANK_COUNT, val));
> > > > > +
> > > > > +		drm_printf(p, "Display Metrics - Flip Info - Pipe[%d]:\n", pipe);
> > > > > +		for (plane = 0; plane < MAX_PLANES_PER_PIPE; plane++) {
> > > > > +			val = metrics->flip_metrics[pipe][plane].part1;
> > > > > +			if (!val)
> > > > > +				continue;
> > > > > +
> > > > > +			drm_printf(p, "\tFlip Info - Plane[%d]:\n", plane);
> > > > > +			drm_printf(p, "\t\tFlip Last Timestamp: %x\n",
> > > > > +				   REG_FIELD_GET(FLIP_P1_LAST, val));
> > > > > +			drm_printf(p, "\t\tFlip Total Count: %d\n",
> > > > > +				   REG_FIELD_GET(FLIP_P1_TOTAL_COUNT,
> > > > > val));
> > > > > +
> > > > > +			val = metrics->flip_metrics[pipe][plane].part2;
> > > > > +
> > > > > +			drm_printf(p, "\t\tFlip Async Count: %d\n",
> > > > > +				   REG_FIELD_GET(FLIP_P2_ASYNC_COUNT,
> > > > > val));
> > > > > +			drm_printf(p, "\t\tFlip Vsync Count: %d\n",
> > > > > +				   REG_FIELD_GET(FLIP_P2_VSYNC_COUNT,
> > > > > val));
> > > > > +		}
> > > > > +	}
> > > > > +}
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_metrics.h
> > > > > b/drivers/gpu/drm/i915/display/intel_metrics.h
> > > > > new file mode 100644
> > > > > index 000000000000..9e41dc9522f3
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_metrics.h
> > > > > @@ -0,0 +1,27 @@
> > > > > +/* SPDX-License-Identifier: MIT */
> > > > > +/*
> > > > > + * Copyright (c) 2024 Intel Corporation  */
> > > > > +
> > > > > +#ifndef __INTEL_METRICS_H__
> > > > > +#define __INTEL_METRICS_H__
> > > > > +
> > > > > +#include <linux/types.h>
> > > > > +
> > > > > +struct drm_printer;
> > > > > +struct intel_crtc;
> > > > > +struct intel_crtc_state;
> > > > > +struct intel_display;
> > > > > +
> > > > > +void intel_metrics_refresh_info(struct intel_display *display,
> > > > > +				struct intel_crtc_state *crtc_state); void
> > > > > +intel_metrics_vblank(struct intel_display *display,
> > > > > +			  struct intel_crtc *intel_crtc); void
> > > intel_metrics_flip(struct
> > > > > +intel_display *display,
> > > > > +			const struct intel_crtc_state *crtc_state,
> > > > > +			int plane, bool async_flip);
> > > > > +void intel_metrics_init(struct intel_display *display); void
> > > > > +intel_metrics_fini(struct intel_display *display); void
> > > > > +intel_metrics_show(struct intel_display *display, struct drm_printer
> > > > > +*p);
> > > > > +
> > > > > +#endif
> > > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > index 860574d04f88..fdd21a41d79d 100644
> > > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > @@ -17,6 +17,7 @@
> > > > > #include "intel_fb.h"
> > > > > #include "intel_fbc.h"
> > > > > #include "intel_frontbuffer.h"
> > > > > +#include "intel_metrics.h"
> > > > > #include "intel_psr.h"
> > > > > #include "intel_psr_regs.h"
> > > > > #include "skl_scaler.h"
> > > > > @@ -1392,6 +1393,7 @@ skl_plane_async_flip(struct intel_plane *plane,
> > > > > 		     bool async_flip)
> > > > > {
> > > > > 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > +	struct intel_display *display = &dev_priv->display;
> > > > > 	enum plane_id plane_id = plane->id;
> > > > > 	enum pipe pipe = plane->pipe;
> > > > > 	u32 plane_ctl = plane_state->ctl;
> > > > > @@ -1404,6 +1406,7 @@ skl_plane_async_flip(struct intel_plane *plane,
> > > > > 	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > > > > 	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> > > > > 			  skl_plane_surf(plane_state, 0));
> > > > > +	intel_metrics_flip(display, crtc_state, plane_id, async_flip);
> > > > > }
> > > > > 
> > > > > static bool intel_format_is_p01x(u32 format) diff --git
> > > > > a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index
> > > > > e5b1715f721e..5133dba2c7de 100644
> > > > > --- a/drivers/gpu/drm/xe/Makefile
> > > > > +++ b/drivers/gpu/drm/xe/Makefile
> > > > > @@ -263,6 +263,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> > > > > 	i915-display/intel_hti.o \
> > > > > 	i915-display/intel_link_bw.o \
> > > > > 	i915-display/intel_lspcon.o \
> > > > > +	i915-display/intel_metrics.o \
> > > > > 	i915-display/intel_modeset_lock.o \
> > > > > 	i915-display/intel_modeset_setup.o \
> > > > > 	i915-display/intel_modeset_verify.o \
> > > > > --
> > > > > 2.44.0
> > > > 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] drm/i915/display: Introduce Display Metrics info
  2024-05-07 17:37         ` Zanoni, Paulo R
@ 2024-05-09 18:41           ` Kumar, Naveen1
  0 siblings, 0 replies; 10+ messages in thread
From: Kumar, Naveen1 @ 2024-05-09 18:41 UTC (permalink / raw)
  To: Zanoni, Paulo R, Vivi, Rodrigo, Souza, Jose
  Cc: Shankar, Uma, Kulkarni, Vandita, Nikula, Jani, intel-gfx,
	Belgaumkar, Vinay, Borah, Chaitanya Kumar



>-----Original Message-----
>From: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
>Sent: Tuesday, May 7, 2024 11:07 PM
>To: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Kumar, Naveen1
><naveen1.kumar@intel.com>; Souza, Jose <jose.souza@intel.com>
>Cc: Shankar, Uma <uma.shankar@intel.com>; Kulkarni, Vandita
><vandita.kulkarni@intel.com>; Nikula, Jani <jani.nikula@intel.com>; intel-
>gfx@lists.freedesktop.org; Belgaumkar, Vinay <vinay.belgaumkar@intel.com>;
>Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
>Subject: Re: [PATCH] drm/i915/display: Introduce Display Metrics info
>
>On Tue, 2024-05-07 at 08:48 -0400, Rodrigo Vivi wrote:
>> On Mon, May 06, 2024 at 11:19:56PM -0400, Kumar, Naveen1 wrote:
>> >
>> >
>> > > -----Original Message-----
>> > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> > > Sent: Monday, May 6, 2024 10:52 PM
>> > > To: Kumar, Naveen1 <naveen1.kumar@intel.com>
>> > > Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma
>> > > <uma.shankar@intel.com>; Kulkarni, Vandita
>> > > <vandita.kulkarni@intel.com>; Nikula, Jani
>> > > <jani.nikula@intel.com>; Belgaumkar, Vinay
>> > > <vinay.belgaumkar@intel.com>; Borah, Chaitanya Kumar
>> > > <chaitanya.kumar.borah@intel.com>
>> > > Subject: Re: [PATCH] drm/i915/display: Introduce Display Metrics
>> > > info
>> > >
>> > > On Mon, May 06, 2024 at 06:03:17AM -0400, Kumar, Naveen1 wrote:
>> > > >
>> > > >
>> > > > > -----Original Message-----
>> > > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> > > > > Sent: Saturday, April 6, 2024 3:39 AM
>> > > > > To: intel-gfx@lists.freedesktop.org
>> > > > > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Shankar, Uma
>> > > > > <uma.shankar@intel.com>; Kulkarni, Vandita
>> > > > > <vandita.kulkarni@intel.com>; Kumar, Naveen1
>> > > > > <naveen1.kumar@intel.com>; Nikula, Jani
>> > > > > <jani.nikula@intel.com>; Belgaumkar, Vinay
>> > > > > <vinay.belgaumkar@intel.com>
>> > > > > Subject: [PATCH] drm/i915/display: Introduce Display Metrics
>> > > > > info
>> > > > >
>> > > > > Introduce a display metrics information through debugfs for a
>> > > > > better view of the vblank and page flips, in special Async flips behavior.
>> > > > >
>> > > > > There is currently an overall expectation that whenever
>> > > > > vblank_mode=0 is used with an graphics application, that
>> > > > > automatically async_flips are happening. However, while
>> > > > > implementing the Display Metrics for GuC SLPC, it was observed
>> > > > > that it is not necessarily true for many of the expected cases.
>> > > > >
>> > > > > So, while the GuC SLPC side of the metrics doesn't get ready,
>> > > > > let's at least bring the debugfs view of it so we can work to
>> > > > > understand and fix any potential issue around our async vblanks.
>> > > > >
>> > > > > Please notice that the every struct here follows exactly the
>> > > > > GuC shared data buffer, so the next step of the integration
>> > > > > would be smooth and almost transparent to this intel_metrics on the
>display side.
>> > > > >
>> > > > > Cc: Uma Shankar <uma.shankar@intel.com>
>> > > > > Cc: Vandita Kulkarni <vandita.kulkarni@intel.com>
>> > > > > Cc: Naveen Kumar <naveen1.kumar@intel.com>
>> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
>> > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > > > > ---
>> > > > > drivers/gpu/drm/i915/Makefile                 |   1 +
>> > > > > drivers/gpu/drm/i915/display/intel_display.c  |  15 +-
>> > > > > .../gpu/drm/i915/display/intel_display_core.h |   2 +
>> > > > > .../drm/i915/display/intel_display_debugfs.c  |  12 +
>> > > > > .../drm/i915/display/intel_display_driver.c   |   5 +
>> > > > > .../gpu/drm/i915/display/intel_display_irq.c  |   3 +
>> > > > > drivers/gpu/drm/i915/display/intel_metrics.c  | 356
>> > > > > ++++++++++++++++++ drivers/gpu/drm/i915/display/intel_metrics.
>> > > > > ++++++++++++++++++ h  |  27
>> > > ++
>> > > > > .../drm/i915/display/skl_universal_plane.c    |   3 +
>> > > > > drivers/gpu/drm/xe/Makefile                   |   1 +
>> > > > > 10 files changed, 424 insertions(+), 1 deletion(-)  create
>> > > > > mode
>> > > > > 100644 drivers/gpu/drm/i915/display/intel_metrics.c
>> > > > > create mode 100644
>> > > > > drivers/gpu/drm/i915/display/intel_metrics.h
>> > > > >
>> > > > > diff --git a/drivers/gpu/drm/i915/Makefile
>> > > > > b/drivers/gpu/drm/i915/Makefile index
>> > > > > af9e871daf1d..a3c8d9f5614c
>> > > > > 100644
>> > > > > --- a/drivers/gpu/drm/i915/Makefile
>> > > > > +++ b/drivers/gpu/drm/i915/Makefile
>> > > > > @@ -291,6 +291,7 @@ i915-y += \
>> > > > > 	display/intel_link_bw.o \
>> > > > > 	display/intel_load_detect.o \
>> > > > > 	display/intel_lpe_audio.o \
>> > > > > +	display/intel_metrics.o \
>> > > > > 	display/intel_modeset_lock.o \
>> > > > > 	display/intel_modeset_setup.o \
>> > > > > 	display/intel_modeset_verify.o \ diff --git
>> > > > > a/drivers/gpu/drm/i915/display/intel_display.c
>> > > > > b/drivers/gpu/drm/i915/display/intel_display.c
>> > > > > index a481c9218138..ca30b8d48e1f 100644
>> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > > > > @@ -94,6 +94,7 @@
>> > > > > #include "intel_link_bw.h"
>> > > > > #include "intel_lvds.h"
>> > > > > #include "intel_lvds_regs.h"
>> > > > > +#include "intel_metrics.h"
>> > > > > #include "intel_modeset_setup.h"
>> > > > > #include "intel_modeset_verify.h"
>> > > > > #include "intel_overlay.h"
>> > > > > @@ -1021,11 +1022,15 @@ static void
>> > > > > intel_post_plane_update(struct intel_atomic_state *state,
>> > > > > 				    struct intel_crtc *crtc) {
>> > > > > 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> > > > > +	struct intel_display *display = &dev_priv->display;
>> > > > > 	const struct intel_crtc_state *old_crtc_state =
>> > > > > 		intel_atomic_get_old_crtc_state(state, crtc);
>> > > > > 	const struct intel_crtc_state *new_crtc_state =
>> > > > > 		intel_atomic_get_new_crtc_state(state, crtc);
>> > > > > 	enum pipe pipe = crtc->pipe;
>> > > > > +	const struct intel_plane_state __maybe_unused *plane_state;
>> > > > > +	struct intel_plane *plane;
>> > > > > +	int i;
>> > > > >
>> > > > > 	intel_psr_post_plane_update(state, crtc);
>> > > > >
>> > > > > @@ -1057,6 +1062,12 @@ static void
>> > > > > intel_post_plane_update(struct intel_atomic_state *state,
>> > > > >
>> > > > > 	if (audio_enabling(old_crtc_state, new_crtc_state))
>> > > > > 		intel_encoders_audio_enable(state, crtc);
>> > > > > +
>> > > > > +	if (!new_crtc_state->do_async_flip) {
>> > > > > +		for_each_new_intel_plane_in_state(state, plane,
>plane_state, i)
>> > > > > +			intel_metrics_flip(display, new_crtc_state,
>plane->id,
>> > > > > +					   false);
>> > > > > +	}
>> > > > > }
>> > > > >
>> > > > > static void intel_crtc_enable_flip_done(struct
>> > > > > intel_atomic_state *state, @@ -
>> > > > > 7139,6 +7150,7 @@ static void intel_atomic_commit_tail(struct
>> > > > > intel_atomic_state *state)  {
>> > > > > 	struct drm_device *dev = state->base.dev;
>> > > > > 	struct drm_i915_private *dev_priv = to_i915(dev);
>> > > > > +	struct intel_display *display = &dev_priv->display;
>> > > > > 	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
>> > > > > 	struct intel_crtc *crtc;
>> > > > > 	struct intel_power_domain_mask
>put_domains[I915_MAX_PIPES] =
>> > > > > {};
>> > > @@
>> > > > > -7261,7 +7273,6 @@ static void intel_atomic_commit_tail(struct
>> > > > > intel_atomic_state *state)
>> > > > > 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
>{
>> > > > > 		if (new_crtc_state->do_async_flip)
>> > > > > 			intel_crtc_disable_flip_done(state, crtc);
>> > > > > -
>> > > > > 		intel_color_wait_commit(new_crtc_state);
>> > > > > 	}
>> > > > >
>> > > > > @@ -7314,6 +7325,8 @@ static void
>> > > > > intel_atomic_commit_tail(struct intel_atomic_state *state)
>> > > > > 		 * FIXME get rid of this funny new->old swapping
>> > > > > 		 */
>> > > > > 		old_crtc_state->dsb =
>fetch_and_zero(&new_crtc_state->dsb);
>> > > > > +
>> > > > > +		intel_metrics_refresh_info(display, new_crtc_state);
>> > > > > 	}
>> > > > >
>> > > > > 	/* Underruns don't always raise interrupts, so check manually
>> > > > > */ diff -- git
>> > > > > a/drivers/gpu/drm/i915/display/intel_display_core.h
>> > > > > b/drivers/gpu/drm/i915/display/intel_display_core.h
>> > > > > index 2167dbee5eea..992db9098566 100644
>> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>> > > > > @@ -42,6 +42,7 @@ struct intel_cdclk_vals;  struct
>> > > > > intel_color_funcs; struct intel_crtc;  struct
>> > > > > intel_crtc_state;
>> > > > > +struct intel_display_metrics;
>> > > > > struct intel_dmc;
>> > > > > struct intel_dpll_funcs;
>> > > > > struct intel_dpll_mgr;
>> > > > > @@ -530,6 +531,7 @@ struct intel_display {
>> > > > > 	struct intel_fbc *fbc[I915_MAX_FBCS];
>> > > > > 	struct intel_frontbuffer_tracking fb_tracking;
>> > > > > 	struct intel_hotplug hotplug;
>> > > > > +	struct intel_display_metrics *metrics;
>> > > > > 	struct intel_opregion *opregion;
>> > > > > 	struct intel_overlay *overlay;
>> > > > > 	struct intel_display_params params; diff --git
>> > > > > a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> > > > > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> > > > > index 3e364891dcd0..f05b9a9ddee0 100644
>> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> > > > > @@ -30,6 +30,7 @@
>> > > > > #include "intel_hdcp.h"
>> > > > > #include "intel_hdmi.h"
>> > > > > #include "intel_hotplug.h"
>> > > > > +#include "intel_metrics.h"
>> > > > > #include "intel_panel.h"
>> > > > > #include "intel_psr.h"
>> > > > > #include "intel_psr_regs.h"
>> > > > > @@ -642,6 +643,16 @@ static int
>> > > > > i915_display_capabilities(struct seq_file *m, void *unused)
>> > > > > 	return 0;
>> > > > > }
>> > > > >
>> > > > > +static int i915_display_metrics(struct seq_file *m, void *unused) {
>> > > > > +	struct drm_i915_private *i915 = node_to_i915(m->private);
>> > > > > +	struct drm_printer p = drm_seq_file_printer(m);
>> > > > > +
>> > > > > +	intel_metrics_show(&i915->display, &p);
>> > > > > +
>> > > > > +	return 0;
>> > > > > +}
>> > > > > +
>> > > > > static int i915_shared_dplls_info(struct seq_file *m, void *unused)  {
>> > > > > 	struct drm_i915_private *dev_priv = node_to_i915(m-
>>private);
>> > > > > @@ -
>> > > > > 1059,6 +1070,7 @@ static const struct drm_info_list
>> > > > > intel_display_debugfs_list[] = {
>> > > > > 	{"i915_power_domain_info", i915_power_domain_info, 0},
>> > > > > 	{"i915_display_info", i915_display_info, 0},
>> > > > > 	{"i915_display_capabilities", i915_display_capabilities, 0},
>> > > > > +	{"i915_display_metrics", i915_display_metrics, 0},
>> > > > > 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
>> > > > > 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
>> > > > > 	{"i915_ddb_info", i915_ddb_info, 0}, diff --git
>> > > > > a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> > > > > b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> > > > > index 87dd07e0d138..767b2d5b3826 100644
>> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> > > > > @@ -46,6 +46,7 @@
>> > > > > #include "intel_hdcp.h"
>> > > > > #include "intel_hotplug.h"
>> > > > > #include "intel_hti.h"
>> > > > > +#include "intel_metrics.h"
>> > > > > #include "intel_modeset_lock.h"
>> > > > > #include "intel_modeset_setup.h"
>> > > > > #include "intel_opregion.h"
>> > > > > @@ -514,6 +515,8 @@ int intel_display_driver_probe(struct
>> > > > > drm_i915_private *i915)
>> > > > >
>> > > > > 	intel_overlay_setup(i915);
>> > > > >
>> > > > > +	intel_metrics_init(&i915->display);
>> > > > > +
>> > > > > 	ret = intel_fbdev_init(&i915->drm);
>> > > > > 	if (ret)
>> > > > > 		return ret;
>> > > > > @@ -611,6 +614,8 @@ void
>> > > > > intel_display_driver_remove_noirq(struct
>> > > > > drm_i915_private *i915)
>> > > > >
>> > > > > 	intel_dp_tunnel_mgr_cleanup(i915);
>> > > > >
>> > > > > +	intel_metrics_fini(&i915->display);
>> > > > > +
>> > > > > 	intel_overlay_cleanup(i915);
>> > > > >
>> > > > > 	intel_gmbus_teardown(i915);
>> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
>> > > > > b/drivers/gpu/drm/i915/display/intel_display_irq.c
>> > > > > index f846c5b108b5..202400a771b2 100644
>> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
>> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
>> > > > > @@ -18,6 +18,7 @@
>> > > > > #include "intel_fifo_underrun.h"
>> > > > > #include "intel_gmbus.h"
>> > > > > #include "intel_hotplug_irq.h"
>> > > > > +#include "intel_metrics.h"
>> > > > > #include "intel_pmdemand.h"
>> > > > > #include "intel_psr.h"
>> > > > > #include "intel_psr_regs.h"
>> > > > > @@ -25,8 +26,10 @@
>> > > > > static void
>> > > > > intel_handle_vblank(struct drm_i915_private *dev_priv, enum
>> > > > > pipe
>> > > > > pipe)  {
>> > > > > +	struct intel_display *display = &dev_priv->display;
>> > > > > 	struct intel_crtc *crtc = intel_crtc_for_pipe(dev_priv,
>> > > > > pipe);
>> > > > >
>> > > > > +	intel_metrics_vblank(display, crtc);
>> > > > > 	drm_crtc_handle_vblank(&crtc->base);
>> > > > > }
>> > > > >
>> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_metrics.c
>> > > > > b/drivers/gpu/drm/i915/display/intel_metrics.c
>> > > > > new file mode 100644
>> > > > > index 000000000000..2cb2b8189b25
>> > > > > --- /dev/null
>> > > > > +++ b/drivers/gpu/drm/i915/display/intel_metrics.c
>> > > > > @@ -0,0 +1,356 @@
>> > > > > +// SPDX-License-Identifier: MIT
>> > > > > +/*
>> > > > > + * Copyright (c) 2024 Intel Corporation  */
>> > > > > +
>> > > > > +#include "intel_metrics.h"
>> > > > > +
>> > > > > +#include <drm/drm_modes.h>
>> > > > > +#include <drm/drm_print.h>
>> > > > > +
>> > > > > +#include "i915_drv.h"
>> > > > > +#include "intel_de.h"
>> > > > > +#include "intel_display_types.h"
>> > > > > +
>> > > > > +/**
>> > > > > + * Display Metrics
>> > > > > + *
>> > > > > + * Provide some display activity overview such as active
>> > > > > +refresh rates,
>> > > > > + * vblank activity and page flip activities.
>> > > > > + * For now it is informative debug only, but later it will be
>> > > > > +expanded
>> > > > > + * to be used for GT frequency selection by GuC SLPC.
>> > > > > + */
>> > > > > +
>> > > > > +/*
>> > > > > + * An event using an work queue is used to avoid any
>> > > > > +disturbance in the
>> > > > > + * critical path that could cause performance impacts.
>> > > > > + */
>> > > > > +struct display_event {
>> > > > > +	struct work_struct work;
>> > > > > +	struct drm_i915_private *i915;
>> > > > > +	struct intel_display *display;
>> > > > > +	bool is_vblank;
>> > > > > +	int pipe;
>> > > > > +	int plane;
>> > > > > +	bool async_flip;
>> > > > > +};
>> > > > > +
>> > > > > +/*
>> > > > > + * Although we could simply save this inside our crtc
>> > > > > +structs, we are
>> > > > > + * already mimicking the GuC SLPC defition of the display
>> > > > > +data, for future
>> > > > > + * usage.
>> > > > > + */
>> > > > > +#define MAX_PIPES		8
>> > > > > +#define MAX_PLANES_PER_PIPE	8
>> > > > > +
>> > > > > +struct display_global_info {
>> > > > > +	u32 version:8;
>> > > > > +	u32 num_pipes:4;
>> > > > > +	u32 num_planes_per_pipe:4;
>> > > > > +	u32 reserved_1:16;
>> > > > > +	u32 refresh_count:16;
>> > > > > +	u32 vblank_count:16;
>> > > > > +	u32 flip_count:16;
>> > > > > +	u32 reserved_2:16;
>> > > > > +	u32 reserved_3[13];
>> > > > > +} __packed;
>> > > > > +
>> > > > > +struct display_refresh_info {
>> > > > > +	u32 refresh_interval:16;
>> > > > > +	u32 is_variable:1;
>> > > > > +	u32 reserved:15;
>> > > > > +} __packed;
>> > > > > +
>> > > > > +/*
>> > > > > + * When used with GuC SLPC, the host must update each 32-bit
>> > > > > +part with a single
>> > > > > + * atomic write so that SLPC will read the contained bit fields together.
>> > > > > + * The host must update the two parts in order - total flip
>> > > > > +count and timestamp
>> > > > > + * first, vsync and async flip counts second.
>> > > > > + * Hence, these items are not defined with individual bitfields.
>> > > > > + */
>> > > > > +#define FLIP_P1_LAST		REG_GENMASK(31, 7)
>> > > > > +#define FLIP_P1_TOTAL_COUNT	REG_GENMASK(6, 0)
>> > > > > +#define FLIP_P2_ASYNC_COUNT	REG_GENMASK(31, 16)
>> > > > > +#define FLIP_P2_VSYNC_COUNT	REG_GENMASK(15, 0)
>> > > > > +
>> > > > > +struct display_flip_metrics {
>> > > > > +	u32 part1;
>> > > > > +	u32 part2;
>> > > > > +} __packed;
>> > > > > +
>> > > > > +/*
>> > > > > + * When used with GuC SLPC, the host must update each 32-bit
>> > > > > +part with a single
>> > > > > + * atomic write, so that SLPC will read the count and timestamp
>together.
>> > > > > + * Hence, this item is not defined with individual bitfields.
>> > > > > + */
>> > > > > +#define VBLANK_LAST	REG_GENMASK(31, 7)
>> > > > > +#define VBLANK_COUNT	REG_GENMASK(6, 0)
>> > > > > +
>> > > > > +struct intel_display_metrics {
>> > > > > +	struct display_global_info global_info;
>> > > > > +	struct display_refresh_info refresh_info[MAX_PIPES];
>> > > > > +	u32 vblank_metrics[MAX_PIPES];
>> > > > > +	struct display_flip_metrics
>> > > > > +	flip_metrics[MAX_PIPES][MAX_PLANES_PER_PIPE];
>> > > > > +} __packed;
>> > > > > +
>> > > > > +/**
>> > > > > + * intel_metrics_refresh_info - Refresh rate information
>> > > > > + * @display: Pointer to the intel_display struct that is in
>> > > > > +use by the gfx
>> > > device.
>> > > > > + * @crtc_state: New CRTC state upon a modeset.
>> > > > > + *
>> > > > > + * To be called on a modeset. It then saves the current
>> > > > > +refresh interval in
>> > > > > + * micro seconds.
>> > > > > + */
>> > > > > +void intel_metrics_refresh_info(struct intel_display *display,
>> > > > > +				struct intel_crtc_state *crtc_state) {
>> > > > > +	struct intel_display_metrics *metrics = display->metrics;
>> > > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > > > > +	struct drm_display_mode *mode = &crtc_state-
>>hw.adjusted_mode;
>> > > > > +	u32 interval_us;
>> > > > > +
>> > > > > +	if (!display->metrics)
>> > > > > +		return;
>> > > > > +
>> > > > > +	interval_us = crtc_state->hw.active ?
>DIV_ROUND_UP(1000000,
>> > > > > +
>	drm_mode_vrefresh(mode)) :
>> > > > > 0;
>> > > > > +
>> > > > > +	metrics->refresh_info[crtc->pipe].refresh_interval =
>interval_us;
>> > > > > +	metrics->refresh_info[crtc->pipe].is_variable = crtc_state-
>> > > > > > uapi.vrr_enabled;
>> > > > > +	metrics->global_info.refresh_count += 1; }
>> > > > > +
>> > > > > +static void metrics_update_vblank(struct
>> > > > > +intel_display_metrics *metrics, int
>> > > > > pipe,
>> > > > > +				  u32 timestamp)
>> > > > > +{
>> > > > > +	u32 vblank;
>> > > > > +
>> > > > > +	vblank = metrics->vblank_metrics[pipe];
>> > > > > +
>> > > > > +	vblank = REG_FIELD_GET(VBLANK_COUNT, vblank);
>> > > > > +	vblank = REG_FIELD_PREP(VBLANK_COUNT, vblank + 1);
>> > > > > +	vblank |= REG_FIELD_PREP(VBLANK_LAST, timestamp);
>> > > > > +
>> > > > > +	/* Write everything at once in preparation for the GuC SLPC
>> > > > > requirement */
>> > > > > +	metrics->vblank_metrics[pipe] = vblank;
>> > > > > +	metrics->global_info.vblank_count += 1; }
>> > > > > +
>> > > > > +static void metrics_update_flip(struct intel_display_metrics
>> > > > > +*metrics, int
>> > > pipe,
>> > > > > +				int plane, bool async_flip, u32
>timestamp) {
>> > > > > +	u32 part1, part2, count;
>> > > > > +
>> > > > > +	part1 = metrics->flip_metrics[pipe][plane].part1;
>> > > > > +	part2 = metrics->flip_metrics[pipe][plane].part2;
>> > > > > +
>> > > > > +	part1 = REG_FIELD_GET(FLIP_P1_TOTAL_COUNT, part1);
>> > > > > +	part1 = REG_FIELD_PREP(FLIP_P1_TOTAL_COUNT, part1 + 1);
>> > > > > +	part1 |= REG_FIELD_PREP(FLIP_P1_LAST, timestamp);
>> > > > > +
>> > > > > +	if (async_flip) {
>> > > > > +		count = REG_FIELD_GET(FLIP_P2_ASYNC_COUNT,
>part2);
>> > > > > +		part2 &= ~FLIP_P2_ASYNC_COUNT;
>> > > > > +		part2 |= REG_FIELD_PREP(FLIP_P2_ASYNC_COUNT,
>count + 1);
>> > > > > +	} else {
>> > > > > +		count = REG_FIELD_GET(FLIP_P2_VSYNC_COUNT,
>part2);
>> > > > > +		part2 &= ~FLIP_P2_VSYNC_COUNT;
>> > > > > +		part2 |= REG_FIELD_PREP(FLIP_P2_VSYNC_COUNT,
>count + 1);
>> > > > > +	}
>> > > > > +
>> > > > > +	/*
>> > > > > +	 * Write everything in this way and this order in preparation for
>the
>> > > > > +	 * GuC SLPC requirement
>> > > > > +	 */
>> > > > > +	metrics->flip_metrics[pipe][plane].part1 = part1;
>> > > > > +	metrics->flip_metrics[pipe][plane].part2 = part2;
>> > > > > +
>> > > > > +	metrics->global_info.flip_count += 1; }
>> > > > > +
>> > > > > +/*
>> > > > > + * Let's use the same register GuC SLPC uses for timestamp.
>> > > > > + * It uses a register that is outside GT domain so GuC
>> > > > > +doesn't need
>> > > > > + * to wake the GT for reading during SLPC loop.
>> > > > > + * This is a single register regarding the GT, so we can read
>> > > > > +directly
>> > > > > + * from here, regarding the GT GuC is in.
>> > > > > + */
>> > > > > +#define MCHBAR_MIRROR_BASE_SNB	0x140000
>> > > > > +#define MCHBAR_BCLK_COUNT
>	_MMIO(MCHBAR_MIRROR_BASE_SNB
>> > > > > + 0x5984)
>> > > > > +#define MTL_BCLK_COUNT		_MMIO(0xc28)
>> > > > > +#define   TIMESTAMP_MASK	REG_GENMASK(30, 6)
>> > > > > +
>> > > > > +static u32 bclk_read_timestamp(struct drm_i915_private *i915) {
>> > > > > +	u32 timestamp;
>> > > > > +
>> > > > > +	if (DISPLAY_VER(i915) >= 14)
>> > > > > +		timestamp = intel_de_read(i915, MTL_BCLK_COUNT);
>> > > > > +	else
>> > > > > +		timestamp = intel_de_read(i915,
>MCHBAR_BCLK_COUNT);
>> > > > > +
>> > > > > +	return REG_FIELD_GET(TIMESTAMP_MASK, timestamp); }
>> > > > > +
>> > > > > +static void display_event_work(struct work_struct *work) {
>> > > > > +	struct display_event *event = container_of(work, struct
>> > > > > +display_event,
>> > > > > work);
>> > > > > +	struct intel_display *display = event->display;
>> > > > > +	u32 timestamp;
>> > > > > +
>> > > > > +	timestamp = bclk_read_timestamp(event->i915);
>> > > > > +
>> > > > > +	if (event->is_vblank) {
>> > > > > +		metrics_update_vblank(display->metrics, event->pipe,
>> > > > > timestamp);
>> > > > > +	} else {
>> > > > > +		metrics_update_flip(display->metrics, event->pipe,
>event-
>> > > > > > plane,
>> > > > > +				    event->async_flip, timestamp);
>> > > > > +	}
>> > > > > +
>> > > > > +	kfree(event);
>> > > > > +}
>> > > > > +
>> > > > > +void intel_metrics_init(struct intel_display *display) {
>> > > > > +	struct intel_display_metrics *metrics;
>> > > > > +
>> > > > > +	metrics = kzalloc(sizeof(*metrics), GFP_KERNEL);
>> > > > > +	if (!metrics)
>> > > > > +		return;
>> > > > > +
>> > > > > +	metrics->global_info.version = 1;
>> > > > > +	metrics->global_info.num_pipes = MAX_PIPES;
>> > > > > +	metrics->global_info.num_planes_per_pipe =
>> > > > > +MAX_PLANES_PER_PIPE;
>> > > > > +
>> > > > > +	display->metrics = metrics;
>> > > > > +}
>> > > > > +
>> > > > > +void intel_metrics_fini(struct intel_display *display) {
>> > > > > +	if (!display->metrics)
>> > > > > +		return;
>> > > > > +
>> > > > > +	kfree(display->metrics);
>> > > > > +}
>> > > > > +
>> > > > > +/**
>> > > > > + * intel_metrics_vblank - Vblank information
>> > > > > + * @display: Pointer to the intel_display struct that is in
>> > > > > +use by the gfx
>> > > device.
>> > > > > + * @crtc: The Intel CRTC that received the vblank interrupt.
>> > > > > + *
>> > > > > + * To be called when a vblank is passed.
>> > > > > + */
>> > > > > +void intel_metrics_vblank(struct intel_display *display,
>> > > > > +			  struct intel_crtc *crtc) {
>> > > > > +	struct display_event *event;
>> > > > > +
>> > > > > +	if (!display->metrics)
>> > > > > +		return;
>> > > > > +
>> > > > > +	event = kmalloc(sizeof(*event), GFP_ATOMIC);
>> > > >
>> > > > HI Rodrigo, after changing kmalloc -> kzalloc, it works fine for
>> > > > us. It does
>> > > correctly prints Global Flip count and Async Flip count.
>> > > > Prior to this change, event->is_vblank in function
>> > > > display_event_work is
>> > > always true and hence it never increases flip count.
>> > >
>> > > thanks for pointing that out. kzalloc is indeed better.
>> > > There's also another kmalloc down that needs to be changed to kzalloc.
>> > >
>> > > Anyway, when you mentioned that you saw the async flip count
>> > > increasing, you are telling about the igt test right?
>> > > or do you have any special compositor change required?
>> >
>> > Hi Rodrigo, async flip count is increasing using both IGT and
>> > Weston/wayland client app
>> > (https://gitlab.freedesktop.org/wayland/weston/-/blob/main/clients/s
>> > imple-egl.c#L1294) Additionally, we had to hack mesa code to use
>> > async flip supported modifiers. Mesa needs to handle async flip scenarios
>and select supported modifiers.
>>
>> Cc: José Roberto de Souza <jose.souza@intel.com>
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> do we have a mesa MR for that?
>
>I haven't seen anything. Can you please create a Mesa merge request or new
>issue and attach the hack you created and describe the problem?
>
>Thanks,
>Paulo

HI Rodrigo &  Paulo,
I have done following workaround or hack in the mesa code to make Async flip work https://gitlab.freedesktop.org/NaveenKumar/mesa/-/commit/0f0eb2db8d19eca136907b583249509007b1ef62.
Though I am trying to come up with a proper patch to make mesa aware of Async flip and enumerate only Async supported modifiers. To do this, I am planning to check if vblank_mode=0 env is set in mesa, then only enumerate and make use of Async flip supported modifiers for that platform. 

As reported earlier, there are issues with modifiers causing Async failures on DG2 & TGL with latest upstream Mesa:
[drm:intel_atomic_check [i915]] [PLANE:31:plane 1A] Modifier 0x10000000000000c does not support async flip -> on DG2
[drm:intel_atomic_check [i915]] [PLANE:31:plane 1A] Modifier 0x100000000000008 does not support async flip -> on TGL

>
>>
>> >
>> > > In my environment here I still only see the async flip increasing
>> > > with IGT or with a very limited cases...
>> > >
>> > > >
>> > > > Thanks Chaitanya for pointing this out.
>> > > > Regards,
>> > > > Naveen Kumar
>> > > >
>> > > > > +	if (!event)
>> > > > > +		return;
>> > > > > +
>> > > > > +	INIT_WORK(&event->work, display_event_work);
>> > > > > +	event->i915 = to_i915(crtc->base.dev);
>> > > > > +	event->display = display;
>> > > > > +	event->is_vblank = true;
>> > > > > +	event->pipe = crtc->pipe;
>> > > > > +	queue_work(system_highpri_wq, &event->work); }
>> > > > > +
>> > > > > +/**
>> > > > > + * intel_metrics_flip - Flip information
>> > > > > + * @display: Pointer to the intel_display struct that is in
>> > > > > +use by the gfx
>> > > device.
>> > > > > + * @crtc_state: New CRTC state upon a page flip.
>> > > > > + * @plane: Which plane ID got the page flip.
>> > > > > + * @async_flip: A boolean to indicate if the page flip was async.
>> > > > > + *
>> > > > > + * To be called on a page flip.
>> > > > > + */
>> > > > > +void intel_metrics_flip(struct intel_display *display,
>> > > > > +			const struct intel_crtc_state *crtc_state,
>> > > > > +			int plane, bool async_flip) {
>> > > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > > > > +	struct display_event *event;
>> > > > > +
>> > > > > +	if (!display->metrics)
>> > > > > +		return;
>> > > > > +
>> > > > > +	event = kmalloc(sizeof(*event), GFP_ATOMIC);
>> > > > Same here, had to change kmalloc -> kzalloc
>> > > >
>> > > > > +	if (!event)
>> > > > > +		return;
>> > > > > +
>> > > > > +	INIT_WORK(&event->work, display_event_work);
>> > > > > +	event->i915 = to_i915(crtc->base.dev);
>> > > > > +	event->display = display;
>> > > > > +	event->pipe = crtc->pipe;
>> > > > > +	event->plane = plane;
>> > > > > +	event->async_flip = async_flip;
>> > > > > +	queue_work(system_highpri_wq, &event->work); }
>> > > > > +
>> > > > > +void intel_metrics_show(struct intel_display *display, struct
>> > > > > +drm_printer *p) {
>> > > > > +	struct intel_display_metrics *metrics = display->metrics;
>> > > > > +	int pipe, plane;
>> > > > > +	u32 val;
>> > > > > +
>> > > > > +	if (!metrics)
>> > > > > +		return;
>> > > > > +
>> > > > > +	drm_printf(p, "\nDisplay Metrics - Globals:\n");
>> > > > > +	drm_printf(p, "\tVersion: %d\n", metrics->global_info.version);
>> > > > > +	drm_printf(p, "\tNum Pipes: %d\n", metrics-
>>global_info.num_pipes);
>> > > > > +	drm_printf(p, "\tNum Planes per Pipe: %d\n",
>> > > > > +		   metrics->global_info.num_planes_per_pipe);
>> > > > > +	drm_printf(p, "\tGlobal Refresh Info Count: %d\n",
>> > > > > +		   metrics->global_info.refresh_count);
>> > > > > +	drm_printf(p, "\tGlobal Vblank Count: %d\n",
>> > > > > +		   metrics->global_info.vblank_count);
>> > > > > +	drm_printf(p, "\tGlobal Flip Count: %d\n",
>> > > > > +		   metrics->global_info.flip_count);
>> > > > > +
>> > > > > +	for (pipe = 0; pipe < MAX_PIPES; pipe++) {
>> > > > > +		if (!metrics->refresh_info[pipe].refresh_interval)
>> > > > > +			continue;
>> > > > > +
>> > > > > +		drm_printf(p, "\nDisplay Metrics - Refresh Info -
>Pipe[%d]:\n",
>> > > > > +			   pipe);
>> > > > > +		drm_printf(p, "\tRefresh Interval: %d\n",
>> > > > > +			   metrics->refresh_info[pipe].refresh_interval);
>> > > > > +		drm_printf(p, "\tIS VRR: %d\n",
>> > > > > +			   metrics->refresh_info[pipe].is_variable);
>> > > > > +
>> > > > > +		drm_printf(p, "Display Metrics - Vblank Info -
>Pipe[%d]:\n",
>> > > > > +			   pipe);
>> > > > > +		val = metrics->vblank_metrics[pipe];
>> > > > > +		drm_printf(p, "\tVBlank Last Timestamp: %x\n",
>> > > > > +			   REG_FIELD_GET(VBLANK_LAST, val));
>> > > > > +		drm_printf(p, "\tVBlank Count: %d\n",
>> > > > > +			   REG_FIELD_GET(VBLANK_COUNT, val));
>> > > > > +
>> > > > > +		drm_printf(p, "Display Metrics - Flip Info -
>Pipe[%d]:\n", pipe);
>> > > > > +		for (plane = 0; plane < MAX_PLANES_PER_PIPE;
>plane++) {
>> > > > > +			val = metrics->flip_metrics[pipe][plane].part1;
>> > > > > +			if (!val)
>> > > > > +				continue;
>> > > > > +
>> > > > > +			drm_printf(p, "\tFlip Info - Plane[%d]:\n",
>plane);
>> > > > > +			drm_printf(p, "\t\tFlip Last Timestamp: %x\n",
>> > > > > +				   REG_FIELD_GET(FLIP_P1_LAST, val));
>> > > > > +			drm_printf(p, "\t\tFlip Total Count: %d\n",
>> > > > > +
>REG_FIELD_GET(FLIP_P1_TOTAL_COUNT,
>> > > > > val));
>> > > > > +
>> > > > > +			val = metrics->flip_metrics[pipe][plane].part2;
>> > > > > +
>> > > > > +			drm_printf(p, "\t\tFlip Async Count: %d\n",
>> > > > > +
>REG_FIELD_GET(FLIP_P2_ASYNC_COUNT,
>> > > > > val));
>> > > > > +			drm_printf(p, "\t\tFlip Vsync Count: %d\n",
>> > > > > +
>REG_FIELD_GET(FLIP_P2_VSYNC_COUNT,
>> > > > > val));
>> > > > > +		}
>> > > > > +	}
>> > > > > +}
>> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_metrics.h
>> > > > > b/drivers/gpu/drm/i915/display/intel_metrics.h
>> > > > > new file mode 100644
>> > > > > index 000000000000..9e41dc9522f3
>> > > > > --- /dev/null
>> > > > > +++ b/drivers/gpu/drm/i915/display/intel_metrics.h
>> > > > > @@ -0,0 +1,27 @@
>> > > > > +/* SPDX-License-Identifier: MIT */
>> > > > > +/*
>> > > > > + * Copyright (c) 2024 Intel Corporation  */
>> > > > > +
>> > > > > +#ifndef __INTEL_METRICS_H__
>> > > > > +#define __INTEL_METRICS_H__
>> > > > > +
>> > > > > +#include <linux/types.h>
>> > > > > +
>> > > > > +struct drm_printer;
>> > > > > +struct intel_crtc;
>> > > > > +struct intel_crtc_state;
>> > > > > +struct intel_display;
>> > > > > +
>> > > > > +void intel_metrics_refresh_info(struct intel_display *display,
>> > > > > +				struct intel_crtc_state *crtc_state);
>void
>> > > > > +intel_metrics_vblank(struct intel_display *display,
>> > > > > +			  struct intel_crtc *intel_crtc); void
>> > > intel_metrics_flip(struct
>> > > > > +intel_display *display,
>> > > > > +			const struct intel_crtc_state *crtc_state,
>> > > > > +			int plane, bool async_flip); void
>> > > > > +intel_metrics_init(struct intel_display *display); void
>> > > > > +intel_metrics_fini(struct intel_display *display); void
>> > > > > +intel_metrics_show(struct intel_display *display, struct
>> > > > > +drm_printer *p);
>> > > > > +
>> > > > > +#endif
>> > > > > diff --git
>> > > > > a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > > > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > > > > index 860574d04f88..fdd21a41d79d 100644
>> > > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > > > > @@ -17,6 +17,7 @@
>> > > > > #include "intel_fb.h"
>> > > > > #include "intel_fbc.h"
>> > > > > #include "intel_frontbuffer.h"
>> > > > > +#include "intel_metrics.h"
>> > > > > #include "intel_psr.h"
>> > > > > #include "intel_psr_regs.h"
>> > > > > #include "skl_scaler.h"
>> > > > > @@ -1392,6 +1393,7 @@ skl_plane_async_flip(struct intel_plane
>*plane,
>> > > > > 		     bool async_flip)
>> > > > > {
>> > > > > 	struct drm_i915_private *dev_priv = to_i915(plane-
>>base.dev);
>> > > > > +	struct intel_display *display = &dev_priv->display;
>> > > > > 	enum plane_id plane_id = plane->id;
>> > > > > 	enum pipe pipe = plane->pipe;
>> > > > > 	u32 plane_ctl = plane_state->ctl; @@ -1404,6 +1406,7 @@
>> > > > > skl_plane_async_flip(struct intel_plane *plane,
>> > > > > 	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id),
>plane_ctl);
>> > > > > 	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
>> > > > > 			  skl_plane_surf(plane_state, 0));
>> > > > > +	intel_metrics_flip(display, crtc_state, plane_id,
>> > > > > +async_flip);
>> > > > > }
>> > > > >
>> > > > > static bool intel_format_is_p01x(u32 format) diff --git
>> > > > > a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> > > > > index e5b1715f721e..5133dba2c7de 100644
>> > > > > --- a/drivers/gpu/drm/xe/Makefile
>> > > > > +++ b/drivers/gpu/drm/xe/Makefile
>> > > > > @@ -263,6 +263,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>> > > > > 	i915-display/intel_hti.o \
>> > > > > 	i915-display/intel_link_bw.o \
>> > > > > 	i915-display/intel_lspcon.o \
>> > > > > +	i915-display/intel_metrics.o \
>> > > > > 	i915-display/intel_modeset_lock.o \
>> > > > > 	i915-display/intel_modeset_setup.o \
>> > > > > 	i915-display/intel_modeset_verify.o \
>> > > > > --
>> > > > > 2.44.0
>> > > >


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-05-09 18:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-05 22:08 [PATCH] drm/i915/display: Introduce Display Metrics info Rodrigo Vivi
2024-04-05 22:36 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2024-04-05 22:36 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-04-05 22:56 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-05-06 10:03 ` [PATCH] " Kumar, Naveen1
2024-05-06 17:22   ` Rodrigo Vivi
2024-05-07  3:19     ` Kumar, Naveen1
2024-05-07 12:48       ` Rodrigo Vivi
2024-05-07 17:37         ` Zanoni, Paulo R
2024-05-09 18:41           ` Kumar, Naveen1

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.