[Mesa-dev] enable pbuffer for drm platform

[Matt Turner]

On Fri, May 29, 2015 at 3:17 PM, Liu, Ying2 <ying2.liu at intel.com> wrote:
> When we tried to enable EGL on our project, we found that there was no
> pbuffer support for drm platform. I got some help from Alan and Kristian.
> Thank you so much! I studied platform_drm.c and compared it to other
> platforms’ code. Finally I was able to create this patch to enable pbuffer
> for drm platform. Our QA team has run full cycle testing and didn’t find any
> regression.  I wonder if anybody could review this patch to see if we could
> submit it to mesa.
>
>
>
> Thanks
>
>
>
> Ying
>
>

Thanks for the patch. I don't know much about EGL or pbuffers, but I
can offer some advice on patch submission.

Firstly, the patch cannot be applied directly, since it's been line
wrapped by your email client. The best way to send patches is with
`git send-email` which will do all the proper formatting to ensure it
can be easily applied.

Also, patches should have an appropriate title and commit summary. The
title is of the form "prefix: Imperative sentence", like "egl: Add
pbuffer support for drm platform." Perhaps a small description of why
this is useful would be nice in the commit summary as well.

> --- mesa-10.5.5.orig/src/egl/drivers/dri2/egl_dri2.c              2015-05-11
> 12:10:59.000000000 -0700
>
> +++ mesa-10.5.5/src/egl/drivers/dri2/egl_dri2.c   2015-05-26
> 04:06:16.353346946 -0700
>
> @@ -869,6 +869,10 @@ dri2_create_context(_EGLDriver *drv, _EG
>
>         */
>
>        if (conf->SurfaceType & EGL_WINDOW_BIT)
>
>           dri2_ctx->base.WindowRenderBuffer = EGL_BACK_BUFFER;
>
> +
>
> +      if (conf->SurfaceType & EGL_PBUFFER_BIT)
>
> +              dri2_ctx->base.WindowRenderBuffer = EGL_BACK_BUFFER;

The indentation on this line is different from the one in the
EGL_WINDOW_BIT case above. Keep it consistent with the surrounding
code.

>
> +
>
>     }
>
>     else
>
>        dri_config = NULL;
>
> diff -rupN mesa-10.5.5.orig/src/egl/drivers/dri2/platform_drm.c
> mesa-10.5.5/src/egl/drivers/dri2/platform_drm.c
>
> --- mesa-10.5.5.orig/src/egl/drivers/dri2/platform_drm.c    2015-05-11
> 12:10:59.000000000 -0700
>
> +++ mesa-10.5.5/src/egl/drivers/dri2/platform_drm.c         2015-05-26
> 04:04:25.293346505 -0700
>
> @@ -122,6 +122,8 @@ dri2_drm_create_surface(_EGLDriver *drv,
>
>        dri2_surf->base.Height = surf->base.height;
>
>        surf->dri_private = dri2_surf;
>
>        break;
>
> +   case EGL_PBUFFER_BIT:
>
> +      break;
>
>     default:
>
>        goto cleanup_surf;
>
>     }
>
> @@ -130,7 +132,7 @@ dri2_drm_create_surface(_EGLDriver *drv,
>
>        dri2_surf->dri_drawable =
>
>           (*dri2_dpy->dri2->createNewDrawable) (dri2_dpy->dri_screen,
>
>
> dri2_conf->dri_double_config,
>
> -                                               dri2_surf->gbm_surf);
>
> +                                               dri2_surf);

I'm not familiar with the code, but this change seems suspect... not
only because immediately below this is another call to
createNewDrawable that does not contain the same change.

>
>     } else {
>
>        assert(dri2_dpy->swrast != NULL);
>
> @@ -153,6 +155,15 @@ dri2_drm_create_surface(_EGLDriver *drv,
>
>     return NULL;
>
> }
>
> +static inline _EGLSurface *

Don't use inline unless you know better than the compiler. In this
case it's particularly unhelpful, since the function is used in a
vtable... so it cannot be inline.

>
> +dri2_drm_create_pbuffer_surface(_EGLDriver *drv, _EGLDisplay *disp,
>
> +                                     _EGLConfig *conf,
>
> +                                     const EGLint *attrib_list)

The style for line-wrapped argument lists is to align them with the opening (

See dri2_drm_create_window_surface for example.

>
> +{
>
> +   return dri2_drm_create_surface(drv, disp, EGL_PBUFFER_BIT, conf,
>
> +                                  NULL, attrib_list);

The same comment applies here as well.

>
> +}
>
> +
>
> static _EGLSurface *
>
> dri2_drm_create_window_surface(_EGLDriver *drv, _EGLDisplay *disp,
>
>                                 _EGLConfig *conf, void *native_window,
>
> @@ -575,7 +586,7 @@ static struct dri2_egl_display_vtbl dri2
>
>     .authenticate = dri2_drm_authenticate,
>
>     .create_window_surface = dri2_drm_create_window_surface,
>
>     .create_pixmap_surface = dri2_drm_create_pixmap_surface,
>
> -   .create_pbuffer_surface = dri2_fallback_create_pbuffer_surface,
>
> +   .create_pbuffer_surface = dri2_drm_create_pbuffer_surface,
>
>     .destroy_surface = dri2_drm_destroy_surface,
>
>     .create_image = dri2_drm_create_image_khr,
>
>     .swap_interval = dri2_fallback_swap_interval,
>
> @@ -596,6 +607,7 @@ dri2_initialize_drm(_EGLDriver *drv, _EG
>
>     struct gbm_device *gbm;
>
>     int fd = -1;
>
>     int i;
>
> +   EGLint surface_type;
>
>     loader_set_logger(_eglLog);
>
> @@ -691,8 +703,9 @@ dri2_initialize_drm(_EGLDriver *drv, _EG
>
>        attr_list[1] = format;
>
>        attr_list[2] = EGL_NONE;
>
> +      surface_type =  EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
>
>        dri2_add_config(disp, dri2_dpy->driver_configs[i],
>
> -                      i + 1, EGL_WINDOW_BIT, attr_list, NULL);
>
> +                      i + 1, surface_type, attr_list, NULL);

Just do EGL_WINDOW_BIT | EGL_PBUFFER_BIT directly without adding the
extra variable.