[Mesa-dev] [PATCH 1/3] vl/dri3: use external texture as back buffers
Nayan Deshmukh
nayan26deshmukh at gmail.com
Thu Oct 27 17:20:30 UTC 2016
On Thu, Oct 27, 2016 at 10:17 PM, Leo Liu <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>
>>>> 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_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.
>
>> 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;
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161027/3e644ba8/attachment-0001.html>
More information about the mesa-dev
mailing list