Message ID | 20240812191639.649841-1-maaz.mombasawala@broadcom.com (mailing list archive) |
---|---|
Headers | show |
Series | drm/vmwgfx: Add support for userspace managed surfaces. | expand |
On Mon, Aug 12, 2024 at 3:16 PM Maaz Mombasawala <maaz.mombasawala@broadcom.com> wrote: > > This series introduces basic support for userspace managed surfaces. The > lifetime and id's of these surfaces is managed by userspace submitted > commands instead of relying on the kernel to manage them. > > Maaz Mombasawala (3): > drm/vmwgfx: Introduce userspace managed surfaces > drm/vmwgfx: Support hw_destroy for userspace managed surfaces > drm/vmwgfx: Add support for older define commands for userspace > surfaces > > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 24 +- > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 331 ++++++++++++++++++++++-- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 196 +++++++++++++- > 3 files changed, 518 insertions(+), 33 deletions(-) > In general that looks great. Do you happen to have the userspace patch somewhere where we could see it? In particular there are three things I'm wondering about: 1) In the first patch you mark the gb surface as may_evict = false; correctly, because if user space is the thing that attaches mob's then kernel can not switch them underneath but then I'd like to see how are the memory pressure situations handled on the user-side, 2) Since now we allow surface destroy commands from userspace could one trigger some kernel oops when running old surface defines with mob_create flag set and issuing the gb surface destroy or will the res->id be reset properly? 3) how is userspace able to select whether it should self-manage the mob's or let the kernel do it? i.e. what flag signifies that the userspace is running on a kernel that is capable of handling this? z
On 8/13/24 19:33, Zack Rusin wrote: > On Mon, Aug 12, 2024 at 3:16 PM Maaz Mombasawala > <maaz.mombasawala@broadcom.com> wrote: >> >> This series introduces basic support for userspace managed surfaces. The >> lifetime and id's of these surfaces is managed by userspace submitted >> commands instead of relying on the kernel to manage them. >> >> Maaz Mombasawala (3): >> drm/vmwgfx: Introduce userspace managed surfaces >> drm/vmwgfx: Support hw_destroy for userspace managed surfaces >> drm/vmwgfx: Add support for older define commands for userspace >> surfaces >> >> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 24 +- >> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 331 ++++++++++++++++++++++-- >> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 196 +++++++++++++- >> 3 files changed, 518 insertions(+), 33 deletions(-) >> > > In general that looks great. Do you happen to have the userspace patch > somewhere where we could see it? In particular there are three things > I'm wondering about: There is a patch for mesa, I'll work on getting that upstreamed too. > 1) In the first patch you mark the gb surface as may_evict = false; > correctly, because if user space is the thing that attaches mob's then > kernel can not switch them underneath but then I'd like to see how are > the memory pressure situations handled on the user-side, I haven't tested this under memory pressure conditions. That should be a userspace issue though. > 2) Since now we allow surface destroy commands from userspace could > one trigger some kernel oops when running old surface defines with > mob_create flag set and issuing the gb surface destroy or will the > res->id be reset properly? For userspace managed surfaces, the driver only accepts ids in the range 1 to 32768 (VMWGFX_NUM_GB_SURFACE) and for an old kernel managed surfaces it only returns ids in range starting at 53040 (VMWGFX_NUM_MOB). So if userspace defines a surface with the old ioctls and then tries to issue the gb_surface_destroy on that surface with that id then the command buffer submission will fail because it won't pass the res checks. > 3) how is userspace able to select whether it should self-manage the > mob's or let the kernel do it? i.e. what flag signifies that the > userspace is running on a kernel that is capable of handling this? There is no such flag right now, I will add this. > > z