From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 13/18] drm/i915/context: create & destroy ioctls Date: Fri, 30 Mar 2012 21:16:52 +0200 Message-ID: <20120330191652.GC23526@phenom.ffwll.local> References: <1332103198-25852-1-git-send-email-ben@bwidawsk.net> <1332103198-25852-14-git-send-email-ben@bwidawsk.net> <20120329193535.GG27737@phenom.ffwll.local> <20120330115514.57519687@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-we0-f177.google.com (mail-we0-f177.google.com [74.125.82.177]) by gabe.freedesktop.org (Postfix) with ESMTP id A54AB9E7A6 for ; Fri, 30 Mar 2012 12:16:09 -0700 (PDT) Received: by werp11 with SMTP id p11so677496wer.36 for ; Fri, 30 Mar 2012 12:16:08 -0700 (PDT) In-Reply-To: <20120330115514.57519687@bwidawsk.net> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Mar 30, 2012 at 11:55:14AM -0700, Ben Widawsky wrote: > On Thu, 29 Mar 2012 21:35:35 +0200 > Daniel Vetter wrote: > > > On Sun, Mar 18, 2012 at 01:39:53PM -0700, Ben Widawsky wrote: > > > Add the interfaces to allow user space to create and destroy contexts. > > > Contexts are destroyed automatically if the file descriptor for the dri > > > device is closed. > > > > > > Signed-off-by: Ben Widawsky > > > --- > > > drivers/gpu/drm/i915/i915_dma.c | 2 + > > > drivers/gpu/drm/i915/i915_drv.h | 4 ++ > > > drivers/gpu/drm/i915/i915_gem_context.c | 64 +++++++++++++++++++++++++++++++ > > > include/drm/i915_drm.h | 16 ++++++++ > > > 4 files changed, 86 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > index 4c7c1dc..fb3fccb 100644 > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > @@ -2333,6 +2333,8 @@ struct drm_ioctl_desc i915_ioctls[] = { > > > DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), > > > DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), > > > DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, intel_sprite_get_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), > > > + DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED), > > > + DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED), > > > }; > > > > > > int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls); > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index c6c2ada..d49615e 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1312,6 +1312,10 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file); > > > int i915_switch_context(struct intel_ring_buffer *ring, > > > struct drm_file *file, > > > int to_id, u32 seqno, u32 flags); > > > +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, > > > + struct drm_file *file); > > > +int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, > > > + struct drm_file *file); > > > > > > /* i915_gem_gtt.c */ > > > int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev); > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > > index d9a08f2..accb3de 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > > @@ -411,3 +411,67 @@ out: > > > kref_put(&to->nref, destroy_hw_context); > > > return ret; > > > } > > > + > > > +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, > > > + struct drm_file *file) > > > +{ > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + struct drm_i915_gem_context_create *args = data; > > > + struct drm_i915_file_private *file_priv = file->driver_priv; > > > + struct intel_ring_buffer *ring = &dev_priv->ring[RCS]; > > > + struct i915_hw_context *ctx; > > > + int ret; > > > + > > > + if (dev_priv->hw_contexts_disabled) > > > + return -EPERM; > > > + > > > + ret = create_hw_context(dev, file_priv, &ctx); > > > > Note that the gem_alloc_object call in here is rather unsafe, due to the > > unsafe statistics-keeping in i915_gem_info_add_obj. Pre-existing looking > > goof-up, but can I maybe volunteer you to fix this? I think we need an > > mm stats spinlock because these counters could be rather big (and atomic_t > > is only 32 bits). > > I can do it. In this series? That bug is pre-existing since gem was merged afaik. So just when you're bored, in a separate series. > > > + if (ret) > > > + return ret; > > > + > > > + /* We need to do a special switch (save-only) either now, or before the > > > + * context is actually used in order to keep the context switch request > > > + * in execbuffer fairly simple. > > > + */ > > > + mutex_lock(&dev->struct_mutex); > > > > > + ret = i915_switch_context(ring, file, ctx->id, ring->get_seqno(ring), > > > + I915_CONTEXT_INITIAL_SWITCH); > > > > With the hw_context->is_intialized change you could ditch this (imo). > > You could ditch it, but I can't make an argument that one way is better > than another. As you stated earlier, you want me to ditch the spinlock > for the context id creation, so I'll still need to acquire struct mutex > anyway. At which point, I think it doesn't hurt to switch now too. > > I'm willing to change this is you can describe a benefit for it. Imo the benefit is that we have one (and just one) clear place where we switch context. Doing a funky switch at create time just feels wonky. Just now I've also noticed that you call ring->get_seqno(ring) - that reads the current seqno from the gpu hws and is absolutely not what you want. So you'd have to add another request (and attach it to the file_priv). I also like that with the is_initialized stored in the context, we nicely abstract away this little detail of how we need to frob the switch command on first use of a context. I don't feel strongly about this, but if you want to stick with explicit initialization, please wrap it all up in a little helper function to make it clear that we don't care about switching and only about initializing. Imo you should then still rip away the flags parameter from i915_switch_context - execbuffer really doesn't (and shouldn't) care about that detail. -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48