| Message ID | 20230307201611.773103-9-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add OAM support for MTL | expand |
On Tue, 07 Mar 2023 12:16:10 -0800, Umesh Nerlige Ramappa wrote: > Hi Umesh, > + /* Defaults when class:instance is not passed */ > + class = I915_ENGINE_CLASS_RENDER; > + instance = 0; > + > for (i = 0; i < n_props; i++) { > u64 oa_period, oa_freq_hz; > u64 id, value; > @@ -4174,7 +4156,13 @@ static int read_properties_unlocked(struct i915_perf *perf, > } > props->poll_oa_period = value; > break; > - case DRM_I915_PERF_PROP_MAX: > + case DRM_I915_PERF_PROP_OA_ENGINE_CLASS: > + class = (u8)value; > + break; > + case DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE: > + instance = (u8)value; > + break; I am wondering since this is uapi we should make it robust. So if the user passes either class or instance he must pass both and we should check for that. If only one is passed we should not implicitly assume the other as we are doing here (if only instance is passed here we will assume RCS and if only class is passed we will assume instance 0). I think making this explicit will avoid confusion later. Thoughts? > + default: > MISSING_CASE(id); > return -EINVAL; > } > @@ -4182,6 +4170,21 @@ static int read_properties_unlocked(struct i915_perf *perf, > uprop += 2; > } > > + props->engine = intel_engine_lookup_user(perf->i915, class, instance); > + if (!props->engine) { > + drm_dbg(&perf->i915->drm, > + "OA engine class and instance invalid %d:%d\n", > + class, instance); > + return -EINVAL; > + } Thanks. -- Ashutosh
On Tue, 07 Mar 2023 12:16:10 -0800, Umesh Nerlige Ramappa wrote: > Hi Umesh, > One or more engines map to a specific OA unit. All reports from these > engines are captured in the OA buffer managed by this OA unit. > > Current i915 OA implementation supports only the OAG unit. OAG primarily > caters to render engine, so i915 OA uses render as the default engine > in the OA implementation. Since there are more OA units on newer > hardware that map to other engines, allow user to pass engine class and > instance to select and program specific OA units. I believe there is another uapi issue to be resolved here: how the engines are associated with OA units, since from the point of view of the uapi there are multiple engines and multiple OA units (even if in the actual implementation at present there is only one OA unit per gt). In the internal tree we had added oa_unit_id to engine_info for this. So if multiple engines had the same oa_unit_id, user could pass class:instance of any of those engines to get oa data from that OA unit (and generally know how engines are hooked up to OA units (the OA unit topology)). So the question is even if we don't implement it as part of this series (or do we have to?) do we at least need to agree to that uapi which will be used to associate OA units with engines? Thanks. -- Ashutosh
On Wed, Mar 08, 2023 at 10:08:13AM -0800, Dixit, Ashutosh wrote: >On Tue, 07 Mar 2023 12:16:10 -0800, Umesh Nerlige Ramappa wrote: >> > >Hi Umesh, > >> One or more engines map to a specific OA unit. All reports from these >> engines are captured in the OA buffer managed by this OA unit. >> >> Current i915 OA implementation supports only the OAG unit. OAG primarily >> caters to render engine, so i915 OA uses render as the default engine >> in the OA implementation. Since there are more OA units on newer >> hardware that map to other engines, allow user to pass engine class and >> instance to select and program specific OA units. > >I believe there is another uapi issue to be resolved here: how the engines >are associated with OA units, since from the point of view of the uapi >there are multiple engines and multiple OA units (even if in the actual >implementation at present there is only one OA unit per gt). In the >internal tree we had added oa_unit_id to engine_info for this. So if >multiple engines had the same oa_unit_id, user could pass class:instance of >any of those engines to get oa data from that OA unit (and generally know >how engines are hooked up to OA units (the OA unit topology)). > >So the question is even if we don't implement it as part of this series (or >do we have to?) do we at least need to agree to that uapi which will be >used to associate OA units with engines? It did make more sense for xehpsdv and other platforms where we had multiple OA units per GT and each GT had render and/or compute engines. In those cases, media and compute/render UMDs may have needed to know that topology. For MTL, I don't think that an end user will benefit from the engine<->oa_unit mapping because (1) media and render are separate GTs (2) there is just one OA unit per GT and (3) assuming media and render/compute are separate UMDs, That's also the reason why the corresponding IGT series just uses a static mapping for MTL. If we come across a case where the UMD needs this info OR we are supporting a platform with multiple OA units per tile, we should add the topology. Thanks, Umesh > >Thanks. >-- >Ashutosh
On Tue, Mar 07, 2023 at 05:45:48PM -0800, Dixit, Ashutosh wrote: >On Tue, 07 Mar 2023 12:16:10 -0800, Umesh Nerlige Ramappa wrote: >> > >Hi Umesh, > >> + /* Defaults when class:instance is not passed */ >> + class = I915_ENGINE_CLASS_RENDER; >> + instance = 0; >> + >> for (i = 0; i < n_props; i++) { >> u64 oa_period, oa_freq_hz; >> u64 id, value; >> @@ -4174,7 +4156,13 @@ static int read_properties_unlocked(struct i915_perf *perf, >> } >> props->poll_oa_period = value; >> break; >> - case DRM_I915_PERF_PROP_MAX: >> + case DRM_I915_PERF_PROP_OA_ENGINE_CLASS: >> + class = (u8)value; >> + break; >> + case DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE: >> + instance = (u8)value; >> + break; > >I am wondering since this is uapi we should make it robust. So if the user >passes either class or instance he must pass both and we should check for >that. If only one is passed we should not implicitly assume the other as we >are doing here (if only instance is passed here we will assume RCS and if >only class is passed we will assume instance 0). I think making this >explicit will avoid confusion later. Thoughts? Agree. We should only allow this configuration as a pair. Thanks, Umesh
On Thu, 09 Mar 2023 14:34:04 -0800, Umesh Nerlige Ramappa wrote: > Hi Umesh, > On Wed, Mar 08, 2023 at 10:08:13AM -0800, Dixit, Ashutosh wrote: > > On Tue, 07 Mar 2023 12:16:10 -0800, Umesh Nerlige Ramappa wrote: > >> > > > >> One or more engines map to a specific OA unit. All reports from these > >> engines are captured in the OA buffer managed by this OA unit. > >> > >> Current i915 OA implementation supports only the OAG unit. OAG primarily > >> caters to render engine, so i915 OA uses render as the default engine > >> in the OA implementation. Since there are more OA units on newer > >> hardware that map to other engines, allow user to pass engine class and > >> instance to select and program specific OA units. > > > > I believe there is another uapi issue to be resolved here: how the engines > > are associated with OA units, since from the point of view of the uapi > > there are multiple engines and multiple OA units (even if in the actual > > implementation at present there is only one OA unit per gt). In the > > internal tree we had added oa_unit_id to engine_info for this. So if > > multiple engines had the same oa_unit_id, user could pass class:instance of > > any of those engines to get oa data from that OA unit (and generally know > > how engines are hooked up to OA units (the OA unit topology)). > > > > So the question is even if we don't implement it as part of this series (or > > do we have to?) do we at least need to agree to that uapi which will be > > used to associate OA units with engines? > > It did make more sense for xehpsdv and other platforms where we had > multiple OA units per GT and each GT had render and/or compute engines. In > those cases, media and compute/render UMDs may have needed to know that > topology. > > For MTL, I don't think that an end user will benefit from the > engine<->oa_unit mapping because > > (1) media and render are separate GTs > (2) there is just one OA unit per GT and > (3) assuming media and render/compute are separate UMDs, Well our uapi does not expose any of this so we are relying on outside information here. So I think it is needed even for MTL, but that's a problem for the person reviewing the uapi. Afaiac we can do it later if/when there is a specific ask for it, it's ok with me as is. I will R-b the patch after we fix the engine class/instance pairing. Thanks. -- Ashutosh > > That's also the reason why the corresponding IGT series just uses a static > mapping for MTL. > > If we come across a case where the UMD needs this info OR we are supporting > a platform with multiple OA units per tile, we should add the topology.
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 9e6da8859284..1b7059e9b10d 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -4013,47 +4013,29 @@ static int read_properties_unlocked(struct i915_perf *perf, struct drm_i915_gem_context_param_sseu user_sseu; u64 __user *uprop = uprops; bool config_sseu = false; + u8 class, instance; u32 i; int ret; memset(props, 0, sizeof(struct perf_open_properties)); props->poll_oa_period = DEFAULT_POLL_PERIOD_NS; - if (!n_props) { - drm_dbg(&perf->i915->drm, - "No i915 perf properties given\n"); - return -EINVAL; - } - - /* At the moment we only support using i915-perf on the RCS. */ - props->engine = intel_engine_lookup_user(perf->i915, - I915_ENGINE_CLASS_RENDER, - 0); - if (!props->engine) { - drm_dbg(&perf->i915->drm, - "No RENDER-capable engines\n"); - return -EINVAL; - } - - if (!engine_supports_oa(props->engine)) { - drm_dbg(&perf->i915->drm, - "Engine not supported by OA %d:%d\n", - I915_ENGINE_CLASS_RENDER, 0); - return -EINVAL; - } - /* Considering that ID = 0 is reserved and assuming that we don't * (currently) expect any configurations to ever specify duplicate * values for a particular property ID then the last _PROP_MAX value is * one greater than the maximum number of properties we expect to get * from userspace. */ - if (n_props >= DRM_I915_PERF_PROP_MAX) { + if (!n_props || n_props >= DRM_I915_PERF_PROP_MAX) { drm_dbg(&perf->i915->drm, - "More i915 perf properties specified than exist\n"); + "Invalid number of i915 perf properties given\n"); return -EINVAL; } + /* Defaults when class:instance is not passed */ + class = I915_ENGINE_CLASS_RENDER; + instance = 0; + for (i = 0; i < n_props; i++) { u64 oa_period, oa_freq_hz; u64 id, value; @@ -4174,7 +4156,13 @@ static int read_properties_unlocked(struct i915_perf *perf, } props->poll_oa_period = value; break; - case DRM_I915_PERF_PROP_MAX: + case DRM_I915_PERF_PROP_OA_ENGINE_CLASS: + class = (u8)value; + break; + case DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE: + instance = (u8)value; + break; + default: MISSING_CASE(id); return -EINVAL; } @@ -4182,6 +4170,21 @@ static int read_properties_unlocked(struct i915_perf *perf, uprop += 2; } + props->engine = intel_engine_lookup_user(perf->i915, class, instance); + if (!props->engine) { + drm_dbg(&perf->i915->drm, + "OA engine class and instance invalid %d:%d\n", + class, instance); + return -EINVAL; + } + + if (!engine_supports_oa(props->engine)) { + drm_dbg(&perf->i915->drm, + "Engine not supported by OA %d:%d\n", + class, instance); + return -EINVAL; + } + if (config_sseu) { ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); if (ret) { @@ -5158,8 +5161,11 @@ int i915_perf_ioctl_version(void) * * 5: Add DRM_I915_PERF_PROP_POLL_OA_PERIOD parameter that controls the * interval for the hrtimer used to check for OA data. + * + * 6: Add DRM_I915_PERF_PROP_OA_ENGINE_CLASS and + * DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE */ - return 5; + return 6; } #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 8df261c5ab9b..250ad04ea00a 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -2758,6 +2758,23 @@ enum drm_i915_perf_property_id { */ DRM_I915_PERF_PROP_POLL_OA_PERIOD, + /** + * Multiple engines may be mapped to the same OA unit. The OA unit is + * identified by class:instance of any engine mapped to it". + * + * This parameter specifies the engine class. + * + * This property is available in perf revision 6. + */ + DRM_I915_PERF_PROP_OA_ENGINE_CLASS, + + /** + * This parameter specifies the engine instance. + * + * This property is available in perf revision 6. + */ + DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE, + DRM_I915_PERF_PROP_MAX /* non-ABI */ };
One or more engines map to a specific OA unit. All reports from these engines are captured in the OA buffer managed by this OA unit. Current i915 OA implementation supports only the OAG unit. OAG primarily caters to render engine, so i915 OA uses render as the default engine in the OA implementation. Since there are more OA units on newer hardware that map to other engines, allow user to pass engine class and instance to select and program specific OA units. UMD specific changes for GPUvis support: https://patchwork.freedesktop.org/patch/522827/?series=114023 https://patchwork.freedesktop.org/patch/522822/?series=114023 https://patchwork.freedesktop.org/patch/522826/?series=114023 https://patchwork.freedesktop.org/patch/522828/?series=114023 https://patchwork.freedesktop.org/patch/522816/?series=114023 https://patchwork.freedesktop.org/patch/522825/?series=114023 v2: (Ashutosh) - Clarify commit message - Add drm_dbg - Clarify uapi description v3: (Ashutosh) - Remove irrelevant info from the uapi comment Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 60 ++++++++++++++++++-------------- include/uapi/drm/i915_drm.h | 17 +++++++++ 2 files changed, 50 insertions(+), 27 deletions(-)