On Thu, Oct 27, 2016 at 10:50 PM, Nayan Deshmukh <nayan26deshm...@gmail.com> wrote:
> > > On Thu, Oct 27, 2016 at 10:17 PM, Leo Liu <leo....@amd.com> wrote: > >> >> >> On 10/27/2016 12:20 PM, Nayan Deshmukh wrote: >> >>> On Thu, Oct 27, 2016 at 10:38:30AM -0400, Leo Liu wrote: >>> >>>> >>>> On 10/24/2016 09:55 AM, Nayan Deshmukh wrote: >>>> >>>>> 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 | 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); >>>>> + >>>>> >>>> I think it's more appropriate if it's called >>>> "set_back_texture_from_output" >>>> >>> Yeah that makes more sense. >>> >>>> 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_c >>>>> reate(scrn->base.pscreen, >>>>> + buffer->texture = (scrn->output_texture) ? >>>>> + scrn->output_texture : >>>>> + scrn->base.pscreen->resource_c >>>>> reate(scrn->base.pscreen, >>>>> &templ); >>>>> >>>> Why not just make it in the same line? >>>> >>>> I was just trying to limit characters to 78 as given in mesa coding >>> style. >>> >> >> It's really matters if just over a few chars. which way do you think it >> will look better? >> >> >> I would prefer it in a single line, anyways I just checked that it used > in single line in entire > vl code. > Damn I miss out some words while I am typing, I meant that this kind of statements are written in single line instead of wrapping in the entire vl code, so It makes sense to have it in that fashion. > > >> >>> 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_c >>>>> reate(scrn->base.pscreen, >>>>> + buffer->texture = (scrn->output_texture) ? >>>>> + scrn->output_texture : >>>>> + scrn->base.pscreen->resource_c >>>>> reate(scrn->base.pscreen, >>>>> &templ); >>>>> >>>> Same as above . >>>> >>>> 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; >>>>> >>>> Same as above. >>>> >>>> + 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, >>>>> >>>> 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; >>>> >>>> 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 >>> >> >> scrn size always updated when resize event happens, and back buffer size >> is updated accordingly. >> >> Exactly but when this happens the back buffer size is updated accordingly > but the output texture will still > be of smaller size and won't get get updated until the next frame so it > will also cause errors when we have a > pixmap of screen size. I tried setting it to screen size instead of buffer > size and got the following errors > > X11 error: BadAlloc (insufficient resources for operation) > X11 error: BadDrawable (invalid Pixmap or Window parameter) > X11 error: BadPixmap (invalid Pixmap parameter) > > 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, >> >> >> thus causing the corruption appears when resizing. >>>> >>>> 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 >>>>> + */ >>>>> >>>> Why do you change to size comparison to < from != ?, it will be a waste >>>> when >>>> from larger window to small one. >>>> >>> 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 >>> >> >> Do you consider the VA-API case? >> >> I did not give much thought to it, need to see how VA-API allocates > buffer. > >> >>> >>>> + 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; >>>>> >>>> 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. >>>> >>> 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. >>> >> >> 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? > > Yes I will remove it then. > >> >> >> >>> You allocate a back buffer, but that cannot be used as output surface by >>>> vdpau. right? >>>> >>> Yes. But this case won't occur anyway >>> >> >> Please remove that to avoid the confusion if that's not necessary. >> >> >> but I just added it for safety as any >>> state tracker would either set output texture or not for all frames. >>> >> >> 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 >> >> >> >>> Regards, >>> Nayan >>> >>>> >>>> Regards, >>>> Leo >>>> >>>> >>>> + >>>>> + /* 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_timest >>>>> amp; >>>>> 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; >>>>> >>>> >> >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev