[PATCH xserver 3/3] modesetting/drmmode: Use drmModeGetFB2

[Emil Velikov]

On 23 March 2018 at 13:50, Daniel Stone <daniels at collabora.com> wrote:
> Much like AddFB -> AddFB2, GetFB2 lets us get multiple buffers back as
> well as modifier information. This lets us use -background none with
> multiplanar formats, or modifiers which can't be inferred via a magic
> side channel.
>
AFAICT there's nothing special (or wrong) with the new API.

The flags field is a bit of an open question - should Xserver or
libdrm manage the value passed to the kernel?
Analogously - should we pass the flags back to the user (drmModeFB2::flags)?

Current design seems perfectly fine IMHO, although others might disagree.

> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 65 ++++++++++++++++++------
>  1 file changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 39ed16f98..82ea49386 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -1080,9 +1080,13 @@ create_pixmap_for_fbcon(drmmode_ptr drmmode, ScrnInfoPtr pScrn, int fbcon_id)
>  {
>      PixmapPtr pixmap = drmmode->fbcon_pixmap;
>      drmModeFBPtr fbcon;
> +    drmModeFB2Ptr fbcon2;
>      ScreenPtr pScreen = xf86ScrnToScreen(pScrn);
>      uint32_t handles[4] = { 0, };
>      CARD32 strides[4] = { 0, }, offsets[4] = { 0, };
> +    uint64_t modifier;
> +    int width, height;
> +    int depth = 0, bpp = 0;
>      int fds[4] = { -1, -1, -1, -1 };
>      int num_fds;
>      int i;
> @@ -1090,31 +1094,63 @@ create_pixmap_for_fbcon(drmmode_ptr drmmode, ScrnInfoPtr pScrn, int fbcon_id)
>      if (pixmap)
>          return pixmap;
>
> -    fbcon = drmModeGetFB(drmmode->fd, fbcon_id);
> -    if (fbcon == NULL)
> -        return NULL;
> +    fbcon2 = drmModeGetFB2(drmmode->fd, fbcon_id);
> +    if (fbcon2) {
Declare and initialize fbcon2 in one go - C99 feature that everyone
has (even the people who use MSVC to build Xserver & friends).
Then wrap the whole fbcon2 hunk in a #ifdef HAVE_GETFB2 + add a
configure/meson check.

Alternatively add a weak drmModeGetFB2 function which returns NULL and
forget all the above ;-)


> +        width = fbcon2->width;
> +        height = fbcon2->height;
> +        memcpy(handles, fbcon2->handles, sizeof(handles));
> +        memcpy(strides, fbcon2->pitches, sizeof(strides));
> +        memcpy(offsets, fbcon2->offsets, sizeof(offsets));
> +        modifier = fbcon2->modifier;
> +        switch (fbcon2->pixel_format) {
> +        case DRM_FORMAT_RGB565:
> +            depth = 16;
Missing bpp?

Idea: Instead of introducing another format <> {depth, bpp} mapping,
might as well add some helpers?

> +            break;
> +        case DRM_FORMAT_XRGB8888:
> +            depth = 24;
> +            bpp = 32;
> +            break;
> +        case DRM_FORMAT_XRGB2101010:
> +            depth = 30;
> +            bpp = 32;
> +        default:
> +            break;
Error instead of silently continuing?
Having a fallback to GetFB1 is possible, but IMHO not something we want.

Regardless of the helper suggestion, but with the rest addressed
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil