Re: [Intel-gfx] [PATCH] drm/modes: Prevent division by zero htotal

[Zhang, Tina]

> -Original Message-
> From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, January 23, 2019 6:56 PM
> To: Zhang, Tina 
> Cc: intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; Adam
> Jackson ; Dave Airlie ; Daniel Vetter
> 
> Subject: Re: [PATCH] drm/modes: Prevent division by zero htotal
> 
> On Wed, Jan 23, 2019 at 03:28:59PM +0800, Tina Zhang wrote:
> > This patch prevents division by zero htotal.
> 
> How did you manage to get here with htotal == 0? This needs backtraces (or if
> this is just about static checkers, a mention of that).
> -Daniel

In GVT-g, we are trying to enable a virtual display w/o setting timings for a 
pipe
(a.k.a htotal=0), then we met the following kernel panic:

[   32.832048] divide error:  [#1] SMP PTI
[   32.833614] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc4-sriov+ #33
[   32.834438] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
rel-1.10.1-0-g8891697-dirty-20180511_165818-tinazhang-linux-1 04/01/2014
[   32.835901] RIP: 0010:drm_mode_hsync+0x1e/0x40
[   32.836004] Code: 31 c0 c3 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 8b 87 
d8 00 00 00 85 c0 75 22 8b 4f 68 85 c9 78 1b 69 47 58 e8 03 00 00 99  f9 b9 
d3 4d 62 10 05 f4 01 00 00 f7 e1 89 d0 c1 e8 06 f3 c3 66
[   32.836004] RSP: :c90ebb90 EFLAGS: 00010206
[   32.836004] RAX:  RBX: 88001c67c8a0 RCX: 
[   32.836004] RDX:  RSI: 88001c67c000 RDI: 88001c67c8a0
[   32.836004] RBP: 88001c7d03a0 R08: 88001c67c8a0 R09: 88001c7d0330
[   32.836004] R10: 822c3a98 R11: 0001 R12: 88001c67c000
[   32.836004] R13: 88001c7d0370 R14: 8207eb78 R15: 88001c67c800
[   32.836004] FS:  () GS:88001da0() 
knlGS:
[   32.836004] CS:  0010 DS:  ES:  CR0: 80050033
[   32.836004] CR2:  CR3: 0220a000 CR4: 06f0
[   32.836004] DR0:  DR1:  DR2: 
[   32.836004] DR3:  DR6: fffe0ff0 DR7: 0400
[   32.836004] Call Trace:
[   32.836004]  intel_mode_from_pipe_config+0x72/0x90
[   32.836004]  intel_modeset_setup_hw_state+0x569/0xf90
[   32.836004]  intel_modeset_init+0x905/0x1db0
[   32.836004]  i915_driver_load+0xb8c/0x1120
[   32.836004]  i915_pci_probe+0x4d/0xb0
[   32.836004]  local_pci_probe+0x44/0xa0
[   32.836004]  ? pci_assign_irq+0x27/0x130
[   32.836004]  pci_device_probe+0x102/0x1c0
[   32.836004]  driver_probe_device+0x2b8/0x480
[   32.836004]  __driver_attach+0x109/0x110
[   32.836004]  ? driver_probe_device+0x480/0x480
[   32.836004]  bus_for_each_dev+0x67/0xc0
[   32.836004]  ? klist_add_tail+0x3b/0x70
[   32.836004]  bus_add_driver+0x1e8/0x260
[   32.836004]  driver_register+0x5b/0xe0
[   32.836004]  ? mipi_dsi_bus_init+0x11/0x11
[   32.836004]  do_one_initcall+0x4d/0x1eb
[   32.836004]  kernel_init_freeable+0x197/0x237
[   32.836004]  ? rest_init+0xd0/0xd0
[   32.836004]  kernel_init+0xa/0x110
[   32.836004]  ret_from_fork+0x35/0x40
[   32.836004] Modules linked in:
[   32.859183] ---[ end trace 525608b0ed0e8665 ]---
[   32.859722] RIP: 0010:drm_mode_hsync+0x1e/0x40
[   32.860287] Code: 31 c0 c3 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 8b 87 
d8 00 00 00 85 c0 75 22 8b 4f 68 85 c9 78 1b 69 47 58 e8 03 00 00 99  f9 b9 
d3 4d 62 10 05 f4 01 00 00 f7 e1 89 d0 c1 e8 06 f3 c3 66
[   32.862680] RSP: :c90ebb90 EFLAGS: 00010206
[   32.863309] RAX:  RBX: 88001c67c8a0 RCX: 
[   32.864182] RDX:  RSI: 88001c67c000 RDI: 88001c67c8a0
[   32.865206] RBP: 88001c7d03a0 R08: 88001c67c8a0 R09: 88001c7d0330
[   32.866359] R10: 822c3a98 R11: 0001 R12: 88001c67c000
[   32.867213] R13: 88001c7d0370 R14: 8207eb78 R15: 88001c67c800
[   32.868075] FS:  () GS:88001da0() 
knlGS:
[   32.868983] CS:  0010 DS:  ES:  CR0: 80050033
[   32.869659] CR2:  CR3: 0220a000 CR4: 06f0
[   32.870599] DR0:  DR1:  DR2: 
[   32.871598] DR3:  DR6: fffe0ff0 DR7: 0400
[   32.872549] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x000b

Since drm_mode_hsync() has the logic to check mode->htotal, I just extend it to 
cover the case htotal==0.

Thanks.

BR,
Tina
> 
> >
> > Signed-off-by: Tina Zhang 
> > Cc: Adam Jackson 
> > Cc: Dave Airlie 
> > Cc: Daniel Vetter 
> > ---
> >  drivers/gpu/drm/drm_modes.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index adce9a2

Re: [Intel-gfx] Local Display Direct Flip Feature Discussion

[Zhang, Tina]

+ intel-gfx and dri-devel

Hi,

This is the Gvt-g local display direct flip discussion. And please reference 
here 
(https://lists.freedesktop.org/archives/intel-gvt-dev/2018-December/004594.html)
 to see what the feature is.

Mostly, with Gvt-g local display direct flip feature, vGPU can directly post 
its framebuffer to the assigned local HW display plane w/o host userspace 
involvement. We propose to use drm_framebuffer to stand for the framebuffer 
attached on a vGPU's plane. Since GVT-g can guarantee this drm_framebuffer is 
updated with the latest info of the framebuffer attached on the vGPU's plane 
during guest page flip, atomic async plane update mechanism is used in i915 to 
help updating the vGPU assigned HW planes efficiently with those special 
drm_framebuffers. 

Host userspace is in charge of the HW plane assignment. And currently we are 
discussing about:
1) Whether the drm_framebuffer can be generic enough to deal with the direct 
flip use cases?
 In our case, the drm_framebuffer has no gem backends, as the display 
memory management is handled by guest.
2) How to expose the drm_framebuffer to the host userspace?
 a) Through vfio/display ioctl
The kernel implementation is easy. But this might turn qemu into a 
display manager. Qemu has to handle all the KMS mode-setting stuff things.
 b) Through drm/i915 ioctl
This needs some code change in kernel space to deal with this ioctl 
between i915 and GVT-g. But the good thing is this ioctl can be used by the 
userspace display manager w/o vfio/display's help.

Thanks.

> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Thursday, December 20, 2018 7:34 PM
> To: Zhang, Tina 
> Cc: Tian, Kevin ; Wang, Zhenyu Z
> ; Wang, Zhi A ; He, Min
> ; Yuan, Hang ; Alex Williamson
> ; Lv, Zhiyuan ; Vetter,
> Daniel ; intel-gvt-dev  d...@lists.freedesktop.org>; Wang, Hongbo 
> Subject: Re: Local Display Direct Flip Feature Discussion
> 
> On Thu, Dec 20, 2018 at 08:45:09AM +, Zhang, Tina wrote:
> >
> >
> > > -Original Message-
> > > From: intel-gvt-dev
> > > [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On Behalf Of
> > > Gerd Hoffmann
> > > Sent: Wednesday, December 19, 2018 7:26 PM
> > > To: Zhang, Tina 
> > > Cc: Tian, Kevin ; Wang, Zhenyu Z
> > > ; Wang, Zhi A ; He,
> > > Min ; Yuan, Hang ; Alex
> > > Williamson ; Lv, Zhiyuan
> > > ; Vetter, Daniel ;
> > > intel-gvt-dev ; Wang, Hongbo
> > > 
> > > Subject: Re: Local Display Direct Flip Feature Discussion
> > >
> > >   Hi,
> > >
> > > > > Isn't a framebuffer just a gem object with metadata (fourcc,
> > > > > width, height, stride, ...) attached?  So I'm wondering how that
> > > > > works in detail.  What happens on page-flip?  Do you make the
> > > > > framebuffer reference another gem object then?  Or do you blit
> > > > > the guest display to the framebuffer?
> > > > The special DRM framebuffer is transparent to the guest display driver.
> > > > We just want to use this object to save the guest framebuffer info
> > > > in host-side.
> > >
> > > Hmm, so this object isn't a normal drm framebuffer.  You are just
> > > masquerading it as drm framebuffer, so you can assign it to a drm
> > > crtc or drm plane.
> > Not so bad actually :-)
> > Host just wants to use a drm framebuffer to describe the drm
> > framebuffer attached on a vGPU's plane. And the only special thing is
> > that this host drm frambuffer has on gem backends, which means host
> > cannot manage the
> 
> ... has no gem ... ?
> 
> > memory of this drm framebuffer. And it's OK, as the guest does the
> management.
> > Besides, generic drm framebuffer was designed to be able to deal with
> > this kind of situation.
> 
> Hmm, ok.  Never dealed with framebuffers not backed by gem objects so far,
> seems to not be very common too.  But the doc comments in
> include/drm/drm_framebuffer.h suggest it is fine indeed.
> 
> What is the plan for hardware cursor support?  Support two framebuffers, for
> primary and cursor?
We got the hardware cursor support in our to-go list: 
https://lists.freedesktop.org/archives/intel-gvt-dev/2018-December/004559.html

We will support both primary and cursor.

> 
> > > It makes sense to allow mapping guest outputs to host outputs.  I
> > > think it is more useful to handle that at crtc level not plane
> > > level, so it'll work for both primary and cursor plane.  I think it
> > > would be cleaner to introduce new drm (generic or i915) APIs for
> > > that instead of

Re: [Intel-gfx] [RFC PATCH 2/7] drm/i915/gvt: Use meta fbs to stand for vGPU's planes

[Zhang, Tina]

> -Original Message-
> From: Zhenyu Wang [mailto:zhen...@linux.intel.com]
> Sent: Monday, December 3, 2018 4:36 PM
> To: Zhang, Tina 
> Cc: intel-gfx@lists.freedesktop.org; Kondapally, Kalyan
> ; intel-gvt-...@lists.freedesktop.org; Wang, Zhi
> A 
> Subject: Re: [RFC PATCH 2/7] drm/i915/gvt: Use meta fbs to stand for vGPU's
> planes
> 
> On 2018.12.03 16:21:04 +0800, Zhenyu Wang wrote:
> > On 2018.12.03 15:35:17 +0800, Tina Zhang wrote:
> > > Create and initialize vGPU meta framebuffers during vGPU's creation.
> > > Each meta framebuffer is an intel_framebuffer. Userspace can get the
> > > userspace visible identifier of that meta framebuffer by accessing
> > > plane_id_index attribute.
> > >
> > > For example:
> > > In "/sys/bus/pci/devices/\:00\:02.0/$vGPU_id/intel_vgpu/"
> > > directory,
> > >
> > >/* plane_id_index: pipe#(bit16:8) and plane#(bit7:0)*/
> > >echo "0x10" > plane_index_id //Set the index to the plane 0 of
> > > pipe 1
> > >
> > >/*
> > > * Dump userspace visible identifier of the meta framebuffer
> > > * standing for the primary plane of the vGPU's pipe one
> > > */
> > >cat plane_index_id //dump the id for plane 0 of pipe 1
> > >
> > > Then userspace can use this id with the exsting KMS IOCTL, e.g.
> > > drmModeSetPlane, to assign a physical plane to this virtual plane.
> >
> > How does user know which plane/pipe is active for vGPU? Looks from
> > current code it has no way to know that. I think for guest display
> > assignment, we need to know the display state of vGPU and it looks
> > there's no state tracking, e.g what if guest driver switches to
> > another plane for display? How could user know that then adjust for
> > assignment?
> >
> > And really don't like sysfs to r/w file for id, even if doesn't use
> > vfio gfx dmabuf interface for guest display object presentation, a
> > vendor specific vfio device ioctl if possible is better.
> >
> 
> And add that this device specific info sysfs is created when mdev device 
> created
> before it's opened for vGPU instance. So I don't think it's proper place to 
> put
> display config info.
It's actually one of the intel_vgpu attributes. And it's created during vGPU 
being created.
Thanks.

BR,
Tina
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH 2/7] drm/i915/gvt: Use meta fbs to stand for vGPU's planes

[Zhang, Tina]

> -Original Message-
> From: Zhang, Tina
> Sent: Monday, December 3, 2018 4:53 PM
> To: 'Zhenyu Wang' 
> Cc: intel-gfx@lists.freedesktop.org; Kondapally, Kalyan
> ; intel-gvt-...@lists.freedesktop.org; Wang, Zhi
> A 
> Subject: RE: [RFC PATCH 2/7] drm/i915/gvt: Use meta fbs to stand for vGPU's
> planes
> 
> 
> 
> > -Original Message-
> > From: Zhenyu Wang [mailto:zhen...@linux.intel.com]
> > Sent: Monday, December 3, 2018 4:21 PM
> > To: Zhang, Tina 
> > Cc: intel-gfx@lists.freedesktop.org; Kondapally, Kalyan
> > ; intel-gvt-...@lists.freedesktop.org;
> > Wang, Zhi A 
> > Subject: Re: [RFC PATCH 2/7] drm/i915/gvt: Use meta fbs to stand for
> > vGPU's planes
> >
> > On 2018.12.03 15:35:17 +0800, Tina Zhang wrote:
> > > Create and initialize vGPU meta framebuffers during vGPU's creation.
> > > Each meta framebuffer is an intel_framebuffer. Userspace can get the
> > > userspace visible identifier of that meta framebuffer by accessing
> > > plane_id_index attribute.
> > >
> > > For example:
> > > In "/sys/bus/pci/devices/\:00\:02.0/$vGPU_id/intel_vgpu/"
> > > directory,
> > >
> > >/* plane_id_index: pipe#(bit16:8) and plane#(bit7:0)*/
> > >echo "0x10" > plane_index_id //Set the index to the plane 0 of
> > > pipe
> > > 1
> > >
> > >/*
> > > * Dump userspace visible identifier of the meta framebuffer
> > > * standing for the primary plane of the vGPU's pipe one
> > > */
> > >cat plane_index_id //dump the id for plane 0 of pipe 1
> > >
> > > Then userspace can use this id with the exsting KMS IOCTL, e.g.
> > > drmModeSetPlane, to assign a physical plane to this virtual plane.
> >
> > How does user know which plane/pipe is active for vGPU? Looks from
> > current code it has no way to know that. I think for guest display
> > assignment, we need to know the display state of vGPU and it looks
> > there's no state tracking, e.g what if guest driver switches to
> > another plane for display? How could user know that then adjust for
> assignment?
> So far, GVT-g has supported multi-heads. In another word, there is only one
> priramy plane for each vGPU.
The "has" here was actually "hasn't". Sorry for this typo.

