[Mesa-dev] [PATCH 1/3] vl/dri3: use external texture as back buffers(v2)
Leo Liu
leo.liu at amd.com
Fri Nov 4 17:52:30 UTC 2016
On 11/04/2016 01:24 PM, Nayan Deshmukh wrote:
> 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.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 | 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
I think the clipping size is nothing to do with the output texture size.
Is it possible just combine it with below and make it simple.
+ uint32_t clip_width, clip_height;
>>> 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?
I think looks better if move the ":' up, or maybe 3 lines.
Please fix the same way for the similar long lines below.
Regards,
Leo
>>> + 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;
More information about the mesa-dev
mailing list