| Message ID | 20230316010101.2590309-13-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add OAM support for MTL | expand |
On Wed, 15 Mar 2023 18:01:01 -0700, Umesh Nerlige Ramappa wrote: > Hi Umesh, Mostly looks good but one nit below. > OAM does not work with media C6 enabled on some steppings of MTL. > Disable OAM if we detect that media C6 was enabled in bios. > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > drivers/gpu/drm/i915/i915_perf.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index 77fae3d80128..4ac6535a0356 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -209,6 +209,7 @@ > #include "gt/intel_gt_regs.h" > #include "gt/intel_lrc.h" > #include "gt/intel_lrc_reg.h" > +#include "gt/intel_rc6.h" > #include "gt/intel_ring.h" > #include "gt/uc/intel_guc_slpc.h" > > @@ -4898,6 +4899,18 @@ static u32 num_perf_groups_per_gt(struct intel_gt *gt) > > static u32 __oam_engine_group(struct intel_engine_cs *engine) > { > + /* > + * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media > + * C6 disable in BIOS. Do not enable OA for media classes if MC6 is > + * enabled in BIOS. > + */ > + if (IS_MTL_MEDIA_STEP(engine->i915, STEP_A0, STEP_C0) && > + intel_check_bios_c6_setup(&engine->gt->rc6)) { > + drm_notice_once(&engine->i915->drm, > + "OAM requires media C6 to be disabled in BIOS\n"); I think the typical mode of working with MTL would be to enable C6 unless OA is needed. But this will print out this notice on every MTL system. So IMO we should print this out only when a OAM perf stream is opened and that fails. Though not sure if it's ok to add a line to an already cluttered dmesg? The default console log level is 4 (WARNING) so maybe ok? https://linuxconfig.org/introduction-to-the-linux-kernel-log-levels Though if we fail the opening of an OAM stream we could make it a drm_warn? > + return PERF_GROUP_INVALID; > + } > + > if (GRAPHICS_VER_FULL(engine->i915) >= IP_VER(12, 70)) { > /* > * There's 1 SAMEDIA gt and 1 OAM per SAMEDIA gt. All media slices > @@ -5317,6 +5330,23 @@ int i915_perf_ioctl_version(struct drm_i915_private *i915) > * > * 7: Add support for video decode and enhancement classes. > */ > + > + /* > + * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media > + * C6 disable in BIOS. Do not enable OA for media classes if MC6 is > + * enabled in BIOS. Maybe add something like "Return version 6 to indicate non-support for OAM." just to make clear. > + */ > + if (IS_MTL_MEDIA_STEP(i915, STEP_A0, STEP_C0)) { > + struct intel_gt *gt; > + int i; > + > + for_each_gt(gt, i915, i) { > + if (gt->type == GT_MEDIA && > + intel_check_bios_c6_setup(>->rc6)) > + return 6; > + } > + } > + > return 7; > } > > -- > 2.36.1 > Thanks. -- Ashutosh
On Thu, 16 Mar 2023 22:14:52 -0700, Dixit, Ashutosh wrote: > > On Wed, 15 Mar 2023 18:01:01 -0700, Umesh Nerlige Ramappa wrote: > > > > Hi Umesh, > > Mostly looks good but one nit below. > > > OAM does not work with media C6 enabled on some steppings of MTL. > > Disable OAM if we detect that media C6 was enabled in bios. > > > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > > --- > > drivers/gpu/drm/i915/i915_perf.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > > index 77fae3d80128..4ac6535a0356 100644 > > --- a/drivers/gpu/drm/i915/i915_perf.c > > +++ b/drivers/gpu/drm/i915/i915_perf.c > > @@ -209,6 +209,7 @@ > > #include "gt/intel_gt_regs.h" > > #include "gt/intel_lrc.h" > > #include "gt/intel_lrc_reg.h" > > +#include "gt/intel_rc6.h" > > #include "gt/intel_ring.h" > > #include "gt/uc/intel_guc_slpc.h" > > > > @@ -4898,6 +4899,18 @@ static u32 num_perf_groups_per_gt(struct intel_gt *gt) > > > > static u32 __oam_engine_group(struct intel_engine_cs *engine) > > { > > + /* > > + * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media > > + * C6 disable in BIOS. Do not enable OA for media classes if MC6 is > > + * enabled in BIOS. > > + */ > > + if (IS_MTL_MEDIA_STEP(engine->i915, STEP_A0, STEP_C0) && > > + intel_check_bios_c6_setup(&engine->gt->rc6)) { > > + drm_notice_once(&engine->i915->drm, > > + "OAM requires media C6 to be disabled in BIOS\n"); > > I think the typical mode of working with MTL would be to enable C6 unless OA > is needed. But this will print out this notice on every MTL system. So IMO > we should print this out only when a OAM perf stream is opened and that > fails. > > Though not sure if it's ok to add a line to an already cluttered dmesg? The > default console log level is 4 (WARNING) so maybe ok? > > https://linuxconfig.org/introduction-to-the-linux-kernel-log-levels > > Though if we fail the opening of an OAM stream we could make it a drm_warn? Another idea: there is a drm_notice in Patch 2 when C6 is disabled, maybe we could just change it to "C6 disabled by BIOS, OAM availbable\n" or something like that and just remove the notice from here. I think we should avoid the notice when C6 is enabled since that is likely to be the default mode. > > > + return PERF_GROUP_INVALID; > > + } > > + > > if (GRAPHICS_VER_FULL(engine->i915) >= IP_VER(12, 70)) { > > /* > > * There's 1 SAMEDIA gt and 1 OAM per SAMEDIA gt. All media slices > > @@ -5317,6 +5330,23 @@ int i915_perf_ioctl_version(struct drm_i915_private *i915) > > * > > * 7: Add support for video decode and enhancement classes. > > */ > > + > > + /* > > + * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media > > + * C6 disable in BIOS. Do not enable OA for media classes if MC6 is > > + * enabled in BIOS. > > Maybe add something like "Return version 6 to indicate non-support for OAM." > just to make clear. > > > + */ > > + if (IS_MTL_MEDIA_STEP(i915, STEP_A0, STEP_C0)) { > > + struct intel_gt *gt; > > + int i; > > + > > + for_each_gt(gt, i915, i) { > > + if (gt->type == GT_MEDIA && > > + intel_check_bios_c6_setup(>->rc6)) > > + return 6; > > + } > > + } > > + > > return 7; > > } > > > > -- > > 2.36.1 > > > > Thanks. > -- > Ashutosh
On Thu, Mar 16, 2023 at 11:06:08PM -0700, Dixit, Ashutosh wrote: >On Thu, 16 Mar 2023 22:14:52 -0700, Dixit, Ashutosh wrote: >> >> On Wed, 15 Mar 2023 18:01:01 -0700, Umesh Nerlige Ramappa wrote: >> > >> >> Hi Umesh, >> >> Mostly looks good but one nit below. >> >> > OAM does not work with media C6 enabled on some steppings of MTL. >> > Disable OAM if we detect that media C6 was enabled in bios. >> > >> > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_perf.c | 30 ++++++++++++++++++++++++++++++ >> > 1 file changed, 30 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >> > index 77fae3d80128..4ac6535a0356 100644 >> > --- a/drivers/gpu/drm/i915/i915_perf.c >> > +++ b/drivers/gpu/drm/i915/i915_perf.c >> > @@ -209,6 +209,7 @@ >> > #include "gt/intel_gt_regs.h" >> > #include "gt/intel_lrc.h" >> > #include "gt/intel_lrc_reg.h" >> > +#include "gt/intel_rc6.h" >> > #include "gt/intel_ring.h" >> > #include "gt/uc/intel_guc_slpc.h" >> > >> > @@ -4898,6 +4899,18 @@ static u32 num_perf_groups_per_gt(struct intel_gt *gt) >> > >> > static u32 __oam_engine_group(struct intel_engine_cs *engine) >> > { >> > + /* >> > + * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media >> > + * C6 disable in BIOS. Do not enable OA for media classes if MC6 is >> > + * enabled in BIOS. >> > + */ >> > + if (IS_MTL_MEDIA_STEP(engine->i915, STEP_A0, STEP_C0) && >> > + intel_check_bios_c6_setup(&engine->gt->rc6)) { >> > + drm_notice_once(&engine->i915->drm, >> > + "OAM requires media C6 to be disabled in BIOS\n"); >> >> I think the typical mode of working with MTL would be to enable C6 unless OA >> is needed. But this will print out this notice on every MTL system. So IMO >> we should print this out only when a OAM perf stream is opened and that >> fails. We could do that. I can move this to the open ioctl. >> >> Though not sure if it's ok to add a line to an already cluttered dmesg? The >> default console log level is 4 (WARNING) so maybe ok? >> >> https://linuxconfig.org/introduction-to-the-linux-kernel-log-levels >> >> Though if we fail the opening of an OAM stream we could make it a drm_warn? Hmm, I was thinking of just keeping it in line with the other failures in open ioctl - a drm_err message, so that it helps debug. > >Another idea: there is a drm_notice in Patch 2 when C6 is disabled, maybe >we could just change it to "C6 disabled by BIOS, OAM availbable\n" or >something like that and just remove the notice from here. I think we should >avoid the notice when C6 is enabled since that is likely to be the default >mode. Ideally patch 2 is required irrespective of the OA issue, since i915 should make sure it honors the bios setting. With that in mind, I would leave the drm message in OA code. > >> >> > + return PERF_GROUP_INVALID; >> > + } >> > + >> > if (GRAPHICS_VER_FULL(engine->i915) >= IP_VER(12, 70)) { >> > /* >> > * There's 1 SAMEDIA gt and 1 OAM per SAMEDIA gt. All media slices >> > @@ -5317,6 +5330,23 @@ int i915_perf_ioctl_version(struct drm_i915_private *i915) >> > * >> > * 7: Add support for video decode and enhancement classes. >> > */ >> > + >> > + /* >> > + * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media >> > + * C6 disable in BIOS. Do not enable OA for media classes if MC6 is >> > + * enabled in BIOS. >> >> Maybe add something like "Return version 6 to indicate non-support for OAM." >> just to make clear. >> will do Thanks, Umesh >> > + */ >> > + if (IS_MTL_MEDIA_STEP(i915, STEP_A0, STEP_C0)) { >> > + struct intel_gt *gt; >> > + int i; >> > + >> > + for_each_gt(gt, i915, i) { >> > + if (gt->type == GT_MEDIA && >> > + intel_check_bios_c6_setup(>->rc6)) >> > + return 6; >> > + } >> > + } >> > + >> > return 7; >> > } >> > >> > -- >> > 2.36.1 >> > >> >> Thanks. >> -- >> Ashutosh
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 77fae3d80128..4ac6535a0356 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -209,6 +209,7 @@ #include "gt/intel_gt_regs.h" #include "gt/intel_lrc.h" #include "gt/intel_lrc_reg.h" +#include "gt/intel_rc6.h" #include "gt/intel_ring.h" #include "gt/uc/intel_guc_slpc.h" @@ -4898,6 +4899,18 @@ static u32 num_perf_groups_per_gt(struct intel_gt *gt) static u32 __oam_engine_group(struct intel_engine_cs *engine) { + /* + * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media + * C6 disable in BIOS. Do not enable OA for media classes if MC6 is + * enabled in BIOS. + */ + if (IS_MTL_MEDIA_STEP(engine->i915, STEP_A0, STEP_C0) && + intel_check_bios_c6_setup(&engine->gt->rc6)) { + drm_notice_once(&engine->i915->drm, + "OAM requires media C6 to be disabled in BIOS\n"); + return PERF_GROUP_INVALID; + } + if (GRAPHICS_VER_FULL(engine->i915) >= IP_VER(12, 70)) { /* * There's 1 SAMEDIA gt and 1 OAM per SAMEDIA gt. All media slices @@ -5317,6 +5330,23 @@ int i915_perf_ioctl_version(struct drm_i915_private *i915) * * 7: Add support for video decode and enhancement classes. */ + + /* + * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media + * C6 disable in BIOS. Do not enable OA for media classes if MC6 is + * enabled in BIOS. + */ + if (IS_MTL_MEDIA_STEP(i915, STEP_A0, STEP_C0)) { + struct intel_gt *gt; + int i; + + for_each_gt(gt, i915, i) { + if (gt->type == GT_MEDIA && + intel_check_bios_c6_setup(>->rc6)) + return 6; + } + } + return 7; }
OAM does not work with media C6 enabled on some steppings of MTL. Disable OAM if we detect that media C6 was enabled in bios. Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)