Re: [PATCH v2 9/9] drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c

[Tvrtko Ursulin]

Hi,

On 18/10/2023 17:19, Zhao Liu wrote:
> Hi Rodrigo and Tvrtko,
>
> It seems this series is missed in v6.5.
> This work should not be forgotten. Let me rebase and refresh the version.

Right it seems we did not manage to social engineer any reviews. Please 
do respin and we will try again.

Regards,

Tvrtko

> Regards,
> Zhao
>
> On Mon, Apr 17, 2023 at 10:53:28AM -0400, Rodrigo Vivi wrote:
> > Date: Mon, 17 Apr 2023 10:53:28 -0400
> > From: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Subject: Re: [PATCH v2 9/9] drm/i915: Use kmap_local_page() in
> >   gem/i915_gem_execbuffer.c
> >
> > 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