[Piglit] [RFC 2/3] framework: gbm: support for creating external buffers

[Pohjolainen, Topi]

On Fri, Mar 01, 2013 at 11:10:43AM -0800, Chad Versace wrote:
> On 02/26/2013 05:15 AM, Topi Pohjolainen wrote:
> > Getting direct access to the gbm device through the waffle display
> > and creating the buffer is rather straightforward thing to do.
> > Writing to the buffer using CPU, however, requires priviledges for
> > the underlying gem-ioctl.
> > 
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > ---
> >  .../piglit-framework-gl/piglit_gbm_framework.c     |   78 ++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> 
> It's clear here that you're trying to test a difficult thing and there is no
> easy way to do it. I have several questions for you for which I don't know the
> answer.
> 
> 
> > diff --git a/tests/util/piglit-framework-gl/piglit_gbm_framework.c b/tests/util/piglit-framework-gl/piglit_gbm_framework.c
> > index 4df3861..00e1425 100644
> > --- a/tests/util/piglit-framework-gl/piglit_gbm_framework.c
> > +++ b/tests/util/piglit-framework-gl/piglit_gbm_framework.c
> > @@ -24,10 +24,18 @@
> >  #include <assert.h>
> >  #include <stdlib.h>
> >  #include <unistd.h>
> > +#include <waffle.h>
> > +#include <waffle_gbm.h>
> > +#include <gbm.h>
> > +#include <libdrm/drm.h>
> > +#include <libdrm/i915_drm.h>
> > +#include <sys/ioctl.h>
> >  
> >  #include "piglit-util-gl-common.h"
> >  #include "piglit_gbm_framework.h"
> >  
> > +#define ALIGN(value, alignment) (((value) + alignment - 1) & ~(alignment - 1))
> > +
> >  static void
> >  enter_event_loop(struct piglit_winsys_framework *winsys_fw)
> >  {
> > @@ -51,6 +59,73 @@ destroy(struct piglit_gl_framework *gl_fw)
> >  	free(winsys_fw);
> >  }
> >  
> > +static void *
> > +map(struct gbm_device *gbm, struct gbm_bo *bo, unsigned n_bytes)
> > +{
> > +        int res;
> > +        struct drm_i915_gem_mmap mmap_arg;
> > +
> > +        mmap_arg.handle = gbm_bo_get_handle(bo).u32;
> > +        mmap_arg.offset = 0;
> > +        mmap_arg.size = n_bytes;
> > +
> > +        res = ioctl(gbm_device_get_fd(gbm), DRM_IOCTL_I915_GEM_MMAP, &mmap_arg);
> > +
> > +        if (res)
> > +                return 0;
> > +
> > +        return (void *)(uintptr_t)mmap_arg.addr_ptr;
> > +}
> 
> This function is specific to i915, but gbm is specific only to drm. Is it possible
> to rewrite this function using only drm ioctls? If it's not possbile, or if it that's a bad idea,
> we should somehow detect here whether gbm is using i915 and skip the test if it's not.

Well, I'm still hoping that it would be possible, but I'm getting more and more
skeptical. I'm only aware of the MMAP and PWRITE which are both intel specific.

I'll take a look if the detection is possible.

> 
> 
> > +
> > +/* At the time of writing this, GBM did not have any alignment
> > + * restrictions on height or width.
> > + */
> > +static void *
> > +create_ext_420_buffer(struct piglit_gl_framework *gl_fw,
> > +		      unsigned w, unsigned h, bool swap_vu,
> > +		      const void *y, const void *u, const void *v)
> > +{
> > +	struct waffle_gbm_display *native = waffle_display_get_native(
> > +		piglit_wfl_framework(gl_fw)->display)->gbm;
> > +	struct gbm_bo *bo = gbm_bo_create(native->gbm_device, w, h,
> > +				swap_vu ? GBM_FORMAT_YVU420 : GBM_FORMAT_YUV420,
> > +				GBM_BO_USE_RENDERING);
> > +	unsigned char *p;
> > +	unsigned y_off = 0;
> > +	unsigned u_off = ALIGN(w * h, 4096);
> > +	unsigned v_off = u_off + ALIGN((w / 2) * (h / 2), 4096);
> > +	unsigned total = v_off + ALIGN((w / 2) * (h / 2), 4096);
> 
> Why the 4096? Are you assuming that the buffer layout is aligned to 4096
> because that is the size of a tile on Intel gpus?
> 
> I expected to see gbm_bo_get_stride() used in these calculations. Using it
> may allow you to remove the hardcoded 4096.
> 

The problem here is that the buffer has two different strides (y has stride
based on full width and u/v-planes based on half), and get_stride() could return
only one.
I've chosen to use the page aligning after I saw Kristian's implementation
allowing each plane to be treated as a separate image itself - check the
YUV-allocation support I wrote for gbm-dri in mesa.

An alternative allowing one to get rid of this separate stride problem would be
to take advantage of Tom's dma_buf extension for EGL images and allocate Y and
U/V planes as separate buffers and allow the EGL extension to combine them
instead of doing it in GBM level.

One still has the former buffer-write-and-intel-specific-ioctl problem to solve,
but these are separate problems anyway.

Topi