[Intel-gfx] [PATCH 1/2] drm/i915: Disallow pin ioctl completely for kms drivers

[Jesse Barnes]

On 02/23/2015 03:40 PM, Daniel Vetter wrote:
> On Mon, Feb 23, 2015 at 01:29:57PM -0800, Jesse Barnes wrote:
>> On 11/24/2014 06:13 AM, Chris Wilson wrote:
>>> On Mon, Nov 24, 2014 at 03:10:05PM +0100, Daniel Vetter wrote:
>>>> On Mon, Nov 24, 2014 at 10:35:29AM +0000, Chris Wilson wrote:
>>>>> Pinning is a useful tool and it would also be useful to have again on
>>>>> gen6+.
>>>>
>>>> I think softpin or similar is doable with ppgtt. But with shared ggtt I'm
>>>> not really enthusiastic about providing this kind of rope to userspace.
>>>> And softpin is a different type of pinning, so we don't really lose
>>>> anything by ripping out the userspace hard pinning ioctls.
>>>
>>> I am not talking about softpin, but being able to pin memory and a GGTT
>>> binding itself is useful.
>>
>> I see you merged this over Chris's objections and then shot down his
>> revert.  I'm not clear on why you're so opposed to the pin ioctl?  It's
>> a privileged op and definitely has its uses as Chris has repeatedly
>> pointed out.
>>
>> Why so insistent on dropping this particular ioctl?  Do you see it
>> causing actual problems?  Or do you just like preventing userspace from
>> doing things you don't agree with?
>>
>> (Sorry, catching up on ancient backlog from intel-gfx, so maybe there's
>> a thread I missed when re-looking at this.  If so, please point me at it.)
> 
> People are way too happy to abuse it instead of using dma-buf. And at
> least some of the uses sna has also cause a bunch of problems with being a
> bit too clever around reloc handling (so we essentially _have_ to take the
> toys away since giving it back would cause regressions).

Some interfaces are more dangerous than others.  But that doesn't mean
they're necessarily bad.

> If there's a real users then we can look at this again imo, but I think
> most things are better solved with proper kernel interfaces since in the
> end the kernel does mm for the gpu, and if userspace interferes we can't
> do that.
> 
> So overall my answer is:
> - re-enable will cause regressions

Which regressions?  In SNA?  It sounded like Chris was the one
requesting this here.  And really, dropping pin altogether was a big
regression in the ABI to begin with and probably shouldn't have been
allowed (the one back in 2013; I think both Chris and Ben objected back
then too).

> - I don't see a justified user

What about SNA?  What about debug?  Yes there's an alternative in the
SNA case, but Chris mentioned it had a huge perf hit.  And fwiw the
Beignet team is using this too, so at the very least it needs to work on
aliasing PPGTT on gen7/7.5.

> - we should never have allowed this with kms to begin with, it was an
>   oversight.

Not sure about that; as Chris mentioned, mlock() has uses too.  It needs
to be limited, obviously, and can cause trouble if you're not careful.
But that's not a reason to disallow it or remove it altogether.

Anyway, the patches have no r-bs or acks, only nacks going back to gen6,
and you're still merging these.  That's what's not sitting well with me.

Jesse