[Mesa-dev] [PATCH 1/3] vl/dri3: use external texture as back buffers
Leo Liu
leo.liu at amd.com
Thu Oct 27 17:37:47 UTC 2016
On 10/27/2016 01:20 PM, Nayan Deshmukh wrote:
>
>
> On Thu, Oct 27, 2016 at 10:17 PM, Leo Liu <leo.liu at amd.com
> <mailto:leo.liu at 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.liu at amd.com
> <mailto:leo.liu at amd.com>>
> Signed-off-by: Nayan Deshmukh
> <nayan26deshmukh at gmail.com
> <mailto:nayan26deshmukh at 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_create(scrn->base.pscreen,
> + buffer->texture = (scrn->output_texture) ?
> + scrn->output_texture :
> + scrn->base.pscreen->resource_create(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.
>
>
> 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);
>
> 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)
You got misunderstood it.
You problem is here:
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;
should be like:
templ.width0 = scrn->output_texture->width0;
templ.height0 = scrn->output_texture->height0;
requested pixmap needs size according to that back buffer, not clips
size, not scrn size.
It was scrn size before, because the scrn size is same as back buffer size.
Regards,
Leo
>
> 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_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;
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161027/21039d0e/attachment-0001.html>
More information about the mesa-dev
mailing list