<div dir="ltr"> <div class="gmail_quote"><div dir="ltr">On Wed, Aug 29, 2018 at 4:38 PM Gurchetan Singh <<a href="mailto:gurchetansingh@chromium.org">gurchetansingh@chromium.org</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Good idea, we do need a way to handle YUV buffers. IIRC zachr@+ also ran into the limitations of DRM_IOCTL_VIRTGPU_RESOURCE_CREATE. Maybe a DRM_IOCTL_VIRTGPU_RESOURCE_CREATE2 that handles: - YUV formats - format modifiers - stride is warranted. </blockquote><div>I am not sure about "format modifiers" since actually I don't know what's that. But I think it's possible to</div><div>handle multi planar format and stride in current DRM_IOCTL_VIRTGPU_RESOURCE_CREATE ioctl.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Wed, Aug 29, 2018 at 1:40 PM Lepton Wu <<a href="mailto:lepton@chromium.org" target="_blank">lepton@chromium.org</a>> wrote: > > On Wed, Aug 29, 2018 at 1:23 PM Lepton Wu <<a href="mailto:lepton@chromium.org" target="_blank">lepton@chromium.org</a>> wrote: > > > > On Wed, Aug 29, 2018 at 1:15 PM Lepton Wu <<a href="mailto:lepton@chromium.org" target="_blank">lepton@chromium.org</a>> wrote: > > > > > > With this and other necessary patches to kernel, mesa etc, video player > > > under android can play videos with YV12 format under virgl + qemu. > > Patches for kerne/mesa/minigbm can be found here for your reference. > > > > kernel: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1194613" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1194613</a> > > mesa: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/mesa/+/1195011" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/third_party/mesa/+/1195011</a> > > minigbm: <a href="https://chromium-review.googlesource.com/c/chromiumos/platform/minigbm/+/1195587" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/platform/minigbm/+/1195587</a> > BTW, currently drm_virtgpu_resource_create doesn't really use stride > info even it has a stride field. > This information is needed since android has some alignment > requirement for yv12 format. > I think this could be addressed by a separate CL to kernel. > > > > My plan is to get virglrenderer change done first since I could get > > comments/suggestion to change interface or > > even implementation detail. After virglrender side change done here , > > then I will send patch to upstream kernel, > > then mesa, minigbm. Also perhaps libdrm since I could get suggestion > > for adding new ioctl. Now I am trying to > > reuse some old ioctl. > > > > > The basic idea is: > > > DRM_IOCTL_VIRTGPU_RESOURCE_CREATE can be used to create additional planes > > > at some offset of an existed bo. Every plane is backed by a host side texture. > > > When guest try to get res_handle for an existed bo, it will get the actual > > > res_handle for that plane by checking offset. > > > --- > > > src/virgl_hw.h | 1 + > > > src/virglrenderer.h | 12 +++++++-- > > > src/vrend_renderer.c | 64 ++++++++++++++++++++++++++++++++++++++++++++ > > > src/vrend_renderer.h | 26 ++++++++++++++++-- > > > 4 files changed, 99 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/virgl_hw.h b/src/virgl_hw.h > > > index 4add191..e8df498 100644 > > > --- a/src/virgl_hw.h > > > +++ b/src/virgl_hw.h > > > @@ -342,4 +342,5 @@ enum virgl_ctx_errors { > > > > > > > > > #define VIRGL_RESOURCE_Y_0_TOP (1 << 0) > > > +#define VIRGL_RESOURCE_PLANE (1 << 1) > > > #endif > > > diff --git a/src/virglrenderer.h b/src/virglrenderer.h > > > index 5baecdd..c41523d 100644 > > > --- a/src/virglrenderer.h > > > +++ b/src/virglrenderer.h > > > @@ -93,8 +93,16 @@ VIRGL_EXPORT int virgl_renderer_get_fd_for_texture2(uint32_t tex_id, int *fd, in > > > > > > struct virgl_renderer_resource_create_args { > > > uint32_t handle; > > > - uint32_t target; > > > - uint32_t format; > > > + union { > > > + struct { > > > + uint32_t target; > > > + uint32_t format; > > > + }; > > > + struct { > > > + uint32_t main_res; > > > + uint32_t offset; > > > + } plane; > > > + }; > > > uint32_t bind; > > > uint32_t width; > > > uint32_t height; > > > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c > > > index 4086abd..b38b37e 100644 > > > --- a/src/vrend_renderer.c > > > +++ b/src/vrend_renderer.c > > > @@ -5144,10 +5144,50 @@ static int vrend_renderer_resource_allocate_texture(struct vrend_resource *gr, > > > return 0; > > > } > > > > > > +static int check_plane_valid(struct vrend_renderer_resource_create_args *args, > > > + struct vrend_renderer_resource_create_args > > > + *plane_args, struct vrend_plane_resource** pr) { > > > + struct vrend_resource *res; > > > + res = vrend_resource_lookup(args->plane.main_res, 0); > > > + if (!res) > > > + return -1; > > > + *plane_args = *args; > > > + /* Assume same target/format for sub planes */ > > > + plane_args->target = res->base.target; > > > + plane_args->format = res->base.format; > > > + plane_args->flags &= ~VIRGL_RESOURCE_PLANE; > > > + for (int i = 0; i < VR_MAX_SUB_PLANES; ++i) { > > > + // Only support to add planes in order to make life simple. > > > + if (args->plane.offset <= res->planes[i].offset) { > > > + fprintf(stderr, "Add planes in wrong order\n"); > > > + return -1; > > > + } > > > + if (res->planes[i].offset == 0) { > > > + *pr = &res->planes[i]; > > > + return 0; > > > + } > > > + } > > > + return -1; > > > +} > > > + > > > int vrend_renderer_resource_create(struct vrend_renderer_resource_create_args *args, struct iovec *iov, uint32_t num_iovs, void *image_oes) > > > { > > > struct vrend_resource *gr; > > > int ret; > > > + struct vrend_renderer_resource_create_args plane_args; > > > + struct vrend_plane_resource *pr; > > > + > > > + if (args->flags & VIRGL_RESOURCE_PLANE) { > > > + ret = check_plane_valid(args, &plane_args, &pr); > > > + if (ret) > > > + return EINVAL; > > > + ret = vrend_renderer_resource_create(&plane_args, iov, num_iovs, image_oes); > > > + if (ret == 0) { > > > + pr->handle = args->handle; > > > + pr->offset = args->plane.offset; > > > + } > > > + return ret; > > > + } > > > > > > ret = check_resource_valid(args); > > > if (ret) > > > @@ -6067,6 +6107,22 @@ int vrend_renderer_transfer_iov(const struct vrend_transfer_info *info, > > > return EINVAL; > > > } > > > > > > + /* If this is a request for sub planes, read/write sub planes. > > > + */ > > > + struct vrend_plane_resource* plane = NULL; > > > + for (int i = 0; i < VR_MAX_SUB_PLANES && res->planes[i].offset; i++) { > > > + if (info->offset >= res->planes[i].offset) > > > + plane = &res->planes[i]; > > > + } > > > + if (plane) { > > > + struct vrend_transfer_info tmp; > > > + tmp = *info; > > > + tmp.handle = plane->handle; > > > + tmp.iovec = iov; > > > + tmp.iovec_cnt = num_iovs; > > > + return vrend_renderer_transfer_iov(&tmp, transfer_mode); > > > + } > > > + > > > if (!check_transfer_bounds(res, info)) > > > return EINVAL; > > > > > > @@ -7886,6 +7942,10 @@ void vrend_renderer_attach_res_ctx(int ctx_id, int resource_id) > > > return; > > > > > > vrend_object_insert_nofree(ctx->res_hash, res, sizeof(*res), resource_id, 1, false); > > > + > > > + for (int i = 0; i < VR_MAX_SUB_PLANES && res->planes[i].offset; ++i) { > > > + vrend_renderer_attach_res_ctx(ctx_id, res->planes[i].handle); > > > + } > > > } > > > > > > static void vrend_renderer_detach_res_ctx_p(struct vrend_context *ctx, int res_handle) > > > @@ -7895,6 +7955,10 @@ static void vrend_renderer_detach_res_ctx_p(struct vrend_context *ctx, int res_h > > > if (!res) > > > return; > > > > > > + for (int i = 0; i < VR_MAX_SUB_PLANES && res->planes[i].offset; ++i) { > > > + vrend_renderer_detach_res_ctx_p(ctx, res->planes[i].handle); > > > + } > > > + > > > vrend_object_remove(ctx->res_hash, res_handle, 1); > > > } > > > > > > diff --git a/src/vrend_renderer.h b/src/vrend_renderer.h > > > index bb76b44..21dceb6 100644 > > > --- a/src/vrend_renderer.h > > > +++ b/src/vrend_renderer.h > > > @@ -49,6 +49,19 @@ struct vrend_context; > > > */ > > > #define VR_MAX_TEXTURE_2D_LEVELS 15 > > > > > > +/* For multi-planar formats like YV12, we just use vrend_resource for the first > > > + * plane as "main" vrend_resource and embed information for additional planes > > > + * as "sub" planes in "main" vrend_resource. So for YV12, we only need 2 "sub" > > > + * planes. > > > + */ > > > +#define VR_MAX_SUB_PLANES 2 > > > + > > > +struct vrend_plane_resource { > > > + GLuint handle; > > > + uint32_t offset; > > > + uint32_t size; > > > +}; > > > + > > > struct vrend_resource { > > > struct pipe_resource base; > > > GLuint id; > > > @@ -68,6 +81,7 @@ struct vrend_resource { > > > struct iovec *iov; > > > uint32_t num_iovs; > > > uint64_t mipmap_offsets[VR_MAX_TEXTURE_2D_LEVELS]; > > > + struct vrend_plane_resource planes[VR_MAX_SUB_PLANES]; > > > }; > > > > > > /* assume every format is sampler friendly */ > > > @@ -160,8 +174,16 @@ void vrend_renderer_context_destroy(uint32_t handle); > > > > > > struct vrend_renderer_resource_create_args { > > > uint32_t handle; > > > - enum pipe_texture_target target; > > > - uint32_t format; > > > + union { > > > + struct { > > > + enum pipe_texture_target target; > > > + uint32_t format; > > > + }; > > > + struct { > > > + uint32_t main_res; > > > + uint32_t offset; > > > + } plane; > > > + }; > > > uint32_t bind; > > > uint32_t width; > > > uint32_t height; > > > -- > > > 2.19.0.rc0.228.g281dcd1b4d0-goog > > > > _______________________________________________ > virglrenderer-devel mailing list > <a href="mailto:virglrenderer-devel@lists.freedesktop.org" target="_blank">virglrenderer-devel@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel</a> </blockquote></div></div>