[RFC xserver 05/16] DRI3/Glamor: Implement PixmapFromBuffers request

[Emil Velikov]

Hi Dan, Louis,

Seems like an earlier patch (03/16) introduces a broken behaviour only
to be fixed here...

On 8 June 2017 at 19:43, Daniel Stone <daniels at collabora.com> wrote:

> @@ -159,11 +196,13 @@ proc_dri3_pixmap_from_buffer(ClientPtr client)
>      if (fd < 0)
>          return BadValue;
>
> -    rc = dri3_pixmap_from_fd(&pixmap,
> -                             drawable->pScreen, fd,
> -                             stuff->width, stuff->height,
> -                             stuff->stride, stuff->depth,
> -                             stuff->bpp);
> +    offset = 0;
> +    stride = stuff->stride;
> +    format = drm_format_for_depth(stuff->bpp, stuff->depth);
> +    rc = dri3_pixmap_from_fds(&pixmap,
> +                              drawable->pScreen, 1, &fd,
> +                              stuff->width, stuff->height,
> +                              &stride, &offset, format, 0);

... this type of hunks is what I have in mind.

As the diff suggests the patch is trying to do way too much at the
same time, IMHO.
- A: Introduce the interface (dri3/dri3.h)
- B: add the implementation
- C: fixes the user (introduced just a patch ago) to use the correct interface
- D: adds the backend implementations

I think it's great to split it roughly as
 - patch X: (A+B)
 - patch X+1: C (squashed with 03/16)
 - patch X+n: D - add backend implementations, all or one at a time

It will be easier to review while no intermittent break will be introduced ;-)

Additionally, the ifndef(EGL_extension...) should be dropped with
fall-back definitions provided as applicable. From a quick look I
cannot find where you're checking the extensions prior to using them.
Search for GLAMOR_CHECK_EGL_EXTENSION and "The EGL layer needs to have
the following" details.

HTH
Emil