[Mesa-dev] enable pbuffer for drm platform

[Liu, Ying2]

Matt,

Thank you so much for reviewing my patch. This is my first time sending out patch to mesa community. Your comments are very helpful. I am in a training right now. I will update my patch according to the comments after that.

Thanks

Ying

-----Original Message-----
From: Matt Turner [mailto:mattst88 at gmail.com] 
Sent: Friday, May 29, 2015 4:13 PM
To: Liu, Ying2
Cc: mesa-dev at lists.freedesktop.org
Subject: Re: [Mesa-dev] enable pbuffer for drm platform

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.