> -----Original Message----- > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@xxxxxxxxxxxxxxxxxxxxx] On > Behalf Of Joonas Lahtinen > Sent: Tuesday, November 7, 2017 9:06 PM > To: Zhang, Tina <tina.zhang@xxxxxxxxx>; zhenyuw@xxxxxxxxxxxxxxx; Wang, Zhi > A <zhi.a.wang@xxxxxxxxx>; daniel@xxxxxxxx; chris@xxxxxxxxxxxxxxxxxx > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; > intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of > > > Joonas Lahtinen > > > Sent: Monday, November 6, 2017 7:24 PM > > > To: Zhang, Tina <tina.zhang@xxxxxxxxx>; zhenyuw@xxxxxxxxxxxxxxx; > > > Wang, Zhi A <zhi.a.wang@xxxxxxxxx>; daniel@xxxxxxxx; > > > chris@xxxxxxxxxxxxxxxxxx > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>; > > > intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx > > > 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@xxxxxxxxx> > > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > > > > Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > > > > > > <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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
