[Mesa-dev] [PATCH] EGL/android: Enhance pbuffer implementation
Tomasz Figa
tfiga at chromium.org
Thu Dec 1 07:09:29 UTC 2016
Hi,
On Fri, Nov 25, 2016 at 12:58 PM, Liu Zhiquan <zhiquan.liu at intel.com> wrote:
> Some dri drivers will pass multiple bits in buffer_mask parameter
> to droid_image_get_buffer(), more than the actual supported buffer
> type combination. For such case, will go through all the bits, and
> will not return error when unsupported buffer is requested, only
> return error when the allocation for supported buffer failed.
Please see my comments inline.
>
> Signed-off-by: Liu Zhiquan <zhiquan.liu at intel.com>
> Signed-off-by: Long, Zhifang <zhifang.long at intel.com>
> ---
> src/egl/drivers/dri2/platform_android.c | 209 +++++++++++++++++++-------------
> 1 file changed, 126 insertions(+), 83 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
> index 373e2c0..c70423d 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -392,13 +392,13 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
> }
>
> if (dri2_surf->dri_image_back) {
> - _eglLog(_EGL_DEBUG, "%s : %d : destroy dri_image_back", __func__, __LINE__);
> + _eglLog(_EGL_DEBUG, "destroy dri_image_back");
> dri2_dpy->image->destroyImage(dri2_surf->dri_image_back);
> dri2_surf->dri_image_back = NULL;
> }
>
> if (dri2_surf->dri_image_front) {
> - _eglLog(_EGL_DEBUG, "%s : %d : destroy dri_image_front", __func__, __LINE__);
> + _eglLog(_EGL_DEBUG, "destroy dri_image_front");
Patch description mentions only a change to handle multiple buffer
bits. Any other changes should be sent in separate patches.
> dri2_dpy->image->destroyImage(dri2_surf->dri_image_front);
> dri2_surf->dri_image_front = NULL;
> }
> @@ -434,81 +434,98 @@ update_buffers(struct dri2_egl_surface *dri2_surf)
> }
>
> static int
> -get_back_bo(struct dri2_egl_surface *dri2_surf)
> +get_front_bo(struct dri2_egl_surface *dri2_surf, unsigned int format)
> {
> struct dri2_egl_display *dri2_dpy =
> dri2_egl_display(dri2_surf->base.Resource.Display);
> - int fourcc, pitch;
> - int offset = 0, fd;
>
> - if (dri2_surf->dri_image_back)
> + if (dri2_surf->dri_image_front)
> + {
style: This file seems to follow the convention of opening brace at
the same line as the statement.
> + _eglLog(_EGL_WARNING, "dri2_image_front allocated !");
This is a normal case, there is no need to print anything here.
> return 0;
> -
> - if (!dri2_surf->buffer)
> - return -1;
> -
> - fd = get_native_buffer_fd(dri2_surf->buffer);
> - if (fd < 0) {
> - _eglLog(_EGL_WARNING, "Could not get native buffer FD");
> - return -1;
> }
>
> - fourcc = get_fourcc(dri2_surf->buffer->format);
> -
> - pitch = dri2_surf->buffer->stride *
> - get_format_bpp(dri2_surf->buffer->format);
> -
> - if (fourcc == -1 || pitch == 0) {
> - _eglLog(_EGL_WARNING, "Invalid buffer fourcc(%x) or pitch(%d)",
> - fourcc, pitch);
> - return -1;
> + if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
> + /* According current EGL spec, front buffer rendering
> + * for window surface is not supported now.
> + * and mesa doesn't have the implemetation of this case.
typo: s/implemetation/implementation/
> + * Add warnning message, but not treat it as error.
typo: s/warnning/warning/
> + */
> + _eglLog(_EGL_DEBUG, "front buffer for window surface is not supported now !");
nit: No need for exclamation mark. Also the message could be a bit
more informational, e.g.
"DRI driver requested unsupported front buffer for window surface"
> +
We can just return 0 here, no need to fall through to the end of the function.
> + } else if (dri2_surf->base.Type == EGL_PBUFFER_BIT) {
We won't be called with anything else than window or pbuffer bit here,
because we don't advertise pixmap support and createPixmapSurface is
stubbed out. For better coding style (less indentation) it's enough to
just return 0 in the if above and then move the code below out of the
conditional block completely.
> +
> + dri2_surf->dri_image_front =
> + dri2_dpy->image->createImage(dri2_dpy->dri_screen,
> + dri2_surf->base.Width,
> + dri2_surf->base.Height,
> + format,
> + 0,
> + dri2_surf);
> + if (!dri2_surf->dri_image_front)
> + {
Style: Brace should be on the same linen as if statement.
> + _eglLog(_EGL_WARNING, "dri2_image_front allocation failed !");
No need for exclamation mark.
> + return -1;
> + }
> + } else {
> + _eglLog(_EGL_WARNING, "pixmap is not supported now !");
> }
This else block is not needed, as I explained above.
>
> - dri2_surf->dri_image_back =
> - dri2_dpy->image->createImageFromFds(dri2_dpy->dri_screen,
> - dri2_surf->base.Width,
> - dri2_surf->base.Height,
> - fourcc,
> - &fd,
> - 1,
> - &pitch,
> - &offset,
> - dri2_surf);
> - if (!dri2_surf->dri_image_back)
> - return -1;
> -
> return 0;
> }
>
> static int
> -droid_image_get_buffers(__DRIdrawable *driDrawable,
> - unsigned int format,
> - uint32_t *stamp,
> - void *loaderPrivate,
> - uint32_t buffer_mask,
> - struct __DRIimageList *images)
> +get_back_bo(struct dri2_egl_surface *dri2_surf, unsigned int format)
> {
> - struct dri2_egl_surface *dri2_surf = loaderPrivate;
> struct dri2_egl_display *dri2_dpy =
> dri2_egl_display(dri2_surf->base.Resource.Display);
> + int fourcc, pitch;
> + int offset = 0, fd;
>
> - images->image_mask = 0;
> - images->front = NULL;
> - images->back = NULL;
> -
> - if (update_buffers(dri2_surf) < 0)
> + if (dri2_surf->dri_image_back) {
> + _eglLog(_EGL_WARNING, "dri_image_back allocated !");
Again, this is a valid case, no need to print anything.
> return 0;
> + }
>
> - if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
> - if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
> - /* According current EGL spec,
> - * front buffer rendering for window surface is not supported now */
> - _eglLog(_EGL_WARNING,
> - "%s:%d front buffer rendering for window surface is not supported!",
> - __func__, __LINE__);
> - return 0;
Again, to keep the code cleaner, let's just handle the invalid case of
pbuffer surface here and quickly return (0, to ignore the error).
> + if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
> + if (!dri2_surf->buffer) {
> + _eglLog(_EGL_WARNING, "Could not get native buffer");
> + return -1;
> + }
> +
> + fd = get_native_buffer_fd(dri2_surf->buffer);
> + if (fd < 0) {
> + _eglLog(_EGL_WARNING, "Could not get native buffer FD");
> + return -1;
> }
>
> + fourcc = get_fourcc(dri2_surf->buffer->format);
> +
> + pitch = dri2_surf->buffer->stride *
> + get_format_bpp(dri2_surf->buffer->format);
> +
> + if (fourcc == -1 || pitch == 0) {
> + _eglLog(_EGL_WARNING, "Invalid buffer fourcc(%x) or pitch(%d)",
> + fourcc, pitch);
> + return -1;
> + }
> +
> + dri2_surf->dri_image_back =
> + dri2_dpy->image->createImageFromFds(dri2_dpy->dri_screen,
> + dri2_surf->base.Width,
> + dri2_surf->base.Height,
> + fourcc,
> + &fd,
> + 1,
> + &pitch,
> + &offset,
> + dri2_surf);
> + if (!dri2_surf->dri_image_back)
> + {
> + _eglLog(_EGL_WARNING, "dri2_image allocation failed !");
> + return -1;
> + }
> + } else if (dri2_surf->base.Type == EGL_PBUFFER_BIT) {
> /* The EGL 1.5 spec states that pbuffers are single-buffered. Specifically,
> * the spec states that they have a back buffer but no front buffer, in
> * contrast to pixmaps, which have a front buffer but no back buffer.
> @@ -521,44 +538,70 @@ droid_image_get_buffers(__DRIdrawable *driDrawable,
> * Pbuffers in the X11 platform mostly work today, so let's just copy its
> * behavior instead of trying to fix (and hence potentially breaking) the
> * world.
> + *
> + * If the behavor changed in future, we should allocate back buffer image
> + * for pbuffer here, like :
> + *
> + * dri2_surf->dri_image_back =
> + * dri2_dpy->image->createImage(dri2_dpy->dri_screen,
> + * dri2_surf->base.Width,
> + * dri2_surf->base.Height,
> + * format,
> + * 0,
> + * dri2_surf);
I don't think it makes sense to add the code in the comment. There
might be some semantic changes in the future, which would make the
comment contain invalid code, which wouldn't be compile-testable.
In other words, it's just enough to describe the rationale behind
current behavior. No need to speculate about future.
> */
> - if (!dri2_surf->dri_image_front &&
> - dri2_surf->base.Type == EGL_PBUFFER_BIT) {
> - dri2_surf->dri_image_front =
> - dri2_dpy->image->createImage(dri2_dpy->dri_screen,
> - dri2_surf->base.Width,
> - dri2_surf->base.Height,
> - format,
> - 0,
> - dri2_surf);
> - }
> + _eglLog(_EGL_DEBUG, "back buffer for pbuffer surface is not supported now !");
"DRI driver requested unsupported back buffer for pbuffer surface"
> + } else {
> + _eglLog(_EGL_WARNING, "pixmap is not supported now !");
> + }
No need to handle pixmap.
> +
> + return 0;
> +}
>
> - if (!dri2_surf->dri_image_front) {
> - _eglLog(_EGL_WARNING,
> - "%s:%d dri2_image front buffer allocation failed!",
> - __func__, __LINE__);
> +/* Some drivers will pass multiple bits in buffer_mask.
> + * For such case, will go through all the bits, and
> + * will not return error when unsupported buffer is requested, only
> + * return error when the allocation for supported buffer failed.
> + */
> +static int
> +droid_image_get_buffers(__DRIdrawable *driDrawable,
> + unsigned int format,
> + uint32_t *stamp,
> + void *loaderPrivate,
> + uint32_t buffer_mask,
> + struct __DRIimageList *images)
> +{
> + struct dri2_egl_surface *dri2_surf = loaderPrivate;
> +
> + images->image_mask = 0;
> + images->front = NULL;
> + images->back = NULL;
> +
> + if (update_buffers(dri2_surf) < 0)
> + return 0;
> +
> + if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
> + if (get_front_bo(dri2_surf, format) < 0) {
> + _eglError(EGL_BAD_PARAMETER, "droid_get_front_bo");
Is there any reason why EGL_BAD_PARAMETER is correct here?
> return 0;
> }
>
> - images->front = dri2_surf->dri_image_front;
> - images->image_mask |= __DRI_IMAGE_BUFFER_FRONT;
> + if (dri2_surf->dri_image_front) {
> + images->front = dri2_surf->dri_image_front;
> + images->image_mask |= __DRI_IMAGE_BUFFER_FRONT;
> + }
> }
>
> if (buffer_mask & __DRI_IMAGE_BUFFER_BACK) {
> - if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
> - if (get_back_bo(dri2_surf) < 0)
> - return 0;
> - }
> -
> - if (!dri2_surf->dri_image_back) {
> - _eglLog(_EGL_WARNING,
> - "%s:%d dri2_image back buffer allocation failed!",
> - __func__, __LINE__);
> + if (get_back_bo(dri2_surf, format) < 0) {
> + _eglError(EGL_BAD_PARAMETER, "droid_get_back_bo");
Same as above.
Best regards,
Tomasz
More information about the mesa-dev
mailing list