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.
> ...
>
> 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
> >> >>>
