On Tue, 27 Sep 2022 10:34:52 -0700, Dixit, Ashutosh wrote: > > On Tue, 27 Sep 2022 09:11:23 -0700, Umesh Nerlige Ramappa wrote: > > > > On Mon, Sep 26, 2022 at 04:28:44PM -0700, Dixit, Ashutosh wrote: > > > On Mon, 26 Sep 2022 14:17:21 -0700, Belgaumkar, Vinay wrote: > > >> > > >> > > >> On 9/26/2022 11:19 AM, Umesh Nerlige Ramappa wrote: > > >> > On Mon, Sep 26, 2022 at 08:56:01AM -0700, Dixit, Ashutosh wrote: > > >> >> On Fri, 23 Sep 2022 13:11:53 -0700, Umesh Nerlige Ramappa wrote: > > >> >>> > > >> >>> From: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx> > > >> >> > > >> >> Hi Umesh/Vinay, > > >> >> > > >> >>> @@ -3254,6 +3265,24 @@ static int i915_oa_stream_init(struct > > >> >>> i915_perf_stream *stream, > > >> >>> intel_engine_pm_get(stream->engine); > > >> >>> intel_uncore_forcewake_get(stream->uncore, FORCEWAKE_ALL); > > >> >>> > > >> >>> + /* > > >> >>> + * Wa_16011777198:dg2: GuC resets render as part of the Wa. This > > >> >>> causes > > >> >>> + * OA to lose the configuration state. Prevent this by overriding > > >> >>> GUCRC > > >> >>> + * mode. > > >> >>> + */ > > >> >>> + if (intel_guc_slpc_is_used(>->uc.guc) && > > >> >>> + intel_uc_uses_guc_rc(>->uc) && > > >> >> > > >> >> Is this condition above correct? E.g. what happens when: > > >> >> > > >> >> a. GuCRC is used but SLPC is not used? > > >> > > >> Currently, we have no way to separate out GuC RC and SLPC. Both are on when > > >> guc submission is enabled/supported. So, the above check is kind of > > >> redundant anyways. > > > > > > That is why I was suggesting changing the if check to an assert or > > > drm_err. So looks like it will work with or without GuC RC, but if we are > > > using GuC RC we should be using SLPC. So: > > > > > > if (GuC_RC && !SLPC) > > > drm_err(); > > > > I am little confused. What's the ask here? Should I just use one of these > > conditions? i.e. > > > > if (intel_guc_slpc_is_used(>->uc.guc)) > > I think let's just use intel_uc_uses_guc_rc and drop > intel_guc_slpc_is_used. I am assuming if SLPC is somehow not on and we try > to use SLPC set/unset param, some kind of error will be reported. With the above this patch is also: Reviewed-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx> > > > > ... > > > > Thanks, > > Umesh > > > > > > > >> Thanks, > > >> > > >> Vinay. > > >> > > >> >> b. GuCRC is not used. Don't we need to disable RC6 in host based RC6 > > >> >> control? > > >> > > > >> > When using host based rc6, existing OA code is using forcewake and a > > >> > reference to engine_pm to prevent rc6. Other questions, directing to > > >> > @Vinay. > > >> > > > >> > Thanks, > > >> > Umesh > > >> > > > >> >> > > >> >> Do we need to worry about these cases? > > >> >> > > >> >> Or if we always expect both GuCRC and SLPC to be used on DG2 then I > > >> >> think > > >> >> let's get rid of these from the if condition and add a drm_err() if we > > >> >> see > > >> >> these not being used and OA is being enabled on DG2? > > >> >> > > >> >> Thanks. > > >> >> -- > > >> >> Ashutosh > > >> >> > > >> >>> + (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0) || > > >> >>> + IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0))) { > > >> >>> + ret = intel_guc_slpc_override_gucrc_mode(>->uc.guc.slpc, > > >> >>> + SLPC_GUCRC_MODE_GUCRC_NO_RC6); > > >> >>> + if (ret) { > > >> >>> + drm_dbg(&stream->perf->i915->drm, > > >> >>> + "Unable to override gucrc mode\n"); > > >> >>> + goto err_config; > > >> >>> + } > > >> >>> + } > > >> >>> + > > >> >>> ret = alloc_oa_buffer(stream); > > >> >>> if (ret) > > >> >>> goto err_oa_buf_alloc; > > >> >>> -- > > >> >>> 2.25.1 > > >> >>>