Re: [Intel-gfx] [PATCH v2 14/15] drm/i915/guc: Support OA when Wa_16011777198 is enabled

From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
To: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 14/15] drm/i915/guc: Support OA when Wa_16011777198 is enabled
Date: Tue, 27 Sep 2022 09:11:23 -0700	[thread overview]
Message-ID: <YzMgq3oGQ51o2Pex@unerlige-ril> (raw)
In-Reply-To: <8735cd7m1f.wl-ashutosh.dixit@intel.com>

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@intel.com>
>> >>
>> >> 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(&gt->uc.guc) &&
>> >>> +        intel_uc_uses_guc_rc(&gt->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(&gt->uc.guc))
...

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(&gt->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
>> >>>