Re: [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

["Gupta, Sourab" <sourab.gupta@intel.com>]

On Fri, 2014-04-11 at 08:55 +0000, Daniel Vetter wrote:
> On Thu, Apr 10, 2014 at 10:12:39AM +0000, Gupta, Sourab wrote:
> > On Wed, 2014-04-09 at 13:06 +0000, Daniel Vetter wrote:
> > > On Tue, Apr 08, 2014 at 06:53:03AM +0000, Gupta, Sourab wrote:
> > > > On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote:
> > > > > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote:
> > > > > > Hi Rodrigo,
> > > > > > In this patch, while freeing the purgeable stolen object, the memory
> > > > > > node also has to be freed, so as to make space for new object. We need
> > > > > > to call drm_mm_remove_node while freeing obj.
> > > > > > 
> > > > > > The below modification patch was floated earlier for this purpose:
> > > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html
> > > > > 
> > > > > Right, I have a v2 locally with the fix you identified.
> > > > > -Chris
> > > > > 
> > > > Ok, Thanks Chris.
> > > 
> > > I'd really prefer if someone would pick up all the
> > > stolen/create2_ioctl/whatever patches, pack them up into a polished
> > > series, add the testcases and submit this all for review and merging.
> > > 
> > > Otherwise this will linger forever and we'll get nowhere. Chris seems
> > > swamped with other stuff, so Sourab could you please take a look at this?
> > > 
> > > Please check with your manager that you have sufficient bandwidth to pull
> > > this through.
> > > 
> > 
> > I'll be on vacation from next week, so I'll be able to gauge this better
> > after coming back.
> > Nevertheless, I have some questions regarding the expectation of
> > userspace code changes required for these patches (i.e. libdrm changes
> > and igt testcases)
> > 
> > 1) For libdrm , I am assuming, a counterpart of
> > drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and
> > take in the parameters needed. 
> > Should the caching of objects from libdrm need to take care of both the
> > placement domains seperately (as in different sets of bo buckets)?
> > Should libdrm be transparent to all the combinations of different
> > parameters being passed by user or should the prohibited combinations be
> > disallowed from libdrm side?
> 
> I'm not sure whether we need a cache implemented in libdrm. Since stolen
> objects are fairly special it's probably easier to just have a simple
> linear cache tailor-made in the respective UMD. So just exposing
> create2_ioctl should be good enough.
> 
> > 2) For the igt, since we have a lot of parameters exposed to user, the
> > number of subtests required may be huge and still they may not test out
> > everything. 
> > So, Is the expectation here to have exhaustive test cases for all the
> > parameters (tiling/cache/domain/madvise/offset etc.) going in as input
> > to the create2 ioctl?
> > For eg. let us say we are going to check the render copy operation of
> > src and dest bo's. Do we need to provide all possible combinations of
> > different (create2 ioctl) input parameters to these src and dest bo's
> > and then run the render copy test for all these combinations.
> > Any guiding pointers from your side as to how we may go about the igt
> > testcases?
> 
> At a high-level there's two parts for igt tests:
> - First is functional tests, where we try to make sure that the feature
>   actually works. I.e. allocate some stolen memory and then do something
>   with it, making sure that data access for the gpu and similar things
>   work. For this we just want some reasonable base coverage so that when
>   we hit a bug somewhere it's easy to extend the testcase to cover that
>   bug with a specific subtest.
> 
> - Then there's interface testing. kernel/userspace is a trust barrier, so
>   we need to make sure that evil userspace can't make the kernel crash
>   with some crazy invalid combination of flags or operations (like create
>   a stolen object and then try to mmap it). Since this is security
>   relevant and also since we can't ever change established userspace ABI I
>   want full coverage of all cases. But this is just about detecting abuse
>   correctly, no functional tests here.
> 
> For details see my two blog posts on the topic:
> 
> http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
> 
> http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html
> 
> Cheers, Daniel

Thanks Daniel,
We'll take care of the above points for libdrm changes and igt.

Regards,
Sourab