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

Submitted by Daniel Stone on March 23, 2018, 1:50 p.m.

Details

Message ID 20180323135025.16162-4-daniels@collabora.com
State New
Headers show
Series "Use DRM GetFB2 ioctl" ( rev: 1 ) in X.org

Not browsing as part of any series.

Commit Message

Daniel Stone March 23, 2018, 1:50 p.m.
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.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 hw/xfree86/drivers/modesetting/drmmode_display.c | 65 ++++++++++++++++++------
 1 file changed, 50 insertions(+), 15 deletions(-)

Patch hide | download patch | download mbox

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) {
+        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;
+            break;
+        case DRM_FORMAT_XRGB8888:
+            depth = 24;
+            bpp = 32;
+            break;
+        case DRM_FORMAT_XRGB2101010:
+            depth = 30;
+            bpp = 32;
+        default:
+            break;
+        }
+        drmModeFreeFB2(fbcon2);
+    }
+    else {
+        fbcon = drmModeGetFB(drmmode->fd, fbcon_id);
+        if (fbcon == NULL)
+            return NULL;
+
+        width = fbcon->width;
+        height = fbcon->height;
+        handles[0] = fbcon->handle;
+        strides[0] = fbcon->pitch;
+        modifier = DRM_FORMAT_MOD_INVALID;
+        depth = fbcon->depth;
+        bpp = fbcon->bpp;
 
-    if (fbcon->depth != pScrn->depth ||
-        fbcon->width != pScrn->virtualX ||
-        fbcon->height != pScrn->virtualY)
-        goto out_free_fb;
+        drmModeFreeFB(fbcon);
+    }
+
+    if (depth != pScrn->depth ||
+        width != pScrn->virtualX || height != pScrn->virtualY)
+        goto out;
 
     /* GBM doesn't have an import path from handles, so we make a
      * dma-buf fd from it and then go through that.
      */
-    handles[0] = fbcon->handle;
     num_fds = handles_to_fds(drmmode, handles, fds);
     if (num_fds == 0) {
         xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
                    "Failed to make prime FD for handle: %d\n", errno);
-        goto out_free_fb;
+        goto out;
     }
-    strides[0] = fbcon->pitch;
 
     pixmap = glamor_pixmap_from_fds(pScreen, num_fds, fds,
-                                    fbcon->width, fbcon->height,
-                                    strides, offsets, fbcon->depth,
-                                    fbcon->bpp, DRM_FORMAT_MOD_INVALID);
+                                    width, height, strides, offsets,
+                                    depth, bpp, modifier);
     if (!pixmap)
         goto out_close_fds;
 
@@ -1124,8 +1160,7 @@  out_close_fds:
         if (fds[i] != -1)
             close(fds[i]);
     }
-out_free_fb:
-    drmModeFreeFB(fbcon);
+out:
     return pixmap;
 }
 #endif

Comments

On 23 March 2018 at 13:50, Daniel Stone <daniels@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@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@collabora.com>

-Emil
Hi Emil,

On 26 March 2018 at 16:22, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 23 March 2018 at 13:50, Daniel Stone <daniels@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.

Thanks for looking at this! I think the flags question is a good one,
and that we should probably make the field RW: userspace declaring
what it supports (analagous to AddFB2), and the kernel overwriting
that with the intersection of what userspace and the kernel support
(not analogous, but since it's a getter rather than an add ...).

>> @@ -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.

Sure; the only reason I didn't add an ifdef check is because there's
no version released with it yet.

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

If you have a good suggestion for implementing weak symbols then I'm
all ears, but last I saw from the Mesa experience no-one could figure
out how to do it properly.

>> +        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?

Correct.

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

I can only see that mapping inside drmmode_create_bo, which is
different as it creates everything with an alpha channel, i.e.
s/XRGB/ARGB/. Are there some others I'm missing?

>> +            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?

Sure.

Cheers,
Daniel