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@xxxxxxxxx>
> > > Sent: Monday, May 6, 2024 10:52 PM
> > > To: Kumar, Naveen1 <naveen1.kumar@xxxxxxxxx>
> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Shankar, Uma <uma.shankar@xxxxxxxxx>;
> > > Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>; Nikula, Jani
> > > <jani.nikula@xxxxxxxxx>; Belgaumkar, Vinay <vinay.belgaumkar@xxxxxxxxx>;
> > > Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>
> > > 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@xxxxxxxxx>
> > > > > Sent: Saturday, April 6, 2024 3:39 AM
> > > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > Cc: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Shankar, Uma
> > > > > <uma.shankar@xxxxxxxxx>; Kulkarni, Vandita
> > > > > <vandita.kulkarni@xxxxxxxxx>; Kumar, Naveen1
> > > > > <naveen1.kumar@xxxxxxxxx>; Nikula, Jani <jani.nikula@xxxxxxxxx>;
> > > > > Belgaumkar, Vinay <vinay.belgaumkar@xxxxxxxxx>
> > > > > 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@xxxxxxxxx>
> > > > > Cc: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
> > > > > Cc: Naveen Kumar <naveen1.kumar@xxxxxxxxx>
> > > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxx>
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> 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
> > > >
