Re: [OSS-Tools] [PATCH platsch V3 3/4] platsch: split into platsch and libplatsch

[LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>]

Hi Ulrich，

Thanks your input, I will address in coming version.
Just question about static link as bellow.

> -----Original Message-----
> From: Ulrich Ölmann <u.oelmann@pengutronix.de>
> Sent: Friday, June 14, 2024 6:57 PM
> To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> Cc: oss-tools@pengutronix.de; m.felsch@pengutronix.de
> Subject: Re: [OSS-Tools] [PATCH platsch V3 3/4] platsch: split into platsch and
> libplatsch
>
> [你通常不会收到来自 u.oelmann@pengutronix.de 的电子邮件。请访问
> https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
>
>
> Hi Qing-Wu Li,
>
> On Thu, Jun 13 2024 at 09:07 +0200, LI Qingwu
> <Qing-wu.Li@leica-geosystems.com.cn> wrote:
> > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > ---
> >  libplatsch.c | 590
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  libplatsch.h |  51 +++++
> >  meson.build  |  13 +-
> >  platsch.c    | 582 +-------------------------------------------------
> >  4 files changed, 658 insertions(+), 578 deletions(-)  create mode
> > 100644 libplatsch.c  create mode 100644 libplatsch.h
> >
> > diff --git a/libplatsch.c b/libplatsch.c new file mode 100644 index
> > 0000000..1d48e0e
> > --- /dev/null
> > +++ b/libplatsch.c
> > @@ -0,0 +1,590 @@
> > +/*
> > + * Copyright (C) 2019 Pengutronix, Uwe Kleine-König
> > +<u.kleine-koenig@pengutronix.de>
> > + *
> > + * Permission to use, copy, modify, and/or distribute this software
> > + * for any purpose with or without fee is hereby granted.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> > + * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
> IMPLIED
> > + * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
> THE
> > + * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
> > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
> FROM
> > +LOSS
> > + * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
> > + * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> > + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + *
> > + * Some code parts base on example code written in 2012 by David
> > +Herrmann
> > + * <dh.herrmann@googlemail.com> and dedicated to the Public Domain.
> > +It was found
> > + * in 2019 on
> > + *
> > +https://raw/
> >
> +.githubusercontent.com%2Fdvdhrm%2Fdocs%2Fmaster%2Fdrm-howto%2Fmo
> deset
> >
> +.c&data=05%7C02%7Cqing-wu.li%40leica-geosystems.com.cn%7C8fc4e5421e
> 3d
> >
> +42d7ffef08dc8c60bec5%7C1b16ab3eb8f64fe39f3e2db7fe549f6a%7C0%7C0%7
> C638
> >
> +539594360927380%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL
> CJQIjoiV
> >
> +2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=5xytXSa
> tTpw4
> > +KV3tRtn9pb%2F%2Fos%2BCbE6dWY%2BbZ95VGd0%3D&reserved=0
> > + */
> > +
> > +#include <assert.h>
> > +#include <ctype.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <getopt.h>
> > +#include <libgen.h>
> > +#include <stdarg.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/mman.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
>
> if there is no requirement for a new order of inclusion please keep the sorting
> found in platsch.c.
>
> > +#include "libplatsch.h"
> > +#include <drm_fourcc.h>
> > +#include <xf86drm.h>
> > +#include <xf86drmMode.h>
>
> The includes xf86drmMode.h and drm_fourcc.h are already included in
> libplatsch.h, so there is probably no need to include them again here.
>
> > +#include "libplatsch.h"
>
> This was already done some lines above.
>
> > +void redirect_stdfd(void)
> > +{
> > +     int devnull = open("/dev/null", O_RDWR, 0);
> > +
> > +     if (devnull < 0) {
> > +             error("Failed to open /dev/null: %m\n");
> > +             return;
> > +     }
> > +
> > +     close(STDIN_FILENO);
> > +     dup2(devnull, STDIN_FILENO);
> > +     close(STDOUT_FILENO);
> > +     dup2(devnull, STDOUT_FILENO);
> > +     close(STDERR_FILENO);
> > +     dup2(devnull, STDERR_FILENO);
> > +     close(devnull);
> > +}
>
> [...]
>
> > +static int drmfd;
> > +
> > +struct modeset_dev *init(void)
> > +{
> > +     static char drmdev[128];
>
> This hasn't been static before, is that really needed?
>
> > +     int ret = 0, i;
> > +
> > +     for (i = 0; i < 64; i++) {
> > +             struct drm_mode_card_res res = {0};
> > +
> > +             /*
> > +              * XXX: Maybe use drmOpen instead?
> > +              * (Where should name/busid come from?)
> > +              * XXX: Loop through drm devices to find one with
> connectors.
> > +              */
> > +             ret = snprintf(drmdev, sizeof(drmdev), DRM_DEV_NAME,
> DRM_DIR_NAME, i);
> > +             if (ret >= sizeof(drmdev)) {
> > +                     error("Huh, device name overflowed buffer\n");
> > +                     goto execinit;
> > +             }
> > +
> > +             drmfd = open(drmdev, O_RDWR | O_CLOEXEC, 0);
> > +             if (drmfd < 0) {
> > +                     error("Failed to open drm device: %m\n");
> > +                     goto execinit;
> > +             }
> > +
> > +             ret = drmIoctl(drmfd, DRM_IOCTL_MODE_GETRESOURCES,
> &res);
> > +             if (ret < 0) {
> > +                     close(drmfd);
> > +                     continue;
> > +             } else {
> > +                     /* Device found */
> > +                     break;
> > +             }
> > +     }
> > +
> > +     if (i == 64) {
> > +             error("No suitable DRM device found\n");
> > +             goto execinit;
> > +     }
> > +
> > +     ret = drmprepare(drmfd);
> > +     if (ret) {
> > +             error("Failed to prepare DRM device\n");
> > +             goto execinit;
> > +     }
> > +
> > +     return modeset_list;
> > +
> > +execinit:
>
> The label should probably be renamed as this new init()-function has nothing to
> do with calling exec() for the next PID1 program anymore.
>
> > +     return NULL;
> > +}
> > +
> > +void update_display(struct modeset_dev *dev) {
> > +     int ret = 0;
> > +
> > +     if (dev->setmode) {
> > +             ret = drmModeSetCrtc(drmfd, dev->crtc_id, dev->fb_id, 0, 0,
> &dev->conn_id, 1,
> > +                                  &dev->mode);
> > +             if (ret) {
> > +                     error("Cannot set CRTC for connector #%u: %m\n",
> dev->conn_id);
> > +             }
> > +             dev->setmode = 0;
>
> This line is new. If it is needed, please add it in a separate commit and describe
> why it is needed.
>
> > +     } else {
> > +             ret = drmModePageFlip(drmfd, dev->crtc_id, dev->fb_id, 0,
> NULL);
> > +             if (ret) {
> > +                     error("Page flip failed on connector #%u: %m\n",
> dev->conn_id);
> > +             }
> > +     }
> > +}
> > +
> > +void draw(struct modeset_dev *dev, const char *dir, const char *base)
> > +{
> > +     draw_buffer(dev, dir, base);
> > +     update_display(dev);
> > +}
> > +
> > +void finish(void) { drmDropMaster(drmfd); }
>
> The original code checked the return value here. Is drmDropMaster()
> guaranteed to always succeed?
>
> > +void deinit(void)
> > +{
> > +     struct modeset_dev *iter;
> > +
> > +     for (iter = modeset_list; iter; iter = iter->next) {
>
> Use after free: you are accessing iter->next here...
>
> > +             if (iter->map) {
> > +                     munmap(iter->map, iter->size);
> > +             }
> > +             if (iter->fb_id) {
> > +                     drmModeRmFB(drmfd, iter->fb_id);
> > +             }
> > +             free(iter);
>
> ... although iter was already freed here.
>
> > +     }
> > +}
> > diff --git a/libplatsch.h b/libplatsch.h new file mode 100644 index
> > 0000000..77f74a9
> > --- /dev/null
> > +++ b/libplatsch.h
> > @@ -0,0 +1,51 @@
> > +#ifndef __LIBPLATSCH_H__
> > +#define __LIBPLATSCH_H__
> > +
> > +#include <xf86drmMode.h>
> > +#include <drm_fourcc.h>
> > +#include "libplatsch.h"
>
> Probably no need to include libplatsch.h from itself.
>
> > +#include <unistd.h>
> > +
> > +
> > +
> > +#define debug(fmt, ...) printf("%s:%d: " fmt, __func__, __LINE__,
> > +##__VA_ARGS__) #define error(fmt, ...) fprintf(stderr, fmt,
> > +##__VA_ARGS__)
> > +
> > +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(*a))
> > +
> > +struct platsch_format {
> > +     uint32_t format;
> > +     uint32_t bpp;
> > +     const char *name;
> > +};
> > +
> > +struct modeset_dev {
> > +     struct modeset_dev *next;
> > +
> > +     uint32_t width;
> > +     uint32_t height;
> > +     uint32_t stride;
> > +     uint32_t size;
> > +     const struct platsch_format *format;
> > +     uint32_t handle;
> > +     void *map;
> > +     bool setmode;
> > +     drmModeModeInfo mode;
> > +     uint32_t fb_id;
> > +     uint32_t conn_id;
> > +     uint32_t crtc_id;
> > +};
> > +
> > +static const struct platsch_format platsch_formats[] = {
> > +     { DRM_FORMAT_RGB565, 16, "RGB565" }, /* default */
> > +     { DRM_FORMAT_XRGB8888, 32, "XRGB8888" }, };
> > +
> > +ssize_t readfull(int fd, void *buf, size_t count); struct modeset_dev
> > +*init(void); void draw(struct modeset_dev *dev, const char *dir,
> > +const char *base); void finish(void); void redirect_stdfd(void); void
> > +update_display(struct modeset_dev *dev);
>
> Only the minority of symbols in the new library are namespaced, compare
> "platsch_format" against "draw" or "finish". Particularly the latter can be
> expected to be exported from third party code which libplatsch is then not able
> to be linked against. Thus please add the prefix "platsch_" to at least all symbols
> that are published via libplatsch.h.
>
> > +#endif
> > \ No newline at end of file
> > diff --git a/meson.build b/meson.build index 9f6be1e..dc55238 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1,15 +1,18 @@
> >  project('platsch', 'c')
> >
> >  platsch_dep = dependency('libdrm', version : '>= 2.4.112', required :
> > true)
>
> One feature of platsch is to start up fast, hence it is intentionally linked against
> static libraries only. Therefore please use
>
>   platsch_dep = dependency('libdrm',
>       version: '>= 2.4.112',
>       static: true,
>       required: true
>   )
>
> instead.
>
> > -sources = ['platsch.c']
> > +sources = ['libplatsch.c']
> >
> > -# Define the headers
> > -headers = ['platsch.h']
> >
> > -# Create the platsch executable
> > -executable('platsch',
> > +libplatsch = static_library('libplatsch',
>
> The resulting library is called "liblibplatsch.a" now which is somewhat
> redundant.
>
> >      sources,
> >      dependencies: platsch_dep,
> > +)
> > +
> > +executable('platsch',
> > +    'platsch.c',
> > +    dependencies: platsch_dep,
> > +    link_with: libplatsch,
>
> Unfortunately picking the static variant of libdrm in its dependency()- statement
> is not enough to get a statically linked executable "platsch"
> as there are still some dynamically linked objects left, e.g. the c-library. Thus
> please add
>
>     link_args: '-static',

The current libdrm recipe of Yocto just generate dynamic link library,
Need to adaptor recipe to generate Static link library, this is the only one for current platsch.
But If involve cairo and png later, there would be more recipe to adaptor, like:
libgobject-2.0
libffi
libglib
libpcre
libpixman-1
libfontconfig
libuuid
libexpat
libfreetype
libpng16

do we really need to link all with static?

>
> to the executable() statement.
>
> Best regards
> Ulrich
>
>
> >      install: true,
> >      include_directories: include_directories('.')
> >  )
>
> [...]
>
> > -execinit:
> >       if (pid1) {
> >               ret = fork();
> >               if (ret < 0) {
> --
> Pengutronix e.K.                           | Ulrich Ölmann
> |
> Industrial Linux Solutions                 |
> http://www.pen/
> gutronix.de%2F&data=05%7C02%7Cqing-wu.li%40leica-geosystems.com.cn%7C
> 8fc4e5421e3d42d7ffef08dc8c60bec5%7C1b16ab3eb8f64fe39f3e2db7fe549f6a
> %7C0%7C0%7C638539594360937338%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C
> %7C%7C&sdata=NT9N%2FPYt2xioiQf4JRpwOgSqjrDHVKQ3tXhiDGEbDFk%3D&r
> eserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |