Re: [PATCH v3 05/16] drm/i915: Disable the "binder"

From: Nirmoy Das <nirmoy.das@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Nirmoy Das" <nirmoy.das@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	"Michał Winiarski" <michal.winiarski@intel.com>
Subject: Re: [PATCH v3 05/16] drm/i915: Disable the "binder"
Date: Fri, 19 Jan 2024 11:47:42 +0100	[thread overview]
Message-ID: <cdbdeae2-041c-43ef-8cba-d57546b50e88@linux.intel.com> (raw)
In-Reply-To: <ZamwS6sLlEdJRv59@intel.com>

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]


On 1/19/2024 12:12 AM, Ville Syrjälä wrote:
> On Wed, Jan 17, 2024 at 06:46:24PM +0100, Nirmoy Das wrote:
>> On 1/17/2024 3:13 PM, Michał Winiarski wrote:
>>> On Tue, Jan 16, 2024 at 09:56:25AM +0200, Ville Syrjala wrote:
>>>> From: Ville Syrjälä<ville.syrjala@linux.intel.com>
>>>>
>>>> Now that the GGTT PTE updates go straight to GSMBASE (bypassing
>>>> GTTMMADR) there should be no more risk of system hangs? So the
>>>> "binder" (ie. update the PTEs via MI_UPDATE_GTT) is no longer
>>>> necessary, disable it.
>>>>
>>>> My main worry with the MI_UPDATE_GTT are:
>>>> - only used on this one platform so very limited testing coverage
>>>> - async so more opprtunities to screw things up
>>>> - what happens if the engine hangs while we're waiting for MI_UPDATE_GTT
>>>>     to finish?
>>>> - requires working command submission, so even getting a working
>>>>     display now depends on a lot more extra components working correctly
>>>>
>>>> TODO: MI_UPDATE_GTT might be interesting as an optimization
>>>> though, so perhaps someone should look into always using it
>>>> (assuming the GPU is alive and well)?
>>>>
>>>> v2: Keep using MI_UPDATE_GTT on VM guests
>>>>
>>>> Cc: Paz Zcharya<pazz@chromium.org>
>>>> Cc: Nirmoy Das<nirmoy.das@intel.com>
>>>> Signed-off-by: Ville Syrjälä<ville.syrjala@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gt/intel_gtt.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
>>>> index 86f73fe558ca..e83dabc56a14 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
>>>> @@ -24,7 +24,8 @@
>>>>    bool i915_ggtt_require_binder(struct drm_i915_private *i915)
>>>>    {
>>>>    	/* Wa_13010847436 & Wa_14019519902 */
>>>> -	return MEDIA_VER_FULL(i915) == IP_VER(13, 0);
>>>> +	return i915_run_as_guest() &&
>>>> +		MEDIA_VER_FULL(i915) == IP_VER(13, 0);
>>> Note that i915_run_as_guest() is not the most reliable way to decide
>>> whether to use MI_UPDATE_GTT or straight to GSMBASE, as it requires the
>>> hypervisor to "opt-in" and set the X86_FEATURE_HYPERVISOR.
>>> If it's not set - the driver will go into GSMBASE, which is not mapped
>>> inside the guest.
>>> Does the system firmware advertise whether GSMBASE is "open" or "closed"
>>> to CPU access in any way?
>> Had a chat with David from IVE team, David suggested to read 0x138914 to
>> determine that.  "GOP needs to qualify the WA by reading GFX MMIO offset
>> 0x138914 and verify the value there is 0x1." -> as per the HSD-22018444074
> OK, so we can confirm the firmware is on board. I suppose no real harm
> in doing so even though it would clearly be a rather weird if someone
> would ship some ancient firmware that doesn't handle this.
>
> But that still won't help with the guest side handling because that
> register will read the same in the guest.


We are back to the same question :/ How about
if (boot_cpu_has(X86_FEATURE_HYPERVISOR) && !i915_run_as_guest()

disable binder

Regards,

Nirmoy

>

[-- Attachment #2: Type: text/html, Size: 4503 bytes --]