diff mbox

[1/3] drm/i915: Added a new 'I915_CPU_MAP_NOT_NEEDED' flag to gem_create ioctl.

Message ID 1394186367-20339-2-git-send-email-sourab.gupta@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

sourab.gupta@intel.com March 7, 2014, 9:59 a.m. UTC 
From: Sourab Gupta <sourab.gupta@intel.com>

Adding the flag 'I915_CPU_MAP_NOT_NEEDED' to gem_create ioctl.
This is to indicate the driver that direct cpu access to this
buffer object is not needed and hence Driver can opt to use Stolen
area as a backing store for it. When this flag is set, try to
allocate the objects from Stolen area. Fallback to shmem in case
of allocation failure.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   17 ++++++++++++-----
 include/uapi/drm/i915_drm.h     |    3 ++-
 2 files changed, 14 insertions(+), 6 deletions(-)

Comments

Chris Wilson March 7, 2014, 10:05 a.m. UTC | #1 
On Fri, Mar 07, 2014 at 03:29:25PM +0530, sourab.gupta@intel.com wrote:
> From: Sourab Gupta <sourab.gupta@intel.com>
> 
> Adding the flag 'I915_CPU_MAP_NOT_NEEDED' to gem_create ioctl.
> This is to indicate the driver that direct cpu access to this
> buffer object is not needed and hence Driver can opt to use Stolen
> area as a backing store for it. When this flag is set, try to
> allocate the objects from Stolen area. Fallback to shmem in case
> of allocation failure.

Besides the issue of
http://blog.ffwll.ch/2013/11/botching-up-ioctls.html, this has an
information leak that we are not clearing the stolen pages. Danny got
extremely fussy over that when I sent this code almost 9 months ago.
-Chris
sourab.gupta@intel.com March 7, 2014, 10:56 a.m. UTC | #2 
Hi Chris,

For the issue of clearing of stolen pages, we have a patch which does memset of stolen objs  after
allocation, before handing the objs back over to userspace. We can apply this patch and send it in next version.

Regards,
Sourab

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Friday, March 07, 2014 3:36 PM
To: Gupta, Sourab
Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Wilson, Chris; Goel, Akash
Subject: Re: [PATCH 1/3] drm/i915: Added a new 'I915_CPU_MAP_NOT_NEEDED' flag to gem_create ioctl.

On Fri, Mar 07, 2014 at 03:29:25PM +0530, sourab.gupta@intel.com wrote:
> From: Sourab Gupta <sourab.gupta@intel.com>
> 
> Adding the flag 'I915_CPU_MAP_NOT_NEEDED' to gem_create ioctl.
> This is to indicate the driver that direct cpu access to this buffer 
> object is not needed and hence Driver can opt to use Stolen area as a 
> backing store for it. When this flag is set, try to allocate the 
> objects from Stolen area. Fallback to shmem in case of allocation 
> failure.

Besides the issue of
http://blog.ffwll.ch/2013/11/botching-up-ioctls.html, this has an information leak that we are not clearing the stolen pages. Danny got extremely fussy over that when I sent this code almost 9 months ago.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre
sourab.gupta@intel.com March 10, 2014, 4:12 p.m. UTC | #3 
Hi Chris,

For the issue mentioned by you ( regarding botching up ioctls), we understand that this is related to the 
compatibility between the older/newer versions of driver/userspace.
In our old implementation, the 'pad' field was replaced with 'flags' in the ioctl structure. This would have 
led to the erroneous behavior when the new userspace is communicating with old driver.
 
So, we are planning to mitigate it using following approach:
1) Extend the ioctl structure with 'flags' field at the end of existing fields
2) Add a driver feature flag, so that newer userspace can figure out whether kernel supports the new 
   version of ioctl. Add checks in libdrm corresponding to the same. 
3) In the driver, check the passed in size for the ioctl structure and zero-extend any mismatch( which
     happens in case of older userspace calling newer kernel).
4) In case of newer userspace calling older driver (wherein feature is not supported), we have a few choices:
        a) The userspace will fallback to using the old ioctl structure. Driver will have the new version of the ioctl structure.
             Since the libdrm shared header files will also now have new ioctl structure definition, we'll have to keep an old 
             definition of the ioctl structure privately in order to send it in the ioctl.
        b) Or, we may keep the old ioctl structure as it is in the driver; and define a new structure in driver which will be used now.
              Then, libdrm will also use this new structure (if feature is supported) or old structure (feature not supported)
       c) Or, we may fail the call altogether, if the newer userspace is trying to communicate with older driver.
             We don't need to keep the two different version of ioctl structures in this case. But this may not be the desired behaviour

