Re: [PATCH 01/03] drm/i915: add a context parameter to {en, dis}able zero address mapping

Chris Wilson
2015-05-28T16:56:30Z

On Thu, May 28, 2015 at 05:52:21PM +0200, Daniel Vetter wrote:
> On Thu, May 28, 2015 at 03:39:26PM +0100, Chris Wilson wrote:
> > On Wed, May 20, 2015 at 05:00:13PM +0300, David Weinehall wrote:
> > > Export a new context parameter that can be set/queried through the
> > > context_{get,set}param ioctls. This parameter is passed as a context
> > > flag and decides whether or not a GPU address mapping is allowed to
> > > be made at address zero. The default is to allow such mappings.
> >
> > Please revert this piece of unreviewed API.
> >
> > The most obvious objection is what value of bias the kernel should be
> > using.
> >
> > Then given that what value does this hold over and above userspace
> > specifying the layout they want?
>
> Yeah it would be redundant with softpinning. But that's only for ocl2, and
> this is apparently needed for for ocl1.x already. Hence imo it's ok to
> pull this ahead a bit, even if redundant.

Look at the interface:

CONTEXT_NO_ZEROMAP:
- it has nothing to do with mapping, it only concerns execbuf object
location
- what is a sensible bias? I certainly do not wish for the hack we have
right now to become permanent (as this patch makes it).
- it can still easily fail given a kernel that operates on pinned
objects when not in full-ppgtt

For a context parameter, something like
CONTEXT_MM_START, CONTEXT_MM_END
that limited the drm_mm range manager for the context (downside is that
such would only be available for full-ppgtt, but really full-ppgtt is
mandatory for this in the first place otherwise it *is* exploitable -
for it not requires a compiler that wouldn't need the NULL pointer to be
all 0). The advantage of that parameter is that it is useful in igt
testing, and even allows for limited rollout of 48bit full-ppgtt, vital
given the workarounds that userspace must buy into before enabling.

And then there is "int flags;". Are we really expecting to use the sign
bit on this bitmask?
-Chris

--
Chris Wilson, Intel Open Source Technology Centre