[Mesa-dev] [PATCH 1/3] vl/dri3: use external texture as back buffers
Nayan Deshmukh
nayan26deshmukh at gmail.com
Thu Oct 27 18:15:49 UTC 2016
On Thu, Oct 27, 2016 at 11:07 PM, Leo Liu <leo.liu at amd.com> wrote:
>
>
> 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> 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>leo.liu at amd.com>
>>>>> Signed-off-by: Nayan Deshmukh < <nayan26deshmukh at gmail.com>
>>>>> 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)
>
>
> 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.
>
> Oh now I get it.
This will work fine but then we have to somehow support cliping in vdpau,
any thoughts Christian?
Regards,
Nayan.
> 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_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/0656e2bd/attachment-0001.html>
More information about the mesa-dev
mailing list