<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, Oct 27, 2016 at 10:17 PM, Leo Liu <<a href="mailto:leo.liu@amd.com" target="_blank">leo.liu@amd.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5"> On 10/27/2016 12:20 PM, Nayan Deshmukh wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> On Thu, Oct 27, 2016 at 10:38:30AM -0400, Leo Liu wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> On 10/24/2016 09:55 AM, Nayan Deshmukh wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Suggested-by: Leo Liu <<a href="mailto:leo.liu@amd.com" target="_blank">leo.liu@amd.com</a>> Signed-off-by: Nayan Deshmukh <<a href="mailto:nayan26deshmukh@gmail.com" target="_blank">nayan26deshmukh@gmail.com</a>> --- src/gallium/auxiliary/vl/vl_winsys.h | 4 ++ src/gallium/auxiliary/vl/vl_winsys_dri3.c | 89 +++++++++++++++++++++++++++---- 2 files changed, 83 insertions(+), 10 deletions(-) diff --git a/src/gallium/auxiliary/vl/vl_winsys.h b/src/gallium/auxiliary/vl/vl_winsys.h index 26db9f2..7c56b48 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_output_texture)(struct vl_screen *vscreen, struct pipe_resource *buffer, + uint32_t width, uint32_t height); + </blockquote> I think it's more appropriate if it's called "set_back_texture_from_output" </blockquote> Yeah that makes more sense. <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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..44d6f4c 100644 --- a/src/gallium/auxiliary/vl/vl_winsys_dri3.c +++ b/src/gallium/auxiliary/vl/vl_winsys_dri3.c @@ -56,6 +56,7 @@ struct vl_dri3_buffer struct xshmfence *shm_fence; bool busy; + bool is_external_texture; uint32_t width, height, pitch; }; @@ -71,6 +72,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; struct vl_dri3_buffer *back_buffers[BACK_BUFFER_NUM]; int cur_back; @@ -105,7 +109,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,13 +241,24 @@ 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) { + templ.width0 = (scrn->output_texture_width) ? + scrn->output_texture_width : + scrn->output_texture->width0; + templ.height0 = (scrn->output_texture_height) ? + scrn->output_texture_height : + scrn->output_texture->height0; + } else { + templ.width0 = scrn->width; + templ.height0 = scrn->height; + } templ.depth0 = 1; templ.array_size = 1; if (scrn->is_different_gpu) { - buffer->texture = scrn->base.pscreen->resource_create(scrn->base.pscreen, + buffer->texture = (scrn->output_texture) ? + scrn->output_texture : + scrn->base.pscreen->resource_create(scrn->base.pscreen, &templ); </blockquote> Why not just make it in the same line? </blockquote> I was just trying to limit characters to 78 as given in mesa coding style. </blockquote> </div></div> It's really matters if just over a few chars. which way do you think it will look better?<div><div class="gmail-h5"> </div></div></blockquote><div>I would prefer it in a single line, anyways I just checked that it used in single line in entire </div><div>vl code. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> if (!buffer->texture) goto unmap_shm; @@ -257,7 +273,9 @@ 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, + buffer->texture = (scrn->output_texture) ? + scrn->output_texture : + scrn->base.pscreen->resource_create(scrn->base.pscreen, &templ); </blockquote> Same as above . <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> if (!buffer->texture) goto unmap_shm; @@ -271,11 +289,20 @@ 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; </blockquote> Same as above. <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + scrn->output_texture = NULL; + scrn->output_texture_width = 0; + scrn->output_texture_height = 0; + 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, </blockquote> The width/height here should be same as buffer texture width/height. That's case before, now that is changed to templ.width0 = (scrn->output_texture_width) ? scrn->output_texture_width : scrn->output_texture->width0; templ.height0 = (scrn->output_texture_height) ? scrn->output_texture_height : scrn->output_texture->height0; </blockquote> I did this to support clipping for vdpau target display as we may have to display a portion of the buffer instead of the entire buffer In the patch you suggested you were using scrn width/height for the buffer width/height but if there is a screen resize event then we are sending a small buffer to X with extra width/height and would lead to error and hence I used buffer width/height </blockquote> </div></div> scrn size always updated when resize event happens, and back buffer size is updated accordingly. </blockquote><div>Exactly but when this happens the back buffer size is updated accordingly but the output texture will still</div><div>be of smaller size and won't get get updated until the next frame so it will also cause errors when we have a</div><div>pixmap of screen size. I tried setting it to screen size instead of buffer size and got the following errors</div><div><div> </div><div>X11 error: BadAlloc (insufficient resources for operation)</div><div>X11 error: BadDrawable (invalid Pixmap or Window parameter)</div><div>X11 error: BadPixmap (invalid Pixmap parameter)</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> The major problem causing resize corruption is now you request pixmaps to X dri3 ext. with wrong size. 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, </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="gmail-h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> thus causing the corruption appears when resizing. <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> scrn->depth, 32, buffer_fd); xcb_dri3_fence_from_fd(scrn->conn, @@ -287,8 +314,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 +335,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 +344,30 @@ 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 + */ </blockquote> Why do you change to size comparison to < from != ?, it will be a waste when from larger window to small one. </blockquote> Since I am setting buffer width/height to texture width/height which will be always greater than the screen size as vdpau allocates slightly larger buffers </blockquote> </div></div> Do you consider the VA-API case? </blockquote><div>I did not give much thought to it, need to see how VA-API allocates buffer. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (!buffer || buffer->width < scrn->width || + buffer->height < scrn->height) + allocate_new_buffer = true; + + /* If we were using a external texture buffer and + * the texture is not provided then we need a new + * buffer + */ + if (buffer && buffer->is_external_texture && + !scrn->output_texture) + allocate_new_buffer = true; </blockquote> Can you elaborate this case? I cannot think of any. the texture is output surface, it should be always provided, otherwise the where vdpau mixer render to. </blockquote> vl_winsys_dri3.c is also used by va, and since we did not change their code it will not set the output texture and would expect us to send a buffer on which it will copy the output. </blockquote> Yeah, we should definitely keep previous code path for VA-API. But this case is not fit to VA-API, because there is no "is_external_texture" for it. + if (buffer && buffer->is_external_texture && + !scrn->output_texture) + allocate_new_buffer = true; This always is false for VA-API, right?</blockquote><div>Yes I will remove it then. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> You allocate a back buffer, but that cannot be used as output surface by vdpau. right? </blockquote> Yes. But this case won't occur anyway </blockquote> Please remove that to avoid the confusion if that's not necessary. <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> but I just added it for safety as any state tracker would either set output texture or not for all frames. </blockquote> If that's the case, the handling here is still not correct, like being said, the back buffer won't be the target for output. Regards, Leo<div class="gmail-HOEnZb"><div class="gmail-h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Regards, Nayan <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Regards, Leo <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* 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 +380,13 @@ dri3_get_back_buffer(struct vl_dri3_screen *scrn) vl_compositor_reset_dirty_area(&scrn->dirty_areas[scrn->cur_back]); 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; + scrn->output_texture = NULL; } pipe_resource_reference(&texture, buffer->texture); @@ -627,6 +682,19 @@ vl_dri3_screen_get_private(struct vl_screen *vscreen) } static void +vl_dri3_screen_set_output_texture(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; @@ -744,6 +812,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_output_texture = vl_dri3_screen_set_output_texture; return &scrn->base; </blockquote></blockquote></blockquote> </div></div></blockquote></div> </div></div>