BR,
Tina
> 
> GVT-g only provides the EDID for one fixed display port and in the guest 
> point of
> view, there is only one pipe having a connected monitor, a.k.a only the planes
> on one pipe can be used by guest OS.
> 
> >
> > And really don't like sysfs to r/w file for id, even if doesn't use
> > vfio gfx dmabuf interface for guest display object presentation, a
> > vendor specific vfio device ioctl if possible is better.
> Yeah, we could consider about that.
> Thanks.
> 
> BR,
> Tina
> >
> > >
> > > Signed-off-by: Tina Zhang 
> > > Cc: Zhenyu Wang 
> > > Cc: Zhi Wang 
> > > ---
> > >  drivers/gpu/drm/i915/gvt/display.c | 150
> > +
> > >  drivers/gpu/drm/i915/gvt/gvt.h |  16 
> > >  drivers/gpu/drm/i915/gvt/kvmgt.c   |  46 
> > >  3 files changed, 212 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/display.c
> > > b/drivers/gpu/drm/i915/gvt/display.c
> > > index df1e141..a9176a1 100644
> > > --- a/drivers/gpu/drm/i915/gvt/display.c
> > > +++ b/drivers/gpu/drm/i915/gvt/display.c
> > > @@ -442,6 +442,152 @@ void intel_gvt_emulate_vblank(struct intel_gvt
> > *gvt)
> > >   mutex_unlock(>lock);
> > >  }
> > >
> > > +struct intel_vgpu_fb_meta_data {
> > > + u32 vgpu_plane_id; /*
> > vgpu_id(23:16):vgpu_pipe(15:8):vgpu_plane(7:0)*/
> > > + struct intel_vgpu *vgpu; // the vGPU this meta_fb belongs to };
> > > +
> > > +static void meta_fb_destroy(struct drm_framebuffer *fb) {
> > > + struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > > +
> > > + if (intel_fb->meta_fb.type_id != INTEL_META_FB_VGPU)
> > > + return;
> > > +
> > > + kfree(intel_fb->meta_fb.private);
> > > + intel_fb->meta_fb.private = NULL;
> > > +
> > > + drm_framebuffer_cleanup(fb);
> > > + kfree(intel_fb);
> > > +}
> > > +
> > > +static void clean_meta_fb(struct intel_vgpu *vgpu) {
> > > + enum pipe pipe;
> > > + enum plane_id plane_id;
> > > + struct intel_framebuffer *intel_fb;

Re: [Intel-gfx] [RFC PATCH 2/7] drm/i915/gvt: Use meta fbs to stand for vGPU's planes

[Zhang, Tina]

> -Original Message-
> From: Zhenyu Wang [mailto:zhen...@linux.intel.com]
> Sent: Monday, December 3, 2018 4:21 PM
> To: Zhang, Tina 
> Cc: intel-gfx@lists.freedesktop.org; Kondapally, Kalyan
> ; intel-gvt-...@lists.freedesktop.org; Wang, Zhi
> A 
> Subject: Re: [RFC PATCH 2/7] drm/i915/gvt: Use meta fbs to stand for vGPU's
> planes
> 
> On 2018.12.03 15:35:17 +0800, Tina Zhang wrote:
> > Create and initialize vGPU meta framebuffers during vGPU's creation.
> > Each meta framebuffer is an intel_framebuffer. Userspace can get the
> > userspace visible identifier of that meta framebuffer by accessing
> > plane_id_index attribute.
> >
> > For example:
> > In "/sys/bus/pci/devices/\:00\:02.0/$vGPU_id/intel_vgpu/"
> > directory,
> >
> >/* plane_id_index: pipe#(bit16:8) and plane#(bit7:0)*/
> >echo "0x10" > plane_index_id //Set the index to the plane 0 of pipe
> > 1
> >
> >/*
> > * Dump userspace visible identifier of the meta framebuffer
> > * standing for the primary plane of the vGPU's pipe one
> > */
> >cat plane_index_id //dump the id for plane 0 of pipe 1
> >
> > Then userspace can use this id with the exsting KMS IOCTL, e.g.
> > drmModeSetPlane, to assign a physical plane to this virtual plane.
> 
> How does user know which plane/pipe is active for vGPU? Looks from current
> code it has no way to know that. I think for guest display assignment, we need
> to know the display state of vGPU and it looks there's no state tracking, e.g 
> what
> if guest driver switches to another plane for display? How could user know 
> that
> then adjust for assignment?
So far, GVT-g has supported multi-heads. In another word, there is only one 
priramy
plane for each vGPU.

GVT-g only provides the EDID for one fixed display port and in the guest point
of view, there is only one pipe having a connected monitor, a.k.a only the 
planes on
one pipe can be used by guest OS.

> 
> And really don't like sysfs to r/w file for id, even if doesn't use vfio gfx 
> dmabuf
> interface for guest display object presentation, a vendor specific vfio 
> device ioctl
> if possible is better.
Yeah, we could consider about that.
Thanks.

BR,
Tina
> 
> >
> > Signed-off-by: Tina Zhang 
> > Cc: Zhenyu Wang 
> > Cc: Zhi Wang 
> > ---
> >  drivers/gpu/drm/i915/gvt/display.c | 150
> +
> >  drivers/gpu/drm/i915/gvt/gvt.h |  16 
> >  drivers/gpu/drm/i915/gvt/kvmgt.c   |  46 
> >  3 files changed, 212 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/display.c
> > b/drivers/gpu/drm/i915/gvt/display.c
> > index df1e141..a9176a1 100644
> > --- a/drivers/gpu/drm/i915/gvt/display.c
> > +++ b/drivers/gpu/drm/i915/gvt/display.c
> > @@ -442,6 +442,152 @@ void intel_gvt_emulate_vblank(struct intel_gvt
> *gvt)
> > mutex_unlock(>lock);
> >  }
> >
> > +struct intel_vgpu_fb_meta_data {
> > +   u32 vgpu_plane_id; /*
> vgpu_id(23:16):vgpu_pipe(15:8):vgpu_plane(7:0)*/
> > +   struct intel_vgpu *vgpu; // the vGPU this meta_fb belongs to };
> > +
> > +static void meta_fb_destroy(struct drm_framebuffer *fb) {
> > +   struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > +
> > +   if (intel_fb->meta_fb.type_id != INTEL_META_FB_VGPU)
> > +   return;
> > +
> > +   kfree(intel_fb->meta_fb.private);
> > +   intel_fb->meta_fb.private = NULL;
> > +
> > +   drm_framebuffer_cleanup(fb);
> > +   kfree(intel_fb);
> > +}
> > +
> > +static void clean_meta_fb(struct intel_vgpu *vgpu) {
> > +   enum pipe pipe;
> > +   enum plane_id plane_id;
> > +   struct intel_framebuffer *intel_fb;
> > +
> > +   for (pipe = 0; pipe < I915_MAX_PIPES; pipe++) {
> > +   for (plane_id = 0; plane_id < I915_MAX_PLANES; plane_id++) {
> > +   intel_fb = vgpu-
> >display.meta_fbs.meta_fb[pipe][plane_id];
> > +   if (!intel_fb)
> > +   drm_framebuffer_put(_fb->base);
> > +
> > +   intel_fb = NULL;
> > +   }
> > +   }
> > +}
> > +
> > +static int meta_fb_create_handle(struct drm_framebuffer *fb,
> > +struct drm_file *file,
> > +unsigned int *handle)
> > +{
> > +   return -ENODEV;
> > +}
> > +
> > +static int meta_fb_dirty(struct drm_framebuffer *fb,
> > +struct drm_file *file,
> > +   

Re: [Intel-gfx] [PATCH v4] drm/i915/gvt: Deliver guest cursor hotspot info

[Zhang, Tina]

> -Original Message-
> From: Zhenyu Wang [mailto:zhen...@linux.intel.com]
> Sent: Monday, April 16, 2018 1:52 PM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: intel-gvt-...@lists.freedesktop.org; kra...@redhat.com; intel-
> g...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>
> Subject: Re: [PATCH v4] drm/i915/gvt: Deliver guest cursor hotspot info
> 
> On 2018.04.16 13:51:38 +0800, Tina Zhang wrote:
> > Guest OS driver uses PV info registers to deliver cursor hotspot info
> > to host. This patch is used to get cursor hotspot info from virtual
> > registers and deliver it to host userspace.
> >
> > v3->v4:
> > - return UINT_MAX when x_hot/y_hot is invalid. (Zhenyu)
> > - correct version.
> >
> > v2->v3:
> > - add validate_hotspot(). (Zhenyu)
> >
> > v1->v2:
> > - name as cursor_x_hot/cursor_y_hot. (Zhenyu)
> > - use i915_reg_t definition instead of magic numbers. (Zhenyu)
> >
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > Cc: Zhenyu Wang <zhen...@linux.intel.com>
> > Cc: Zhi Wang <zhi.a.w...@intel.com>
> > Cc: Gerd Hoffmann <kra...@redhat.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/dmabuf.c | 22 --
> >  drivers/gpu/drm/i915/gvt/fb_decoder.c |  3 +++
> >  drivers/gpu/drm/i915/gvt/handlers.c   |  4 ++--
> >  drivers/gpu/drm/i915/gvt/vgpu.c   |  3 +++
> >  drivers/gpu/drm/i915/i915_pvinfo.h|  5 -
> >  5 files changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
> > b/drivers/gpu/drm/i915/gvt/dmabuf.c
> > index 6f4f8e9..a7c7082 100644
> > --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> > +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> > @@ -192,6 +192,14 @@ static struct drm_i915_gem_object
> *vgpu_create_gem(struct drm_device *dev,
> > return obj;
> >  }
> >
> > +static bool validate_hotspot(struct intel_vgpu_cursor_plane_format
> > +*c) {
> > +   if (c && (c->x_hot <= c->width) && (c->y_hot <= c->height))
> > +   return true;
> 
> No way c could be NULL here.
See, we check c first.

BR,
Tina
> 
> > +   else
> > +   return false;
> > +}
> > +
> >  static int vgpu_get_plane_info(struct drm_device *dev,
> > struct intel_vgpu *vgpu,
> > struct intel_vgpu_fb_info *info,
> > @@ -229,12 +237,14 @@ static int vgpu_get_plane_info(struct drm_device
> *dev,
> > info->x_pos = c.x_pos;
> > info->y_pos = c.y_pos;
> >
> > -   /* The invalid cursor hotspot value is delivered to host
> > -* until we find a way to get the cursor hotspot info of
> > -* guest OS.
> > -*/
> > -   info->x_hot = UINT_MAX;
> > -   info->y_hot = UINT_MAX;
> > +   if (validate_hotspot()) {
> > +   info->x_hot = c.x_hot;
> > +   info->y_hot = c.y_hot;
> > +   } else {
> > +   info->x_hot = UINT_MAX;
> > +   info->y_hot = UINT_MAX;
> > +   }
> > +
> > info->size = (((info->stride * c.height * c.bpp) / 8)
> > + (PAGE_SIZE - 1)) >> PAGE_SHIFT;
> > } else {
> > diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c
> > b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> > index 1c12068..5e7468b 100644
> > --- a/drivers/gpu/drm/i915/gvt/fb_decoder.c
> > +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> > @@ -36,6 +36,7 @@
> >  #include 
> >  #include "i915_drv.h"
> >  #include "gvt.h"
> > +#include "i915_pvinfo.h"
> >
> >  #define PRIMARY_FORMAT_NUM 16
> >  struct pixel_format {
> > @@ -384,6 +385,8 @@ int intel_vgpu_decode_cursor_plane(struct
> intel_vgpu *vgpu,
> > plane->y_pos = (val & _CURSOR_POS_Y_MASK) >>
> _CURSOR_POS_Y_SHIFT;
> > plane->y_sign = (val & _CURSOR_SIGN_Y_MASK) >>
> _CURSOR_SIGN_Y_SHIFT;
> >
> > +   plane->x_hot = vgpu_vreg_t(vgpu, vgtif_reg(cursor_x_hot));
> > +   plane->y_hot = vgpu_vreg_t(vgpu, vgtif_reg(cursor_y_hot));
> > return 0;
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> > b/drivers/gpu/drm/i915/gvt/handlers.c
> > index a33c1c3e..2c824a9 100644
> > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > @@ -1201,8 +1201,8 @@ static int 

Re: [Intel-gfx] [PATCH v18 0/6] drm/i915/gvt: Dma-buf support for GVT-g

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Zhenyu Wang
> Sent: Thursday, November 23, 2017 2:13 PM
> To: Gerd Hoffmann <kra...@redhat.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; alex.william...@redhat.com; intel-
> g...@lists.freedesktop.org; joonas.lahti...@linux.intel.com; Wang, Zhi A
> <zhi.a.w...@intel.com>; linux-ker...@vger.kernel.org;
> zhen...@linux.intel.com; Zhang, Tina <tina.zh...@intel.com>;
> kwankh...@nvidia.com; Lv, Zhiyuan <zhiyuan...@intel.com>; dan...@ffwll.ch;
> ch...@chris-wilson.co.uk; intel-gvt-...@lists.freedesktop.org; Yuan, Hang
> <hang.y...@intel.com>
> Subject: Re: [PATCH v18 0/6] drm/i915/gvt: Dma-buf support for GVT-g
> 
> On 2017.11.15 11:49:00 +0100, Gerd Hoffmann wrote:
> > On Wed, Nov 15, 2017 at 05:11:49PM +0800, Tina Zhang wrote:
> > > v17->v18:
> > > 1) unmap vgpu's opregion when destroying vgpu.
> > > 2) update comments for VFIO_DEVICE_GET_GFX_DMABUF. (Alex)
> >
> > > This patch set adds the dma-buf support for intel GVT-g.
> > >
> > > dma-buf is an uniform mechanism to share DMA buffers across
> > > different devices and subsystems. dma-buf for intel GVT-g is mainly
> > > used to share the vgpu's framebuffer to userspace to leverage
> > > userspace graphics stacks to render the framebuffer to the display 
> > > monitor.
> > >
> > > The main idea is that we create a gem object and set vgpu's
> > > framebuffer as its backing storage. Then, export a dma-buf associated with
> this gem object.
> > > With the fd of this dma-buf, userspace can directly handle this buffer.
> > >
> > > This patch set can be tried with the following example:
> > >   git://git.kraxel.org/qemu  branch: work/intel-vgpu
> > >
> > > A topic branch with the latest patch set is:
> > > https://github.com/intel/gvt-linux.git   branch: topic/dmabuf
> >
> > Tested-by: Gerd Hoffmann <kra...@redhat.com>
> >
> 
> After debugging with Tina on one left race that fixed by
> https://lists.freedesktop.org/archives/intel-gvt-dev/2017-
> November/002505.html
The next version of this patch set will include this patch.
Thanks.

BR,
Tina
> 
> I still need below qemu fix for proper cursor handling, otherwise qemu just
> crashed when I click in my terminal program which hides cursor then.
> 
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c index
> e500ec2cb1..d9a044b080 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -169,8 +169,9 @@ static void vfio_display_dmabuf_update(void *opaque)
>  cursor = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_CURSOR);
>  if (vdev->cursor != cursor) {
>  vdev->cursor = cursor;
> -dpy_gl_cursor_dmabuf(vdev->display_con,
> - >buf);
> +if (cursor)
> +dpy_gl_cursor_dmabuf(vdev->display_con,
> + >buf);
>  free_bufs = true;
>  }
>  if (cursor != NULL) {
> 
> And with these it seems pretty fine now that I'll queue them up for -next 
> pull.
> 
> thanks
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915: Introduce GEM proxy

[Zhang, Tina]

I think we might miss the discussion in the first version. Here is the address.
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-November/002461.html

For convenience, I just paste it here:

> -Original Message-
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Tuesday, October 17, 2017 10:37 PM
> To: Zhang, Tina ; zhenyuw at linux.intel.com; Lv, 
> Zhiyuan
> ; Wang, Zhi A ; Tian, Kevin
> ; daniel at ffwll.ch
> Cc: Zhang, Tina ; intel-gfx at 
> lists.freedesktop.org; intel-
> gvt-dev at lists.freedesktop.org; Daniel Vetter 
> Subject: Re: [PATCH v1 1/2] drm/i915: Introduce GEM proxy
> 
> Quoting Tina Zhang (2017-10-16 09:57:33)
> > 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.
> >
> > V1:
> > - the patch is separated from the "Dma-buf support for Gvt-g"
> >   patch-set. (Joonas)
> >
> > 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
> 
> gem_busy: sees the local object, not the proxy activity; that's ok
> get_caching: returns the constant value
> pread: works by forcing GTT access
> pwrite: works by forcing GTT access
> mmap: disallowed by !filp, errno fixup in next patch
> mmap_gtt: works
> set-domain: fixed
> sw-finish: noted as impossible (please add checks)
I thought we were agreed on this: 
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-July/001523.html
So, in this version I just add the comments without checking

> set-tiling: fixed
> get_tiling: returns constant value
> madvise: works, since it's a discard of the local page array 
> overlay-put-image?
No. The original idea is that the GEM proxy could support madvise as it only 
invokes unpin/pin. And supporting madivse might benefit userspace.
So, do you think we'd better ban GEM proxy in madvise?

> not supported on any relevant platforms!
> gem_wait: works on local bo (as expected)
> 
> execbuf: works (or should at least work fine on a proxy object as either a 
> batch
> or one of the auxiliaries)
> 
> Hmm, use as a batch + cmdparser is broken; should result in ENODEV to
> userspace. An admittedly odd result, but not an oops.
GEM proxy isn't designed to be used as a batch buffer, only as an auxiliary. So 
I think here you mean we need to ban GEM proxy in 
i915_gem_execbuffer/i915_gem_execbuffer2, right?

> 
> display: pin-to-display and fencing will work, I think, and flush_for_display 
> is
> irrelevant as no direct cpu access.
> 
> dmabuf: works partially, but what doesn't work is optional. Userspace mmap is
> barred, most inter-driver interaction is barred. On paper a nuisance should it
> want to be used, but we should not oops. Hmm, no, we need to make the
> GEM_BUG_ON(!struct_page(obj)) in
> i915_gem_object_pin_map() a real return.
So, as this can benefit all kinds of gem object w/o backing storage, can we 
split the following part into another patch?
Thanks.

BR,
Tina
> 
> Please add
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index d699ea3ab80b..6a7ca9a502ba
> 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2650,7 +2650,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);
> 
> ret = mutex_lock_interruptible(>mm.lock);
> if (ret)
> 
> and my
> Reviewed-by: Chris Wilson  -Chris

The following is the third version:
> -Original Message-
> From: Zhang, Tina
> Sent: Monday, November 13, 2017 2:50 PM
> To: ch...@chris-w

Re: [Intel-gfx] [PATCH v1 1/2] drm/i915: Introduce GEM proxy

[Zhang, Tina]

> -Original Message-
> From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> Sent: Tuesday, October 17, 2017 10:37 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>; Tian, Kevin
> <kevin.t...@intel.com>; dan...@ffwll.ch
> Cc: Zhang, Tina <tina.zh...@intel.com>; intel-gfx@lists.freedesktop.org; 
> intel-
> gvt-...@lists.freedesktop.org; Daniel Vetter <daniel.vet...@ffwll.ch>
> Subject: Re: [PATCH v1 1/2] drm/i915: Introduce GEM proxy
> 
> Quoting Tina Zhang (2017-10-16 09:57:33)
> > 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.
> >
> > V1:
> > - the patch is separated from the "Dma-buf support for Gvt-g"
> >   patch-set. (Joonas)
> >
> > 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
> 
> gem_busy: sees the local object, not the proxy activity; that's ok
> get_caching: returns the constant value
> pread: works by forcing GTT access
> pwrite: works by forcing GTT access
> mmap: disallowed by !filp, errno fixup in next patch
> mmap_gtt: works
> set-domain: fixed
> sw-finish: noted as impossible (please add checks)
I thought we were agreed on this: 
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-July/001523.html
So, in this version I just add the comments without checking

> set-tiling: fixed
> get_tiling: returns constant value
> madvise: works, since it's a discard of the local page array 
> overlay-put-image?
No. The original idea is that the GEM proxy could support madvise as it only 
invokes unpin/pin. And supporting madivse might benefit userspace.
So, do you think we'd better ban GEM proxy in madvise?

> not supported on any relevant platforms!
> gem_wait: works on local bo (as expected)
> 
> execbuf: works (or should at least work fine on a proxy object as either a 
> batch
> or one of the auxiliaries)
> 
> Hmm, use as a batch + cmdparser is broken; should result in ENODEV to
> userspace. An admittedly odd result, but not an oops.
GEM proxy isn't designed to be used as a batch buffer, only as an auxiliary. So 
I think here you mean we need to ban GEM proxy in 
i915_gem_execbuffer/i915_gem_execbuffer2, right?

> 
> display: pin-to-display and fencing will work, I think, and flush_for_display 
> is
> irrelevant as no direct cpu access.
> 
> dmabuf: works partially, but what doesn't work is optional. Userspace mmap is
> barred, most inter-driver interaction is barred. On paper a nuisance should it
> want to be used, but we should not oops. Hmm, no, we need to make the
> GEM_BUG_ON(!struct_page(obj)) in
> i915_gem_object_pin_map() a real return.
So, as this can benefit all kinds of gem object w/o backing storage, can we 
split the following part into another patch?
Thanks.

BR,
Tina
> 
> Please add
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index d699ea3ab80b..6a7ca9a502ba
> 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2650,7 +2650,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);
> 
> ret = mutex_lock_interruptible(>mm.lock);
> if (ret)
> 
> and my
> Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk> -Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Introduce GEM proxy

[Zhang, Tina]

Hi,

As the "Dma-buf support for GVT-g" patch-set relys on this patch, can I collect 
the comments for this version? Do we all agree on it?
Thanks.

Comments of the previous version:
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-October/002278.html

BR,
Tina

> -Original Message-
> From: Zhang, Tina
> Sent: Wednesday, November 1, 2017 5:22 PM
> To: zhen...@linux.intel.com; Wang, Zhi A <zhi.a.w...@intel.com>;
> dan...@ffwll.ch; joonas.lahti...@linux.intel.com; ch...@chris-wilson.co.uk
> Cc: Zhang, Tina <tina.zh...@intel.com>; intel-gfx@lists.freedesktop.org; 
> intel-
> gvt-...@lists.freedesktop.org; Daniel Vetter <daniel.vet...@ffwll.ch>
> Subject: [PATCH v2 1/2] drm/i915: Introduce GEM proxy
> 
> 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.zh...@intel.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c| 25 -
>  drivers/gpu/drm/i915/i915_gem_object.h | 11 +--
> drivers/gpu/drm/i915/i915_gem_tiling.c |  8 
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 82a1003..13bc18d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1593,6 +1593,16 @@ i915_gem_set_domain_ioctl(struct drm_device
> *dev, void *data,
>   if (err)
>   goto out;
> 
> + /* Proxy objects do not control access to the backing storage, ergo
> +  * they cannot be used as a means to manipulate the cache domain
> +  * tracking for that backing storage. The proxy object is always
> +  * considered to be outside of any cache domain.
> +  */
> + if (i915_gem_object_is_proxy(obj)) {
> + err = -ENXIO;
> + goto out;
> + }
> +
>   /* Flush and acquire obj->pages so that we are coherent through
>* direct access in memory with previous cached writes through
>* shmemfs and that our cache domain tracking remains valid.
> @@ -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);
> 
>   ret = mutex_lock_interruptible(>mm.lock);
>   if (ret)
> @@ -3766,6 +3781,14 @@ int i915_gem_set_caching_ioctl(struct drm_device
> *dev, void *data,
>   if (!obj)
>   return -ENOENT;
> 
> + /* The caching mode of proxy object is handled by its generator, and
> not
> +  * expected to be changed by user mode.
> +  */
> + if (i915_gem_object_is_proxy(obj)) {
> + ret = -ENXIO;
> + goto out;
> + }
> +
>   if (obj->cache_level == level)
>   goto out;
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_object.h
> b/drivers/gpu/drm/i915/i915_

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Introduce GEM proxy

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Joonas Lahtinen
> Sent: Tuesday, November 7, 2017 9:06 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Wang, Zhi
> A <zhi.a.w...@intel.com>; dan...@ffwll.ch; ch...@chris-wilson.co.uk
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/2] drm/i915: Introduce GEM proxy
> 
> On Tue, 2017-11-07 at 04:53 +, Zhang, Tina wrote:
> > > -Original Message-
> > > From: intel-gvt-dev
> > > [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On Behalf Of
> > > Joonas Lahtinen
> > > Sent: Monday, November 6, 2017 7:24 PM
> > > To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com;
> > > Wang, Zhi A <zhi.a.w...@intel.com>; dan...@ffwll.ch;
> > > ch...@chris-wilson.co.uk
> > > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>;
> > > intel-gfx@lists.freedesktop.org; intel-gvt-...@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.zh...@intel.com>
> > > > Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > > > Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > > > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > >
> > > 
> > >
> > > > @@ -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

Re: [Intel-gfx] [PATCH v17 5/6] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Friday, November 10, 2017 3:03 PM
> To: Alex Williamson <alex.william...@redhat.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; Yuan, Hang <hang.y...@intel.com>;
> Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> joonas.lahti...@linux.intel.com; linux-ker...@vger.kernel.org;
> zhen...@linux.intel.com; ch...@chris-wilson.co.uk; kwankh...@nvidia.com;
> Lv, Zhiyuan <zhiyuan...@intel.com>; dan...@ffwll.ch; Zhang, Tina
> <tina.zh...@intel.com>; intel-gvt-...@lists.freedesktop.org; Wang, Zhi A
> <zhi.a.w...@intel.com>
> Subject: Re: [PATCH v17 5/6] vfio: ABI for mdev display dma-buf operation
> 
> On Thu, Nov 09, 2017 at 01:54:57PM -0700, Alex Williamson wrote:
> > On Thu, 9 Nov 2017 19:35:14 +0100
> > Gerd Hoffmann <kra...@redhat.com> wrote:
> >
> > >   Hi,
> > >
> > > > struct vfio_device_gfx_plane_info lacks the head field we've been
> > > > discussing.  Thanks,
> > >
> > > Adding multihead support turned out to not be that easy.  There are
> > > corner cases like a single framebuffer spawning both heads.  Also it
> > > would be useful to somehow hint to the guest which heads it should use.
> > >
> > > In short:  Proper multihead support is more complex than just adding
> > > a head field for later use.  So in a short private discussion with
> > > Tina we came to the conclusion that it will be better add multihead
> > > support to the API when the first driver wants use it, so we can
> > > actually test the interface and make sure we didn't miss anything.
> > > Adding a incomplete multihead API now doesn't help anybody.
> >
> > Do you think we can enable multi-head and preserve backwards
> > compatibility within this API proposed here?
> 
> Yes, I think we can.  Adding new fields is possible thanks to the argsz field 
> at the
> start of the struct, so we easily add the new fields (head, framebuffer 
> rectangle,
> whatever else is needed).  If the new fields are not present the driver can 
> simply
> assume head=0.
> 
> Does the driver set argsz too?  If so userspace can detect whenever the driver
> supports the multihead API extension (before going to probe for
> head=1) that way.  If not we probably need an additional probe flag for that.
> But in any case I'm confident this is solvable.
> 
> Passing hints about the display configuration to the guest needs a new ioctl, 
> so
> we don't have compatibility issues there.
Thanks. So, do we all agree on this? If so, the next version will be submitted 
soon
with some updated comments.
Thanks.


BR,
Tina
> 
> cheers,
>   Gerd
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v17 0/6] drm/i915/gvt: Dma-buf support for GVT-g

[Zhang, Tina]

And here is the "git diff" of (git://git.kraxel.org/qemu), which is used to 
test this version:

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 1cdc926..06234b2 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -36,7 +36,7 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice 
*vdev,
uint32_t plane_type)
 {
 struct vfio_device_gfx_plane_info plane;
-struct vfio_device_gfx_dmabuf_fd gfd;
+int dmabuf_fd;
 VFIODMABuf *dmabuf;
 static int errcnt;
 int ret;
@@ -84,11 +84,10 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice 
*vdev,
 }
 }

-memset(, 0, sizeof(gfd));
-gfd.argsz = sizeof(gfd);
-gfd.dmabuf_id = plane.dmabuf_id;
-ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_GFX_DMABUF, );
-if (ret < 0) {
+
+
+dmabuf_fd = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_GFX_DMABUF, 
_id);
+if (dmabuf_fd < 0) {
 fprintf(stderr, "(%d) ioctl VFIO_DEVICE_GET_GFX_DMABUF: %s\r",
 ++errcnt, strerror(errno));
 return NULL;
@@ -103,7 +102,7 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice 
*vdev,
 (plane.drm_format >> 16) & 0xff,
 (plane.drm_format >> 24) & 0xff,
 (plane_type == DRM_PLANE_TYPE_PRIMARY) ? "primary" : "cursor",
-gfd.dmabuf_fd);
+dmabuf_fd);

 dmabuf = g_new0(VFIODMABuf, 1);
 dmabuf->dmabuf_id  = plane.dmabuf_id;
@@ -111,7 +110,7 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice 
*vdev,
 dmabuf->buf.height = plane.height;
 dmabuf->buf.stride = plane.stride;
 dmabuf->buf.fourcc = plane.drm_format;
-dmabuf->buf.fd = gfd.dmabuf_fd;
+dmabuf->buf.fd = dmabuf_fd;

 QTAILQ_INSERT_HEAD(>dmabufs, dmabuf, next);
 return dmabuf;
diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index c80bb5e..aee81f2 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -536,6 +536,8 @@ struct vfio_device_gfx_plane_info {
__u32 size; /* size of plane in bytes, align on page*/
__u32 x_pos;/* horizontal position of cursor plane, upper left 
corner in pixels */
__u32 y_pos;/* vertical position of cursor plane, upper left corner 
in lines*/
+   __u32 x_hot;
+   __u32 y_hot;
union {
__u32 region_index; /* region index */
__u32 dmabuf_id;    /* dma-buf id */


Thanks.

BR,
Tina

> -Original Message-
> From: Zhang, Tina
> Sent: Friday, November 10, 2017 7:20 AM
> To: 'Gerd Hoffmann' <kra...@redhat.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; linux-ker...@vger.kernel.org; intel-
> g...@lists.freedesktop.org; joonas.lahti...@linux.intel.com;
> kwankh...@nvidia.com; zhen...@linux.intel.com; ch...@chris-wilson.co.uk;
> alex.william...@redhat.com; Lv, Zhiyuan <zhiyuan...@intel.com>;
> dan...@ffwll.ch; Yuan, Hang <hang.y...@intel.com>; intel-gvt-
> d...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>
> Subject: RE: [PATCH v17 0/6] drm/i915/gvt: Dma-buf support for GVT-g
> 
> 
> 
> > -Original Message-
> > From: intel-gvt-dev
> > [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On Behalf Of Gerd
> > Hoffmann
> > Sent: Thursday, November 9, 2017 10:10 PM
> > To: Zhang, Tina <tina.zh...@intel.com>
> > Cc: Tian, Kevin <kevin.t...@intel.com>; linux-ker...@vger.kernel.org;
> > intel- g...@lists.freedesktop.org; joonas.lahti...@linux.intel.com;
> > kwankh...@nvidia.com; zhen...@linux.intel.com;
> > ch...@chris-wilson.co.uk; alex.william...@redhat.com; Lv, Zhiyuan
> > <zhiyuan...@intel.com>; dan...@ffwll.ch; Yuan, Hang
> > <hang.y...@intel.com>; intel-gvt- d...@lists.freedesktop.org; Wang, Zhi
> > A <zhi.a.w...@intel.com>
> > Subject: Re: [PATCH v17 0/6] drm/i915/gvt: Dma-buf support for GVT-g
> >
> > On Thu, Nov 09, 2017 at 05:33:56PM +0800, Tina Zhang wrote:
> > > v16->v17:
> > > 1) modify VFIO_DEVICE_GET_GFX_DMABUF interface. (Alex)
> > > 2) add comments for x_hot/y_hot. (Gerd)
> >
> > Hmm, doesn't apply cleanly here.
> > Tried 4.14-rc8 + gem proxy v2 + this series.
> > Seems the patches depend on unmerged gvt changes.
> > Do you have a git branch somewhere?
> Please use this:
> https://github.com/intel/gvt-linux.git
> branch: topic/dmabuf
> Thanks.
> 
> BR,
> Tina
> >
> > thanks,
> >   Gerd
> >
> > ___
> > intel-gvt-dev mailing list
> > intel-gvt-...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v17 0/6] drm/i915/gvt: Dma-buf support for GVT-g

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Thursday, November 9, 2017 10:10 PM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; linux-ker...@vger.kernel.org; intel-
> g...@lists.freedesktop.org; joonas.lahti...@linux.intel.com;
> kwankh...@nvidia.com; zhen...@linux.intel.com; ch...@chris-wilson.co.uk;
> alex.william...@redhat.com; Lv, Zhiyuan <zhiyuan...@intel.com>;
> dan...@ffwll.ch; Yuan, Hang <hang.y...@intel.com>; intel-gvt-
> d...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>
> Subject: Re: [PATCH v17 0/6] drm/i915/gvt: Dma-buf support for GVT-g
> 
> On Thu, Nov 09, 2017 at 05:33:56PM +0800, Tina Zhang wrote:
> > v16->v17:
> > 1) modify VFIO_DEVICE_GET_GFX_DMABUF interface. (Alex)
> > 2) add comments for x_hot/y_hot. (Gerd)
> 
> Hmm, doesn't apply cleanly here.
> Tried 4.14-rc8 + gem proxy v2 + this series.
> Seems the patches depend on unmerged gvt changes.
> Do you have a git branch somewhere?
Please use this:
https://github.com/intel/gvt-linux.git
branch: topic/dmabuf
Thanks.

BR,
Tina
> 
> thanks,
>   Gerd
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Tuesday, November 7, 2017 4:03 PM
> To: Zhang, Tina <tina.zh...@intel.com>; alex.william...@redhat.com;
> ch...@chris-wilson.co.uk; joonas.lahti...@linux.intel.com;
> zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A
> <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; dan...@ffwll.ch;
> kwankh...@nvidia.com
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > > Add a head field here?  People asked @ kvm forum about multihead
> > > support.
> > > Even if the initial driver version doesn't support it we could add a
> > > field so it becomes easier to add it at some point in the future.
> > >
> > > Probing for available heads could be done with the PROBE flag, i.e.
> > > flags = PROBE | DMABUF, plane_type = PRIMARY, head = 0, 1, ...
> >
> > Does the multihead mean multiple display monitor? So each head can
> > have its own one primary plane, one cursor plane and maybe some sprite
> > planes if supported.
> 
> Yes and yes.
> 
> > And the flow could be like this:
> > 1) flags = PROBE | DMABUF, then return the number of heads in
> > vfio_device_gfx_plane_info.head.
> 
> I'd suggest to use { .flags = PROBE | DMABUF, .head = 0 } to check whenever
> dmabuf is supported for head 0, then { .flags = PROBE | DMABUF, .head = 1 } to
> check for head 1 support, ...
> 
> Driver returns success if the head is supported and -EINVAL otherwise, 
> simliar to
> the dmabuf vs. region probing.
> 
> It is less confusing because .head is always an input field then.
> 
> It is also a bit more flexible because different heads can support different 
> modes
> (I don't expect drivers will actually use that though).
> 
> > 2) flags = DMABUF with plane_type = PRIMARY, head = 0, 1, ..., then
> > return the id of the exposed framebuffer of that head.
> 
> Yes.
> 
> > Another question is if the sprite plane is supported, how we're going
> > to identify them, as there could be several sprite planes for one
> > display monitor.
> 
> Do you mean DRM_PLANE_TYPE_OVERLAY?
Yes.

> 
> > Add another field "num_plane" for it? Then, I guess the flow could be
> > like this:
> > 1) flags = PROBE | DMABUF, then return the number of heads in
> > vfio_device_gfx_plane_info.head.
> > 2) flags = PROBE | DMABUF and head = 0, 1, ..., and plane_type =
> > PRIMARY/CURSOR/SPRITE, then return the num_plane.
> 
> I'd suggest to do it simliar to the head probe outlined above, i.e.
> userspace calls { .flags = PROBE | DMABUF, .head = 0, .plane_type = OVERLAY,
> plane_num = 0, 1, 2, ... } and the driver returns -EINVAL or 0 depending on
> whenever it is supported or not.
Agree. Thanks.

BR,
Tina
> 
> cheers,
>   Gerd
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Monday, November 6, 2017 5:01 PM
> To: Zhang, Tina <tina.zh...@intel.com>; alex.william...@redhat.com;
> ch...@chris-wilson.co.uk; joonas.lahti...@linux.intel.com;
> zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A
> <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; dan...@ffwll.ch;
> kwankh...@nvidia.com
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *struct
> > vfio_device_query_gfx_plane)
> > + *
> > + * Set the drm_plane_type and flags, then retrieve the gfx plane
> > info.
> > + *
> > + * flags supported:
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF
> are
> > set
> > + *   to ask if the mdev supports dma-buf. 0 on support, -EINVAL on
> > no
> > + *   support for dma-buf.
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION
> are
> > set
> > + *   to ask if the mdev supports region. 0 on support, -EINVAL on no
> > + *   support for region.
> > + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION
> is set
> > + *   with each call to query the plane info.
> > + * - Others are invalid and return -EINVAL.
> > + *
> > + * Return: 0 on success, -ENODEV with all out fields zero on mdev
> > + * device initialization, -errno on other failure.
> 
> Should also not here that it is not an error if the guest has not defined a 
> plane
> yet.  The ioctl should return success in that case and zero-initialize plane 
> info
> (drm_format + size + width + height fields).
Indeed. I just need to update the comments, as this version is implemented with 
this in mind. Thanks.

> 
> > + */
> > +struct vfio_device_gfx_plane_info {
> > +   __u32 argsz;
> > +   __u32 flags;
> > +#define VFIO_GFX_PLANE_TYPE_PROBE (1 << 0) #define
> > +VFIO_GFX_PLANE_TYPE_DMABUF (1 << 1) #define
> > +VFIO_GFX_PLANE_TYPE_REGION (1 << 2)
> > +   /* in */
> > +   __u32 drm_plane_type;   /* type of plane:
> > DRM_PLANE_TYPE_* */
> 
> Add a head field here?  People asked @ kvm forum about multihead support.
> Even if the initial driver version doesn't support it we could add a field so 
> it
> becomes easier to add it at some point in the future.
> 
> Probing for available heads could be done with the PROBE flag, i.e.
> flags = PROBE | DMABUF, plane_type = PRIMARY, head = 0, 1, ...
Does the multihead mean multiple display monitor? So each head can have its own 
one primary plane, one cursor plane and maybe some sprite planes if supported.
And the flow could be like this:
1) flags = PROBE | DMABUF, then return the number of heads in 
vfio_device_gfx_plane_info.head.
2) flags = DMABUF with plane_type = PRIMARY, head = 0, 1, ..., then return the 
id of the exposed framebuffer of that head.
Am I correct?

Another question is if the sprite plane is supported, how we're going to 
identify them, as there could be several sprite planes for one display monitor.
Add another field "num_plane" for it? Then, I guess the flow could be like this:
1) flags = PROBE | DMABUF, then return the number of heads in 
vfio_device_gfx_plane_info.head.
2) flags = PROBE | DMABUF and head = 0, 1, ..., and plane_type = 
PRIMARY/CURSOR/SPRITE, then return the num_plane.
3) flags = DMABUF with plane_type = PRIMARY, head = 0, 1, ..., num_plane =0, 1, 
..., then return the id of the exposed framebuffer of that head.
Does it make sense?
Thanks.

> 
> > +   __u32 x_hot;/* horizontal position of cursor hotspot */
> > +   __u32 y_hot;/* vertical position of cursor hotspot */
> 
> Needs documentation how the driver signals "no hotspot information available"
> (using 0x for example).
This version has implemented this. I will update the comments.
Thanks.

BR,
Tina
> 
> cheers,
>   Gerd
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, November 7, 2017 4:37 AM
> To: Gerd Hoffmann <kra...@redhat.com>
> Cc: Zhang, Tina <tina.zh...@intel.com>; Tian, Kevin <kevin.t...@intel.com>;
> Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> joonas.lahti...@linux.intel.com; linux-ker...@vger.kernel.org;
> zhen...@linux.intel.com; ch...@chris-wilson.co.uk; kwankh...@nvidia.com;
> Lv, Zhiyuan <zhiyuan...@intel.com>; dan...@ffwll.ch; intel-gvt-
> d...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>
> Subject: Re: [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation
> 
> On Mon, 06 Nov 2017 10:05:34 +0100
> Gerd Hoffmann <kra...@redhat.com> wrote:
> 
> >   Hi,
> >
> > > > I thought we had agreed to make this behave similar to
> > > > VFIO_GROUP_GET_DEVICE_FD, the ioctl would take a __u32 dmabuf_id
> > > > and return the file descriptor as the ioctl return value.  Thanks,
> > >
> > > If we follow VFIO_GROUP_GET_DEVICE_FD, we would lose flags
> > > functionality.
> > > Zhi and Zhenyu, how do you think about it?
> >
> > The ioctl is simple enough that not having flags should not be a
> > problem I think.
> >
> > Also note that dmabuf_id is received using the PLANE_INFO ioctl, so
> > should the need arise to negotiate something in the future chances are
> > high that it can be done using the PLANE_INFO ioctl flags.
> 
> Right, the ioctl is "get fd for thing" so we have control of "thing".
> I think we had this same discussion on v15.  Thanks,
Then, OK. Thanks.

BR,
Tina
> 
> Alex
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Object w/o backing stroage is banned by -ENXIO

[Zhang, Tina]

> -Original Message-
> From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
> Sent: Monday, November 6, 2017 6:57 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Wang, Zhi
> A <zhi.a.w...@intel.com>; dan...@ffwll.ch; ch...@chris-wilson.co.uk
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org
> Subject: Re: [PATCH v2 2/2] drm/i915: Object w/o backing stroage is banned by
> -ENXIO
> 
> Hi Tina,
> 
> Please send this patch alone (or in the beginning of your series), so it can 
> be
> merged already.
> 
> That'll save you the effort of carrying this patch.
Thank you very much :)

BR,
Tina
> 
> Regards, Joonas
> 
> On Wed, 2017-11-01 at 17:22 +0800, Tina Zhang wrote:
> > -ENXIO should be returned when operations are banned from changing
> > backing storage of objects without backing storage.
> >
> > v2:
> > - update the patch description and subject to just mention objects w/o
> >   backing storage, instead of "GEM proxy". (Joonas)
> >
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 13bc18d..e85721c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1713,7 +1713,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev,
> void *data,
> >  */
> > if (!obj->base.filp) {
> > i915_gem_object_put(obj);
> > -   return -EINVAL;
> > +   return -ENXIO;
> > }
> >
> > addr = vm_mmap(obj->base.filp, 0, args->size,
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Introduce GEM proxy

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Joonas Lahtinen
> Sent: Monday, November 6, 2017 7:24 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Wang, Zhi
> A <zhi.a.w...@intel.com>; dan...@ffwll.ch; ch...@chris-wilson.co.uk
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@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.zh...@intel.com>
> > Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> 
> 
> 
> > @@ -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/002278.html
Thanks.

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

Re: [Intel-gfx] [PATCH v16 1/6] drm/i915/gvt: Add framebuffer decoder support

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Monday, November 6, 2017 4:49 PM
> To: Zhang, Tina <tina.zh...@intel.com>; alex.william...@redhat.com;
> ch...@chris-wilson.co.uk; joonas.lahti...@linux.intel.com;
> zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A
> <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; dan...@ffwll.ch;
> kwankh...@nvidia.com
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; 
> linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v16 1/6] drm/i915/gvt: Add framebuffer decoder support
> 
>   Hi,
> 
> > +static struct pixel_format bdw_pixel_formats[] = {
> > +   {DRM_FORMAT_C8, 8, "8-bit Indexed"},
> 
> > +static struct pixel_format skl_pixel_formats[] = {
> > +   {DRM_FORMAT_YUYV, 16, "16-bit packed YUYV (8:8:8:8 MSB-
> > V:Y2:U:Y1)"},
> 
> Broadwell and Skylake.
> 
> What is the state for newer chipsets?
> 
> Is Kabylake supported by gvt meanwhile?  Does it need specific fb decoder
> support, or is it compatible with skylake?
Kabylake is supported by gvt. And the fb part should be compatible with 
skylake. But I haven't tested it yet.
Besides, the current version needs to add some platform checking logic to 
support Kabylake.
Thanks.

BR,
Tina
> 
> cheers,
>   Gerd
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Alex Williamson
> Sent: Monday, November 6, 2017 10:39 AM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; Daniel Vetter 
> <daniel.vet...@ffwll.ch>;
> intel-gfx@lists.freedesktop.org; joonas.lahti...@linux.intel.com; linux-
> ker...@vger.kernel.org; zhen...@linux.intel.com; ch...@chris-wilson.co.uk;
> kwankh...@nvidia.com; Lv, Zhiyuan <zhiyuan...@intel.com>; dan...@ffwll.ch;
> intel-gvt-...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>;
> kra...@redhat.com
> Subject: Re: [PATCH v16 5/6] vfio: ABI for mdev display dma-buf operation
> 
> On Mon,  6 Nov 2017 10:19:17 +0800
> Tina Zhang <tina.zh...@intel.com> wrote:
> 
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user query and
> > get a plane and its related information. So far, two types of buffers
> > are
> > supported: buffers based on dma-buf and buffers based on region.
> >
> > This ioctl can be invoked with:
> > 1) either DMABUF or REGION flag. Vendor driver returns a plane_info
> > successfully only when the specific kind of buffer is supported.
> > 2) flag PROBE. And at the same time either DMABUF or REGION must be
> > set, so that vendor driver can return success only when the specific
> > kind of buffer is supported.
> >
> > Add VFIO_DEVICE_GET_GFX_DMABUF ioctl command to let user get an
> > exposed dma-buf fd of a specific dmabuf_id which was returned in
> > VFIO_DEVICE_QUERY _GFX_PLANE ioctl command.
> >
> > The life cycle of an exposed MDEV buffer is handled by userspace and
> > tracked by kernel space. The returned dmabuf_id in struct vfio_device_
> > query_gfx_plane can be a new id of a new exposed buffer or an old id
> > of a re-exported buffer. Host user can check the value of dmabuf_id to
> > see if it needs to create new resources according to the new exposed
> > buffer or just re-use the existing resource related to the old buffer.
> >
> > v16:
> > - add x_hot and y_hot fields. (Gerd)
> > - add comments for VFIO_DEVICE_GET_GFX_DMABUF. (Alex)
> > - rebase to 4.14.0-rc6.
> >
> > v15:
> > - add a ioctl to get a dmabuf for a given dmabuf id. (Gerd)
> >
> > v14:
> > - add PROBE, DMABUF and REGION flags. (Alex)
> >
> > v12:
> > - add drm_format_mod back. (Gerd and Zhenyu)
> > - add region_index. (Gerd)
> >
> > v11:
> > - rename plane_type to drm_plane_type. (Gerd)
> > - move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info.
> >   (Gerd)
> > - remove drm_format_mod, start fields. (Daniel)
> > - remove plane_id.
> >
> > v10:
> > - refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE. (Alex) (Gerd)
> >
> > v3:
> > - add a field gvt_plane_info in the drm_i915_gem_obj structure to save
> >   the decoded plane information to avoid look up while need the plane
> >   info. (Gerd)
> >
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > Cc: Gerd Hoffmann <kra...@redhat.com>
> > Cc: Alex Williamson <alex.william...@redhat.com>
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > ---
> >  include/uapi/linux/vfio.h | 68
> > +++
> >  1 file changed, 68 insertions(+)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..6c55668 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,74 @@ struct vfio_pci_hot_reset {
> >
> >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE +
> 13)
> >
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *struct vfio_device_query_gfx_plane)
> > + *
> > + * Set the drm_plane_type and flags, then retrieve the gfx plane info.
> > + *
> > + * flags supported:
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF
> are set
> > + *   to ask if the mdev supports dma-buf. 0 on support, -EINVAL on no
> > + *   support for dma-buf.
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION
> are set
> > + *   to ask if the mdev supports region. 0 on support, -EINVAL on no
> > + *   support for region.
> > + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION
> is set
> > + *   with each call to query the plane info.
> > + * - Others are invalid and return -EINVAL.
> > +

Re: [Intel-gfx] [PATCH v15 6/7] drm/i915: Introduce GEM proxy

[Zhang, Tina]

> -Original Message-
> From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
> Sent: Tuesday, October 10, 2017 9:02 PM
> To: Zhang, Tina <tina.zh...@intel.com>; alex.william...@redhat.com;
> kra...@redhat.com; ch...@chris-wilson.co.uk; zhen...@linux.intel.com; Lv,
> Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>; Tian,
> Kevin <kevin.t...@intel.com>; dan...@ffwll.ch; kwankh...@nvidia.com
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org; linux-ker...@vger.kernel.org
> Subject: Re: [Intel-gfx] [PATCH v15 6/7] drm/i915: Introduce GEM proxy
> 
> On Tue, 2017-10-10 at 13:58 +0300, Joonas Lahtinen wrote:
> > On Tue, 2017-10-10 at 17:50 +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.
> > >
> > > 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.zh...@intel.com>
> > > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > > Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> >
> > 
> >
> > > +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> > > @@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops {
> > > unsigned int flags;
> > >  #define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0)
> > >  #define I915_GEM_OBJECT_IS_SHRINKABLE   BIT(1)
> > > +#define I915_GEM_OBJECT_IS_PROXY   BIT(2)
> >
> > Please fix the indent to match. Do convert the above two lines to use
> > TAB character too.
> >
> > 
> >
> > > @@ -1690,7 +1704,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev,
> void *data,
> > >*/
> > >   if (!obj->base.filp) {
> > >   i915_gem_object_put(obj);
> > > - return -EINVAL;
> > > + return -ENXIO;
> > >   }
> >
> > This still needs to be a separate patch.
> >
> > With those fixes, this is;
> >
> > Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> 
> Also, send the produced two patches (this and the split patch) as a standalone
> series for easier testing and merging.
Got it. Thanks.

BR,
Tina
> 
> CI seems to have hard time applying the whole series.
> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v15 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, October 11, 2017 3:17 AM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; zhen...@linux.intel.com;
> Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>;
> Tian, Kevin <kevin.t...@intel.com>; dan...@ffwll.ch; kwankh...@nvidia.com;
> intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; linux-
> ker...@vger.kernel.org; Daniel Vetter <daniel.vet...@ffwll.ch>
> Subject: Re: [PATCH v15 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Tue, 10 Oct 2017 17:50:05 +0800
> Tina Zhang <tina.zh...@intel.com> wrote:
> 
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> > and get the plan and its related information. This ioctl can be invoked 
> > with:
> 
> s/plan/plane/
Sorry about this typo :).

> 
> > 1) either flag DMABUF or REGION is set. Vendor driver returns success
> > and the plane_info only when the specific kind of buffer is supported.
> > 2) flag PROBE is set with either DMABUF or REGION. Vendor driver
> > returns success only when the specific kind of buffer is supported.
> >
> > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > or an old fd of a re-exported dma-buf. Host user mode can check the
> > value of fd and to see if it needs to create new resource according to
> > the new fd or just use the existed resource related to the old fd.
> >
> > v15:
> > - add a ioctl to get a dmabuf for a given dmabuf id. (Gerd)
> >
> > v14:
> > - add PROBE, DMABUF and REGION flags. (Alex)
> >
> > v12:
> > - add drm_format_mod back. (Gerd and Zhenyu)
> > - add region_index. (Gerd)
> >
> > v11:
> > - rename plane_type to drm_plane_type. (Gerd)
> > - move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info.
> >   (Gerd)
> > - remove drm_format_mod, start fields. (Daniel)
> > - remove plane_id.
> >
> > v10:
> > - refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE. (Alex) (Gerd)
> >
> > v3:
> > - add a field gvt_plane_info in the drm_i915_gem_obj structure to save
> >   the decoded plane information to avoid look up while need the plane
> >   info. (Gerd)
> >
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > Cc: Gerd Hoffmann <kra...@redhat.com>
> > Cc: Alex Williamson <alex.william...@redhat.com>
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > ---
> >  include/uapi/linux/vfio.h | 62
> > +++
> >  1 file changed, 62 insertions(+)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..fdf9a9c 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,68 @@ struct vfio_pci_hot_reset {
> >
> >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE +
> 13)
> >
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *struct vfio_device_query_gfx_plane)
> > + *
> > + * Set the drm_plane_type and flags, then retrieve the gfx plane info.
> > + *
> > + * flags supported:
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF
> are set
> > + *   to ask if the mdev supports dma-buf. 0 on support, -EINVAL on no
> > + *   support for dma-buf.
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION
> are set
> > + *   to ask if the mdev supports region. 0 on support, -EINVAL on no
> > + *   support for region.
> > + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION
> is set
> > + *   with each call to query the plane info.
> 
> So dmabuf_id is effectively just a token that can be fed into GET_GFX_DMABUF
> to get the fd.  The implementation of the token is vendor specific, but can be
> thought of as some sort of sequence ID or generation ID (but not necessarily
> monotonically increasing), so GET_GFX_DMABUF may fail if the previously
> provided dmabuf_id is no longer valid.  Do I have this correct?
Exactly. GET_GFX_DMABUF may fail if the dmabuf_id is no longer valid. -EINVAL 
will be
returned in that case. 
And dmabuf_id is invalid when:
1) user space has closed all dma-buf fds which are exposed with the dmabuf_id,
2) or the dmabuf_id isn't returned from QUERY_GFX_PLANE.
I will add the comments. Thanks.

> 
> > + 

Re: [Intel-gfx] [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Friday, October 6, 2017 8:13 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Wang, Zhi
> A <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; Alex
> Williamson <alex.william...@redhat.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > Yeah, that could solve the problem. But I'm not sure if it could be
> > acceptable.
> > Zhenyu, can you share your comments?
> 
> Ping?  Any progress here?  We are at 4.14-rc3 already.  v15 is needed really
> soon now otherwise the 4.15 merge window will be missed.
V15 will be submitted next week, after discussion with maintainers.
Thanks.

BR,
Tina
> 
> cheers,
>   Gerd

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Friday, September 29, 2017 6:21 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Wang, Zhi
> A <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; Alex
> Williamson <alex.william...@redhat.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > For example, if the old reused dmabuf_obj is released just after query
> > ioctl return it,  the next get_fd ioctl would return error as the
> > dmabuf_obj has already been closed.
> 
> My branch already grabs an extra reference when creating a new dmabuf_obj,
> which will be dropped on GET_DMABUF ioctl, exactly to avoid the dmabuf_obj
> disappear between QUERY_PLANE and GET_DMABUF ioctls.
Yeah, that could solve the problem. But I'm not sure if it could be acceptable.
Zhenyu, can you share your comments?
Thanks.

BR,
Tina

> 
> Can easily be extended to handle the reuse case too.
> 
> https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-v14=9959109ae
> 52cf15e119715a6b7de080fb849e3d2
> 
> While being at it also cleanup properly on close (so we don't leak structs in 
> case
> userspace never calls GET_DMABUF for a plane).
> 
> https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-v14=c0b0c407e
> 33904e749dec1ef44ec01099c16d39f
> 
> > > > Do you think the fd interface is enough for all kinds of buffer
> > > > exposed by Mdev?
> > >
> > > What kind of buffers do you have in mind which might not be covered?
> >
> > I thinking about the case that would like to postpone the buffers
> > releasing operation, after user space has closed all the fd.
> 
> Work fine.  qemu can import the dma-buf as opengl texture, which creates a
> extra reference.  Then close the fd.  dma-buf continues to exist as long as 
> the
> texture referencing it exists.
> 
> > Later these buffers may be used to expose to other kinds of fd to user
> > space.
> 
> Sorry, I don't understand that sentence.
> 
> cheers,
>   Gerd

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Friday, September 29, 2017 4:03 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Wang, Zhi
> A <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; Alex
> Williamson <alex.william...@redhat.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > > > Does the generic way need the close ioctl?
> > >
> > > I think we don't need a close ioctl anyway.
> >
> > Can you share your thoughts?
> 
> See other mail.  I think the race can be fixed by changing the locking, so a 
> explicit
> close ioctl isn't needed.
Yeah, I understand your idea. But unfortunately, it cannot solve the current 
issue.
There will still be a racing condition between releasing dmabuf_obj and reusing 
it.
For example, if the old reused dmabuf_obj is released just after query ioctl 
return it,  the next get_fd ioctl would
return error as the dmabuf_obj has already been closed.

> 
> > Do you think the fd interface is enough for all kinds of buffer
> > exposed by Mdev?
> 
> What kind of buffers do you have in mind which might not be covered?
I thinking about the case that would like to postpone the buffers releasing 
operation, after user space has closed all the fd.
Later these buffers may be used to expose to other kinds of fd to user space.
Thanks.

BR,
Tina
> 
> cheers,
>   Gerd

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Friday, September 29, 2017 3:29 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Wang, Zhi
> A <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; Alex
> Williamson <alex.william...@redhat.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Fri, 2017-09-29 at 07:04 +, Zhang, Tina wrote:
> > So, there won't be dmabuf leaking problem, as we release all the
> > dmabuf_obj in the release ops when user space crashing.
> >
> > Can we just stop considering the way to fix the dmabuf life-cycle
> > issue and try to just consider the generic way to handle buffer
> > exposing?
> 
> Can you describe in more detail what you have in mind?
> 
> > Does the generic way need the close ioctl?
> 
> I think we don't need a close ioctl anyway.
Can you share your thoughts?
Do you think the fd interface is enough for all kinds of buffer exposed by Mdev?
Thanks.

BR,
Tina

> 
> cheers,
>   Gerd
> 

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

So, there won't be dmabuf leaking problem, as we release all the dmabuf_obj in 
the release ops when user space crashing.

Can we just stop considering the way to fix the dmabuf life-cycle issue and try 
to just consider the generic way to handle buffer exposing?
Does the generic way need the close ioctl?
In my opinion, it's like to build up a producer-consumer way to expose the 
buffer:

Create buffer and return its info
Mdev devices  -> User space
   
<- 
(producer)  Close it
 (consumer)

Alex and Gerd, can you share your thoughts?
Thanks.


BR,
Tina

> -Original Message-
> From: Zhang, Tina
> Sent: Friday, September 29, 2017 7:43 AM
> To: 'Gerd Hoffmann' <kra...@redhat.com>; zhen...@linux.intel.com; Wang,
> Zhi A <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org; Alex Williamson
> <alex.william...@redhat.com>; Lv, Zhiyuan <zhiyuan...@intel.com>
> Subject: RE: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation
> 
> Thanks for the patch. Actually, I did the same thing in my local repo and 
> also, I
> have a patch for the local Qemu repo to test it. I will send them out later.
> 
> The reason why I want to propose the close IOCTL is because that the current
> lock (fb_obj_list_lock), cannot sync the intel_vgpu_fb_info releasing and
> reusing.
> You see, the intel_vgpu_fb_info reusing and releasing are in different 
> threads.
> There is a case that intel_vgpu_find_dmabuf can return a intel_vgpu_fb_obj,
> while the intel_vgpu_fb_obj is on the way to be released. That's the problem.
> 
> The invoker of the close IOCTL is only Qemu. So, if the Qemu crashes, the 
> whole
> vGPU's resource is going to be released. We can handle our dmabuf_obj to be
> released there.
> 
> Thanks.
> 
> BR,
> Tina
> 
> > -Original Message-
> > From: intel-gvt-dev
> > [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On Behalf Of Gerd
> > Hoffmann
> > Sent: Wednesday, September 27, 2017 6:11 PM
> > To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Wang,
> > Zhi A <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>;
> > intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org;
> > Alex Williamson <alex.william...@redhat.com>; Lv, Zhiyuan
> > <zhiyuan...@intel.com>
> > Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf
> > operation
> >
> >   Hi,
> >
> > > So, there is a problem about the releasing cached dmabuf_obj. We
> > > cannot rely on the drm_i915_gem_object_ops.release() to release the
> > > cached dmabuf_obj, as this release operation is running in another
> > > thread, which leads to a racing condition and tricky to be solved
> > > without touching other modules.
> >
> > PLANE_INFO just creates a intel_vgpu_dmabuf_obj.
> >
> > GET_DMABUF creates a fresh proxy gem object and dmabuf.
> >
> > proxy gem object references intel_vgpu_dmabuf_obj but not the other
> > way around.  Then you can simply refcount intel_vgpu_dmabuf_obj and be
> > done with it.
> >
> > https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-
> v14=350a0e83
> > 4
> > 971e6f53d7235d8b6167bed4dccf074
> >
> > Note: Patch renamed intel_vgpu_dmabuf_obj to intel_vgpu_fb_obj,
> > because it doesn't refer to dmabufs any more.  It basically carries
> > the guest plane/framebuffer information and the ID associated with it.
> >
> > > So, in order to solve that kind of problem, I’d like to add one more
> > > ioctl, which is used for user mode to close the dmabuf_obj.
> >
> > Depending on userspace notifying the kernel for that kind of cleanups
> > is a bad idea.  What happens in case userspace crashes?  Do you leak dmabufs
> then?
> >
> > cheers,
> >   Gerd
> >
> > ___
> > intel-gvt-dev mailing list
> > intel-gvt-...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

Thanks for the patch. Actually, I did the same thing in my local repo and also, 
I have a patch for the local Qemu repo to test it. I will send them out later.

The reason why I want to propose the close IOCTL is because that the current 
lock (fb_obj_list_lock), cannot sync the intel_vgpu_fb_info releasing and 
reusing.
You see, the intel_vgpu_fb_info reusing and releasing are in different threads. 
There is a case that intel_vgpu_find_dmabuf can return a intel_vgpu_fb_obj, 
while the intel_vgpu_fb_obj
is on the way to be released. That's the problem.

The invoker of the close IOCTL is only Qemu. So, if the Qemu crashes, the whole 
vGPU's resource is going to be released. We can handle our dmabuf_obj to be 
released there.

Thanks.

BR,
Tina

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Wednesday, September 27, 2017 6:11 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Wang, Zhi
> A <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-...@lists.freedesktop.org; Alex Williamson
> <alex.william...@redhat.com>; Lv, Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > So, there is a problem about the releasing cached dmabuf_obj. We
> > cannot rely on the drm_i915_gem_object_ops.release() to release the
> > cached dmabuf_obj, as this release operation is running in another
> > thread, which leads to a racing condition and tricky to be solved
> > without touching other modules.
> 
> PLANE_INFO just creates a intel_vgpu_dmabuf_obj.
> 
> GET_DMABUF creates a fresh proxy gem object and dmabuf.
> 
> proxy gem object references intel_vgpu_dmabuf_obj but not the other way
> around.  Then you can simply refcount intel_vgpu_dmabuf_obj and be done
> with it.
> 
> https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-v14=350a0e834
> 971e6f53d7235d8b6167bed4dccf074
> 
> Note: Patch renamed intel_vgpu_dmabuf_obj to intel_vgpu_fb_obj, because it
> doesn't refer to dmabufs any more.  It basically carries the guest
> plane/framebuffer information and the ID associated with it.
> 
> > So, in order to solve that kind of problem, I’d like to add one more
> > ioctl, which is used for user mode to close the dmabuf_obj.
> 
> Depending on userspace notifying the kernel for that kind of cleanups is a bad
> idea.  What happens in case userspace crashes?  Do you leak dmabufs then?
> 
> cheers,
>   Gerd
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

It's really tricky to handle the cached dmabuf_obj's life-cycle without 
touching other kernel modules (e.g. i915 or dmabuf).The proposed two ioctls 
will be helpful.

So, there is a problem about the releasing cached dmabuf_obj. We cannot rely on 
the drm_i915_gem_object_ops.release() to release the cached dmabuf_obj,
as this release operation is running in another thread, which leads to a racing 
condition and tricky to be solved without touching other modules.

So, in order to solve that kind of problem, I’d like to add one more ioctl, 
which is used for user mode to close the dmabuf_obj. 

Including the proposed two ioctls,  the incremental patch is like this:

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index bf40f7b..6aa6860 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -538,12 +538,33 @@ struct vfio_device_gfx_plane_info {
__u32 y_pos;/* vertical position of cursor plane, upper left corner 
in lines*/
union {
__u32 region_index; /* region index */
-   __s32 fd;   /* dma-buf fd */
+   __s32 dmabuf_id;/* dma-buf fd */
};
 };

 #define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)

+struct vfio_device_gfx_dmabuf_fd {
+ __u32 argsz;
+ __u32 flags;
+ /* in */
+__u32 dmabuf_id;
+/* out */
+__s32 dmabuf_fd;
+};
+
+#define VFIO_DEVICE_GET_GFX_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 15)
+
+
+struct vfio_device_gfx_buffer {
+__u32 argsz;
+__u32 flags;
+/* in */
+__u32 id;
+};
+
+#define VFIO_DEVICE_CLOSE_BUF _IO(VFIO_TYPE, VFIO_BASE + 16)

And here are some details:

1. VFIO_DEVICE_QUERY_GFX_PLANE: is used for user mode to ask kernel part to 
create a buffer (in dmabuf case is dmabuf_obj in kernel) and return the buffer 
info.

2. VFIO_DEVICE_GET_GFX_DMABUF: is used for user mode to ask kernel part which 
interface it likes the buffer to be wrapped.
Actually, I think there could be a bunch of types, including DMBUF type.
So, maybe we can change the IOCTL's name to some generic name and use flags 
field to distinguish them.

In dmabuf case, a new dmabuf will be created each time with this ioctl invoked, 
and installed with a new fd.
The dmabuf is just a wrapper of kernel dmabuf_obj distinguished by dmabuf_id. 
So there could be more
than one dmabuf as the wrappers of the same dmabuf_obj, if this ioctl was 
invoked with the same dmabuf_id
many times. The dmabuf and its fd is fully controlled by user mode. And the 
VFIO_DEVICE_CLOSE_BUF can
guarantee the referenced dmabuf_obj will be released at last, after all the 
dmabufs are released.

3. VFIO_DEVICE_CLOSE_BUF: is used for user mode to tell kernel part to release 
that buffer.
In dmabuf case, this ioctl won't release the dmabuf_obj at once. It just 
decreases the reference count of the dmabuf_obj
and make sure this dmabuf_obj won't be reused through 
VFIO_DEVICE_QUERY_GFX_PLANE again. At last, after all the
referencing dmabuf are released by user mode, the dmabuf_obj will be released 
by kernel.

Thanks,

BR,
Tina

> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Tuesday, September 26, 2017 3:12 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; Alex
> Williamson <alex.william...@redhat.com>; Daniel Vetter
> <daniel.vet...@ffwll.ch>
> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> [ bringing a private discussion back to the list ]
> 
> > The dma-buf's life cycle is handled by user mode and tracked by
> > kernel.
> > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > or an old fd of a re-exported dma-buf. Host user mode can check the
> > value of fd and to see if it needs to create new resource according to
> > the new fd or just use the existed resource related to the old fd.
> 
> Ok, this idea has a fundamental flaw:  The life cycle of the dma-buf and the 
> file
> handle are not identical.  The dma-buf can exist longer than the file handle 
> in
> case other references to the dma-buf exist.  So when trying to use the file 
> handle
> as identifier for the dma-buf you'll end up with all sorts of strange effects.
> 
> So, I'd suggest to use a id instead, and add a ioctl to get a dmabuf for a 
> given id
> (incremental patch):
> 
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -538,12 +538,22 @@ struct vfio_device_gfx_plane_info {
> __u32 y_pos;/* vertical position of cursor plane, upper left 
> corner in
> lines*/
> union {
> __u32 region_index; /* region index */
> -   __s32 fd;   /* dma-buf fd */
>

Re: [Intel-gfx] [PATCH v14 0/7] drm/i915/gvt: Dma-buf support for GVT-g

[Zhang, Tina]

Thanks Zhi and Gerd for testing v14 patch-set.

> -Original Message-
> From: Wang, Zhi A
> Sent: Friday, August 25, 2017 8:53 PM
> To: Gerd Hoffmann <kra...@redhat.com>; Zhang, Tina
> <tina.zh...@intel.com>; zhen...@linux.intel.com; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Tian, Kevin <kevin.t...@intel.com>;
> alex.william...@redhat.com; ch...@chris-wilson.co.uk; dan...@ffwll.ch;
> joonas.lahti...@linux.intel.com; kwankh...@nvidia.com
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org
> Subject: RE: [PATCH v14 0/7] drm/i915/gvt: Dma-buf support for GVT-g
> 
> Hi Gerd:
> Thanks for the testing. We will find out the problem and refresh the whole
> patch series.
> 
> Thanks,
> Zhi.
> 
> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Friday, August 25, 2017 2:47 PM
> To: Zhang, Tina <tina.zh...@intel.com>; zhen...@linux.intel.com; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>; Tian, Kevin
> <kevin.t...@intel.com>; alex.william...@redhat.com; chris@chris-
> wilson.co.uk; dan...@ffwll.ch; joonas.lahti...@linux.intel.com;
> kwankh...@nvidia.com
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org
> Subject: Re: [PATCH v14 0/7] drm/i915/gvt: Dma-buf support for GVT-g
> 
> On Fri, 2017-08-18 at 18:21 +0800, Tina Zhang wrote:
> > v13->v14:
> > 1) add PROBE, DMABUF and REGION flags. (Alex)
> > 2) return -ENXIO when gem proxy object is banned by ioctl.
> >    (Chris) (Daniel)
> > 3) add some details about the float pixel format. (Daniel)
> > 4) add F suffix to the defined name. (Daniel)
> > 5) refine pixel format table. (Zhenyu)
> 
> Ok, patch series applies cleanly to 4.13-rc6.  The new flags are working fine.
> 
> However, I see VFIO_DEVICE_QUERY_GFX_PLANE failures which I think should
> not be there.  When the guest didn't define a plane yet I get "No such device"
> errors instead of a plane_info struct with fields (drm_format, width, height, 
> size)
> set to zero.  I also see "Bad address" errors now and then with no obvious 
> cause.
The idea is to return "No such device" error with fields set to zero.
There are two cases, in which the "No such device" error is returned: one is 
the guest IGD driver
has not finished the initialization and the other is the plane is disabled by 
guest IGD driver.
If we prefer to return success in these two situations with fields set to zero, 
I can add it in the
next version.

We didn't meet the "Bad address" errors before. I will try your qemu repo to 
see whether I can
meet the issue. Thanks.
 
> Cursor support isn't working too.
It seems there is some issue in i915 of version 4.13-rc6, which blocks the 
cursor on native platform.
I just tried the rc7 (the latest staging), there is on such issue.
Thanks.

Tina
> 
> I'm testing with "-display egl-headless -spice gl=off,port=...".  In that
> configuration qemu will import the dma-bufs as textures and reads them using
> ReadPixels for display.
> 
> qemu branch: https://www.kraxel.org/cgit/qemu/log/?h=work/intel-vgpu
> 
> The qemu branch has support for both dmabufs and regions.  The region- based
> display code is totaly untested though.
> 
> cheers,
>   Gerd

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Return -EPERM when i915_gem_mmap_ioctl handling prime objects

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Chris Wilson
> Sent: Tuesday, August 15, 2017 11:02 PM
> To: Daniel Vetter <dan...@ffwll.ch>
> Cc: intel-gfx <intel-gfx@lists.freedesktop.org>; intel-gvt-dev  d...@lists.freedesktop.org>; Zhang, Tina <tina.zh...@intel.com>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Return -EPERM when
> i915_gem_mmap_ioctl handling prime objects
> 
> Quoting Daniel Vetter (2017-08-15 15:48:03)
> > On Tue, Aug 15, 2017 at 4:35 PM, Chris Wilson <ch...@chris-wilson.co.uk>
> wrote:
> > > Quoting Chris Wilson (2017-08-15 15:32:41)
> > >> Quoting Daniel Vetter (2017-08-15 15:25:46)
> > >> > On Tue, Aug 15, 2017 at 3:18 PM, Chris Wilson <chris@chris-
> wilson.co.uk> wrote:
> > >> > > Quoting Daniel Vetter (2017-08-15 11:49:51)
> > >> > >> On Tue, Aug 15, 2017 at 10:42 AM, Tina Zhang
> <tina.zh...@intel.com> wrote:
> > >> > >> > Prime objects have no backing filp to GEM mmap pages from.
> > >> > >> > So, for Prime objects, such operation is not permitted.
> > >> > >>
> > >> > >> EPERM is when you don't have enough permissions, but it's
> > >> > >> possible (e.g. a feature requiring root, or drm master).
> > >> > >> -EINVAL is if something is invalid, and not even root can
> > >> > >> change that. So from a consistency pov, EINVAL is the right error 
> > >> > >> code
> here I think.
> > >> > >
> > >> > > Consistency is that we wanted the same error code for all
> > >> > > objects where you did not have the ability to change the underlying
> storage.
> > >> > >
> > >> > > The question is that an access issue or a permission issue. But
> > >> > > at the very least, mmap_ioctl is the odd one out. Which the
> > >> > > changelog did not explain and being sent out of context does not 
> > >> > > help.
> > >> >
> > >> > Which other ioctl give you an EPERM for something that doesn't
> > >> > even work when you're root and/or drm master or whatever it is
> > >> > that gives you permission? I thought we've been pretty consistent
> > >> > with that one ...
> > >>
> > >> https://patchwork.freedesktop.org/series/28709/
> > >
> > > Oops, https://patchwork.freedesktop.org/series/27844/
> > >
> > >> Short story is that we add a new set of second class GEM objects
> > >> that are not allowed to change the backing storage or details of the PTE.
> > >>
> > >> Not happy about the dysfunctional GEM objects, but we do want a
> > >> clear and consistent indication as to why we start rejecting certain 
> > >> ioctls.
> >
> > I guess two questions:
> > - Does userspace really care about the different return value?
> 
> I'm expecting to get a bug report as soon as someone tries to mix it with an 
> EGL
> image. Once we hand out dma-bufs to partial objects, they will show up
> everywhere and randomly break. Trying to save hassle later.
> 
> > Or is
> > the use case more that we have very specific userspace which knows not
> > to do stupid things with these special gvt dma-bufs? If no, then I'd
> > go with EINVAL + DRM_DEBUG_DRIVER.
> 
> DRM_DEBUG, for user error not driver and because we don't have a dedicated
> channel for complete error messages to give to the user. :| Behind a private
> channel we could report more details that only make sense to the caller, or 
> may
> simply need to be kept private. Dream on!
> 
> > - If we need that special errno, can we take something else? EPERM imo
> > has fairly specific meaning. ENODEV/ENOTTY are more the "not supported
> > on this thing" error codes, if we need a special one. They also have
> > other meanings attached already, but then everything excpe EINVAL has
> > when we do an ioctl, since the vfs can already throw these at you
> > anyway.
> 
> ENODEV at the ioctl level we already have to mean that the device doesn't
> support the operation, but not the object. (Then internally we've used ENODEV
> to indicate programmer error.)
> 
> ENOTTY too easy to confuse with the absent ioctl?
> 
> ENXIO is not bad, basically says the remote channel does not support the
> operation.
"ENXIO" looks fine to me. If everyone is onboard with this, we will replace 
"EINVAL" with it
in this patch, and also the ones in 
https://patchwork.freedesktop.org/patch/168810/
Thanks.

BR,
Tina

> -Chris
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v13 2/7] drm: Introduce RGB 64-bit 16:16:16:16 float format

[Zhang, Tina]

