From: "Kumar, Naveen1" <naveen1.kumar@intel.com>
To: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>,
"Vivi, Rodrigo" <rodrigo.vivi@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"
<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
Date: Thu, 9 May 2024 18:41:23 +0000 [thread overview]
Message-ID: <DM6PR11MB325816770DC189185C6A59CCA7E62@DM6PR11MB3258.namprd11.prod.outlook.com> (raw)
In-Reply-To: <92a1e9bdc0b959d777ab69e690127b1c5bb6ce0f.camel@intel.com>
>-----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
>> > > >
prev parent reply other threads:[~2024-05-09 18:41 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM6PR11MB325816770DC189185C6A59CCA7E62@DM6PR11MB3258.namprd11.prod.outlook.com \
--to=naveen1.kumar@intel.com \
--cc=chaitanya.kumar.borah@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
--cc=jose.souza@intel.com \
--cc=paulo.r.zanoni@intel.com \
--cc=rodrigo.vivi@intel.com \
--cc=uma.shankar@intel.com \
--cc=vandita.kulkarni@intel.com \
--cc=vinay.belgaumkar@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).