[PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[Damien Lespiau]

This reverts commit 02f6bcccf7c324115747aae2f0addd6af5d321cd.

The OA buffer can contain global data (in particular, not linked to a
context or a single batch execution) about GPU events (eg. hw context
switches, rc6 transitions, frequency changes, ...) and needs to be
mapped to GGTT. The pin ioctl provided a way to do that.

Admittedly, this change broke what seems to be a valid use case of
pinning a buffer in GGTT, even when PPGTT is used (which is the reason
invoked in the commit message).

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Tomasz Madajczak <tomasz.madajczak@intel.com>
Cc: Adam Rutkowski <adam.j.rutkowski@intel.com>

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1794a04..8019809 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4143,9 +4143,6 @@ i915_gem_pin_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_gem_object *obj;
 	int ret;
 
-	if (INTEL_INFO(dev)->gen >= 6)
-		return -ENODEV;
-
 	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
 		return ret;
-- 
1.8.3.1

Re: [PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[Chris Wilson]

On Thu, Jul 03, 2014 at 08:12:35AM +0100, Damien Lespiau wrote:
> This reverts commit 02f6bcccf7c324115747aae2f0addd6af5d321cd.
> 
> The OA buffer can contain global data (in particular, not linked to a
> context or a single batch execution) about GPU events (eg. hw context
> switches, rc6 transitions, frequency changes, ...) and needs to be
> mapped to GGTT. The pin ioctl provided a way to do that.
>
> Admittedly, this change broke what seems to be a valid use case of
> pinning a buffer in GGTT, even when PPGTT is used (which is the reason
> invoked in the commit message).
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tomasz Madajczak <tomasz.madajczak@intel.com>
> Cc: Adam Rutkowski <adam.j.rutkowski@intel.com>
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk> # who didn't like his tools
                                                    being confiscated
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[Daniel Vetter]

On Thu, Jul 03, 2014 at 08:12:35AM +0100, Damien Lespiau wrote:
> This reverts commit 02f6bcccf7c324115747aae2f0addd6af5d321cd.
> 
> The OA buffer can contain global data (in particular, not linked to a
> context or a single batch execution) about GPU events (eg. hw context
> switches, rc6 transitions, frequency changes, ...) and needs to be
> mapped to GGTT. The pin ioctl provided a way to do that.
> 
> Admittedly, this change broke what seems to be a valid use case of
> pinning a buffer in GGTT, even when PPGTT is used (which is the reason
> invoked in the commit message).

Global OA buffers should be handled by the kernel and exposed through
perf, imo. I think I'll go lalala on this a bit longer ...
-Daniel

> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tomasz Madajczak <tomasz.madajczak@intel.com>
> Cc: Adam Rutkowski <adam.j.rutkowski@intel.com>
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1794a04..8019809 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4143,9 +4143,6 @@ i915_gem_pin_ioctl(struct drm_device *dev, void *data,
>  	struct drm_i915_gem_object *obj;
>  	int ret;
>  
> -	if (INTEL_INFO(dev)->gen >= 6)
> -		return -ENODEV;
> -
>  	ret = i915_mutex_lock_interruptible(dev);
>  	if (ret)
>  		return ret;
> -- 
> 1.8.3.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[Jesse Barnes]

On Mon, 7 Jul 2014 23:04:55 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jul 03, 2014 at 08:12:35AM +0100, Damien Lespiau wrote:
> > This reverts commit 02f6bcccf7c324115747aae2f0addd6af5d321cd.
> > 
> > The OA buffer can contain global data (in particular, not linked to a
> > context or a single batch execution) about GPU events (eg. hw context
> > switches, rc6 transitions, frequency changes, ...) and needs to be
> > mapped to GGTT. The pin ioctl provided a way to do that.
> > 
> > Admittedly, this change broke what seems to be a valid use case of
> > pinning a buffer in GGTT, even when PPGTT is used (which is the reason
> > invoked in the commit message).
> 
> Global OA buffers should be handled by the kernel and exposed through
> perf, imo. I think I'll go lalala on this a bit longer ...

Why?  Because allowing the pin ioctl as root is such a problem?  You
need to come up with an alternative proposal and we need to get it
implemented in some reasonable amount of time if we're not going to
just do the simple thing that's already been shown to work...

IOW don't plug your ears and say "lalala" for too long.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[Damien Lespiau]

Hi Daniel,

On Mon, Jul 07, 2014 at 11:04:55PM +0200, Daniel Vetter wrote:
> On Thu, Jul 03, 2014 at 08:12:35AM +0100, Damien Lespiau wrote:
> > This reverts commit 02f6bcccf7c324115747aae2f0addd6af5d321cd.
> > 
> > The OA buffer can contain global data (in particular, not linked to a
> > context or a single batch execution) about GPU events (eg. hw context
> > switches, rc6 transitions, frequency changes, ...) and needs to be
> > mapped to GGTT. The pin ioctl provided a way to do that.
> > 
> > Admittedly, this change broke what seems to be a valid use case of
> > pinning a buffer in GGTT, even when PPGTT is used (which is the reason
> > invoked in the commit message).
> 
> Global OA buffers should be handled by the kernel and exposed through
> perf, imo. I think I'll go lalala on this a bit longer ...

Do you think that we can unblock this now that we have somewhat of path
forward with Rob Bragg's work? I'm still uneasy with a precedent where
we break working applications and it takes a long time for us to revert
the offending change?

Thanks!

-- 
Damien

Re: [PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[Daniel Vetter]

On Fri, Sep 19, 2014 at 03:25:55PM +0100, Damien Lespiau wrote:
> Hi Daniel,
> 
> On Mon, Jul 07, 2014 at 11:04:55PM +0200, Daniel Vetter wrote:
> > On Thu, Jul 03, 2014 at 08:12:35AM +0100, Damien Lespiau wrote:
> > > This reverts commit 02f6bcccf7c324115747aae2f0addd6af5d321cd.
> > > 
> > > The OA buffer can contain global data (in particular, not linked to a
> > > context or a single batch execution) about GPU events (eg. hw context
> > > switches, rc6 transitions, frequency changes, ...) and needs to be
> > > mapped to GGTT. The pin ioctl provided a way to do that.
> > > 
> > > Admittedly, this change broke what seems to be a valid use case of
> > > pinning a buffer in GGTT, even when PPGTT is used (which is the reason
> > > invoked in the commit message).
> > 
> > Global OA buffers should be handled by the kernel and exposed through
> > perf, imo. I think I'll go lalala on this a bit longer ...
> 
> Do you think that we can unblock this now that we have somewhat of path
> forward with Rob Bragg's work? I'm still uneasy with a precedent where
> we break working applications and it takes a long time for us to revert
> the offending change?

I still consider pinning stuff behind the kernels back too evil. So if
people want that I'd like to see the use-case and why we can't do this
differently.

And I've never approved of the pin ioctl but really loudly complained
about each occasion I've spotted, so I think the internal users just have
to keep the pieces for a bit longer.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[Bloomfield, Jon]

On Fri, Sep 19, 2014 Daniel Vetter wrote:
> I still consider pinning stuff behind the kernels back too evil. So if people
> want that I'd like to see the use-case and why we can't do this differently.
> 
> And I've never approved of the pin ioctl but really loudly complained about
> each occasion I've spotted, so I think the internal users just have to keep the
> pieces for a bit longer.
> -Daniel

Daniel. All that's being asked for is a re-instatement of the old mechanism until
a better solution can be designed and implemented. 

It may well be evil, but the mechanism was there, and was the only known
way to handle the OA buffer, so why wouldn't someone use it ? You've broken 
userspace before you provided any alternative solution.

Please, let's revert this while a more elegant solution is designed, implemented,
reviewed, re-implemented, reviewed again, and maybe one day merged.

Remember that it will take a while to filter down to people even after you merge
any new solution to nightly, or even drm-next. If we have to wait for the new
design, it's not likely to reach us until next year.

Can't we just agree that we're evil, but turn a blind eye while we're coaxed slowly
back towards the light ?

Jon

Re: [PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[Dave Airlie]

On 2 October 2014 20:40, Bloomfield, Jon <jon.bloomfield@intel.com> wrote:
> On Fri, Sep 19, 2014 Daniel Vetter wrote:
>> I still consider pinning stuff behind the kernels back too evil. So if people
>> want that I'd like to see the use-case and why we can't do this differently.
>>
>> And I've never approved of the pin ioctl but really loudly complained about
>> each occasion I've spotted, so I think the internal users just have to keep the
>> pieces for a bit longer.
>> -Daniel
>
> Daniel. All that's being asked for is a re-instatement of the old mechanism until
> a better solution can be designed and implemented.
>
> It may well be evil, but the mechanism was there, and was the only known
> way to handle the OA buffer, so why wouldn't someone use it ? You've broken
> userspace before you provided any alternative solution.
>
> Please, let's revert this while a more elegant solution is designed, implemented,
> reviewed, re-implemented, reviewed again, and maybe one day merged.
>
> Remember that it will take a while to filter down to people even after you merge
> any new solution to nightly, or even drm-next. If we have to wait for the new
> design, it's not likely to reach us until next year.
>
> Can't we just agree that we're evil, but turn a blind eye while we're coaxed slowly
> back towards the light ?

I thought we had an agreement that no features that were specific to
the non-open source userspace drivers would be merged to the kernel,
due to security and maintenance concerns, i.e. exactly this concern,
we now have to maintain an interface that we can't regression test in
public.

Maybe next time who ever made the decision to force merge code will
learn why its a bad idea.

Personally I don't think this interface should be put back in, unless
Mesa/Beigenet/libva uses it, does it even have igt tests?

Dave.

Re: [PATCH] drm/i915: Revert "drm/i915: Reject the pin ioctl on gen6+"

[Damien Lespiau]

Hi Dave,

On Fri, Oct 03, 2014 at 07:12:53AM +1000, Dave Airlie wrote:
> I thought we had an agreement that no features that were specific to
> the non-open source userspace drivers would be merged to the kernel,
> due to security and maintenance concerns, i.e. exactly this concern,
> we now have to maintain an interface that we can't regression test in
> public.

Actually, the pin ioctl() has been here for quite some time, since the
introduction of GEM, see: 

  http://lwn.net/Articles/283798/ section 8.2

That's why one side is arguing that it's not acceptable to break
user-space (open or closed , it doesn't matter at this point).

Daniel is saying that's a legacy artefact and it shouldn't be used
anymore, but breaking a user-space application is enough for me to at
least suggest a revert.

HTH,

-- 
Damien