On Fri, Nov 04, 2016 at 12:20:51PM -0400, Leo Liu wrote: > Hi Nayan, > > With this patch, the resizing corruption is fixed, thanks for that. > > Still a few comments below. > > > On 11/04/2016 03:08 AM, Nayan Deshmukh wrote: > > dri3 allows us to send handle of a texture directly to X > > so this patch allows a state tracker to directly send its > > texture to X to be used as back buffer and avoids extra > > copying > > > > v2: use clip width/height to display a portion of the surface > > > > Suggested-by: Leo Liu <leo....@amd.com> > > Signed-off-by: Nayan Deshmukh <nayan26deshm...@gmail.com> > > --- > > src/gallium/auxiliary/vl/vl_winsys.h | 4 ++ > > src/gallium/auxiliary/vl/vl_winsys_dri3.c | 109 > > ++++++++++++++++++++++++++---- > > 2 files changed, 99 insertions(+), 14 deletions(-) > > > > diff --git a/src/gallium/auxiliary/vl/vl_winsys.h > > b/src/gallium/auxiliary/vl/vl_winsys.h > > index 26db9f2..908e9c6 100644 > > --- a/src/gallium/auxiliary/vl/vl_winsys.h > > +++ b/src/gallium/auxiliary/vl/vl_winsys.h > > @@ -59,6 +59,10 @@ struct vl_screen > > void * > > (*get_private)(struct vl_screen *vscreen); > > + void > > + (*set_back_texture_from_output)(struct vl_screen *vscreen, struct > > pipe_resource *buffer, > > + uint32_t width, uint32_t height); > > + > > struct pipe_screen *pscreen; > > struct pipe_loader_device *dev; > > }; > > diff --git a/src/gallium/auxiliary/vl/vl_winsys_dri3.c > > b/src/gallium/auxiliary/vl/vl_winsys_dri3.c > > index 2929928..9739be4 100644 > > --- a/src/gallium/auxiliary/vl/vl_winsys_dri3.c > > +++ b/src/gallium/auxiliary/vl/vl_winsys_dri3.c > > @@ -31,6 +31,7 @@ > > #include <X11/xshmfence.h> > > #include <xcb/dri3.h> > > #include <xcb/present.h> > > +#include <xcb/xfixes.h> > > #include "loader.h" > > @@ -56,7 +57,9 @@ struct vl_dri3_buffer > > struct xshmfence *shm_fence; > > bool busy; > > + bool is_external_texture; > > uint32_t width, height, pitch; > > + uint32_t clip_width, clip_height; > > }; > > struct vl_dri3_screen > > @@ -71,6 +74,9 @@ struct vl_dri3_screen > > xcb_special_event_t *special_event; > > struct pipe_context *pipe; > > + struct pipe_resource *output_texture; > > + uint32_t output_texture_width; > > + uint32_t output_texture_height; > > Are these clipping sizes? Yes indeed, may be something like output_texture_clip_width would be more appropriate > > > struct vl_dri3_buffer *back_buffers[BACK_BUFFER_NUM]; > > int cur_back; > > @@ -105,7 +111,8 @@ dri3_free_back_buffer(struct vl_dri3_screen *scrn, > > xcb_free_pixmap(scrn->conn, buffer->pixmap); > > xcb_sync_destroy_fence(scrn->conn, buffer->sync_fence); > > xshmfence_unmap_shm(buffer->shm_fence); > > - pipe_resource_reference(&buffer->texture, NULL); > > + if (!buffer->is_external_texture) > > + pipe_resource_reference(&buffer->texture, NULL); > > if (buffer->linear_texture) > > pipe_resource_reference(&buffer->linear_texture, NULL); > > FREE(buffer); > > @@ -236,14 +243,23 @@ dri3_alloc_back_buffer(struct vl_dri3_screen *scrn) > > templ.format = PIPE_FORMAT_B8G8R8X8_UNORM; > > templ.target = PIPE_TEXTURE_2D; > > templ.last_level = 0; > > - templ.width0 = scrn->width; > > - templ.height0 = scrn->height; > > + if (scrn->output_texture) { > > + buffer->clip_width = (scrn->output_texture_width) ? > > scrn->output_texture_width : scrn->output_texture->width0; > > + buffer->clip_height = (scrn->output_texture_height) ? > > scrn->output_texture_height : scrn->output_texture->height0; > > Can you wrap these lines and many new added lines below? > Normally make a line less than ~80 chars, and make lines from above and > below balanced. > I think the whole purpose of this is to make more readable and looks nicer. > buffer->clip_width = (scrn->output_texture_width) ? scrn->output_texture_width : scrn->output_texture->width0;
Is something like this fine? > > + templ.width0 = scrn->output_texture->width0; > > + templ.height0 = scrn->output_texture->height0; > > + } else { > > + templ.width0 = scrn->width; > > + templ.height0 = scrn->height; > > + buffer->clip_width = scrn->width; > > + buffer->clip_height = scrn->height; > > + } > > templ.depth0 = 1; > > templ.array_size = 1; > > if (scrn->is_different_gpu) { > > - buffer->texture = > > scrn->base.pscreen->resource_create(scrn->base.pscreen, > > - &templ); > > + buffer->texture = (scrn->output_texture) ? scrn->output_texture : > > scrn->base.pscreen->resource_create(scrn->base.pscreen, > > + > > &templ); > > if (!buffer->texture) > > goto unmap_shm; > > @@ -257,8 +273,8 @@ dri3_alloc_back_buffer(struct vl_dri3_screen *scrn) > > goto no_linear_texture; > > } else { > > templ.bind |= PIPE_BIND_SCANOUT | PIPE_BIND_SHARED; > > - buffer->texture = > > scrn->base.pscreen->resource_create(scrn->base.pscreen, > > - &templ); > > + buffer->texture = (scrn->output_texture) ? scrn->output_texture : > > scrn->base.pscreen->resource_create(scrn->base.pscreen, > > + > > &templ); > > if (!buffer->texture) > > goto unmap_shm; > > pixmap_buffer_texture = buffer->texture; > > @@ -271,11 +287,15 @@ dri3_alloc_back_buffer(struct vl_dri3_screen *scrn) > > usage); > > buffer_fd = whandle.handle; > > buffer->pitch = whandle.stride; > > + buffer->width = templ.width0; > > + buffer->height = templ.height0; > > + buffer->is_external_texture = (scrn->output_texture) ? true : false; > > + > > Is "is_external_texture" redundant ? Cannot think of any case that it will > be different from status of scrn->output_texture? > With the current version of the patch it is redundant, in the previous patch I was setting output_texture to NULL after every frame and hence I forgot to remove this > > xcb_dri3_pixmap_from_buffer(scrn->conn, > > (pixmap = xcb_generate_id(scrn->conn)), > > scrn->drawable, > > 0, > > - scrn->width, scrn->height, buffer->pitch, > > + buffer->width, buffer->height, > > buffer->pitch, > > scrn->depth, 32, > > buffer_fd); > > xcb_dri3_fence_from_fd(scrn->conn, > > @@ -287,8 +307,6 @@ dri3_alloc_back_buffer(struct vl_dri3_screen *scrn) > > buffer->pixmap = pixmap; > > buffer->sync_fence = sync_fence; > > buffer->shm_fence = shm_fence; > > - buffer->width = scrn->width; > > - buffer->height = scrn->height; > > xshmfence_trigger(buffer->shm_fence); > > @@ -310,6 +328,7 @@ dri3_get_back_buffer(struct vl_dri3_screen *scrn) > > { > > struct vl_dri3_buffer *buffer; > > struct pipe_resource *texture = NULL; > > + bool allocate_new_buffer = false; > > assert(scrn); > > @@ -318,8 +337,22 @@ dri3_get_back_buffer(struct vl_dri3_screen *scrn) > > return NULL; > > buffer = scrn->back_buffers[scrn->cur_back]; > > - if (!buffer || buffer->width != scrn->width || > > - buffer->height != scrn->height) { > > + /* This is normal case when our buffer is smaller > > + * than the screen this will be same for external > > + * texture > > + */ > > + if (!buffer || buffer->width < scrn->width || > > + buffer->height < scrn->height) > > + allocate_new_buffer = true; > > Have you considered the comment on this from the V1? i.e. how about VA-API? > While looking at it right now, I got an idea of dealing with it in a way in which vaapi won't be affected at all, v3 is on its way > > + > > + /* In case of a single gpu we need to get the > > + * handle and pixmap for the texture that is set > > + */ > > + if (buffer && buffer->is_external_texture && > > + !scrn->is_different_gpu) > > + allocate_new_buffer = true; > > + > > + if (allocate_new_buffer) { > > struct vl_dri3_buffer *new_buffer; > > new_buffer = dri3_alloc_back_buffer(scrn); > > @@ -332,6 +365,14 @@ dri3_get_back_buffer(struct vl_dri3_screen *scrn) > > vl_compositor_reset_dirty_area(&scrn->dirty_areas[scrn->cur_back]); > > Do we still need dirty_area for this direct output as back buffer case? > No we don't I 'll put an if statement here. Thanks for the review. Regards, Nayan > > Regards, > Leo > > > > buffer = new_buffer; > > scrn->back_buffers[scrn->cur_back] = buffer; > > + } else if (buffer->is_external_texture) { > > + /* In case of different gpu we can reuse the linear > > + * texture so we only need to set the external > > + * texture for copying > > + */ > > + buffer->texture = scrn->output_texture; > > + buffer->clip_width = scrn->output_texture_width; > > + buffer->clip_height = scrn->output_texture_height; > > } > > pipe_resource_reference(&texture, buffer->texture); > > @@ -500,6 +541,8 @@ vl_dri3_flush_frontbuffer(struct pipe_screen *screen, > > uint32_t options = XCB_PRESENT_OPTION_NONE; > > struct vl_dri3_buffer *back; > > struct pipe_box src_box; > > + xcb_xfixes_region_t region; > > + xcb_rectangle_t rectangle; > > back = scrn->back_buffers[scrn->cur_back]; > > if (!back) > > @@ -511,8 +554,16 @@ vl_dri3_flush_frontbuffer(struct pipe_screen *screen, > > return; > > } > > + rectangle.x = 0; > > + rectangle.y = 0; > > + rectangle.width = back->clip_width; > > + rectangle.height = back->clip_height; > > + > > + region = xcb_generate_id(scrn->conn); > > + xcb_xfixes_create_region(scrn->conn, region, 2, &rectangle); > > + > > if (scrn->is_different_gpu) { > > - u_box_origin_2d(scrn->width, scrn->height, &src_box); > > + u_box_origin_2d(back->width, back->height, &src_box); > > scrn->pipe->resource_copy_region(scrn->pipe, > > back->linear_texture, > > 0, 0, 0, 0, > > @@ -528,7 +579,7 @@ vl_dri3_flush_frontbuffer(struct pipe_screen *screen, > > scrn->drawable, > > back->pixmap, > > (uint32_t)(++scrn->send_sbc), > > - 0, 0, 0, 0, > > + 0, region, 0, 0, > > None, None, > > back->sync_fence, > > options, > > @@ -627,6 +678,19 @@ vl_dri3_screen_get_private(struct vl_screen *vscreen) > > } > > static void > > +vl_dri3_screen_set_back_texture_from_output(struct vl_screen *vscreen, > > struct pipe_resource *buffer, > > + uint32_t width, uint32_t height) > > +{ > > + struct vl_dri3_screen *scrn = (struct vl_dri3_screen *)vscreen; > > + > > + assert(scrn); > > + > > + scrn->output_texture = buffer; > > + scrn->output_texture_width = width; > > + scrn->output_texture_height = height; > > +} > > + > > +static void > > vl_dri3_screen_destroy(struct vl_screen *vscreen) > > { > > struct vl_dri3_screen *scrn = (struct vl_dri3_screen *)vscreen; > > @@ -675,6 +739,9 @@ vl_dri3_screen_create(Display *display, int screen) > > xcb_dri3_open_reply_t *open_reply; > > xcb_get_geometry_cookie_t geom_cookie; > > xcb_get_geometry_reply_t *geom_reply; > > + xcb_xfixes_query_version_cookie_t xfixes_cookie; > > + xcb_xfixes_query_version_reply_t *xfixes_reply; > > + xcb_generic_error_t *error; > > int fd; > > assert(display); > > @@ -689,12 +756,25 @@ vl_dri3_screen_create(Display *display, int screen) > > xcb_prefetch_extension_data(scrn->conn , &xcb_dri3_id); > > xcb_prefetch_extension_data(scrn->conn, &xcb_present_id); > > + xcb_prefetch_extension_data (scrn->conn, &xcb_xfixes_id); > > extension = xcb_get_extension_data(scrn->conn, &xcb_dri3_id); > > if (!(extension && extension->present)) > > goto free_screen; > > extension = xcb_get_extension_data(scrn->conn, &xcb_present_id); > > if (!(extension && extension->present)) > > goto free_screen; > > + extension = xcb_get_extension_data(scrn->conn, &xcb_xfixes_id); > > + if (!(extension && extension->present)) > > + goto free_screen; > > + > > + xfixes_cookie = xcb_xfixes_query_version(scrn->conn, > > XCB_XFIXES_MAJOR_VERSION, XCB_XFIXES_MINOR_VERSION); > > + xfixes_reply = xcb_xfixes_query_version_reply(scrn->conn, > > xfixes_cookie, &error); > > + if (!xfixes_reply || error || xfixes_reply->major_version < 2) { > > + free(error); > > + free(xfixes_reply); > > + goto free_screen; > > + } > > + free(xfixes_reply); > > open_cookie = xcb_dri3_open(scrn->conn, RootWindow(display, screen), > > None); > > open_reply = xcb_dri3_open_reply(scrn->conn, open_cookie, NULL); > > @@ -744,6 +824,7 @@ vl_dri3_screen_create(Display *display, int screen) > > scrn->base.set_next_timestamp = vl_dri3_screen_set_next_timestamp; > > scrn->base.get_private = vl_dri3_screen_get_private; > > scrn->base.pscreen->flush_frontbuffer = vl_dri3_flush_frontbuffer; > > + scrn->base.set_back_texture_from_output = > > vl_dri3_screen_set_back_texture_from_output; > > return &scrn->base; > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev