[xserver,2/3] modesetting/drmmode: Use glamor_pixmap_from_fds

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

Details

Message ID 20180323135025.16162-3-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.
glamor_pixmap_from_fds saves us the trouble of creating the pixmap and
dealing with its header ourselves; instead we pass the whole thing off
to Glamor.

This requires us to provide FDs rather than handles for the import, but
Glamor was already doing that internally, so no functional change.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 hw/xfree86/drivers/modesetting/drmmode_display.c | 52 +++++++++++++++++++-----
 1 file changed, 42 insertions(+), 10 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 4b22abd0f..39ed16f98 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -30,6 +30,7 @@ 
 #endif
 
 #include <errno.h>
+#include <fcntl.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 #include <unistd.h>
@@ -1058,6 +1059,21 @@  drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
     drmmode_crtc->dpms_mode = mode;
 }
 
+static int
+handles_to_fds(drmmode_ptr drmmode, uint32_t *handles, int *fds)
+{
+    int err;
+    int i;
+
+    for (i = 0; i < 4 && handles[i]; i++) {
+        err = drmPrimeHandleToFD(drmmode->fd, handles[i], O_CLOEXEC, &fds[i]);
+        if (err != 0)
+            return 0;
+    }
+
+    return i;
+}
+
 #ifdef GLAMOR_HAS_GBM
 static PixmapPtr
 create_pixmap_for_fbcon(drmmode_ptr drmmode, ScrnInfoPtr pScrn, int fbcon_id)
@@ -1065,7 +1081,11 @@  create_pixmap_for_fbcon(drmmode_ptr drmmode, ScrnInfoPtr pScrn, int fbcon_id)
     PixmapPtr pixmap = drmmode->fbcon_pixmap;
     drmModeFBPtr fbcon;
     ScreenPtr pScreen = xf86ScrnToScreen(pScrn);
-    Bool ret;
+    uint32_t handles[4] = { 0, };
+    CARD32 strides[4] = { 0, }, offsets[4] = { 0, };
+    int fds[4] = { -1, -1, -1, -1 };
+    int num_fds;
+    int i;
 
     if (pixmap)
         return pixmap;
@@ -1079,19 +1099,31 @@  create_pixmap_for_fbcon(drmmode_ptr drmmode, ScrnInfoPtr pScrn, int fbcon_id)
         fbcon->height != pScrn->virtualY)
         goto out_free_fb;
 
-    pixmap = drmmode_create_pixmap_header(pScreen, fbcon->width,
-                                          fbcon->height, fbcon->depth,
-                                          fbcon->bpp, fbcon->pitch, NULL);
-    if (!pixmap)
+    /* 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;
-
-    ret = glamor_egl_create_textured_pixmap(pixmap, fbcon->handle, fbcon->pitch);
-    if (!ret) {
-      FreePixmap(pixmap);
-      pixmap = NULL;
     }
+    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);
+    if (!pixmap)
+        goto out_close_fds;
 
     drmmode->fbcon_pixmap = pixmap;
+out_close_fds:
+    for (i = 0; i < ARRAY_SIZE(fds); i++) {
+        if (fds[i] != -1)
+            close(fds[i]);
+    }
 out_free_fb:
     drmModeFreeFB(fbcon);
     return pixmap;

Comments

On 23 March 2018 at 13:50, Daniel Stone <daniels@collabora.com> wrote:
> glamor_pixmap_from_fds saves us the trouble of creating the pixmap and
> dealing with its header ourselves; instead we pass the whole thing off
> to Glamor.
>
> This requires us to provide FDs rather than handles for the import, but
> Glamor was already doing that internally, so no functional change.
>
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 52 +++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 4b22abd0f..39ed16f98 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -30,6 +30,7 @@
>  #endif
>
>  #include <errno.h>
> +#include <fcntl.h>
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
>  #include <unistd.h>
> @@ -1058,6 +1059,21 @@ drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
>      drmmode_crtc->dpms_mode = mode;
>  }
>
> +static int
> +handles_to_fds(drmmode_ptr drmmode, uint32_t *handles, int *fds)
Nit: Although uncommon in the xserver codebase, I can use uint32_t
handles[4] ;-)

> +{
> +    int err;
> +    int i;
> +
> +    for (i = 0; i < 4 && handles[i]; i++) {
> +        err = drmPrimeHandleToFD(drmmode->fd, handles[i], O_CLOEXEC, &fds[i]);
> +        if (err != 0)
> +            return 0;
Close the existing fds ones on error?

With the fd leak plugged (regardless of the nit)
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Aside: there's plenty of duplication in the area + some partial depth
<> format mappings.

-Emil
Hi,

On 26 March 2018 at 16:11, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 23 March 2018 at 13:50, Daniel Stone <daniels@collabora.com> wrote:
>> +    for (i = 0; i < 4 && handles[i]; i++) {
>> +        err = drmPrimeHandleToFD(drmmode->fd, handles[i], O_CLOEXEC, &fds[i]);
>> +        if (err != 0)
>> +            return 0;
> Close the existing fds ones on error?
>
> With the fd leak plugged (regardless of the nit)
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Right you are; it can also be fixed by s/out_free_fb/out_close_fds/ in
the caller's failure path (below the 'Failed to make prime FD for
handle' message).

> Aside: there's plenty of duplication in the area + some partial depth
> <> format mappings.

There sure is, but I decided to leave those as-is for now.

Cheers,
Daniel