> -Original Message-
> From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, August 15, 2017 10:50 PM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; ville.syrj...@linux.intel.com;
> zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A
> <zhi.a.w...@intel.com>; alex.william...@redhat.com; kra...@redhat.com;
> ch...@chris-wilson.co.uk; dan...@ffwll.ch; kwankh...@nvidia.com; Tian, Kevin
> <kevin.t...@intel.com>; Chen, Xiaoguang <xiaoguang.c...@intel.com>
> Subject: Re: [PATCH v13 2/7] drm: Introduce RGB 64-bit 16:16:16:16 float
> format
> 
> On Tue, Jul 25, 2017 at 05:28:15PM +0800, Tina Zhang wrote:
> > The RGB 64-bit 16:16:16:16 float pixel format is needed by windows
> > guest VM. This patch is to introduce the format to drm.
> >
> > v1:
> > Suggested by Ville to submit this patch to dri-devel.
> >
> > Signed-off-by: Xiaoguang Chen <xiaoguang.c...@intel.com>
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h
> > b/include/uapi/drm/drm_fourcc.h index 7586c46..3e002e3 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -113,6 +113,10 @@ extern "C" {
> >
> >  #define DRM_FORMAT_AYUVfourcc_code('A', 'Y', 'U', 'V') /* 
> > [31:0]
> A:Y:Cb:Cr 8:8:8:8 little endian */
> >
> > +/* 64 bpp RGB */
> > +#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /*
> > +[63:0] x:R:G:B 16:16:16:16 little endian */ #define
> > +DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0]
> > +x:B:G:R 16:16:16:16 little endian */
> 
> I think the comment should go a bit more into detail what the float format is
> supposed to look like. And I think we should have a F or _FLOAT suffix to the
> defined name, since the same layout could also work for integers (and some hw
> somewhere probably has that. Maybe also put that F into the last slot of the
> fourcc.
Thanks. I prefer to add F and also put F into the last slot of the fourcc. 
I'm not sure about the code rule. Does it make sense?
#define DRM_FORMAT_XRGB161616F fourcc_code('X', 'R', '3', 'F')
Where '3' stands for the three fields (R, G, B) without Alpha.

BR,
Tina
> -Daniel
> > +
> >  /*
> >   * 2 plane RGB + A
> >   * index 0 = RGB plane, same format as the corresponding non _A8
> > format has
> > --
> > 2.7.4
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 0/3] vGPU full 48bit ppgtt support

[Zhang, Tina]

> -Original Message-
> From: Zhenyu Wang [mailto:zhen...@linux.intel.com]
> Sent: Monday, August 14, 2017 6:12 PM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Joonas Lahtinen
> <joonas.lahti...@linux.intel.com>; intel-gvt-...@lists.freedesktop.org
> Subject: Re: [PATCH v10 0/3] vGPU full 48bit ppgtt support
> 
> On 2017.08.14 10:09:38 +, Zhang, Tina wrote:
> > > Just refresh against Joonas's disconnect 32/48bit ppgtt support
> > > patch and give full view on this series. If ok, I'll include them for 
> > > gvt-next pull.
> > Thanks Zhenyu. Looks like one patch is missing:
> > https://lists.freedesktop.org/archives/intel-gvt-dev/2017-August/00164
> > 1.html This patch can fix the rebooting issue with 48bit full ppgtt.
> 
> I've applied that one already as its a fix for generic shadow issue, this 
> just shows
> changes for vgpu full ppgtt, so skipped that.
OK. Thanks

BR,
Tina
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 0/3] vGPU full 48bit ppgtt support

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Zhenyu Wang
> Sent: Monday, August 14, 2017 3:42 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Joonas Lahtinen ; intel-gvt-
> d...@lists.freedesktop.org
> Subject: [PATCH v10 0/3] vGPU full 48bit ppgtt support
> 
> Just refresh against Joonas's disconnect 32/48bit ppgtt support patch and give
> full view on this series. If ok, I'll include them for gvt-next pull.
Thanks Zhenyu. Looks like one patch is missing:
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-August/001641.html
This patch can fix the rebooting issue with 48bit full ppgtt.
Thanks.

Tina
> 
> Thanks
> 
> Joonas Lahtinen (1):
>   drm/i915: Disconnect 32 and 48 bit ppGTT support
> 
> Tina Zhang (2):
>   drm/i915: Enable guest i915 full ppgtt functionality
>   drm/i915/gvt: Fix guest i915 full ppgtt blocking issue
> 
>  drivers/gpu/drm/i915/gvt/gtt.c  | 45 +++
> --
>  drivers/gpu/drm/i915/gvt/vgpu.c |  1 +
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 17 +-
> drivers/gpu/drm/i915/i915_pvinfo.h  |  8 ++-
>  drivers/gpu/drm/i915/i915_vgpu.c|  7 ++
>  drivers/gpu/drm/i915/i915_vgpu.h|  3 +++
>  7 files changed, 58 insertions(+), 24 deletions(-)
> 
> --
> 2.14.0
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Alex Williamson
> Sent: Tuesday, August 8, 2017 1:43 AM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; kwankh...@nvidia.com; kra...@redhat.com;
> intel-gvt-...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>; Lv,
> Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Mon, 7 Aug 2017 08:11:43 +
> "Zhang, Tina" <tina.zh...@intel.com> wrote:
> 
> > After going through the previous discussions, here are some summaries may
> be related to the current discussion:
> > 1. How does user mode figure the device capabilities between region and
> dma-buf?
> > VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case.
> > Otherwise, the mdev supports dma-buf.
> 
> Why do we need to make this assumption?  What happens when dma-buf is
> superseded?  What happens if a device supports both dma-buf and regions?
> We have a flags field in vfio_device_gfx_plane_info, doesn't it make sense to 
> use
> it to identify which field, between region_index and fd, is valid?  We could 
> even
> put region_index and fd into a union with the flag bits indicating how to
> interpret the union, but I'm not sure everyone was onboard with this idea.
> Seems like a waste of 4 bytes not to do that though.
It seems we discussed this idea before:
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001304.html
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001333.html
Thanks.

Tina
> 
> Thinking further, is the user ever in a situation where they query the 
> graphics
> plane info and can handle either a dma-buf or a region?  It seems more likely
> that the user needs to know early on which is supported and would then require
> that they continue to see compatible plane information...  Should the user 
> then
> be able to specify whether they want a dma-buf or a region?  Perhaps these 
> flag
> bits are actually input and the return should be -errno if the driver cannot
> produce something compatible.
From the previously discussion, it seems user space workflow will look quite 
different
for these two cases. So once user space finds out which case is supported, it 
just uses
that case, and won't change it.

Meanwhile, I'm not sure whether there will be a mdev would like to support both 
region
and dma-buf cases. In my opinion, either region or dma-buf is supported by one 
mdev. (Yeah,
agree, there may be other cases in future)
It's like we want to propose a general interface used to share guest's buffer 
with host. And the
general interface, so far, has two choice: region and dma-buf. So each mdev 
likes this interface
can implement one kind of it and gets the benefit from the general interface.
So, if we think about this, the difference in user mode should be as little as 
possible.
Thanks.

Tina
> 
> Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In this
> initial implementation, DMABUF or REGION would always be set by the user to
> request that type of interface.  Additionally, the QUERY bit could be set to 
> probe
> compatibility, thus if PROBE and REGION are set, the vendor driver would 
> return
> success only if it supports the region based interface.  If PROBE and DMABUF 
> are
> set, the vendor driver returns success only if the dma-buf based interface is
> supported.  The value of the remainder of the structure is undefined for 
> PROBE.
> Additionally setting both DMABUF and REGION is invalid.  Undefined flags bits
> must be validated as zero by the drivers for future use (thus if we later 
> define
> DMABUFv2, an older driver should automatically return -errno when probed or
> requested).
> 
> It seems like this handles all the cases, the user can ask what's supported 
> and
> specifies the interface they want on every call.  The user therefore can also
> choose between region_index and fd and we can make that a union.
Agree, that's a good proposal, which can handle all the cases.
I'm just not sure about the usage case of "on every call". In previous 
discussion, it seems we think static is enough.
Thanks.

Tina

> 
> > 2. For dma-buf, how to differentiate unsupported vs not initialized?
> > For dma-buf, when the mdev doesn't support some arguments, -EINVAL will
> be returned. And -errno will return when meeting other failures, like -ENOMEM.
> > If the mdev is not initialized, there won't be any returned err. Just zero 
> > all the
> fields in structure vfio_device_gfx_plane_info.
> 
> So w

Re: [Intel-gfx] [PATCH v13 6/7] drm/i915: Introduce GEM proxy

[Zhang, Tina]

> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of
> Joonas Lahtinen
> Sent: Monday, August 7, 2017 4:24 PM
> To: Zhang, Tina <tina.zh...@intel.com>; intel-gfx@lists.freedesktop.org; 
> intel-
> gvt-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
> ville.syrj...@linux.intel.com; zhen...@linux.intel.com; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>;
> alex.william...@redhat.com; kra...@redhat.com; ch...@chris-wilson.co.uk;
> dan...@ffwll.ch; kwankh...@nvidia.com; Tian, Kevin <kevin.t...@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v13 6/7] drm/i915: Introduce GEM proxy
> 
> On ti, 2017-07-25 at 17:28 +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.
> >
> 
> It is a good idea to add a changelog here to indicate what was changed since 
> the
> last patch revision. It'll be easier for the reviewer to spot the differences.
> 
> It's also a good idea to add Cc: line for somebody who provided previous 
> review,
> this will, too, allow spotting the patch quicker.
Indeed. Thanks.

Tina
> 
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> 
> 
> 
> > @@ -1730,7 +1744,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev,
> void *data,
> >      */
> >     if (!obj->base.filp) {
> >     i915_gem_object_put(obj);
> > -   return -EINVAL;
> > +   return -EPERM;
> >     }
> 
> This hunk should be separate bugfix patch, but other than that the patch looks
> OK to me.
Thanks. I will send this patch separately.

Tina
> 
> I'll let Chris comment if he feels previous comments were adequately 
> addressed.
> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

After going through the previous discussions, here are some summaries may be 
related to the current discussion:
1. How does user mode figure the device capabilities between region and dma-buf?
VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
Otherwise, the mdev supports dma-buf.

2. For dma-buf, how to differentiate unsupported vs not initialized?
For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be 
returned. And -errno will return when meeting other failures, like -ENOMEM.
If the mdev is not initialized, there won't be any returned err. Just zero all 
the fields in structure vfio_device_gfx_plane_info.

3. The id field in structure vfio_device_gfx_plane_info
So far we haven't figured out the usage of this field for dma-buf usage. So, 
this field is changed to "region_index" and only used for region usage.
In previous discussions, we thought this "id" field might be used for both 
dma-buf and region usages.
So, we proposed some ways, like adding flags field to the structure. Another 
way to do it was to add this:
enum vfio_device_gfx_type {
 VFIO_DEVICE_GFX_NONE,
 VFIO_DEVICE_GFX_DMABUF,
 VFIO_DEVICE_GFX_REGION,
 };
 
 struct vfio_device_gfx_query_caps {
 __u32 argsz;
 __u32 flags;
 enum vfio_device_gfx_type;
 };
Obviously, we don't need to consider this again, unless we find the 
"region_index" means something for dma-buf usage.

Thanks.

BR,
Tina

> -----Original Message-
> From: Zhang, Tina
> Sent: Monday, August 7, 2017 11:23 AM
> To: 'Alex Williamson' <alex.william...@redhat.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; kwankh...@nvidia.com; kra...@redhat.com;
> intel-gvt-...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>; Lv,
> Zhiyuan <zhiyuan...@intel.com>
> Subject: RE: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> 
> 
> > -Original Message-
> > From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On
> > Behalf Of Alex Williamson
> > Sent: Thursday, August 3, 2017 10:43 PM
> > To: Zhang, Tina <tina.zh...@intel.com>
> > Cc: Tian, Kevin <kevin.t...@intel.com>;
> > intel-gfx@lists.freedesktop.org; dri- de...@lists.freedesktop.org;
> > kwankh...@nvidia.com; kra...@redhat.com;
> > intel-gvt-...@lists.freedesktop.org; Wang, Zhi A
> > <zhi.a.w...@intel.com>; Lv, Zhiyuan <zhiyuan...@intel.com>
> > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > operation
> >
> > On Thu, 3 Aug 2017 07:08:15 +
> > "Zhang, Tina" <tina.zh...@intel.com> wrote:
> >
> > > > -Original Message-
> > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > Sent: Thursday, August 3, 2017 11:38 AM
> > > > To: Zhang, Tina <tina.zh...@intel.com>
> > > > Cc: Tian, Kevin <kevin.t...@intel.com>;
> > > > intel-gfx@lists.freedesktop.org; dri- de...@lists.freedesktop.org;
> > > > kwankh...@nvidia.com; kra...@redhat.com;
> > > > intel-gvt-...@lists.freedesktop.org; Wang, Zhi A
> > > > <zhi.a.w...@intel.com>; Lv, Zhiyuan <zhiyuan...@intel.com>
> > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > > operation
> > > >
> > > > On Thu, 3 Aug 2017 03:17:09 +
> > > > "Zhang, Tina" <tina.zh...@intel.com> wrote:
> > > >
> > > > > > -Original Message-
> > > > > > From: dri-devel
> > > > > > [mailto:dri-devel-boun...@lists.freedesktop.org]
> > > > > > On Behalf Of Alex Williamson
> > > > > > Sent: Wednesday, August 2, 2017 5:18 AM
> > > > > > To: Zhang, Tina <tina.zh...@intel.com>
> > > > > > Cc: Tian, Kevin <kevin.t...@intel.com>; kra...@redhat.com;
> > > > > > intel- g...@lists.freedesktop.org; Wang, Zhi A
> > > > > > <zhi.a.w...@intel.com>; kwankh...@nvidia.com;
> > > > > > dri-de...@lists.freedesktop.org; intel-gvt-
> > > > > > d...@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan...@intel.com>
> > > > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display
> > > > > > dma-buf operation
> > > > > >
> > > > > > On Tue, 25 Jul 2017 17:28:18 +0800 Tina Zhang
> > > > > > <tina.zh...@intel.com> wrote:
> > > > > >
> > > > > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl comm

Re: [Intel-gfx] [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of
> Alex Williamson
> Sent: Thursday, August 3, 2017 10:43 PM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; kwankh...@nvidia.com; kra...@redhat.com;
> intel-gvt-...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>; Lv,
> Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Thu, 3 Aug 2017 07:08:15 +
> "Zhang, Tina" <tina.zh...@intel.com> wrote:
> 
> > > -Original Message-
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Thursday, August 3, 2017 11:38 AM
> > > To: Zhang, Tina <tina.zh...@intel.com>
> > > Cc: Tian, Kevin <kevin.t...@intel.com>;
> > > intel-gfx@lists.freedesktop.org; dri- de...@lists.freedesktop.org;
> > > kwankh...@nvidia.com; kra...@redhat.com;
> > > intel-gvt-...@lists.freedesktop.org; Wang, Zhi A
> > > <zhi.a.w...@intel.com>; Lv, Zhiyuan <zhiyuan...@intel.com>
> > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > operation
> > >
> > > On Thu, 3 Aug 2017 03:17:09 +
> > > "Zhang, Tina" <tina.zh...@intel.com> wrote:
> > >
> > > > > -Original Message-
> > > > > From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org]
> > > > > On Behalf Of Alex Williamson
> > > > > Sent: Wednesday, August 2, 2017 5:18 AM
> > > > > To: Zhang, Tina <tina.zh...@intel.com>
> > > > > Cc: Tian, Kevin <kevin.t...@intel.com>; kra...@redhat.com;
> > > > > intel- g...@lists.freedesktop.org; Wang, Zhi A
> > > > > <zhi.a.w...@intel.com>; kwankh...@nvidia.com;
> > > > > dri-de...@lists.freedesktop.org; intel-gvt-
> > > > > d...@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan...@intel.com>
> > > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > > > operation
> > > > >
> > > > > On Tue, 25 Jul 2017 17:28:18 +0800 Tina Zhang
> > > > > <tina.zh...@intel.com> wrote:
> > > > >
> > > > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user
> mode
> > > > > > query and get the plan and its related information.
> > > > > >
> > > > > > The dma-buf's life cycle is handled by user mode and tracked by 
> > > > > > kernel.
> > > > > > The returned fd in struct vfio_device_query_gfx_plane can be a
> > > > > > new fd or an old fd of a re-exported dma-buf. Host User mode
> > > > > > can check the value of fd and to see if it needs to create new
> > > > > > resource according to the new fd or just use the existed
> > > > > > resource related to the old
> > > fd.
> > > > > >
> > > > > > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > > > > > ---
> > > > > >  include/uapi/linux/vfio.h | 28 
> > > > > >  1 file changed, 28 insertions(+)
> > > > > >
> > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230 100644
> > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > > > > >
> > > > > >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE +
> > > > > 13)
> > > > > >
> > > > > > +/**
> > > > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE
> +
> > > 14,
> > > > > > +struct vfio_device_query_gfx_plane)
> > > > > > + *
> > > > > > + * Set the drm_plane_type and retrieve information about the gfx
> plane.
> > > > > > + *
> > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > + */
> > > > > > +struct vfio_device_gfx_plane_info {
> > > > > > +   __u32 argsz;
> > > > > > +   __u32 flags;
> > > > > > +   /* in */
> > > > &g

Re: [Intel-gfx] [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Thursday, August 3, 2017 11:38 AM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; kwankh...@nvidia.com; kra...@redhat.com;
> intel-gvt-...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>; Lv,
> Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Thu, 3 Aug 2017 03:17:09 +
> "Zhang, Tina" <tina.zh...@intel.com> wrote:
> 
> > > -Original Message-
> > > From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On
> > > Behalf Of Alex Williamson
> > > Sent: Wednesday, August 2, 2017 5:18 AM
> > > To: Zhang, Tina <tina.zh...@intel.com>
> > > Cc: Tian, Kevin <kevin.t...@intel.com>; kra...@redhat.com; intel-
> > > g...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>;
> > > kwankh...@nvidia.com; dri-de...@lists.freedesktop.org; intel-gvt-
> > > d...@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan...@intel.com>
> > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > operation
> > >
> > > On Tue, 25 Jul 2017 17:28:18 +0800
> > > Tina Zhang <tina.zh...@intel.com> wrote:
> > >
> > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode
> > > > query and get the plan and its related information.
> > > >
> > > > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > > > The returned fd in struct vfio_device_query_gfx_plane can be a new
> > > > fd or an old fd of a re-exported dma-buf. Host User mode can check
> > > > the value of fd and to see if it needs to create new resource
> > > > according to the new fd or just use the existed resource related to the 
> > > > old
> fd.
> > > >
> > > > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > > > ---
> > > >  include/uapi/linux/vfio.h | 28 
> > > >  1 file changed, 28 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > index ae46105..827a230 100644
> > > > --- a/include/uapi/linux/vfio.h
> > > > +++ b/include/uapi/linux/vfio.h
> > > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > > >
> > > >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE +
> > > 13)
> > > >
> > > > +/**
> > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE +
> 14,
> > > > +struct vfio_device_query_gfx_plane)
> > > > + *
> > > > + * Set the drm_plane_type and retrieve information about the gfx plane.
> > > > + *
> > > > + * Return: 0 on success, -errno on failure.
> > > > + */
> > > > +struct vfio_device_gfx_plane_info {
> > > > +   __u32 argsz;
> > > > +   __u32 flags;
> > > > +   /* in */
> > > > +   __u32 drm_plane_type;   /* type of plane: DRM_PLANE_TYPE_* */
> > > > +   /* out */
> > > > +   __u32 drm_format;   /* drm format of plane */
> > > > +   __u64 drm_format_mod;   /* tiled mode */
> > > > +   __u32 width;/* width of plane */
> > > > +   __u32 height;   /* height of plane */
> > > > +   __u32 stride;   /* stride of plane */
> > > > +   __u32 size; /* size of plane in bytes, align on page*/
> > > > +   __u32 x_pos;/* horizontal position of cursor plane, upper 
> > > > left corner
> > > in pixels */
> > > > +   __u32 y_pos;/* vertical position of cursor plane, upper 
> > > > left corner in
> > > lines*/
> > > > +   __u32 region_index;
> > > > +   __s32 fd;   /* dma-buf fd */
> > >
> > > How do I know which of these is valid, region_index or fd?  Can I
> > > ask for one vs the other?  What are the errno values to
> > > differentiate unsupported vs not initialized?  Is there a "probe"
> > > flag that I can use to determine what the device supports without 
> > > allocating
> those resources yet?
> > Dma-buf won't use region_index, which means region_index is zero all the
> time for dma-buf usage.
> > As we discussed, there won't be errno if not initialized, just keep all 
> > fields zero.
> > I will add the comments about these in the next version. Thanks
> 
> Zero is a valid region index.
If zero is valid, can we find out other invalid value? How about 0x?

Tina
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of
> Alex Williamson
> Sent: Wednesday, August 2, 2017 5:18 AM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; kra...@redhat.com; intel-
> g...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>;
> kwankh...@nvidia.com; dri-de...@lists.freedesktop.org; intel-gvt-
> d...@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Tue, 25 Jul 2017 17:28:18 +0800
> Tina Zhang <tina.zh...@intel.com> wrote:
> 
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> > and get the plan and its related information.
> >
> > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > or an old fd of a re-exported dma-buf. Host User mode can check the
> > value of fd and to see if it needs to create new resource according to
> > the new fd or just use the existed resource related to the old fd.
> >
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > ---
> >  include/uapi/linux/vfio.h | 28 
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..827a230 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> >
> >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE +
> 13)
> >
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > +struct vfio_device_query_gfx_plane)
> > + *
> > + * Set the drm_plane_type and retrieve information about the gfx plane.
> > + *
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +struct vfio_device_gfx_plane_info {
> > +   __u32 argsz;
> > +   __u32 flags;
> > +   /* in */
> > +   __u32 drm_plane_type;   /* type of plane: DRM_PLANE_TYPE_* */
> > +   /* out */
> > +   __u32 drm_format;   /* drm format of plane */
> > +   __u64 drm_format_mod;   /* tiled mode */
> > +   __u32 width;/* width of plane */
> > +   __u32 height;   /* height of plane */
> > +   __u32 stride;   /* stride of plane */
> > +   __u32 size; /* size of plane in bytes, align on page*/
> > +   __u32 x_pos;/* horizontal position of cursor plane, upper left 
> > corner
> in pixels */
> > +   __u32 y_pos;/* vertical position of cursor plane, upper left corner 
> > in
> lines*/
> > +   __u32 region_index;
> > +   __s32 fd;   /* dma-buf fd */
> 
> How do I know which of these is valid, region_index or fd?  Can I ask for one 
> vs
> the other?  What are the errno values to differentiate unsupported vs not
> initialized?  Is there a "probe" flag that I can use to determine what the 
> device
> supports without allocating those resources yet?
Dma-buf won't use region_index, which means region_index is zero all the time 
for dma-buf usage. 
As we discussed, there won't be errno if not initialized, just keep all fields 
zero.
I will add the comments about these in the next version. Thanks

Tina

> 
> Kirti, does this otherwise meet your needs?
> 
> Thanks,
> Alex
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Friday, July 28, 2017 4:27 PM
> To: Zhang, Tina <tina.zh...@intel.com>; intel-gfx@lists.freedesktop.org; 
> intel-
> gvt-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
> ville.syrj...@linux.intel.com; zhen...@linux.intel.com; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>;
> alex.william...@redhat.com; ch...@chris-wilson.co.uk; dan...@ffwll.ch;
> kwankh...@nvidia.com; Tian, Kevin <kevin.t...@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > struct vfio_device_query_gfx_plane)
> > + *
> > + * Set the drm_plane_type and retrieve information about the gfx
> > plane.
> + *
> > + * Return: 0 on success, -errno on failure.
> 
> I think this should be more verbose, especially documenting that the "guest
> driver didn't initialize the display yet" case isn't and error and fields 
> should be set
> to zero then (as discussed on the list).
I can add this in the next version. 
Thanks.

Tina
> 
> > + */
> > +struct vfio_device_gfx_plane_info {
> > +   __u32 argsz;
> > +   __u32 flags;
> > +   /* in */
> > +   __u32 drm_plane_type;   /* type of plane:
> > DRM_PLANE_TYPE_* */
> > +   /* out */
> > +   __u32 drm_format;   /* drm format of plane */
> > +   __u64 drm_format_mod;   /* tiled mode */
> > +   __u32 width;/* width of plane */
> > +   __u32 height;   /* height of plane */
> > +   __u32 stride;   /* stride of plane */
> > +   __u32 size; /* size of plane in bytes, align on
> > page*/
> > +   __u32 x_pos;/* horizontal position of cursor plane,
> > upper left corner in pixels */
> > +   __u32 y_pos;/* vertical position of cursor plane,
> > upper left corner in lines*/
> > +   __u32 region_index;
> > +   __s32 fd;   /* dma-buf fd */
> > +};
> 
> Looks good to me.
> 
> Unfortunately I havn't been able to test the whole series yet due to being 
> busy
> with other stuff, and I'm about to leave for my summer vacation.  Will be back
> online on Aug 21st.
Fine to me. I will also update our qemu sample code and some wiki according to 
the current interface in the next version, which
may give you some help for your test.
Thanks.

Tina

> 
> cheers,
>   Gerd
> 
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v12 6/6] drm/i915: Introduce GEM proxy

[Zhang, Tina]

> -Original Message-
> From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> Sent: Wednesday, July 19, 2017 7:20 PM
> To: Zhang, Tina <tina.zh...@intel.com>; intel-gfx@lists.freedesktop.org; 
> intel-
> gvt-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
> ville.syrj...@linux.intel.com; zhen...@linux.intel.com; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>;
> alex.william...@redhat.com; kra...@redhat.com; dan...@ffwll.ch;
> kwankh...@nvidia.com; Tian, Kevin <kevin.t...@intel.com>
> Cc: Zhang, Tina <tina.zh...@intel.com>
> Subject: Re: [PATCH v12 6/6] drm/i915: Introduce GEM proxy
> 
> s/GEM proxy/a GEM proxy object/
> 
> Quoting Tina Zhang (2017-07-19 11:59:05)
> 
> Rationale goes here.
Thanks for the comments. The idea behind the GEM proxy is that we want to 
propose a kind of GEM which content is produced by the guest VM and used by 
host. So, to host, this kind of GEM should be read only (both for CPU and GPU), 
and host cannot use ioctls to change or modify the GEM.
I will add more comments in the next version. Thanks.

