[PATCH v2 1/2] drm/i915: Introduce GEM proxy

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On
> Behalf Of Joonas Lahtinen
> Sent: Tuesday, November 7, 2017 9:06 PM
> To: Zhang, Tina <tina.zhang at intel.com>; zhenyuw at linux.intel.com; Wang, Zhi
> A <zhi.a.wang at intel.com>; daniel at ffwll.ch; chris at chris-wilson.co.uk
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>; intel-gfx at lists.freedesktop.org;
> intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [PATCH v2 1/2] drm/i915: Introduce GEM proxy
> 
> On Tue, 2017-11-07 at 04:53 +0000, Zhang, Tina wrote:
> > > -----Original Message-----
> > > From: intel-gvt-dev
> > > [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On Behalf Of
> > > Joonas Lahtinen
> > > Sent: Monday, November 6, 2017 7:24 PM
> > > To: Zhang, Tina <tina.zhang at intel.com>; zhenyuw at linux.intel.com;
> > > Wang, Zhi A <zhi.a.wang at intel.com>; daniel at ffwll.ch;
> > > chris at chris-wilson.co.uk
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>;
> > > intel-gfx at lists.freedesktop.org; intel-gvt-dev at lists.freedesktop.org
> > > Subject: Re: [PATCH v2 1/2] drm/i915: Introduce GEM proxy
> > >
> > > On Wed, 2017-11-01 at 17:22 +0800, Tina Zhang wrote:
> > > > GEM proxy is a kind of GEM, whose backing physical memory is
> > > > pinned and produced by guest VM and is used by host as read only.
> > > > With GEM proxy, host is able to access guest physical memory
> > > > through GEM object interface. As GEM proxy is such a special kind
> > > > of GEM, a new flag I915_GEM_OBJECT_IS_PROXY is introduced to ban
> > > > host from changing the backing storage of GEM proxy.
> > > >
> > > > v2:
> > > > - return -ENXIO when pin and map pages of GEM proxy to kernel space.
> > > >   (Chris)
> > > >
> > > > Here are the histories of this patch in "Dma-buf support for Gvt-g"
> > > > patch-set:
> > > >
> > > > v14:
> > > > - return -ENXIO when gem proxy object is banned by ioctl.
> > > >   (Chris) (Daniel)
> > > >
> > > > v13:
> > > > - add comments to GEM proxy. (Chris)
> > > > - don't ban GEM proxy in i915_gem_sw_finish_ioctl. (Chris)
> > > > - check GEM proxy bar after finishing i915_gem_object_wait.
> > > > (Chris)
> > > > - remove GEM proxy bar in i915_gem_madvise_ioctl.
> > > >
> > > > v6:
> > > > - add gem proxy barrier in the following ioctls. (Chris)
> > > >   i915_gem_set_caching_ioctl
> > > >   i915_gem_set_domain_ioctl
> > > >   i915_gem_sw_finish_ioctl
> > > >   i915_gem_set_tiling_ioctl
> > > >   i915_gem_madvise_ioctl
> > > >
> > > > Signed-off-by: Tina Zhang <tina.zhang at intel.com>
> > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > > Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > >
> > > <SNIP>
> > >
> > > > @@ -1649,6 +1659,10 @@ i915_gem_sw_finish_ioctl(struct drm_device
> > >
> > > *dev, void *data,
> > > >  	if (!obj)
> > > >  		return -ENOENT;
> > > >
> > > > +	/* Proxy objects are barred from CPU access, so there is no
> > > > +	 * need to ban sw_finish as it is a nop.
> > > > +	 */
> > > > +
> > > >  	/* Pinned buffers may be scanout, so flush the cache */
> > > >  	i915_gem_object_flush_if_display(obj);
> > > >  	i915_gem_object_put(obj);
> > > > @@ -2614,7 +2628,8 @@ void *i915_gem_object_pin_map(struct
> > >
> > > drm_i915_gem_object *obj,
> > > >  	void *ptr;
> > > >  	int ret;
> > > >
> > > > -	GEM_BUG_ON(!i915_gem_object_has_struct_page(obj));
> > > > +	if (unlikely(!i915_gem_object_has_struct_page(obj)))
> > > > +		return ERR_PTR(-ENODEV);
> > >
> > > You should have marked this change in the changelog and then marked
> > > the Reviewed-by tags to be valid only to the previous version of this patch.
> > >
> > > It's not a fair game to claim a patch to be "Reviewed-by" at the
> > > current version, when you've made changes that were not agreed upon.
> >
> > I thought we were agreed on this :)
> >
> > >
> > > So that's some meta-review. Back to the actual review;
> > >
> > > Which codepath was hitting the GEM_BUG_ON? Wondering if it would be
> > > cleaner to avoid the call to this function on that single codepath.
> >
> > Here is the previously comments:
> > https://lists.freedesktop.org/archives/intel-gvt-dev/2017-October/0022
> > 78.html
> > Thanks.
> 
> I never even noticed such an e-mail, so the correct response would've been;
> 
> Reviewed-by: Joonas #vX
> Reviewed-by: Chris
> 
> Where #vX is the version I actually agreed to.
> 
> Reviewed-by tags are are ones you need to be especially careful about in
> addition to the Signed-off-bys because they carry special meaning:
> 
> https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html#r
> eviewer-s-statement-of-oversight
Indeed, thank you :)

BR,
Tina
> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev