[Mesa-dev] [PATCH 1/3] vl/dri3: use external texture as back buffers
Leo Liu
leo.liu at amd.com
Thu Oct 27 16:47:22 UTC 2016
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>
>>> Signed-off-by: Nayan Deshmukh <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?
>
>>> 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.
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?
>
>>> + 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?
>
>> 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;
More information about the mesa-dev
mailing list