> 
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c| 26 +-
> >  drivers/gpu/drm/i915/i915_gem_object.h |  9 +
> > drivers/gpu/drm/i915/i915_gem_tiling.c |  5 +
> >  3 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 1b2dfa8..ebacc04 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1612,6 +1612,12 @@ i915_gem_set_domain_ioctl(struct drm_device
> *dev, void *data,
> > if (!obj)
> > return -ENOENT;
> >
> > +   /* proxy gem object does not support setting domain */
> 
> Yes, this is what the code is doing. The comment tells us why.
> 
> /*
>  * Proxy objects do not control access to the backing storage, ergo
>  * they cannot be used as a means to manipulate the cache domain
>  * tracking for that backing storage. The proxy object is always
>  * considered to be outside of any cache domain.
>  */
> 
> However, set-domain does have some other side-effects that includes waiting
> which should still be performed, i.e. this check should be after the lockless 
> wait.
Is it the requirement of this ioctl? Other ioctls here in this patch, won't 
need it?

> 
> > +   if (i915_gem_object_is_proxy(obj)) {
> > +   err = -EPERM;
> > +   goto out;
> > +   }
> > +
> > /* Try to flush the object off the GPU without holding the lock.
> >  * We will repeat the flush holding the lock in the normal manner
> >  * to catch cases where we are gazumped.
> > @@ -1680,6 +1686,12 @@ i915_gem_sw_finish_ioctl(struct drm_device
> *dev, void *data,
> > if (!obj)
> > return -ENOENT;
> >
> 
> A comment could explain that since proxy objects are barred from CPU access,
> we do not need to ban sw_finish as it is a nop.
Agree.

> 
> > +   /* proxy gem obj does not support this operation */
> > +   if (i915_gem_object_is_proxy(obj)) {
> > +   i915_gem_object_put(obj);
> > +   return -EPERM;
> > +   }
> > +
> > /* Pinned buffers may be scanout, so flush the cache */
> > i915_gem_object_flush_if_display(obj);
> > i915_gem_object_put(obj);
> > @@ -1730,7 +1742,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev,
> void *data,
> >  */
> > if (!obj->base.filp) {
> > i915_gem_object_put(obj);
> > -   return -EINVAL;
> > +   return -EPERM;
> > }
> >
> > addr = vm_mmap(obj->base.filp, 0, args->size, @@ -3764,6
> > +3776,12 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void
> *data,
> > if (!obj)
> > return -ENOENT;
> >
> > +   /* proxy gem obj does not support setting caching mode */
> 
> More rationale. Also is the proxied object (its target) also banned from 
> changing
> the PTE bits or do we inherit all such changes automatically?
From v2, cache_level isn't be set during the GEM generation, i.e. 
cache_level=0. And the PTE bits which is set by guest, won't be modified by 
host.

> 
> > +   if (i915_gem_object_is_proxy(obj)) {
> > +   ret = -EPERM;
> > +   goto out;
> > +   }
> > +
> > if (obj->cache_level == level)
>

Re: [Intel-gfx] [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Zhenyu Wang
> Sent: Monday, July 17, 2017 10:27 AM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; intel-gfx@lists.freedesktop.org;
> kwankh...@nvidia.com; zhen...@linux.intel.com; ch...@chris-wilson.co.uk;
> alex.william...@redhat.com; Lv, Zhiyuan <zhiyuan...@intel.com>;
> dan...@ffwll.ch; intel-gvt-...@lists.freedesktop.org; Wang, Zhi A
> <zhi.a.w...@intel.com>; Gerd Hoffmann <kra...@redhat.com>
> Subject: Re: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> 
> On 2017.07.17 01:10:03 +, Zhang, Tina wrote:
> > > > +struct vfio_device_gfx_plane_info {
> > > > +   __u32 argsz;
> > > > +   __u32 flags;
> > > > +   /* in */
> > > > +   __u32 drm_plane_type;   /* type of plane:
> > > > DRM_PLANE_TYPE_* */
> > > > +   /* out */
> > > > +   __u32 drm_format;   /* drm format of plane */
> > >
> > > DRM_FORMAT_*
> > >
> > > drm_format_mod is gone?  Not needed?
> > > How tiled buffers are handled then?
> > Drm_format_mod is used as one of the plane's characteristics when judging
> the dma-buf of the plane is new to expose or an old exposed one. As from V10
> we leave this comparing logic to kernel, user mode might not need this field 
> any
> more. If user mode wants, this can be also got though drm APIs. For example,
> eglCreateImageKHR() uses drm APIs to get the tiling format, without asking the
> invoker to explicitly use it as a parameter.
> > Do you think this field is still needed?
> >
> 
> Of course we need that modifier for complete format info. Don't just think for
> i915 usage, there's possible modifier for other vendor driver, and it's 
> required
> for e.g ADDFB2 in drm kms. Pls add it back in next version.
We definitely can add it back if it is thought to be useful. Just some curious, 
as we don't produce the content in the GEM buffer (a.k.a we just expose it 
here), why we need to know the tile mode. In addition, DRM API can support the 
functionality to get the tile mode ot GEM buffer. 

Thanks,
Tina
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Monday, July 17, 2017 7:03 PM
> To: Kirti Wankhede <kwankh...@nvidia.com>; Zhang, Tina
> <tina.zh...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; linux-
> ker...@vger.kernel.org; intel-gfx@lists.freedesktop.org;
> alex.william...@redhat.com; zhen...@linux.intel.com; chris@chris-
> wilson.co.uk; Lv, Zhiyuan <zhiyuan...@intel.com>; intel-gvt-
> d...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > No need of flag here. If vGPU driver is not loaded in the guest, there
> > is no surface being managed by vGPU, in that case this size will be
> > zero.
> 
> Ok, we certainly have the same situation with intel.  When the guest driver 
> is not
> loaded (yet) there is no valid surface.
> 
> We should cleanly define what the ioctl should do in that case, so all drivers
> behave the same way.
> 
> I'd suggest that all fields defining the surface (drm_format, width, height, 
> stride,
> size) should be set to zero in that case.
Yeah, it's reasonable. How about the return value? Currently, the ioctl also 
returns "-ENODEV" in that situation.
 
thanks,
Tina
> 
> cheers,
>   Gerd

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Friday, July 14, 2017 6:11 PM
> To: Zhang, Tina <tina.zh...@intel.com>; alex.william...@redhat.com;
> ch...@chris-wilson.co.uk; zhen...@linux.intel.com; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>; Tian, Kevin
> <kevin.t...@intel.com>; dan...@ffwll.ch; kwankh...@nvidia.com
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org
> Subject: Re: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > +struct vfio_device_gfx_plane_info {
> > +   __u32 argsz;
> > +   __u32 flags;
> > +   /* in */
> > +   __u32 drm_plane_type;   /* type of plane:
> > DRM_PLANE_TYPE_* */
> > +   /* out */
> > +   __u32 drm_format;   /* drm format of plane */
> 
> DRM_FORMAT_*
> 
> drm_format_mod is gone?  Not needed?
> How tiled buffers are handled then?
Drm_format_mod is used as one of the plane's characteristics when judging the 
dma-buf of the plane is new to expose or an old exposed one. As from V10 we 
leave this comparing logic to kernel, user mode might not need this field any 
more. If user mode wants, this can be also got though drm APIs. For example, 
eglCreateImageKHR() uses drm APIs to get the tiling format, without asking the 
invoker to explicitly use it as a parameter. 
Do you think this field is still needed?

Tina
> 
> cheers,
>   Gerd
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Zhenyu Wang
> Sent: Wednesday, July 12, 2017 5:49 PM
> To: Daniel Vetter <dan...@ffwll.ch>
> Cc: Tian, Kevin <kevin.t...@intel.com>; intel-gfx@lists.freedesktop.org;
> alex.william...@redhat.com; zhen...@linux.intel.com; chris@chris-
> wilson.co.uk; kwankh...@nvidia.com; kra...@redhat.com; Zhang, Tina
> <tina.zh...@intel.com>; intel-gvt-...@lists.freedesktop.org; Wang, Zhi A
> <zhi.a.w...@intel.com>; Lv, Zhiyuan <zhiyuan...@intel.com>
> Subject: Re: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> 
> On 2017.07.12 09:56:11 +0200, Daniel Vetter wrote:
> > On Wed, Jul 12, 2017 at 06:56:53AM +, Zhang, Tina wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Zhang, Tina
> > > > Sent: Wednesday, July 12, 2017 2:11 PM
> > > > To: alex.william...@redhat.com; kra...@redhat.com;
> > > > ch...@chris-wilson.co.uk; zhen...@linux.intel.com; Lv, Zhiyuan
> > > > <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>; Tian,
> > > > Kevin <kevin.t...@intel.com>; dan...@ffwll.ch;
> > > > kwankh...@nvidia.com
> > > > Cc: Zhang, Tina <tina.zh...@intel.com>;
> > > > intel-gfx@lists.freedesktop.org; intel-
> > > > gvt-...@lists.freedesktop.org
> > > > Subject: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf
> > > > operation
> > > >
> > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode
> > > > query and get the plan and its related information.
> > > >
> > > > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > > > The returned fd in struct vfio_device_query_gfx_plane can be a new
> > > > fd or an old fd of a re-exported dma-buf. Host User mode can check
> > > > the value of fd and to see if it needs to create new resource
> > > > according to the new fd or just use the existed resource related to the 
> > > > old
> fd.
> > > >
> > > > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > > >
> > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > index
> > > > ae46105..c176cc8 100644
> > > > --- a/include/uapi/linux/vfio.h
> > > > +++ b/include/uapi/linux/vfio.h
> > > > @@ -502,6 +502,32 @@ struct vfio_pci_hot_reset {
> > > >
> > > >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE +
> 13)
> > > >
> > > > +/**
> > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE +
> 14,
> > > > struct
> > > > +vfio_device_query_gfx_plane)
> > > > + *
> > > > + * Set the drm_plane_type and retrieve information about the gfx plane.
> > > > + *
> > > > + * Return: 0 on success, -errno on failure.
> > > > + */
> > > > +struct vfio_device_gfx_plane_info {
> > > > +   __u32 argsz;
> > > > +   __u32 flags;
> > > > +   /* in */
> > > > +   __u32 drm_plane_type;   /* type of plane: DRM_PLANE_TYPE_* */
> > > > +   /* out */
> > > > +   __u32 drm_format;   /* drm format of plane */
> > > > +   __u32 width;/* width of plane */
> > > > +   __u32 height;   /* height of plane */
> > > > +   __u32 stride;   /* stride of plane */
> > > > +   __u32 size; /* size of plane in bytes, align on page*/
> > > > +   __u32 x_pos;/* horizontal position of cursor plane, upper 
> > > > left corner
> > > > in pixels */
> > > > +   __u32 y_pos;/* vertical position of cursor plane, upper 
> > > > left corner in
> > > > lines*/
> > > > +   __s32 fd;   /* dma-buf fd */
> > > > +};
> > > > +
> > > I remove plane_id as I cannot find its meaning for dmabuf usage. Also, I
> don't know how it could be used by region usage.
> > > So, if someone can explain its usage, I can add it back. Thanks.
> > >
> > > Another open is, so far, our design is focused on dmabuf based gfx
> > > plane only. Can we go a step further to consider a general Buffer
> > > sharing interface? If we reference V4L2, we can see dmabuf can be
> > > considered as one kind of buffers, we can have  more kinds of
> > > buffers, l

Re: [Intel-gfx] [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Kirti Wankhede
> Sent: Wednesday, July 12, 2017 8:45 PM
> To: Zhang, Tina <tina.zh...@intel.com>; Gerd Hoffmann
> <kra...@redhat.com>; Tian, Kevin <kevin.t...@intel.com>; linux-
> ker...@vger.kernel.org; intel-gfx@lists.freedesktop.org;
> alex.william...@redhat.com; zhen...@linux.intel.com; chris@chris-
> wilson.co.uk; Lv, Zhiyuan <zhiyuan...@intel.com>; intel-gvt-
> d...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> 
> 
> On 7/12/2017 1:10 PM, Daniel Vetter wrote:
> > On Wed, Jul 12, 2017 at 02:31:40AM +, Zhang, Tina wrote:
> >>
> >>
> >>> -Original Message-
> >>> From: intel-gvt-dev
> >>> [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On Behalf Of
> >>> Daniel Vetter
> >>> Sent: Tuesday, July 11, 2017 5:13 PM
> >>> To: Gerd Hoffmann <kra...@redhat.com>
> >>> Cc: Tian, Kevin <kevin.t...@intel.com>;
> >>> linux-ker...@vger.kernel.org; intel- g...@lists.freedesktop.org;
> >>> alex.william...@redhat.com; zhen...@linux.intel.com;
> >>> ch...@chris-wilson.co.uk; Kirti Wankhede <kwankh...@nvidia.com>; Lv,
> >>> Zhiyuan <zhiyuan...@intel.com>; dan...@ffwll.ch; Zhang, Tina
> >>> <tina.zh...@intel.com>; intel-gvt- d...@lists.freedesktop.org; Wang,
> >>> Zhi A <zhi.a.w...@intel.com>
> >>> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf
> >>> operation
> >>>
> >>> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
> >>>>   Hi,
> >>>>
> >>>>>> +struct vfio_device_query_gfx_plane {
> >>>>>> +  __u32 argsz;
> >>>>>> +  __u32 flags;
> >>>>>> +  struct vfio_device_gfx_plane_info plane_info;
> >>>>>> +  __u32 plane_type;
> >>>>>> +  __s32 fd; /* dma-buf fd */
> >>>>>> +  __u32 plane_id;
> >>>>>> +};
> >>>>>> +
> >>>>>
> >>>>> It would be better to have comment here about what are expected
> >>>>> values for plane_type and plane_id.
> >>>>
> >>>> plane_type is DRM_PLANE_TYPE_*.
> >>>>
> >>>> yes, a comment saying so would be good, same for drm_format which
> >>>> is DRM_FORMAT_*.  While looking at these two: renaming plane_type
> >>>> to drm_plane_type (for consistency) is probably a good idea too.
> >>>>
> >>>> plane_id needs a specification.
> >>>
> >>> Why do you need plane_type? With universal planes the plane_id along
> >>> is sufficient to identify a plane on a given drm device instance. I'd just
> remove it.
> >>> -Daniel
> >> The plane_type here, is to ask the mdev vendor driver to return the
> information according to the value in field plane_type. So, it's a input 
> field.
> >> The values in plane_type field is the same of drm_plane_type. And yes, it's
> better to use drm_plane_type instead of plane_id.
> >
> > I have no idea what you mean here, I guess that just shows that
> > discussing an ioctl struct without solid definitions of what field
> > does what and why is not all that useful. What exactly it plane_id for then?
> >
> 
> plane type could be DRM_PLANE_TYPE_PRIMARY or
> DRM_PLANE_TYPE_CURSOR.
> 
> In case when VFIO region is used to provide surface to QEMU, plane_id would
> be region index, for example region 10 could be used for primary surface and
> region 11 could be used for cursor surface. So in that case, mdev vendor 
> driver
> should return plane_type and its corresponding plane_id.
Thanks, Kirti, do you mean there will be multiple DRM_PLANE_TYPE_PRIMARY and 
multiple DRM_PLANE_TYPE_CURSOR planes existing in the same time and region 
usage needs to use plane_id to distinguish among them? Is it for the multiple 
output or that's the typical way of region usage? Thanks.

Tina

> 
> Thanks,
> Kirti
> 
> > This just confused me more ...
> > -Daniel
> >
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Zhang, Tina
> Sent: Wednesday, July 12, 2017 2:11 PM
> To: alex.william...@redhat.com; kra...@redhat.com; ch...@chris-wilson.co.uk;
> zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A
> <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; dan...@ffwll.ch;
> kwankh...@nvidia.com
> Cc: Zhang, Tina <tina.zh...@intel.com>; intel-gfx@lists.freedesktop.org; 
> intel-
> gvt-...@lists.freedesktop.org
> Subject: [PATCH v11 1/1] vfio: ABI for mdev display dma-buf operation
> 
> Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> and get the plan and its related information.
> 
> The dma-buf's life cycle is handled by user mode and tracked by kernel.
> The returned fd in struct vfio_device_query_gfx_plane can be a new fd or an
> old fd of a re-exported dma-buf. Host User mode can check the value of fd and
> to see if it needs to create new resource according to the new fd or just use 
> the
> existed resource related to the old fd.
> 
> Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index
> ae46105..c176cc8 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,32 @@ struct vfio_pci_hot_reset {
> 
>  #define VFIO_DEVICE_PCI_HOT_RESET_IO(VFIO_TYPE, VFIO_BASE + 13)
> 
> +/**
> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> struct
> +vfio_device_query_gfx_plane)
> + *
> + * Set the drm_plane_type and retrieve information about the gfx plane.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct vfio_device_gfx_plane_info {
> + __u32 argsz;
> + __u32 flags;
> + /* in */
> + __u32 drm_plane_type;   /* type of plane: DRM_PLANE_TYPE_* */
> + /* out */
> + __u32 drm_format;   /* drm format of plane */
> + __u32 width;/* width of plane */
> + __u32 height;   /* height of plane */
> + __u32 stride;   /* stride of plane */
> + __u32 size; /* size of plane in bytes, align on page*/
> + __u32 x_pos;/* horizontal position of cursor plane, upper left 
> corner
> in pixels */
> + __u32 y_pos;/* vertical position of cursor plane, upper left corner 
> in
> lines*/
> + __s32 fd;   /* dma-buf fd */
> +};
> +
I remove plane_id as I cannot find its meaning for dmabuf usage. Also, I don't 
know how it could be used by region usage.
So, if someone can explain its usage, I can add it back. Thanks.

Another open is, so far, our design is focused on dmabuf based gfx plane only. 
Can we go a step further to consider a general
Buffer sharing interface? If we reference V4L2, we can see dmabuf can be 
considered as one kind of buffers, we can have  more
kinds of buffers, like mmap buffer and so on. So, if we think about that, we 
may need another ioctl to ask the mdev device which
kind of buffer it supports, and then use the VFIO_DEVICE_QUERY_* ioctl to get 
it back for user mode accessing. Different kinds
of mdev devices can have different query ioctl name and structure. Is it 
reasonable?
Thanks.

Tina

> +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> +
> +
>  /*  API for Type1 VFIO IOMMU  */
> 
>  /**
> --
> 2.7.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Daniel Vetter
> Sent: Tuesday, July 11, 2017 5:16 PM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; linux-ker...@vger.kernel.org; intel-
> g...@lists.freedesktop.org; kwankh...@nvidia.com; zhen...@linux.intel.com;
> ch...@chris-wilson.co.uk; alex.william...@redhat.com; Lv, Zhiyuan
> <zhiyuan...@intel.com>; dan...@ffwll.ch; intel-gvt-...@lists.freedesktop.org;
> Wang, Zhi A <zhi.a.w...@intel.com>; kra...@redhat.com
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> On Thu, Jul 06, 2017 at 06:29:55AM +0800, Tina Zhang wrote:
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> > and get the plan and its related information.
> >
> > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > or an old fd of a re-exported dma-buf. Host User mode can check the
> > value of fd and to see if it need to creat new resource according to
> > the new fd or just use the existed resource related to the old fd.
> >
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..c92bc69 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,36 @@ struct vfio_pci_hot_reset {
> >
> >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE +
> 13)
> >
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *   struct vfio_device_query_gfx_plane)
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +
> > +struct vfio_device_gfx_plane_info {
> > +   __u64 start;
> > +   __u64 drm_format_mod;
> > +   __u32 drm_format;
> > +   __u32 width;
> > +   __u32 height;
> > +   __u32 stride;
> > +   __u32 size;
> > +   __u32 x_pos;
> > +   __u32 y_pos;
> > +};
> 
> Would be good to have a detailed spec of all this stuff, plus what's it meant 
> to be
> used for. I assume that e.g. start would be the opaque cookie thing we've 
> talked
> about, for dma-buf reuse? Otherwise I'm not sure what it's good for, since the
> same gpu vram address can be reused for different memory objects ...

Yes, I will add more comments in the next version. Here, some historical reason 
might be helpful to understand. Previously, we reported all the information to 
user mode and let user mode to decide whether it was an exported dmabuf or not. 
If it was an exported dmabuf, user mode can directly use its cached resources, 
without needing to create again. This design turned out to have some 
limitations:
1). User mode has to keep so many information to do the comparing.
2). The design needs at least two ioctls, with one for query info and the other 
one for exporting dmabuf. So, there will be a time window between these two 
ioctls, during which the guest framebuffer might be changed.

In this patch, we leave the comparing logic to kernel, and return the dmabuf fd 
everytime when user mode calling VFIO_DEVICE_QUERY_GFX_PLANE ioctl. User mode 
can compare the value of fd to see whether it can reuse the resource of the old 
fd, or need to create new according to the new fd.
If we take the idea in this patch, we don't need so many fields in the struct 
vfio_device_gfx_plane_info which seem meaningless to user mode. I'm going to 
remove some in the next version, including start.
Thanks.

Tina
> > +
> > +struct vfio_device_query_gfx_plane {
> > +   __u32 argsz;
> > +   __u32 flags;
> > +   struct vfio_device_gfx_plane_info plane_info;
> > +   __u32 plane_type;
> > +   __s32 fd; /* dma-buf fd */
> > +   __u32 plane_id;
> > +};
> 
> As mentioned in the other reply already, I'm not sure what plane_type is for.
> Otherwise this looks ok-ish, but hard to tell without more detailed spec.
> -Daniel
> 
> > +
> > +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> > +
> > +
> >  /*  API for Type1 VFIO IOMMU  */
> >
> >  /**
> > --
> > 2.7.4
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Tuesday, July 11, 2017 2:08 PM
> To: Zhang, Tina <tina.zh...@intel.com>; alex.william...@redhat.com;
> ch...@chris-wilson.co.uk; zhen...@linux.intel.com; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>; Tian, Kevin
> <kevin.t...@intel.com>; dan...@ffwll.ch; kwankh...@nvidia.com
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; 
> linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *   struct vfio_device_query_gfx_plane)
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +
> > +struct vfio_device_gfx_plane_info {
> > +   __u64 start;
> > +   __u64 drm_format_mod;
> > +   __u32 drm_format;
> > +   __u32 width;
> > +   __u32 height;
> > +   __u32 stride;
> > +   __u32 size;
> > +   __u32 x_pos;
> > +   __u32 y_pos;
> > +};
> 
> Do we want keep that as separate struct?  Given we now have only a single
> struct using that as sub-struct it looks pointless, at least from a API point 
> of view.
> Does the driver use the struct internally?
Driver has another struct which is super-set of these fields. Yes, we can move 
all these fields into struct vfio_device_query_gfx_plane. Also, we can remove 
some of these fields which may seem useless for user mode.
Thanks.

Tina
> 
> > +
> > +struct vfio_device_query_gfx_plane {
> > +   __u32 argsz;
> > +   __u32 flags;
> > +   struct vfio_device_gfx_plane_info plane_info;
> > +   __u32 plane_type;
> > +   __s32 fd; /* dma-buf fd */
> > +   __u32 plane_id;
> 
> What is plane_id?
I cannot figure out the mean of plane_id either. If I remember correctly, it 
was asked by region usage. Of course, if no one needs it, I'd like to remove it.
Thanks.

Tina

> 
> cheers,
>   Gerd

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Daniel Vetter
> Sent: Tuesday, July 11, 2017 5:13 PM
> To: Gerd Hoffmann <kra...@redhat.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; linux-ker...@vger.kernel.org; intel-
> g...@lists.freedesktop.org; alex.william...@redhat.com;
> zhen...@linux.intel.com; ch...@chris-wilson.co.uk; Kirti Wankhede
> <kwankh...@nvidia.com>; Lv, Zhiyuan <zhiyuan...@intel.com>;
> dan...@ffwll.ch; Zhang, Tina <tina.zh...@intel.com>; intel-gvt-
> d...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> >
> > > > +struct vfio_device_query_gfx_plane {
> > > > +   __u32 argsz;
> > > > +   __u32 flags;
> > > > +   struct vfio_device_gfx_plane_info plane_info;
> > > > +   __u32 plane_type;
> > > > +   __s32 fd; /* dma-buf fd */
> > > > +   __u32 plane_id;
> > > > +};
> > > > +
> > >
> > > It would be better to have comment here about what are expected
> > > values for plane_type and plane_id.
> >
> > plane_type is DRM_PLANE_TYPE_*.
> >
> > yes, a comment saying so would be good, same for drm_format which is
> > DRM_FORMAT_*.  While looking at these two: renaming plane_type to
> > drm_plane_type (for consistency) is probably a good idea too.
> >
> > plane_id needs a specification.
> 
> Why do you need plane_type? With universal planes the plane_id along is
> sufficient to identify a plane on a given drm device instance. I'd just 
> remove it.
> -Daniel
The plane_type here, is to ask the mdev vendor driver to return the information 
according to the value in field plane_type. So, it's a input field.
The values in plane_type field is the same of drm_plane_type. And yes, it's 
better to use drm_plane_type instead of plane_id. 
Thanks.

Tina
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Kirti Wankhede [mailto:kwankh...@nvidia.com]
> Sent: Thursday, July 6, 2017 10:02 PM
> To: Zhang, Tina <tina.zh...@intel.com>; alex.william...@redhat.com;
> kra...@redhat.com; ch...@chris-wilson.co.uk; zhen...@linux.intel.com; Lv,
> Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>; Tian,
> Kevin <kevin.t...@intel.com>; dan...@ffwll.ch
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; 
> linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation
> 
> 
> 
> On 7/6/2017 3:59 AM, Tina Zhang wrote:
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> > and get the plan and its related information.
> >
> > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > or an old fd of a re-exported dma-buf. Host User mode can check the
> > value of fd and to see if it need to creat new resource according to
> > the new fd or just use the existed resource related to the old fd.
> >
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..c92bc69 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,36 @@ struct vfio_pci_hot_reset {
> >
> >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE +
> 13)
> >
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *   struct vfio_device_query_gfx_plane)
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +
> > +struct vfio_device_gfx_plane_info {
> > +   __u64 start;
> > +   __u64 drm_format_mod;
> > +   __u32 drm_format;
> > +   __u32 width;
> > +   __u32 height;
> > +   __u32 stride;
> > +   __u32 size;
> > +   __u32 x_pos;
> > +   __u32 y_pos;
> > +};
> > +
> 
> Above structure looks good to me.
> 
> > +struct vfio_device_query_gfx_plane {
> > +   __u32 argsz;
> > +   __u32 flags;
> > +   struct vfio_device_gfx_plane_info plane_info;
> > +   __u32 plane_type;
> > +   __s32 fd; /* dma-buf fd */
> > +   __u32 plane_id;
> > +};
> > +
> 
> It would be better to have comment here about what are expected values for
> plane_type and plane_id.
I will add the comments for these fields. Thanks.

Tina
> 
> Thanks,
> Kirti
> 
> > +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> > +
> > +
> >  /*  API for Type1 VFIO IOMMU  */
> >
> >  /**
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10] drm/i915/gvt: Dma-buf support for GVT-g

[Zhang, Tina]

Hi, 

This is the version 10 ABI interface. I took Gerd's advice to submit the 
interface only. If everything is fine, I can submit the whole patch set.
So, how do you think about this ABI interface.
Thanks.

BR,
Tina

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Tina Zhang
> Sent: Thursday, July 6, 2017 6:30 AM
> To: alex.william...@redhat.com; kra...@redhat.com; ch...@chris-wilson.co.uk;
> zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A
> <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; dan...@ffwll.ch;
> kwankh...@nvidia.com
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; 
> linux-
> ker...@vger.kernel.org; Zhang, Tina <tina.zh...@intel.com>
> Subject: [PATCH v10] drm/i915/gvt: Dma-buf support for GVT-g
> 
> 
> v9->v10:
> 1) remove dma-buf management
> 2) refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE
> 3) track the dma-buf create and release in kernel mode
> 
> v8->v9:
> 1) refine the dma-buf ioctl definition
> 2) add a lock to protect the dmabuf list
> 3) move drm format change to a separate patch
> 4) codes cleanup
> 
> v7->v8:
> 1) refine framebuffer decoder code
> 2) fix a bug in decoding primary plane
> 
> v6->v7:
> 1) release dma-buf related allocations in dma-buf's associated release 
> function.
> 2) refine ioctl interface for querying plane info or create dma-buf
> 3) refine framebuffer decoder code
> 4) the patch series is based on 4.12.0-rc1
> 
> v5->v6:
> 1) align the dma-buf life cycle with the vfio device.
> 2) add the dma-buf releated operations in a separate patch.
> 3) i915 releated changes.
> 
> v4->v5:
> 1) fix bug while checking whether the gem obj is gvt's dma-buf when user
> change caching mode or domains. Add a helper function to do it.
> 2) add definition for the query plane and create dma-buf.
> 
> v3->v4:
> 1) fix bug while checking whether the gem obj is gvt's dma-buf when set 
> caching
> mode or doamins.
> 
> v2->v3:
> 1) add a field gvt_plane_info in the drm_i915_gem_obj structure to save the
> decoded plane information to avoid look up while need the plane info.
> 2) declare a new flag I915_GEM_OBJECT_IS_GVT_DMABUF in
> drm_i915_gem_object to represent the gem obj for gvt's dma-buf. The tiling
> mode, caching mode and domains can not be changed for this kind of gem
> object.
> 3) change dma-buf related information to be more generic. So other vendor can
> use the same interface.
> 
> v1->v2:
> 1) create a management fd for dma-buf operations.
> 2) alloc gem object's backing storage in gem obj's get_pages() callback.
> 
> This patch set adds the dma-buf support for intel GVT-g.
> dma-buf is a uniform mechanism to share DMA buffers across different devices
> and sub-systems.
> dma-buf for intel GVT-g is mainly used to share the vgpu's framebuffer to 
> other
> users or sub-systems so they can use the dma-buf to show the desktop of a vm
> which uses intel vgpu.
> 
> The main idea is we create a gem object and set vgpu's framebuffer as the
> backing storage of this gem object. And associate this gem obj to a dma-buf
> object then export this dma-buf at the meantime generate a file descriptor for
> this dma-buf. Finally deliver this file descriptor to user space. And user 
> can use
> this dma-buf fd to do render or other operations.
> User need to create a fd(for intel GVT-g dma-buf support it is a:dma-buf
> management fd) then user can use this fd to query the plane information or
> create a dma-buf. The life cycle of this fd is managed by GVT-g user do not 
> need
> to care about that.
> 
> We have an example program on how to use the dma-buf. You can download
> the program to have a try. Good luck :) git repo:
> https://github.com/01org/igvtg-qemu branch:kvmgt_dmabuf_example
> 
> 
> Tina Zhang (1):
>   vfio: ABI for mdev display dma-buf operation
> 
>  include/uapi/linux/vfio.h | 30 ++
>  1 file changed, 30 insertions(+)
> 
> --
> 2.7.4
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v1] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -Original Message-
> From: Zhang, Tina
> Sent: Tuesday, July 4, 2017 9:04 AM
> To: alex.william...@redhat.com; kra...@redhat.com; ch...@chris-wilson.co.uk;
> zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A
> <zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>; dan...@ffwll.ch;
> kwankh...@nvidia.com
> Cc: Zhang, Tina <tina.zh...@intel.com>; intel-gfx@lists.freedesktop.org; 
> intel-
> gvt-...@lists.freedesktop.org; linux-ker...@vger.kernel.org
> Subject: [PATCH v1] vfio: ABI for mdev display dma-buf operation
> 
> Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> and get the plan and its related information.
> 
> The dma-buf's life cycle is handled by user mode and tracked by kernel.
> The returned fd in struct vfio_device_query_gfx_plane can be a new fd or an
> old fd of a re-exported dma-buf. Host User mode can check the value of fd and
> to see if it need to creat new resource according to the new fd or just use 
> the
> existed resource related to the old fd.
> 
> Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index
> ae46105..c92bc69 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,36 @@ struct vfio_pci_hot_reset {
> 
>  #define VFIO_DEVICE_PCI_HOT_RESET_IO(VFIO_TYPE, VFIO_BASE + 13)
> 
> +/**
> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> + *   struct vfio_device_query_gfx_plane)
> + * Return: 0 on success, -errno on failure.
> + */
> +
> +struct vfio_device_gfx_plane_info {
> + __u64 start;
> + __u64 drm_format_mod;
> + __u32 drm_format;
> + __u32 width;
> + __u32 height;
> + __u32 stride;
> + __u32 size;
> + __u32 x_pos;
> + __u32 y_pos;
> +};
In this version, we don't rely on user mode to compare and find out whether a 
new dmabuf needs to be created. So, some of fields can be removed if user mode 
thinks they are not interesting any more. 

> +
> +struct vfio_device_query_gfx_plane {
> + __u32 argsz;
> + __u32 flags;
> + struct vfio_device_gfx_plane_info plane_info;
> + __u32 plane_type;
> + __s32 fd; /* dma-buf fd */
> + __u32 plane_id;
> +};
Still cannot figure out what the plane_id stands for in dmabuf case. As it 
might be needed by region usage, just add it here.

> +
> +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> +
> +
>  /*  API for Type1 VFIO IOMMU  */
> 
>  /**
> --
> 2.7.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Daniel Vetter
> Sent: Thursday, June 29, 2017 4:39 PM
> To: Gerd Hoffmann <kra...@redhat.com>
> Cc: Wang, Zhenyu Z <zhenyu.z.w...@intel.com>; intel-
> g...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Chen, Xiaoguang
> <xiaoguang.c...@intel.com>; Zhang, Tina <tina.zh...@intel.com>; Alex
> Williamson <alex.william...@redhat.com>; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Kirti Wankhede <kwankh...@nvidia.com>; intel-gvt-
> d...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Thu, Jun 29, 2017 at 08:41:53AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> >
> > > > Does gvt track the live cycle of all dma-bufs it has handed out?
> > >
> > > The V9 implementation does track the dma-bufs' live cycle. The
> > > original idea was that leaving the dma-bufs' live cycle management
> > > to user mode.
> >
> > That is still the case, user space decides which dma-bufs it'll go
> > keep cached.  But kernel space can see what user space is doing, so
> > there is no need to explicitly tell the kernel whenever a cached
> > dma-buf exists or not.
> 
> We do the same trick in drm_prime.c, keeping a cache of exported dma-buf
> around for re-exporting. Since for prime sharing the use-case is almost always
> re-importing as a drm gem buffer again we can then on re-import also tell
> userspace whether it already has that buffer in it's userspace buffer manager,
> but that's an additional optimization. With plain dma-buf we could achieve the
> same by wiring up a real stat() implementation with unique inode numbers (atm
> they all share the anon_inode singleton). But thus far no one asked for that.

Thanks. I'm going to submit the v10 version of ABI interface.

> 
> btw I'm lost a bit in the discussion (was on vacation), but I think all the 
> concerns
> I've noticed with the initial rfc have been raised already, so things look 
> good. I'll
> check the next rfc once that shows up.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Tuesday, June 27, 2017 2:13 PM
> To: Alex Williamson <alex.william...@redhat.com>
> Cc: Wang, Zhenyu Z <zhenyu.z.w...@intel.com>; intel-
> g...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Chen, Xiaoguang
> <xiaoguang.c...@intel.com>; Zhang, Tina <tina.zh...@intel.com>; Kirti
> Wankhede <kwankh...@nvidia.com>; Lv, Zhiyuan <zhiyuan...@intel.com>;
> intel-gvt-...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
>   Hi,
> 
> > Hmm, I don't like that interface.  Can you cite examples of other
> > ioctls that behave this way?  It doesn't feel like an elegant user
> > interface; the user can get the dmabuf, but only after they query the
> > dmabuf, even though the get-dmabuf ioctl returns the same data as the
> > query-plane ioctl, but they can't get the dmabuf if the plane has
> > changed in the interim, which is not something the user can know.  Are
> > we causing our own problems with this model of cycling through dmabuf
> > fds?  We talked previously about an enum of plane types, primary and
> > cursor.  What if the user was simply able to get a dmabuf fd for each
> > of those and they queried the current plane information via those fds?
> > IOW, the fd is persistent and specific to a given plane type, but the
> > format within it is dynamic.
> 
> Will not work due to how dma-bufs are designed.
> 
> But, yes, the QUERY then GET split is ugly for a number of reasons.
> 
> Does gvt track the live cycle of all dma-bufs it has handed out?
The V9 implementation does track the dma-bufs' live cycle. The original idea 
was that leaving the dma-bufs' live cycle management to user mode.

> If so, then maybe we can let the kernel check whenever a dma-buf for the
> current plane exists?  And if that isn't the case hand out a dma- buf right 
> away,
> without expecting userspace explicitly asking for it?
I think this is a good advice. We are going to try this idea and add some 
tracking logic to kernel mode.

> 
> That will simplify the interface and remove the race condition at the expense 
> of
> some additional bookkeeping in the kernel.
In this case, maybe one ioctl like QUERY_PLAN is enough. We can block it this 
ioctl and return it when the fd and info are ready.

Thanks.
Tina


> 
> cheers,
>   Gerd
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -Original Message-
> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Tuesday, June 20, 2017 6:58 PM
> To: Zhang, Tina <tina.zh...@intel.com>; Alex Williamson
> <alex.william...@redhat.com>
> Cc: intel-gfx@lists.freedesktop.org; linux-ker...@vger.kernel.org; Kirti
> Wankhede <kwankh...@nvidia.com>; Chen, Xiaoguang
> <xiaoguang.c...@intel.com>; intel-gvt-...@lists.freedesktop.org; Lv, Zhiyuan
> <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>; Wang, Zhenyu
> Z <zhenyu.z.w...@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
> > Hi,
> >
> > Thanks for all the comments. Here are the summaries:
> >
> > 1. Modify the structures to make it more general.
> > struct vfio_device_gfx_plane_info {
> > __u64 start;
> > __u64 drm_format_mod;
> > __u32 drm_format;
> > __u32 width;
> > __u32 height;
> > __u32 stride;
> > __u32 size;
> > __u32 x_pos;
> > __u32 y_pos;
> > __u32 generation;
> > };
> 
> Looks good to me.
> 
> > struct vfio_device_query_gfx_plane {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID  (1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID   (1 << 1)
> > struct vfio_device_gfx_plane_info plane_info;
> > __u32 id;
> > };
> 
> I'm not convinced the flags are a great idea.  Whenever dmabufs or a region is
> used is a static property of the device, not of each individual plane.
> 
> 
> I think we should have this for userspace to figure:
> 
> enum vfio_device_gfx_type {
> VFIO_DEVICE_GFX_NONE,
> VFIO_DEVICE_GFX_DMABUF,
> VFIO_DEVICE_GFX_REGION,
> };
> 
> struct vfio_device_gfx_query_caps {
> __u32 argsz;
> __u32 flags;
> enum vfio_device_gfx_type;
> };
> 
> 
> Then this to query the plane:
> 
> struct vfio_device_gfx_query_plane {
> __u32 argsz;
> __u32 flags;
> struct vfio_device_gfx_plane_info plane_info;  /* out */
> __u32 plane_type;  /* in  */
> };
> 
> 
> 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device
> fd.
> > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > vfio_device_gfx_plane_info.
> 
> Yes.
> 
> > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
> 
> Yes.  The plane might have changed between query-plane and get-dmabuf ioctl
> calls though, we must make sure we handle that somehow.  Current patches
> return plane_info on get-dmabuf ioctl too, so userspace can see what it 
> actually
> got.
> 
> With the generation we can also do something different:  Pass in plane_type 
> and
> generation, and have VFIO_DEVICE_GET_DMABUF_FD return an error in case
> the generation doesn't match.  In that case it doesn't make much sense any
> more to have a separate plane_info struct, which was added so we don't have
> to duplicate things in query-plane and get- dmabuf ioctl structs.
Comparing with the current patch, this would make user space a little bit 
harder to
get the dmabuf by calling VFIO_DEVICE_GET_DMABUF ioctl. Is it efficient for
user mode usage?

Thanks
Tina
> 
> cheers,
>   Gerd

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Wednesday, June 21, 2017 7:04 PM
> To: Zhang, Tina <tina.zh...@intel.com>; Alex Williamson
> <alex.william...@redhat.com>
> Cc: Wang, Zhenyu Z <zhenyu.z.w...@intel.com>; intel-
> g...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Chen, Xiaoguang
> <xiaoguang.c...@intel.com>; Kirti Wankhede <kwankh...@nvidia.com>; Lv,
> Zhiyuan <zhiyuan...@intel.com>; intel-gvt-...@lists.freedesktop.org; Wang,
> Zhi A <zhi.a.w...@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Wed, 2017-06-21 at 09:20 +, Zhang, Tina wrote:
> > Thanks for all the comments. I'm planning to cook the next version of
> > this patch set
> 
> How about posting only this patch instead of the whole series until we've 
> settled
> the interfaces?
OK.

> 
> > Could the following two works?
> > #define VFIO_DEVICE_FLAGS_DMABUF  (1 << 5)/* vfio-dmabuf
> > device */
> 
> VFIO_DEVICE_FLAGS_GFX_DMABUF?
> 
> > 2. vfio_device_gfx_plane_info
> > struct vfio_device_gfx_plane_info {
> > __u64 start;-> offset
> > __u64 drm_format_mod;
> > __u32 drm_format;
> > __u32 width;
> > __u32 height;
> > __u32 stride;
> > __u32 size;
> > __u32 x_pos;
> > __u32 y_pos;
> > };
> > > Does it make sense to have a "generation" field in the plane_info
> > > struct (which gets increased each time the struct changes) ?
> 
> > Well,  Gerd, can you share more details about how to use this field in
> > user mode, so that we can figure out a way to support it? Thanks.
> 
> generation would be increased each time one of the fields in
> vfio_device_gfx_plane_info changes, typically on mode switches (width/height
> changes) and pageflips (offset changes).  So userspace can simply compare
> generation instead of comparing every field to figure whenever something
> changed compared to the previous poll.
Make sense for dma-buf. Thanks.

> 
> >
> > 3. vfio_device_query_gfx_plane
> > struct vfio_device_query_gfx_plane {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_GFX_PLANE_FLAGS_REGION_ID  (1 << 0)
> > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID   (1 << 1)
> > struct vfio_device_gfx_plane_info plane_info;
> > __u32 id;
> > __u32 plane_type;
> > };
> > So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or
> > DRM_PLANE_TYPE_CURSOR.
> 
> 
> >  If the newly added plane_type is used for this, the id field may be
> > useless in dmabuf usage. Do you have any idea about the usage of this
> > id field in dmabuf usage?
> 
> plane_type should be DRM_PLANE_TYPE_PRIMARY or
> DRM_PLANE_TYPE_CURSOR for dmabuf.
> 
> Given that nvidia doesn't support a separate cursor plane in their region they
> would support DRM_PLANE_TYPE_PRIMARY only.
> 
> I can't see yet what id would be useful for.
> 
> Likewise I can't see yet what the VFIO_GFX_PLANE_FLAGS_* are good for.
> 
> cheers,
>   Gerd
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

Thanks for all the comments. I'm planning to cook the next version of this 
patch set which I'd like to include all the comments and ideas. Here is the 
summary:

1.  How to figure the device capabilities
struct vfio_device_info {
__u32   argsz;
__u32   flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0)/* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI   (1 << 1)/* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)/* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW   (1 << 4)/* vfio-ccw device */
__u32   num_regions;/* Max region index + 1 */
__u32   num_irqs;   /* Max IRQ index + 1 */
};
> We could use two flag bits to indicate dmabuf or graphics region support. 
Could the following two works?
#define VFIO_DEVICE_FLAGS_DMABUF  (1 << 5)/* vfio-dmabuf device */
#define VFIO_DEVICE_FLAGS_GFX_REGION   (1 << 6)/* vfio-gfx-region 
device */

2. vfio_device_gfx_plane_info
struct vfio_device_gfx_plane_info {
__u64 start;-> offset
__u64 drm_format_mod;
__u32 drm_format;
__u32 width;
__u32 height;
__u32 stride;
__u32 size;
__u32 x_pos;
__u32 y_pos;
};
> Does it make sense to have a "generation" field in the plane_info struct 
> (which gets increased each time the struct changes) ?
Well,  Gerd, can you share more details about how to use this field in user 
mode, so that we can figure out a way to support it? Thanks.

3. vfio_device_query_gfx_plane
struct vfio_device_query_gfx_plane {
__u32 argsz;
__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID  (1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID   (1 << 1)
struct vfio_device_gfx_plane_info plane_info;
__u32 id;
__u32 plane_type;
};
So far, dmabuf use id for DRM_PLANE_TYPE_PRIMARY or DRM_PLANE_TYPE_CURSOR. If 
the newly added plane_type is used for this, the id field may be useless in 
dmabuf usage. Do you have any idea about the usage of this id field in dmabuf 
usage?

4. Two ioctl commands
VFIO_DEVICE_QUERY_GFX_PLANE
VFIO_DEVICE_GET_FD

5. > Then we should kill off the manager fd unless there are arguments that 
still give it value.
Agree.

If there is anything I miss, please tell us.

Thanks.
Tina


> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, June 21, 2017 7:22 AM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>; intel-gfx@lists.freedesktop.org;
> linux-ker...@vger.kernel.org; Kirti Wankhede <kwankh...@nvidia.com>;
> Chen, Xiaoguang <xiaoguang.c...@intel.com>; intel-gvt-
> d...@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A
> <zhi.a.w...@intel.com>; Wang, Zhenyu Z <zhenyu.z.w...@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Tue, 20 Jun 2017 23:01:53 +
> "Zhang, Tina" <tina.zh...@intel.com> wrote:
> 
> > > -----Original Message-
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Tuesday, June 20, 2017 11:00 PM
> > > To: Gerd Hoffmann <kra...@redhat.com>
> > > Cc: Zhang, Tina <tina.zh...@intel.com>;
> > > intel-gfx@lists.freedesktop.org; linux- ker...@vger.kernel.org;
> > > Kirti Wankhede <kwankh...@nvidia.com>; Chen, Xiaoguang
> > > <xiaoguang.c...@intel.com>; intel-gvt-...@lists.freedesktop.org;
> > > Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A
> > > <zhi.a.w...@intel.com>; Wang, Zhenyu Z <zhenyu.z.w...@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based
> > > dma-buf operations
> > >
> > > On Tue, 20 Jun 2017 12:57:36 +0200
> > > Gerd Hoffmann <kra...@redhat.com> wrote:
> > >
> > > > On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
> > > > > Hi,
> > > > >
> > > > > Thanks for all the comments. Here are the summaries:
> > > > >
> > > > > 1. Modify the structures to make it more general.
> > > > > struct vfio_device_gfx_plane_info {
> > > > >   __u64 start;
> > > > >   __u64 drm_format_mod;
> > > > >   __u32 drm_format;
> > > > >   __u32 width;
> > > > >   __u32 height;
> > > > >   __u32 stride;
> > > > >   __u32 size;
> > > > >   __u32 x_pos;
> > > > >   __u32 y_pos;
&g

Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/gvt: Fix guest i915 48bit full ppgtt blocking issue

[Zhang, Tina]

This patch should be merged to intel-gvt-dev. Here just for the review.
Thanks.

BR,
Tina

> -Original Message-
> From: Patchwork [mailto:patchw...@emeril.freedesktop.org]
> Sent: Tuesday, June 20, 2017 10:06 PM
> To: Zhang, Tina <tina.zh...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: ✗ Fi.CI.BAT: failure for drm/i915/gvt: Fix guest i915 48bit full 
> ppgtt
> blocking issue
> 
> == Series Details ==
> 
> Series: drm/i915/gvt: Fix guest i915 48bit full ppgtt blocking issue
> URL   : https://patchwork.freedesktop.org/series/26049/
> State : failure
> 
> == Summary ==
> 
>   CHK include/config/kernel.release
>   CHK include/generated/uapi/linux/version.h
>   CHK include/generated/utsrelease.h
>   CHK include/generated/bounds.h
>   CHK include/generated/timeconst.h
>   CHK include/generated/asm-offsets.h
>   CALLscripts/checksyscalls.sh
>   CHK scripts/mod/devicetable-offsets.h
>   CHK include/generated/compile.h
>   CHK kernel/config_data.h
>   CC [M]  drivers/gpu/drm/i915/gvt/vgpu.o In file included from
> drivers/gpu/drm/i915/gvt/gvt.h:38:0,
>  from drivers/gpu/drm/i915/gvt/vgpu.c:35:
> drivers/gpu/drm/i915/gvt/vgpu.c: In function ‘populate_pvinfo_page’:
> ./include/linux/compiler-gcc.h:161:2: error: ‘struct vgt_if’ has no member
> named ‘vgt_caps’
>   __builtin_offsetof(a, b)
>   ^
> drivers/gpu/drm/i915/gvt/mmio.h:75:9: note: in definition of macro
> ‘INTEL_GVT_MMIO_OFFSET’
>   typeof(reg) __reg = reg; \
>  ^
> drivers/gpu/drm/i915/gvt/vgpu.c:46:2: note: in expansion of macro ‘vgpu_vreg’
>   vgpu_vreg(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_PPGTT_48BIT;
>   ^
> drivers/gpu/drm/i915/i915_pvinfo.h:103:2: note: in expansion of macro
> ‘_MMIO’
>   _MMIO((VGT_PVINFO_PAGE + offsetof(struct vgt_if, x)))
>   ^
> ./include/linux/stddef.h:16:32: note: in expansion of macro
> ‘__compiler_offsetof’
>  #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
> ^
> drivers/gpu/drm/i915/i915_pvinfo.h:103:27: note: in expansion of macro
> ‘offsetof’
>   _MMIO((VGT_PVINFO_PAGE + offsetof(struct vgt_if, x)))
>^
> drivers/gpu/drm/i915/gvt/vgpu.c:46:18: note: in expansion of macro ‘vgtif_reg’
>   vgpu_vreg(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_PPGTT_48BIT;
>   ^
> ./include/linux/compiler-gcc.h:161:2: error: ‘struct vgt_if’ has no member
> named ‘vgt_caps’
>   __builtin_offsetof(a, b)
>   ^
> drivers/gpu/drm/i915/gvt/mmio.h:75:22: note: in definition of macro
> ‘INTEL_GVT_MMIO_OFFSET’
>   typeof(reg) __reg = reg; \
>   ^
> drivers/gpu/drm/i915/gvt/vgpu.c:46:2: note: in expansion of macro ‘vgpu_vreg’
>   vgpu_vreg(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_PPGTT_48BIT;
>   ^
> drivers/gpu/drm/i915/i915_pvinfo.h:103:2: note: in expansion of macro
> ‘_MMIO’
>   _MMIO((VGT_PVINFO_PAGE + offsetof(struct vgt_if, x)))
>   ^
> ./include/linux/stddef.h:16:32: note: in expansion of macro
> ‘__compiler_offsetof’
>  #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
> ^
> drivers/gpu/drm/i915/i915_pvinfo.h:103:27: note: in expansion of macro
> ‘offsetof’
>   _MMIO((VGT_PVINFO_PAGE + offsetof(struct vgt_if, x)))
>^
> drivers/gpu/drm/i915/gvt/vgpu.c:46:18: note: in expansion of macro ‘vgtif_reg’
>   vgpu_vreg(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_PPGTT_48BIT;
>   ^
> drivers/gpu/drm/i915/gvt/vgpu.c:46:41: error:
> ‘VGT_CAPS_FULL_PPGTT_48BIT’ undeclared (first use in this function)
>   vgpu_vreg(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_PPGTT_48BIT;
>  ^
> drivers/gpu/drm/i915/gvt/vgpu.c:46:41: note: each undeclared identifier is
> reported only once for each function it appears in
> scripts/Makefile.build:302: recipe for target 
> 'drivers/gpu/drm/i915/gvt/vgpu.o'
> failed
> make[4]: *** [drivers/gpu/drm/i915/gvt/vgpu.o] Error 1
> scripts/Makefile.build:561: recipe for target 'drivers/gpu/drm/i915' failed
> make[3]: *** [drivers/gpu/drm/i915] Error 2
> scripts/Makefile.build:561: recipe for target 'drivers/gpu/drm' failed
> make[2]: *** [drivers/gpu/drm] Error 2
> scripts/Makefile.build:561: recipe for target 'drivers/gpu' failed
> make[1]: *** [drivers/gpu] Error 2
> Makefile:1016: recipe for target 'drivers' failed
> make: *** [drivers] Error 2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, June 20, 2017 11:00 PM
> To: Gerd Hoffmann <kra...@redhat.com>
> Cc: Zhang, Tina <tina.zh...@intel.com>; intel-gfx@lists.freedesktop.org; 
> linux-
> ker...@vger.kernel.org; Kirti Wankhede <kwankh...@nvidia.com>; Chen,
> Xiaoguang <xiaoguang.c...@intel.com>; intel-gvt-...@lists.freedesktop.org;
> Lv, Zhiyuan <zhiyuan...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com>;
> Wang, Zhenyu Z <zhenyu.z.w...@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Tue, 20 Jun 2017 12:57:36 +0200
> Gerd Hoffmann <kra...@redhat.com> wrote:
> 
> > On Tue, 2017-06-20 at 08:41 +, Zhang, Tina wrote:
> > > Hi,
> > >
> > > Thanks for all the comments. Here are the summaries:
> > >
> > > 1. Modify the structures to make it more general.
> > > struct vfio_device_gfx_plane_info {
> > >   __u64 start;
> > >   __u64 drm_format_mod;
> > >   __u32 drm_format;
> > >   __u32 width;
> > >   __u32 height;
> > >   __u32 stride;
> > >   __u32 size;
> > >   __u32 x_pos;
> > >   __u32 y_pos;
> > >   __u32 generation;
> > > };
> >
> > Looks good to me.
> >
> > > struct vfio_device_query_gfx_plane {
> > >   __u32 argsz;
> > >   __u32 flags;
> > > #define VFIO_GFX_PLANE_FLAGS_REGION_ID(1 << 0)
> > > #define VFIO_GFX_PLANE_FLAGS_PLANE_ID (1 << 1)
> > >   struct vfio_device_gfx_plane_info plane_info;
> > >   __u32 id;
> > > };
> >
> > I'm not convinced the flags are a great idea.  Whenever dmabufs or a
> > region is used is a static property of the device, not of each
> > individual plane.
> >
> >
> > I think we should have this for userspace to figure:
> >
> > enum vfio_device_gfx_type {
> > VFIO_DEVICE_GFX_NONE,
> > VFIO_DEVICE_GFX_DMABUF,
> > VFIO_DEVICE_GFX_REGION,
> > };
> >
> > struct vfio_device_gfx_query_caps {
> > __u32 argsz;
> > __u32 flags;
> > enum vfio_device_gfx_type;
> > };
> 
> We already have VFIO_DEVICE_GET_INFO which returns:
> 
> struct vfio_device_info {
> __u32   argsz;
> __u32   flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0)/* Device supports reset */
> #define VFIO_DEVICE_FLAGS_PCI   (1 << 1)/* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
> #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)/* vfio-amba device */
> #define VFIO_DEVICE_FLAGS_CCW   (1 << 4)/* vfio-ccw device */
> __u32   num_regions;/* Max region index + 1 */
> __u32   num_irqs;   /* Max IRQ index + 1 */
> };
> 
> We could use two flag bits to indicate dmabuf or graphics region support.
> vfio_device_gfx_query_caps seems to imply a new ioctl, which would be
> unnecessary.
> 
> > Then this to query the plane:
> >
> > struct vfio_device_gfx_query_plane {
> > __u32 argsz;
> > __u32 flags;
> > struct vfio_device_gfx_plane_info plane_info;  /* out */
> > __u32 plane_type;  /* in  */
> > };
> 
> I'm not sure why we're using an enum for something that can currently be
> defined with 2 bits, seems like this would be another good use of flags.  We
> could even embed an enum into the flags if we want to leave some expansion
> room, 4 bits maybe?  Also, I was imagining that a device could support 
> multiple
> graphics regions, that's where specifying the "id" as a region index seemed
> useful.  We lose that ability here unless we go back to defining a flag bit to
> specify how to interpret this last field.
> 
> > 2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio
> > device fd.
> > > VFIO_DEVICE_QUERY_GFX_PLANE : used to query
> > > vfio_device_gfx_plane_info.
> >
> > Yes.
> >
> > > VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.
> 
> I'm not convinced this adds value, but I'll list it as an option:
> 
> VFIO_DEVICE_QUERY(VFIO_DEVICE_GFX_PLANE)
> VFIO_DEVICE_GET_FD(VFIO_DEVICE_GFX_DMABUF_FD)
> 
> The benefit is that it might help to avoid a proliferation of ioctls on the 
> device the
> pain is that we need to either define a field or section of flags which 
> identify
> what is being queried or what ty

Re: [Intel-gfx] [PATCH v9 3/7] drm: Extend the drm format

[Zhang, Tina]

> -Original Message-
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
> Ville Syrjälä
> Sent: Thursday, June 15, 2017 6:22 PM
> To: Chen, Xiaoguang 
> Cc: intel-gfx@lists.freedesktop.org; linux-ker...@vger.kernel.org;
> kra...@redhat.com; intel-gvt-...@lists.freedesktop.org; Lv, Zhiyuan
> 
> Subject: Re: [Intel-gfx] [PATCH v9 3/7] drm: Extend the drm format
> 
> On Thu, Jun 15, 2017 at 04:00:07PM +0800, Xiaoguang Chen wrote:
> > Add new drm format which will be used by GVT-g.
> >
> > Signed-off-by: Xiaoguang Chen 
> > ---
> >  include/uapi/drm/drm_fourcc.h | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h
> > b/include/uapi/drm/drm_fourcc.h index 55e3010..2681862 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -113,6 +113,10 @@ extern "C" {
> >
> >  #define DRM_FORMAT_AYUVfourcc_code('A', 'Y', 'U', 'V') /* 
> > [31:0]
> A:Y:Cb:Cr 8:8:8:8 little endian */
> >
> > +/* 64 bpp RGB */
> > +#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /*
> > +[63:0] x:R:G:B 16:16:16:16 little endian */ #define
> > +DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0]
> > +x:B:G:R 16:16:16:16 little endian */
> 
> Are these supposed to be the half float formats? If so the docs are lacking. 
> Also
> this sort of stuff must be sent to dri-devel for everyone to see.
> 
> That said, I don't really like having them in any gvt code until they're 
> handled by
> the driver proper.
This is needed by some Apps running on Windows VM. These can be separated to 
another patch set where more can be included, e.g. docs.
Thanks.

> 
> > +
> >  /*
> >   * 2 plane RGB + A
> >   * index 0 = RGB plane, same format as the corresponding non _A8
> > format has
> > --
> > 2.7.4
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf operations

[Zhang, Tina]

Hi,

Thanks for all the comments. Here are the summaries:

1. Modify the structures to make it more general.
struct vfio_device_gfx_plane_info {
__u64 start;
__u64 drm_format_mod;
__u32 drm_format;
__u32 width;
__u32 height;
__u32 stride;
__u32 size;
__u32 x_pos;
__u32 y_pos;
__u32 generation;
};
struct vfio_device_query_gfx_plane {
__u32 argsz;
__u32 flags;
#define VFIO_GFX_PLANE_FLAGS_REGION_ID  (1 << 0)
#define VFIO_GFX_PLANE_FLAGS_PLANE_ID   (1 << 1)
struct vfio_device_gfx_plane_info plane_info;
__u32 id; 
};
2. Remove dmabuf mgr fd and add these two ioctl commands to the vfio device fd.
VFIO_DEVICE_QUERY_GFX_PLANE : used to query vfio_device_gfx_plane_info.
VFIO_DEVICE_GET_DMABUF_FD: used to create and return the dmabuf fd.

Am I correct? Thanks.

Tina


> -Original Message-
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
> Alex Williamson
> Sent: Monday, June 19, 2017 10:56 PM
> To: Gerd Hoffmann 
> Cc: intel-gfx@lists.freedesktop.org; linux-ker...@vger.kernel.org; Kirti
> Wankhede ; Chen, Xiaoguang
> ; intel-gvt-...@lists.freedesktop.org; Lv, Zhiyuan
> 
> Subject: Re: [Intel-gfx] [PATCH v9 5/7] vfio: Define vfio based dma-buf
> operations
> 
> On Mon, 19 Jun 2017 08:38:32 +0200
> Gerd Hoffmann  wrote:
> 
> >   Hi,
> >
> > > My suggestion was to use vfio device fd for this ioctl and have
> > > dmabuf mgr fd as member in above query_plane structure, for region
> > > type it would be set to 0.
> >
> > Region type should be DRM_PLANE_TYPE_PRIMARY
> >
> > > Can't mmap that page to get surface information. There is no way to
> > > synchronize between QEMU reading this mmapped region and vendor
> > > driver writing it. There could be race condition in these two
> > > operations.
> > > Read
> > > on this page should be trapped and blocking, so that surface in that
> > > region is only updated when its asked for.
> >
> > Does it make sense to have a "generation" field in the plane_info
> > struct (which gets increased each time the struct changes) ?
> 
> It seems less cumbersome than checking each field to see if it has changed.
> Thanks,
> 
> Alex
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v7 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-g

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Xiaoguang Chen
> Sent: Wednesday, June 7, 2017 3:45 PM
> To: alex.william...@redhat.com; kra...@redhat.com; ch...@chris-wilson.co.uk;
> intel-gfx@lists.freedesktop.org; linux-ker...@vger.kernel.org;
> zhen...@linux.intel.com; Lv, Zhiyuan ; intel-gvt-
> d...@lists.freedesktop.org; Wang, Zhi A ; Tian, Kevin
> 
> Cc: Chen, Xiaoguang 
> Subject: [PATCH v7 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-g
> 
> decode frambuffer attributes of primary, cursor and sprite plane
> 
> Signed-off-by: Xiaoguang Chen 
> ---
>  drivers/gpu/drm/i915/gvt/Makefile |   3 +-
>  drivers/gpu/drm/i915/gvt/display.c|   2 +-
>  drivers/gpu/drm/i915/gvt/display.h|   2 +
>  drivers/gpu/drm/i915/gvt/fb_decoder.c | 439
> ++
>  drivers/gpu/drm/i915/gvt/fb_decoder.h | 167 +
>  drivers/gpu/drm/i915/gvt/gvt.h|   1 +
>  include/uapi/drm/drm_fourcc.h |   6 +
>  7 files changed, 618 insertions(+), 2 deletions(-)  create mode 100644
> drivers/gpu/drm/i915/gvt/fb_decoder.c
>  create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h
> 
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile
> b/drivers/gpu/drm/i915/gvt/Makefile
> index b123c20..192ca26 100644
> --- a/drivers/gpu/drm/i915/gvt/Makefile
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -1,7 +1,8 @@
>  GVT_DIR := gvt
>  GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o
> firmware.o \
>   interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
> - execlist.o scheduler.o sched_policy.o render.o cmd_parser.o
> + execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \
> + fb_decoder.o
> 
>  ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
>  i915-y   += $(addprefix $(GVT_DIR)/,
> $(GVT_SOURCE))
> diff --git a/drivers/gpu/drm/i915/gvt/display.c
> b/drivers/gpu/drm/i915/gvt/display.c
> index e0261fc..f5f63c5 100644
> --- a/drivers/gpu/drm/i915/gvt/display.c
> +++ b/drivers/gpu/drm/i915/gvt/display.c
> @@ -67,7 +67,7 @@ static int edp_pipe_is_enabled(struct intel_vgpu *vgpu)
>   return 1;
>  }
> 
> -static int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
> +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
>  {
>   struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> 
> diff --git a/drivers/gpu/drm/i915/gvt/display.h
> b/drivers/gpu/drm/i915/gvt/display.h
> index d73de22..b46b868 100644
> --- a/drivers/gpu/drm/i915/gvt/display.h
> +++ b/drivers/gpu/drm/i915/gvt/display.h
> @@ -179,4 +179,6 @@ int intel_vgpu_init_display(struct intel_vgpu *vgpu,
> u64 resolution);  void intel_vgpu_reset_display(struct intel_vgpu *vgpu);  
> void
> intel_vgpu_clean_display(struct intel_vgpu *vgpu);
> 
> +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe);
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c
> b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> new file mode 100644
> index 000..0825949
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> @@ -0,0 +1,439 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a
> + * copy of this software and associated documentation files (the
> +"Software"),
> + * to deal in the Software without restriction, including without
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> +sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> +next
> + * paragraph) shall be included in all copies or substantial portions
> +of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT
> +SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR
> +OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> +ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +DEALINGS IN THE
> + * SOFTWARE.
> + *
> + * Authors:
> + *Kevin Tian 
> + *
> + * Contributors:
> + *Bing Niu 
> + *Xu Han 
> + *Ping Gao 
> + *Xiaoguang Chen 
> + *Yang Liu 
> + *
> + */
> +
> +#include 
> +#include "i915_drv.h"
> +#include "gvt.h"
> +
> +#define PRIMARY_FORMAT_NUM   16
> +struct 

Re: [Intel-gfx] [PATCH v3] drm/i915: Enable guest i915 full ppgtt functionality

[Zhang, Tina]

> -Original Message-
> From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
> Sent: Tuesday, May 30, 2017 4:38 PM
> To: Zhang, Tina <tina.zh...@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: zhen...@linux.intel.com; intel-gvt-...@lists.freedesktop.org
> Subject: Re: [PATCH v3] drm/i915: Enable guest i915 full ppgtt functionality
> 
> On ma, 2017-05-22 at 16:19 +0800, Tina Zhang wrote:
> > Enable the guest i915 full ppgtt functionality when host can provide
> > this capability. vgt_caps is introduced to guest i915 driver to get
> > the vgpu capabilities from the device model. VGT_CPAS_FULL_PPGTT is
> > one of the capabilities type to let guest i915 dirver know that the
> > guest i915 full ppgtt is supported by device model.
> >
> > Changes since v1:
> > - Use u32 instead of uint32_t (Joonas)
> > - Move VGT_CAPS_FULL_PPGTT introduction to this patch and use #define
> >   instead of enum (Joonas)
> > - Rewrite the vgpu full ppgtt capability checking logic. (Joonas)
> > - Some coding style refine. (Joonas)
> >
> > Changes since v2:
> > - Divide the whole patch set into two separate patch series, with one
> >   patch in i915 side to check guest i915 full ppgtt capability and
> > enable
> >   it when this capability is supported by the device model, and the
> > other
> >   one in gvt side which fixs the blocking issue and enables the device
> >   model to provide the capability to guest. And this patch focuses on
> > guest
> >   i915 side. (Joonas)
> > - Change the title from "introduce vgt_caps to pvinfo" to
> >   "Enable guest i915 full ppgtt functionality". (Tina)
> >
> > Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> 
> I just noticed there is INTEL_VGT_IF_VERSION when I was looking to make sure
> that vgt_if is zeroed. Neither the version is incremented nor do I
> see VGT_PVINFO_PAGE getting zeroed.
Vgt_if gets its initial values populated by device model through reading 
hardware registers, named "SW Virtualization Reserved MMIO rang". These 
registers are always zeros when reading by device model. So, we use this way to 
initialize the vgt_if with zeros.
About the INTEL_VGT_IF_VERSION, agree that we need to doc a Spec to describe it 
in detail, I guess. But for this vgt_cap register definition, I'm afraid we 
don't need to bump the version. Because, actually, this register is already 
defined in the version 1.0, and we just didn't use it before.

> 
> What measures are in place to make sure running a new i915 under older
> DOM0 won't result in corruption?
This won't be a problem, I guess, as we have already considered this situation. 
(Thanks for Zhenyu's comments about this problem, before)
I915 with this new patch, will check whether the cap is enabled by device 
model. As the device model is old, this cap should not be enabled and the i915 
in the guest will get value zero, which means device model cannot support the 
full ppgtt feature. So, in this way, the guest i915 can only use the aliasing 
ppgtt which is also the only choice for guest i915 before this patch.

> 
> The dependencies between i915 and gvt are rather tricky, so we'd need
> INTEL_VGT_IF_VERSION minor increment and also a one line change (zeroing of
> the new caps register) from gvt code to the same patch, otherwise bisecting 
> will
> break.
> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/4] drm/i915/gvt: reorder the shadow ppgtt update process by adding entry first

[Zhang, Tina]

> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Joonas Lahtinen
> Sent: Monday, May 15, 2017 6:50 PM
> To: Zhang, Tina <tina.zh...@intel.com>; intel-gvt-...@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915/gvt: reorder the shadow ppgtt
> update process by adding entry first
> 
> On pe, 2017-05-12 at 17:37 +0800, Tina Zhang wrote:
> > Guest i915 driver may update the ppgtt table just before this workload
> > is going to be submitted to the hardware by device model. This case
> > wasn't handled well by device model before, due to the small time
> > window between removing old ppgtt entry and adding the new one. Errors
> > occur when the workload is executed by hardware during that small time
> > window. This patch is to remove this time window by adding the new
> > ppgtt entry first and then remove the old one.
> >
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> 
> This should be reviewed on the GVT mailing list, and this should include the
> squashed hunk which exports the newly added capability.
Thanks for the comments. I'm sorry I didn’t get it, here. Do you want me to 
remove this patch from this patch set?
Well, the reason this patch is here is because we met a GPU hang issue when 
guest i915 using full ppgtt. This is a blocking issue for enabling guest i915 
full ppgtt functionality. This patch is to fix that issue. So, with this patch, 
the device model can have the capability to support guest i915 full ppgtt 
functionality. And the other patches in this patch set are used for guest to 
communicate with host whether the full ppgtt capability is supported.

> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx