Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Apr 2023 10:53:28 -0400
From:   Rodrigo Vivi <rodrigo.vivi@intel.com>
To:     Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
CC:     Zhao Liu <zhao1.liu@linux.intel.com>,
        "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        David Airlie <airlied@gmail.com>,
        "Daniel Vetter" <daniel@ffwll.ch>,
        Matthew Auld <matthew.auld@intel.com>,
        Thomas =?iso-8859-1?Q?Hellstr=F6m?= 
        <thomas.hellstrom@linux.intel.com>,
        Nirmoy Das <nirmoy.das@intel.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Chris Wilson <chris@chris-wilson.co.uk>,
        Christian =?iso-8859-1?Q?K=F6nig?= <christian.koenig@amd.com>,
        <intel-gfx@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        Zhenyu Wang <zhenyu.z.wang@intel.com>,
        "Zhao Liu" <zhao1.liu@intel.com>
Subject: Re: [PATCH v2 9/9] drm/i915: Use kmap_local_page() in
 gem/i915_gem_execbuffer.c
Message-ID: <ZD1daLWcOslJ/wdt@intel.com>
References: <20230329073220.3982460-1-zhao1.liu@linux.intel.com>
 <64265ef8725fe_375f7e294a@iweiny-mobl.notmuch>
 <fdc8a470-1e6b-815d-e367-a9df1b0b14dd@linux.intel.com>
 <2177327.1BCLMh4Saa@suse>
 <1b341218-f0e2-a613-2ac6-107064a813ca@linux.intel.com>
 <ZDku5SJhl2Ve51UC@liuzhao-OptiPlex-7080>
 <048d4dba-153f-5d32-75fc-d7e7144d1e9c@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <048d4dba-153f-5d32-75fc-d7e7144d1e9c@linux.intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?FkpeuuS99DtuExt6wMrEO0ICpVXVmUqqsnHtLEg+V/NQ3RG3qmzBAyztp+lY?=
 =?us-ascii?Q?c4NL2VS0j7jhkla8vIPK7S2rMcG3DhURdXnhUAVqpxNrBxXb2V7d9AzD2DAD?=
 =?us-ascii?Q?/XjTbLUPD67QP3J/CfFUHlC95DHK1TCllSGiEHL/Fm0NFktdixLafv3qbibw?=
 =?us-ascii?Q?uMspxZpYKOKvOMyPrH/88LyDDZG2ljf/w03GOkj7j6KiW78J0h2mFkJGnOUY?=
 =?us-ascii?Q?MJtwbgH0WfgyV0J05mzQXri2TIY8pWIRtKHe6dByFA1Ei/cvSOyNGzwC56B5?=
 =?us-ascii?Q?NQCq7L6Zjfersiec+4LYDisTjB6aIP016AVJ7OZFZQBTvqu8u7cehD07qaPX?=
 =?us-ascii?Q?0W24ui8nq57ePQL4bufqSIIsrsZJzwk8QvE/93dSZZHulBxKoqLE6XOlKvbk?=
 =?us-ascii?Q?sURO+KcLzizawhT/9rrs9zATtU+GvXbWPqvd3+L38qc5bP7KQYG6gFnoiDwJ?=
 =?us-ascii?Q?EHQgCRYNaM2u4mT1PYG2qe9btuWtPygszkuyhTgD9ufnB/wF0r+5Cv2YsqwP?=
 =?us-ascii?Q?y3tI1BgIxUNm9kDilZjcf5MgbrJv/7YzcnywK1y91X7eYY4gnTyB6Wt7tSwA?=
 =?us-ascii?Q?yKv2hi569l1lsBLBraThr5kT8FLk48O0nhgdPla/5uJ4cb/Euncl2sVm4S+P?=
 =?us-ascii?Q?9xjKebqt9eC7+Kz5akKoStj7kMGPocWkU/vYo6ioN3ozaYzBC/adojub+8fM?=
 =?us-ascii?Q?ucrHWsym7EExyyXRnASY3OhpraCBEiaxzsn90nOYpx7HiIon3HsoiFVhgrJY?=
 =?us-ascii?Q?pH7mw/fwVKZZuo7KRDKWTHQzMun5DOqkdDHE1MUub/MpAhsayZLiYjighU/W?=
 =?us-ascii?Q?EIfq3m3fY7UZPo7BqOTfYlwZah9wBpwf7hTCEF4k4tg3BoOJwsqpCrs9KZNk?=
 =?us-ascii?Q?OmT6V81903JU5Ye+vrTI0BIMzLDKGebd1rT+jmGyUe2A/NDH+Us4S2lNDcrO?=
 =?us-ascii?Q?XQXZiY1cEOMkvKgqoRVzdwRftLeTaL5cJuFgSWSK8TBg4dn7xvGNhDivr1QU?=
 =?us-ascii?Q?65s6EUbhl+Y7oMljQPUYd2NHL0vi3d1zzYrNs70d311AC53N0qJvEPIrLaiR?=
 =?us-ascii?Q?iXaZdGBKES2aLsEQIvXqNIzmdst25/FYpVdA6Z45CptB3cxGzw0Ds/JZz9BP?=
 =?us-ascii?Q?/QK18hrqKAKuqxUWmMTut3rpPLVCqJsN2pmG8q7l9pzNzX/5CdlmZncdqhJA?=
 =?us-ascii?Q?nXc3bMJNH9VA2/9CRKXKBBjlRC+8oDL0L/nu+4V6XgX/zbOsUikf8G4eCf4D?=
 =?us-ascii?Q?3M6UxF4fEx2gnC8QbIolRInrNGg9SkI6+LbP/Xo2yuqT+z4PqrzSVoXojDMm?=
 =?us-ascii?Q?Pcl8y4gpl9kEjpvP3wKZ7z97oBWHlAEtVihOTvleNPqBPRrUlDivnrKEbaRn?=
 =?us-ascii?Q?seGhqMWJLcvYjIjv66z5aAXCH54sD/sN0pk/SXmxSYNpgGjo+wUqBuV35ydx?=
 =?us-ascii?Q?DIyzffzqIBYh4fEFMyq31GiEttgZHpeHVCOALM94EGsVfFZVaS2IzJMQt0/J?=
 =?us-ascii?Q?zImKvarQtYApndwSG0w9xmx3S0JTKh0eYUgZda1nbF+DN9eMgJjMCMrWHHYM?=
 =?us-ascii?Q?iUZ1hV7fIA3kOWRfFyfAfjgSqeeiG2UP/6Ke2zDV?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d888c533-b0e0-4236-b8c7-08db3f5384b0
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6059.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Apr 2023 14:53:34.9435
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3UniaKW6M/9DCt0tshrtXGEigBE17RsZ9v+ZIkynQOHlBcsANiGLp1lAlfej9IMwF6JGJ4ms7Qo02hVFvPzFww==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB7376
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Apr 17, 2023 at 12:24:45PM +0100, Tvrtko Ursulin wrote:
> 
> On 14/04/2023 11:45, Zhao Liu wrote:
> > Hi Tvrtko,
> > 
> > On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
> > 
> > [snip]
> > 
> > > > 
> > > > [snip]
> > > > > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > > > > Matt, being the last to touch this area, perhaps you could have a look?
> > > > > Because I notice we have a fallback iomap path which still uses
> > > > > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > > > > safe, does the iomap side also needs converting to
> > > > > io_mapping_map_local_wc? Or they have separate requirements?
> > > > 
> > > > AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> > > > kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> > > > context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> > > > 
> > > > I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> > > > io_mapping_unmap_local_wc since the address is local to context.
> > > > 
> > > > However, not being an expert, reading your note now I suspect that I'm missing
> > > > something. Can I ask why you think that page-faults disabling might be
> > > > necessary?
> > > 
> > > I am not saying it is, was just unsure and wanted some people who worked on this code most recently to take a look and confirm.
> > > 
> > > I guess it will work since the copying is done like this anyway:
> > > 
> > > 		/*
> > > 		 * This is the fast path and we cannot handle a pagefault
> > > 		 * whilst holding the struct mutex lest the user pass in the
> > > 		 * relocations contained within a mmaped bo. For in such a case
> > > 		 * we, the page fault handler would call i915_gem_fault() and
> > > 		 * we would try to acquire the struct mutex again. Obviously
> > > 		 * this is bad and so lockdep complains vehemently.
> > > 		 */
> > > 		pagefault_disable();
> > > 		copied = __copy_from_user_inatomic(r, urelocs, count * sizeof(r[0]));
> > > 		pagefault_enable();
> > > 		if (unlikely(copied)) {
> > > 			remain = -EFAULT;
> > > 			goto out;
> > > 		}
> > > 
> > > Comment is a bit outdated since we don't use that global "struct mutex" any longer, but in any case, if there is a page fault on the mapping where we need to recurse into i915 again to satisfy if, we seem to have code already to handle it. So kmap_local conversion I *think* can't regress anything.
> > 
> > Thanks for your explanation!
> > 
> > > 
> > > Patch to convert the io_mapping_map_atomic_wc can indeed come later.
> > 
> > Okay, I will also look at this.
> > 
> > > 
> > > In terms of logistics - if we landed this series to out branch it would be queued only for 6.5. Would that work for you?
> > 
> > Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?
> 
> Yes, but just because we failed to review and merge in time, not because you
> did not provide patches in time.

It is worth mentioning that under drm we close the merge window earlier.
Around -rc5.

So, Linus' merge window for 6.4 didn't happen yet. But our drm-next that
is going to be sent there is already closed.

> 
> Regards,
> 
> Tvrtko
>