From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CA753C432BE for ; Mon, 2 Aug 2021 06:29:43 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8947160EEA for ; Mon, 2 Aug 2021 06:29:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 8947160EEA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 169376E40C; Mon, 2 Aug 2021 06:29:43 +0000 (UTC) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1E1216E027; Mon, 2 Aug 2021 06:29:41 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10063"; a="210287731" X-IronPort-AV: E=Sophos;i="5.84,288,1620716400"; d="scan'208";a="210287731" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Aug 2021 23:29:39 -0700 X-IronPort-AV: E=Sophos;i="5.84,288,1620716400"; d="scan'208";a="440447251" Received: from adixit-mobl1.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.17.228]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Aug 2021 23:29:39 -0700 Date: Sun, 01 Aug 2021 23:29:38 -0700 Message-ID: <87r1fc5pgt.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Matthew Auld Cc: , , Maarten Lankhorst , Daniel Vetter , Ramalingam C In-Reply-To: References: <20210728103041.1669985-1-matthew.auld@intel.com> <20210728103041.1669985-4-matthew.auld@intel.com> <87eebi3ume.wl-ashutosh.dixit@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [Intel-gfx] [PATCH i-g-t v2 04/11] lib/i915/gem_mman: add fixed mode to gem_mmap__cpu X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Thu, 29 Jul 2021 01:50:45 -0700, Matthew Auld wrote: > Hi Matt, > On 29/07/2021 00:07, Dixit, Ashutosh wrote: > > On Wed, 28 Jul 2021 03:30:34 -0700, Matthew Auld wrote: > >> > >> diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c > >> index 337d28fb..6f5e6d72 100644 > >> --- a/lib/i915/gem_mman.c > >> +++ b/lib/i915/gem_mman.c > >> @@ -434,7 +434,13 @@ void *gem_mmap__device_coherent(int fd, uint32_t handle, uint64_t offset, > >> */ > >> void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot) > >> { > >> - return __gem_mmap(fd, handle, offset, size, prot, 0); > >> + void *ptr; > >> + > >> + ptr = __gem_mmap(fd, handle, offset, size, prot, 0); > >> + if (!ptr) > >> + ptr = __gem_mmap_offset__fixed(fd, handle, offset, size, prot); > >> + > >> + return ptr; > > > > What about __gem_mmap__wc? Also shouldn't we just fix the __gem_mmap_offset > > fallback in __gem_mmap and that will take care of both __gem_mmap__cpu and > > __gem_mmap__wc? > > For gem_mmap__wc it felt like slightly too much lying, since on discrete > smem-only buffers are always wb, and so the __wc here is not what the user > gets with the new FIXED mode. Correct. > gem_mmap__device_coherent() I think matches this new behaviour well, > where we don't explicitly state what the mapping type is, but instead > just guarantee that the returned mapping is device coherent. My rough > thinking was to convert most users of __wc over to __device_coherent(), > at least in the tests that we care about for discrete? Correct, though note that ALL tests will run on discrete (though with smem-only buffers with today's IGT since I don't believe we have added any tests which create lmem buffers yet). > On the other hand if we are happy with the lie, I don't think anything will > break, and pretty much all testscases using mmap I think should just > magically work on discrete, and it does mean a less less work vs converting > to __device_coherent? I lost you here since I really don't know what you mean by not changing to __device_coherent because afaiu gem_mmap__wc and gem_mmap_offset__wc will fail on discrete so we can't just not do anything. Next, I have no idea what is working and what is not working with the present series on DG1 (since the CI is showing all red on DG1 and I don't believe DG1 is included in showing regressions) and whether or not we have completed these FIXED related changes, so is this series complete or not? Afais since this series hasn't done anything about wc (except __device_coherent), any test which does wc will fail on discrete. > > > > (I think it will actually also fix __gem_mmap__device_coherent and > > __gem_mmap__cpu_coherent but maybe we can still have those patches in this > > series especially if they save a couple of system calls). Last, I believe I have a very simple solution to this FIXED conundrum. Which I believe works (you guys can tell me if it doesn't) but what I am not sure is whether it is an acceptable programming paradigm. Let us say, on discrete, we decide that we won't make any changes to any of the tests (we'll change on the library), and the tests themselves will not even be aware of the FIXED mode. The tests always ask for WC or WB but under the hood we will convert those calls to FIXED on discrete. So the tests ask for WC or WB but that will mean FIXED on discrete. Then I believe all we need to do is make a single very simple change to the function __gem_mmap_offset(). In this functions, if we see WC or WB we will convert that to FIXED before issuing the ioctl (or alternative issue the ioctl but if it returns error we will retry with FIXED). This works even for __gem_mmap() because __gem_mmap() itself falls back to __gem_mmap_offset(). So afais we don't need to make ANY changes to ANY of the higher level functions because we have fixed it in the lowest level function. If we do it this way we won't need most of the first 7 patches in the series. (This is what I was saying in the previous reply too). But like I said whether we want to do this or not we have to decide and I want other CC'd reviewers to chime in on this. It doesn't mean that we cannot make changes to higher level functions if it makes things more efficient or if we want to proliferate the FIXED mode throughout IGT but I think changing the lowest level function is sufficient to fix failing tests. Thanks. -- Ashutosh