| Message ID | 1456927221-32421-8-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote: > @@ -1006,8 +1019,9 @@ static cairo_surface_t *get_cairo_surface(int fd, struct igt_fb *fb) > create_cairo_surface__gtt(fd, fb); > } > > - gem_set_domain(fd, fb->gem_handle, > - I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); > + if (!fb->is_dumb) > + gem_set_domain(fd, fb->gem_handle, I915_GEM_DOMAIN_CPU, > + I915_GEM_DOMAIN_CPU); At the risk of opening a can-of-worms, what is the synchronisation protocol for dumb buffers? Even CPU access to a dumb needs set-domain on Intel. -Chris
On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote: > > @@ -1006,8 +1019,9 @@ static cairo_surface_t *get_cairo_surface(int > > fd, struct igt_fb *fb) > > create_cairo_surface__gtt(fd, fb); > > } > > > > - gem_set_domain(fd, fb->gem_handle, > > - I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); > > + if (!fb->is_dumb) > > + gem_set_domain(fd, fb->gem_handle, > > I915_GEM_DOMAIN_CPU, > > + I915_GEM_DOMAIN_CPU); > At the risk of opening a can-of-worms, what is the synchronisation > protocol for dumb buffers? Even CPU access to a dumb needs set-domain > on > Intel. Then Intel is broken, because the literal entire point of dumb buffers is that you do not require driver-specific calls to operate them. Map, populate, unmap, display. Cheers, Daniel
On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote: > On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote: > > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote: > > > @@ -1006,8 +1019,9 @@ static cairo_surface_t *get_cairo_surface(int > > > fd, struct igt_fb *fb) > > > create_cairo_surface__gtt(fd, fb); > > > } > > > > > > - gem_set_domain(fd, fb->gem_handle, > > > - I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); > > > + if (!fb->is_dumb) > > > + gem_set_domain(fd, fb->gem_handle, > > > I915_GEM_DOMAIN_CPU, > > > + I915_GEM_DOMAIN_CPU); > > At the risk of opening a can-of-worms, what is the synchronisation > > protocol for dumb buffers? Even CPU access to a dumb needs set-domain > > on > > Intel. > > Then Intel is broken, because the literal entire point of dumb buffers > is that you do not require driver-specific calls to operate them. > > Map, populate, unmap, display. Don't forget to call dirtyfb then. -Chris
On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote: > > On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote: > > > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote: > > > > - gem_set_domain(fd, fb->gem_handle, > > > > - I915_GEM_DOMAIN_CPU, > > > > I915_GEM_DOMAIN_CPU); > > > > + if (!fb->is_dumb) > > > > + gem_set_domain(fd, fb->gem_handle, > > > > I915_GEM_DOMAIN_CPU, > > > > + I915_GEM_DOMAIN_CPU); > > > At the risk of opening a can-of-worms, what is the > > > synchronisation > > > protocol for dumb buffers? Even CPU access to a dumb needs set- > > > domain > > > on > > > Intel. > > Then Intel is broken, because the literal entire point of dumb > > buffers > > is that you do not require driver-specific calls to operate them. > > > > Map, populate, unmap, display. > Don't forget to call dirtyfb then. Are you talking about frontbuffer rendering, or pageflipping between two dumb buffers?
On Wed, Mar 02, 2016 at 02:40:44PM +0000, Daniel Stone wrote: > > On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote: > > On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote: > > > On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote: > > > > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote: > > > > > - gem_set_domain(fd, fb->gem_handle, > > > > > - I915_GEM_DOMAIN_CPU, > > > > > I915_GEM_DOMAIN_CPU); > > > > > + if (!fb->is_dumb) > > > > > + gem_set_domain(fd, fb->gem_handle, > > > > > I915_GEM_DOMAIN_CPU, > > > > > + I915_GEM_DOMAIN_CPU); > > > > At the risk of opening a can-of-worms, what is the > > > > synchronisation > > > > protocol for dumb buffers? Even CPU access to a dumb needs set- > > > > domain > > > > on > > > > Intel. > > > Then Intel is broken, because the literal entire point of dumb > > > buffers > > > is that you do not require driver-specific calls to operate them. > > > > > > Map, populate, unmap, display. > > Don't forget to call dirtyfb then. > > Are you talking about frontbuffer rendering, or pageflipping between > two dumb buffers? Afaik, no one yet tracks damage on a backbuffer before a flip. But we don't constrain the tests to backbuffer as we do need to exercise frontbuffer rendering and iirc those tests all use set-domain. I don't see any PSR/FBC testing using the dumb framebuffers... Or is the dumb framebuffer purely a backbuffer + flip model? -Chris
On Wed, 2016-03-02 at 14:54 +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 02:40:44PM +0000, Daniel Stone wrote: > > On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote: > > > Don't forget to call dirtyfb then. > > Are you talking about frontbuffer rendering, or pageflipping > > between > > two dumb buffers? > Afaik, no one yet tracks damage on a backbuffer before a flip. But we > don't constrain the tests to backbuffer as we do need to exercise > frontbuffer rendering and iirc those tests all use set-domain. I > don't > see any PSR/FBC testing using the dumb framebuffers... Or is the dumb > framebuffer purely a backbuffer + flip model? Right, Weston strictly uses pageflipping, because frontbuffer rendering isn't pretty. xf86-video-modesetting uses frontbuffer rendering, and does DirtyFB. So I'd say that's our ABI. Cheers, Daniel
On Wed, Mar 02, 2016 at 02:54:20PM +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 02:40:44PM +0000, Daniel Stone wrote: > > > > On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote: > > > On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote: > > > > On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote: > > > > > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote: > > > > > > - gem_set_domain(fd, fb->gem_handle, > > > > > > - I915_GEM_DOMAIN_CPU, > > > > > > I915_GEM_DOMAIN_CPU); > > > > > > + if (!fb->is_dumb) > > > > > > + gem_set_domain(fd, fb->gem_handle, > > > > > > I915_GEM_DOMAIN_CPU, > > > > > > + I915_GEM_DOMAIN_CPU); > > > > > At the risk of opening a can-of-worms, what is the > > > > > synchronisation > > > > > protocol for dumb buffers? Even CPU access to a dumb needs set- > > > > > domain > > > > > on > > > > > Intel. > > > > Then Intel is broken, because the literal entire point of dumb > > > > buffers > > > > is that you do not require driver-specific calls to operate them. > > > > > > > > Map, populate, unmap, display. > > > Don't forget to call dirtyfb then. > > > > Are you talking about frontbuffer rendering, or pageflipping between > > two dumb buffers? > > Afaik, no one yet tracks damage on a backbuffer before a flip. But we > don't constrain the tests to backbuffer as we do need to exercise > frontbuffer rendering and iirc those tests all use set-domain. I don't > see any PSR/FBC testing using the dumb framebuffers... Or is the dumb > framebuffer purely a backbuffer + flip model? Yeah, but for those tests we do have explict set_domain calls. Anyway the dumb model is mmap + either flip (setcrtc, setplane, page_flip, atomic) or dirtyfb. I think for low level functions like these exposing this explicitly to tests is ok. If you mix dumb with rendering you simply need to know what you're doing. But for rendering that's a requirement anyway. -Daniel
On Sat, Mar 05, 2016 at 01:24:19PM +0100, Daniel Vetter wrote: > On Wed, Mar 02, 2016 at 02:54:20PM +0000, Chris Wilson wrote: > > On Wed, Mar 02, 2016 at 02:40:44PM +0000, Daniel Stone wrote: > > > > > > On Wed, 2016-03-02 at 14:39 +0000, Chris Wilson wrote: > > > > On Wed, Mar 02, 2016 at 02:22:58PM +0000, Daniel Stone wrote: > > > > > On Wed, 2016-03-02 at 14:21 +0000, Chris Wilson wrote: > > > > > > On Wed, Mar 02, 2016 at 03:00:14PM +0100, Tomeu Vizoso wrote: > > > > > > > - gem_set_domain(fd, fb->gem_handle, > > > > > > > - I915_GEM_DOMAIN_CPU, > > > > > > > I915_GEM_DOMAIN_CPU); > > > > > > > + if (!fb->is_dumb) > > > > > > > + gem_set_domain(fd, fb->gem_handle, > > > > > > > I915_GEM_DOMAIN_CPU, > > > > > > > + I915_GEM_DOMAIN_CPU); > > > > > > At the risk of opening a can-of-worms, what is the > > > > > > synchronisation > > > > > > protocol for dumb buffers? Even CPU access to a dumb needs set- > > > > > > domain > > > > > > on > > > > > > Intel. > > > > > Then Intel is broken, because the literal entire point of dumb > > > > > buffers > > > > > is that you do not require driver-specific calls to operate them. > > > > > > > > > > Map, populate, unmap, display. > > > > Don't forget to call dirtyfb then. > > > > > > Are you talking about frontbuffer rendering, or pageflipping between > > > two dumb buffers? > > > > Afaik, no one yet tracks damage on a backbuffer before a flip. But we > > don't constrain the tests to backbuffer as we do need to exercise > > frontbuffer rendering and iirc those tests all use set-domain. I don't > > see any PSR/FBC testing using the dumb framebuffers... Or is the dumb > > framebuffer purely a backbuffer + flip model? > > Yeah, but for those tests we do have explict set_domain calls. Anyway the > dumb model is mmap + either flip (setcrtc, setplane, page_flip, atomic) or > dirtyfb. I think for low level functions like these exposing this > explicitly to tests is ok. > > If you mix dumb with rendering you simply need to know what you're doing. > But for rendering that's a requirement anyway. Ok, maybe I should read the patch, it's a high-level function ;-) In that case, yes please add a dirtyfb ioctl call _after_ the rendering to make it all just work. Since we don't know how callers use it. But for library completeness I think it'd be good to have kmstest_ wrappers for both dumb mmap and dirtyfb (maybe just defer to the libdrm one) like with the dumb create one. -Daniel
diff --git a/lib/igt_fb.c b/lib/igt_fb.c index 3e76a419b3ee..cd1605308308 100644 --- a/lib/igt_fb.c +++ b/lib/igt_fb.c @@ -984,7 +984,20 @@ static void destroy_cairo_surface__gtt(void *arg) static void create_cairo_surface__gtt(int fd, struct igt_fb *fb) { - void *ptr = gem_mmap__gtt(fd, fb->gem_handle, fb->size, PROT_READ | PROT_WRITE); + void *ptr; + + if (fb->is_dumb) { + struct drm_mode_map_dumb arg = {}; + + arg.handle = fb->gem_handle; + + do_ioctl(fd, DRM_IOCTL_MODE_MAP_DUMB, &arg); + + ptr = mmap(0, fb->size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, arg.offset); + igt_assert(ptr != MAP_FAILED); + } else + ptr = gem_mmap__gtt(fd, fb->gem_handle, fb->size, + PROT_READ | PROT_WRITE); fb->cairo_surface = cairo_image_surface_create_for_data(ptr, @@ -1006,8 +1019,9 @@ static cairo_surface_t *get_cairo_surface(int fd, struct igt_fb *fb) create_cairo_surface__gtt(fd, fb); } - gem_set_domain(fd, fb->gem_handle, - I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); + if (!fb->is_dumb) + gem_set_domain(fd, fb->gem_handle, I915_GEM_DOMAIN_CPU, + I915_GEM_DOMAIN_CPU); igt_assert(cairo_surface_status(fb->cairo_surface) == CAIRO_STATUS_SUCCESS); return fb->cairo_surface;
If a buffer object is dumb, call DRM_IOCTL_MODE_MAP_DUMB when mapping it. Also, don't call DRM_IOCTL_I915_GEM_SET_DOMAIN on dumb buffers. Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> --- lib/igt_fb.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)