On 10/27/2016 01:20 PM, Nayan Deshmukh wrote:
On Thu, Oct 27, 2016 at 10:17 PM, Leo Liu <leo....@amd.com
<mailto:leo....@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....@amd.com
<mailto:leo....@amd.com>>
Signed-off-by: Nayan Deshmukh
<nayan26deshm...@gmail.com
<mailto:nayan26deshm...@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;
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