Can you provide your opinion regarding the above approaches.

Regards,
Sourab


-----Original Message-----
From: Gupta, Sourab 
Sent: Friday, March 07, 2014 4:26 PM
To: Chris Wilson
Cc: intel-gfx@lists.freedesktop.org; Wilson, Chris; Goel, Akash; daniel@ffwll.ch
Subject: RE: [PATCH 1/3] drm/i915: Added a new 'I915_CPU_MAP_NOT_NEEDED' flag to gem_create ioctl.

Hi Chris,

For the issue of clearing of stolen pages, we have a patch which does memset of stolen objs  after allocation, before handing the objs back over to userspace. We can apply this patch and send it in next version.

Regards,
Sourab

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
Sent: Friday, March 07, 2014 3:36 PM
To: Gupta, Sourab
Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Wilson, Chris; Goel, Akash
Subject: Re: [PATCH 1/3] drm/i915: Added a new 'I915_CPU_MAP_NOT_NEEDED' flag to gem_create ioctl.

On Fri, Mar 07, 2014 at 03:29:25PM +0530, sourab.gupta@intel.com wrote:
> From: Sourab Gupta <sourab.gupta@intel.com>
> 
> Adding the flag 'I915_CPU_MAP_NOT_NEEDED' to gem_create ioctl.
> This is to indicate the driver that direct cpu access to this buffer 
> object is not needed and hence Driver can opt to use Stolen area as a 
> backing store for it. When this flag is set, try to allocate the 
> objects from Stolen area. Fallback to shmem in case of allocation 
> failure.

Besides the issue of
http://blog.ffwll.ch/2013/11/botching-up-ioctls.html, this has an information leak that we are not clearing the stolen pages. Danny got extremely fussy over that when I sent this code almost 9 months ago.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre
Chris Wilson March 10, 2014, 10:07 p.m. UTC | #4 
On Mon, Mar 10, 2014 at 04:12:23PM +0000, Gupta, Sourab wrote:
> 
> Hi Chris,
> 
> For the issue mentioned by you ( regarding botching up ioctls), we understand that this is related to the 
> compatibility between the older/newer versions of driver/userspace.
> In our old implementation, the 'pad' field was replaced with 'flags' in the ioctl structure. This would have 
> led to the erroneous behavior when the new userspace is communicating with old driver.

You missed the important point that there is no guarrantee that current
userspace is not stuffing garbage into the pad field. It is.

You have to go with a new ioctl. So you may as well design one that
tries to fulfil today's varied needs for a constructor.  For example,
here's one I prepared earlier,
http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=create2&id=401fa740adcaf252d0149cdd63d5fdf5e3969907
-Chris
sourab.gupta@intel.com March 17, 2014, 9:44 a.m. UTC | #5 
On Mon, 2014-03-10 at 22:07 +0000, 'Chris Wilson' wrote:
> On Mon, Mar 10, 2014 at 04:12:23PM +0000, Gupta, Sourab wrote:
> > 
> > Hi Chris,
> > 
> > For the issue mentioned by you ( regarding botching up ioctls), we understand that this is related to the 
> > compatibility between the older/newer versions of driver/userspace.
> > In our old implementation, the 'pad' field was replaced with 'flags' in the ioctl structure. This would have 
> > led to the erroneous behavior when the new userspace is communicating with old driver.
> 
> You missed the important point that there is no guarrantee that current
> userspace is not stuffing garbage into the pad field. It is.
> 
> You have to go with a new ioctl. So you may as well design one that
> tries to fulfil today's varied needs for a constructor.  For example,
> here's one I prepared earlier,
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=create2&id=401fa740adcaf252d0149cdd63d5fdf5e3969907
> -Chris
> 

Hi Chris,

We went through this patch and this fulfills the requirements from the
stolen mem perspective, and this looks like a replacement of the first
patch we have submitted.
This patch seems to be in your private branch. When do you plan to merge
this patch with drm-intel branch?
Have you also prepared the corresponding libdrm side patches? If so, can
you share those also. Actually, the igt patch which we submitted earlier
would also need to be modified according to the libdrm changes.

Also,
When do you plan to merge the second and third patch in the series
(truncation of stolen mem objs) as they seem to be fine to us.

Regards,
Sourab
sourab.gupta@intel.com March 21, 2014, 1:39 p.m. UTC | #6 
On Mon, 2014-03-17 at 15:15 +0530, sourab gupta wrote:
> On Mon, 2014-03-10 at 22:07 +0000, 'Chris Wilson' wrote:
> > On Mon, Mar 10, 2014 at 04:12:23PM +0000, Gupta, Sourab wrote:
> > > 
> > > Hi Chris,
> > > 
> > > For the issue mentioned by you ( regarding botching up ioctls), we understand that this is related to the 
> > > compatibility between the older/newer versions of driver/userspace.
> > > In our old implementation, the 'pad' field was replaced with 'flags' in the ioctl structure. This would have 
> > > led to the erroneous behavior when the new userspace is communicating with old driver.
> > 
> > You missed the important point that there is no guarrantee that current
> > userspace is not stuffing garbage into the pad field. It is.
> > 
> > You have to go with a new ioctl. So you may as well design one that
> > tries to fulfil today's varied needs for a constructor.  For example,
> > here's one I prepared earlier,
> > http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=create2&id=401fa740adcaf252d0149cdd63d5fdf5e3969907
> > -Chris
> > 
> 
> Hi Chris,
> 
> We went through this patch and this fulfills the requirements from the
> stolen mem perspective, and this looks like a replacement of the first
> patch we have submitted.
> This patch seems to be in your private branch. When do you plan to merge
> this patch with drm-intel branch?
> Have you also prepared the corresponding libdrm side patches? If so, can
> you share those also. Actually, the igt patch which we submitted earlier
> would also need to be modified according to the libdrm changes.
> 
> Also,
> When do you plan to merge the second and third patch in the series
> (truncation of stolen mem objs) as they seem to be fine to us.
> 
> Regards,
> Sourab
> 

Hi Chris,
We came across an earlier patch submitted by you regarding the ioctl for
user specified placement.
(http://lists.freedesktop.org/archives/intel-gfx/2013-July/029843.html)

As we understand, there were issues regarding the dma-buf support
regarding this patch.
Can you please tell us, whether it is planned to be taken up at a later
date (with/without dma-buf support).

Also, if the zeroing out of memory is required, we can provide an
additional patch on top of our earlier patches in order to clear the
memory gotten from stolen mem this way.

Regards,
Sourab
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 177c207..22b531a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -225,9 +225,11 @@  static int
 i915_gem_create(struct drm_file *file,
 		struct drm_device *dev,
 		uint64_t size,
-		uint32_t *handle_p)
+		uint32_t *handle_p,
+		uint32_t flags)
+
 {
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *obj=NULL;
 	int ret;
 	u32 handle;
 
@@ -236,7 +238,12 @@  i915_gem_create(struct drm_file *file,
 		return -EINVAL;
 
 	/* Allocate the new object */
-	obj = i915_gem_alloc_object(dev, size);
+	if (flags & I915_CPU_MAP_NOT_NEEDED)
+		obj = i915_gem_object_create_stolen(dev, size);
+
+	if (obj == NULL)
+		obj = i915_gem_alloc_object(dev, size);
+
 	if (obj == NULL)
 		return -ENOMEM;
 
@@ -259,7 +266,7 @@  i915_gem_dumb_create(struct drm_file *file,
 	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 64);
 	args->size = args->pitch * args->height;
 	return i915_gem_create(file, dev,
-			       args->size, &args->handle);
+			       args->size, &args->handle, args->flags);
 }
 
 /**
@@ -272,7 +279,7 @@  i915_gem_create_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_gem_create *args = data;
 
 	return i915_gem_create(file, dev,
-			       args->size, &args->handle);
+			       args->size, &args->handle, args->flags);
 }
 
 static inline int
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 126bfaa..8625f54 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -431,7 +431,8 @@  struct drm_i915_gem_create {
 	 * Object handles are nonzero.
 	 */
 	__u32 handle;
-	__u32 pad;
+	__u32 flags;
+#define I915_CPU_MAP_NOT_NEEDED 0x1
 };
 
 struct drm_i915_gem_pread {